Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test platforms #323

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Fix test platforms #323

merged 2 commits into from
Jul 29, 2024

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Jul 13, 2024

This fixes 2 things:

  1. Always return a non-nil image config for build artifacts regardless of if they are an image [1].
  2. Tests: set platform on returned llb.State [2]

[1] Stop returning nil image config

This can cause some issues in buildkit libs and also yields a cryptic
error on the docker CLI for these non-container targets when the build
is not outputing tot he local dir.

One specific case right now is the dockerui client which drives all the
build targets will panic on the windowscross/zip target when using
windows as a target platform.

In the past this has also caused panics in dockerd and buildkit. These
have been fixed upstream and were considered security issues due to a
frontend being able to panic dockerd, but I don't think we should
continue doing this.

Something to consider in the future is to either replace our usage of
dockerui to drive builds (not great because it is the canonical
implementation for interacting with docker) and also returning a custom
OCI artifact type for these non-container builds OR we could work
upstream to make dockerui support a more generic type.

[2] Set platform on returned llb state

In my case I am trying to run tests from a Mac connecting to a
linux/amd64 machine over SSH.
The windows signing tests end up failing due to the platform missing
from the llb.State generated for the the unzipper intermediate state.
It tries to run a command in the unzip state but fails due to the
architecture not being compataible.

Before this change reqToState is returning an llb.State without a
platform set on it. Subsequent .Run() calls on that state will
default to the platform of the client (darwin/arm64).

This changes extracts the first (and should be only in the case of our
tests) platform from the returned result and adds that to the returned
llb.State.

@cpuguy83 cpuguy83 marked this pull request as ready for review July 13, 2024 17:36
@cpuguy83 cpuguy83 requested a review from a team as a code owner July 13, 2024 17:36
@cpuguy83 cpuguy83 requested a review from adamperlin July 15, 2024 19:46
@cpuguy83 cpuguy83 force-pushed the fix_test_platforms branch 2 times, most recently from 4456105 to 1e180c6 Compare July 24, 2024 22:05
test/helpers_test.go Outdated Show resolved Hide resolved
This can cause some issues in buildkit libs and also yields a cryptic
error on the docker CLI for these non-container targets when the build
is not outputing tot he local dir.

One specific case right now is the dockerui client which drives all the
build targets will panic on the windowscross/zip target when using
windows as a target platform.

In the past this has also caused panics in dockerd and buildkit. These
have been fixed upstream and were considered security issues due to a
frontend being able to panic dockerd, but I don't think we should
continue doing this.

Something to consider in the future is to either replace our usage of
dockerui to drive builds (not great because it is the canonical
implementation for interacting with docker) and also returning a custom
OCI artifact type for these non-container builds OR we could work
upstream to make dockerui support a more generic type.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
In my case I am trying to run tests from a Mac connecting to a
linux/amd64 machine over SSH.
The windows signing tests end up failing due to the platform missing
from the `llb.State` generated for the the unzipper intermediate state.
It tries to run a command in the unzip state but fails due to the
architecture not being compataible.

Before this change `reqToState` is returning an `llb.State` without a
platform set on it. Subsequent `\.Run()` calls on that state will
default to the platform of the client (darwin/arm64).

This changes extracts the first (and should be only in the case of our
tests) platform from the returned result and adds that to the returned
`llb.State`.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83 cpuguy83 merged commit 63f2350 into Azure:main Jul 29, 2024
9 checks passed
@cpuguy83 cpuguy83 deleted the fix_test_platforms branch July 29, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants