stage_executor: reset platform in systemcontext for every stage. #5971
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: flouthoc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Just wondering how to test this in CI since |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
flouthoc
force-pushed
the
reset-context-platform
branch
from
d15172e to
8cef977
Compare
flouthoc
added
the
No New Tests
|
Temp adding |
|
The "header-builtin" test does something similar, so I'd suggest ARG FOREIGNARCH
FROM --platform=linux/$FOREIGNARCH busybox AS foreign
FROM busybox
COPY --from=foreign /bin/busybox /bin/busybox.foreign
RUN arch
RUN ls -l /bin/busybox /bin/busybox.foreign
RUN ! cmp /bin/busybox /bin/busybox.foreignDefine the |
flouthoc
force-pushed
the
reset-context-platform
branch
3 times, most recently
from
ab73040 to
cbc1002
Compare
Every stage now has its own copy of systemcontext. On processing of every stage platform spec in systemcontext must be correctly reset. Closes: containers#5968 Signed-off-by: flouthoc <[email protected]>
flouthoc
force-pushed
the
reset-context-platform
branch
from
cbc1002 to
4010772
Compare
flouthoc
removed
the
No New Tests
| @@ -59,6 +59,7 @@ import ( | |||
| // name to the image that it produces. | |||
| type StageExecutor struct { | |||
| ctx context.Context | |||
| systemContext types.SystemContext | |||
There was a problem hiding this comment.
types.SystemContext values in structs are customarily pointers. This should be a pointer.
| // In a multi-stage build where `FROM --platform=<>` was used then we must | ||
| // reset context for new stages so that new stages don't inherit unexpected | ||
| // `--platform` from prior stages. | ||
| if stage.Builder.Platform != "" || (stage.Position != 0 && builderSystemContext.ArchitectureChoice == "" && builderSystemContext.VariantChoice == "" && builderSystemContext.OSChoice == "") { |
There was a problem hiding this comment.
Would this be simpler if the builderSystemContext had its ...Choice fields cleared as part of initializing it?
| @test "build must reset platform for stages if needed" { | ||
| otherarch="arm64" | ||
| # just make sure that other arch is not equivalent to host arch | ||
| if [[ "$otherarch" == "$myarch" ]]; then |
There was a problem hiding this comment.
$myarch has not been defined at this point, so the test is always false.
| if [[ "$otherarch" == "$myarch" ]]; then | ||
| otherarch="amd64" | ||
| fi | ||
| run_buildah build $WITH_POLICY_JSON --build-arg FOREIGNARCH=$otherarch -f $BUDFILES/multiarch/Containerfile.reset-platform $BUDFILES/multiarch |
There was a problem hiding this comment.
This should also test a case where the the TARGETPLATFORM build-arg is set, so that we don't regress on that.
On processing of every stage platform spec in systemcontext must be correctly reset.
[NO NEW TESTS NEEDED]
Closes: #5968
What type of PR is this?
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?