dockerfile: maintain osversion/variant from the base image #5714
Conversation
| } | ||
| if platformOpt.targetPlatform.OSVersion != "" || !sameOsArch { | ||
| target.image.OSVersion = platformOpt.targetPlatform.OSVersion | ||
| } | ||
| if platformOpt.targetPlatform.OSFeatures != nil { |
There was a problem hiding this comment.
This OSFeatures line doesn't make much sense to me either as it can create duplicates and if !sameOsArch then probably should not be merged. Shouldn't be impactful as OSFeatures can't be passed via opt atm. but that will change. I opened containerd/platforms#23 as I think some of it should be handled in upstream platforms pkg.
There was a problem hiding this comment.
This isn't merging the OSFeatures, it's replacing them, so I don't understand where duplicates would come from? It should just be a straight-up copy of platformOpt.targetPlatform.OSFeatures.
It probably should have had the same !sameOsArch check added though. Not particularly impactful, last time I looked into it, OSFeatures was 100% unused and the OCI Working Group that might want it for future use didn't seem interested in it, in favour of a separate artifact that covered its use-cases.
There was a problem hiding this comment.
This isn't merging the OSFeatures, it's replacing them
Ah, yeah. I guess I looked that it was appending to the old OSFeatures, but it is appending to empty array (slices.Clone)
There was a problem hiding this comment.
Yeah, I had to open-code slices.Clone in the original PR because it wasn't available until Go 1.21 and we were declaring Go 1.20 as supported at that time.
That's no longer the case so we could clean that up for readability now.
Restores the behavior from v0.12. Signed-off-by: Tonis Tiigi <[email protected]>
tonistiigi
force-pushed
the
fix-osversion-base
branch
from
326ae5a to
a4c1c94
Compare
| target.image.OS = platformOpt.targetPlatform.OS | ||
| target.image.Architecture = platformOpt.targetPlatform.Architecture | ||
| target.image.Variant = platformOpt.targetPlatform.Variant | ||
| target.image.OSVersion = platformOpt.targetPlatform.OSVersion | ||
| if platformOpt.targetPlatform.Variant != "" || !sameOsArch { |
There was a problem hiding this comment.
I'm not sure i understand the sameOsArch check here.
Also the test does not cover it.
There was a problem hiding this comment.
The meaning of this section seems to be that if explicit platform was set then it rewrites the one in the loaded base image. I guess it is mainly related to the case where single arch image is loaded with FROM even if it is for a wrong arch. I don't set the variant/osversion in this case to avoid cases where --platform linux/* build would get osversion(or wrong variant) from the base image.
There was a problem hiding this comment.
I apologise for the late response, but the change makes sense to me. This was working before #4387 by default, as these two fields were unconditionally inherited from the base, so this fix, including the sameOSArch check, seems fine to me.
| } | ||
| if platformOpt.targetPlatform.OSVersion != "" || !sameOsArch { | ||
| target.image.OSVersion = platformOpt.targetPlatform.OSVersion | ||
| } | ||
| if platformOpt.targetPlatform.OSFeatures != nil { |
There was a problem hiding this comment.
This isn't merging the OSFeatures, it's replacing them, so I don't understand where duplicates would come from? It should just be a straight-up copy of platformOpt.targetPlatform.OSFeatures.
It probably should have had the same !sameOsArch check added though. Not particularly impactful, last time I looked into it, OSFeatures was 100% unused and the OCI Working Group that might want it for future use didn't seem interested in it, in favour of a separate artifact that covered its use-cases.
Restores the behavior from v0.12.
fix #5713
@TBBle @cpuguy83
Signef-off-by: Tonis Tiigi [email protected]