Update uses of Image platform fields in OCI image-spec#3104
Update uses of Image platform fields in OCI image-spec#3104tonistiigi merged 1 commit intomoby:masterfrom
Conversation
| img := ocispecs.Image{} | ||
| img.Architecture = pl.Architecture | ||
| img.OS = pl.OS | ||
| img.Variant = pl.Variant |
There was a problem hiding this comment.
This is where I noticed that in some cases we're discarding the OSVersion and OSFeatures fields (there may be other locations where we do this). Perhaps this is intentional, but we should probably review the code to see if we unintentionally discard those fields, and update the code where needed.
|
Hm... now alpine having issues? |
17714ec to
f51b779
Compare
|
I don't think we would need to make such predictions and I don't consider the per-field setting cleaner. If it breaks, we will fix it with the vendor update. |
|
I agree the struct literal is nicer. The point is that fixing it in BuildKit itself is easy, but I know we have a complicated dependency tree, which means that any other project may go through hell if this needs to be updated, and it may be out of their hands. I'm trying to prevent that; I think the slightly less pretty (at least temporary) code is worth the trade-off. |
I don't understand this. This change here only matters if user is including buildkit and if we fix it in buildkit then user would only change their code. There is no backward compatibility case as well as gomod defines the minimum version requirement. So if the upstream PR gets merged then just update our gomod to use that change. |
|
The issue is that opencontainers/image-spec is (either directly or indirectly) a dependency of many projects (runc, containerd, moby, docker/cli, docker/compose, docker/buildx). Any of those may update their "minimum required version" of the spec at any point, which will force any project that may require a newer version of those (and which also depends on BuildKit) to update. At that point, there won't be a version of BuildKit that's compatible. With the circular dependency of BuildKit on Moby, getting back to a compatible state may take multiple rounds of updating things. |
The OCI image spec is considering to change the Image struct and embedding the Platform type (see opencontainers/image-spec#959) in the go implementation. BuildKit currently uses some struct-literals to propagate the platform fields, which will break once those changes in the OCI spec are merged. Ideally (once that change arrives) we would update the code to set the Platform information as a whole, instead of assigning related fields individually, but in some cases in the code, image platform information is only partially set (for example, OSVersion and OSFeatures are not preserved in all cases). This may be on purpose, so needs to be reviewed. This patch keeps the current behavior (assigning only specific fields), but removes the use of struct-literals to make the code compatible with the upcoming changes in the image-spec module. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
f51b779 to
09afe32
Compare
|
@tonistiigi PTAL; image-spec v1.1.0-rc3 was released, and BuildKit can't build with it now 😢 (see moby/moby#45437) |
Origin: moby/buildkit#3104 Applied-Upstream: buildkit v0.11, engine v25 Gbp-Pq: Name engine-buildkit-image-spec-platform.patch
The OCI image spec is considering to change the Image struct and embedding the
Platform type (see opencontainers/image-spec#959) in the go implementation.
BuildKit currently uses some struct-literals to propagate the platform fields,
which will break once those changes in the OCI spec are merged.
Ideally (once that change arrives) we would update the code to set the Platform
information as a whole, instead of assigning related fields individually, but
in some cases in the code, image platform information is only partially set
(for example, OSVersion and OSFeatures are not preserved in all cases). This
may be on purpose, so needs to be reviewed.
This patch keeps the current behavior (assigning only specific fields), but
removes the use of struct-literals to make the code compatible with the
upcoming changes in the image-spec module.
NOTE:
I added cherry-pick labels for this, to prevent issues for projects that depend both on BuildKit and other projects that may be using the image-spec, which (through go modules) may force them to update the image-spec version to a version with the linked patch.