many: copy boot files from build#2410
Conversation
In the past I implemented `BootFile` as a `[2]string`. This already had some criticism back then. I now have a usecase to pass more information in the `BootFile` so let's turn it into a struct with `src` and `dst` fields. Signed-off-by: Simon de Vlieger <cmdr@supakeen.com>
The `raw` and `raw_ostree` manifests shared a lot of code. Let's deduplicate it so it's easier to expand it and make it work on *both* in the future. Signed-off-by: Simon de Vlieger <cmdr@supakeen.com>
When the `FromBuild` property is set on a `BootFile` the source pipeline is the build pipeline; instead of the root tree. This allows us to have certain firmware packages to only be in the build pipeline instead of having to be installed in the root tree. This *also* allows us to omit the problematic `efi-srpm-macros` package which is depended upon by same firmware packages that want to write directly into the ESP. This is problematic because the ESP path is defined by `efi-srpm-macros` at build time; and can be different from what it is in our partition tables or customizations. The caveat is that without these packages installed in the root tree they also won't receive updates. This is, I guess, a necessary evil until this can be addressed in Fedora itself. Signed-off-by: Simon de Vlieger <cmdr@supakeen.com>
brlane-rht
left a comment
There was a problem hiding this comment.
Looks good so far, manifest checksums still match :)
|
I was briefly confused by the change to the call to |
avitova
left a comment
There was a problem hiding this comment.
So I was trying out the drellabot on this PR, so do not mind it, if it produces something sensible, feel free to use it, just wanted to post this idea here now.
| treeBootFiles, buildBootFiles := splitBootFiles(p.treePipeline.platform.GetBootFiles()) | ||
| if len(treeBootFiles) > 0 { | ||
| treeInputName := "root-tree" | ||
| bootCopyInputs := osbuild.NewPipelineTreeInputs(treeInputName, p.treePipeline.Name()) | ||
| stage, err := bootFilesCopyStage(treeBootFiles, bootCopyInputs, treeInputName, p.Filename(), pt) | ||
| if err != nil { | ||
| return osbuild.Pipeline{}, err |
There was a problem hiding this comment.
In order to make the serialize() function not huge, could we create a function similar to this
func (p *RawImage) addBootFilesStage(
pipeline *osbuild.Pipeline,
bootFiles []platform.BootFile,
inputName string,
sourcePipelineName string,
pt *disk.PartitionTable,
) error {
if len(bootFiles) == 0 {
return nil
}
bootCopyInputs := osbuild.NewPipelineTreeInputs(inputName, sourcePipelineName)
stage, err := bootFilesCopyStage(bootFiles, bootCopyInputs, inputName, p.Filename(), pt)
if err != nil {
return err
}
pipeline.AddStage(stage)
return nil
}
And then you could call something like
if err := p.addBootFilesStage(&pipeline, treeBootFiles, "root-tree", p.treePipeline.Name(), pt); err != nil {
return osbuild.Pipeline{}, err
}
and later the same for buildTree?
There was a problem hiding this comment.
I had something similar initially but if I recall it doesn't generalize over the setup of filtering and the copy inputs so it doesn't save that much space?
There was a problem hiding this comment.
Yea, looking at the drellabot PR it's +13/-13 and +1/-3 not a huge saving so unless you or someone else insists that it makes things more clear I probably won't address this.
There was a problem hiding this comment.
Yeah, it was not as much about saving space, but rather about making the code a tiny bit more readable. I don't quite like what drellabot did anyway. I like those tests that it generated, but I am not sure if we even need such coverage:) So, especially if you are saying that you already tried to implement this before, I am good with the current solution.
In my quest to get things working on systems that don't behave quite the same as other systems I've addressed a concern that was previously raised (turning
boot_filesinto a struct instead of a[2]string).I've then gone and added a property to that struct that allows for the file to be copied from the build pipeline instead of from whatever the root filesystem is.
This is useful in a few ways:
uboot-images-armv8).efi-srpm-macrosand you want to use files from a package that uses those macros to write directly into%{_esp_root}(which is .../boot/efi).See teamsbc/distribution#24 for more details on 2.
The latter comes with the caveat that one won't get updates to those files as the package is never inside the actual booted system. This isn't a particularly big problem since if those packages do get installed they can't write to what they think is the ESP root anyhow; as it's defined as
/boot/efiwhich doesn't exist). However, this is still useful to be able to actually build an image that has the ESP directly at/boot.Perhaps in a follow-up we can format in the information we have about the partition table so the definitions do not need to contain hard coded paths but could instead refer to
{{.ESP}}or{{.XBOOTLDR}}or such. But, that's follow-up material.