Setting --arch should set the TARGETARCH build arg by rhatdan · Pull Request #5478 · containers/buildah

rhatdan · 2024-04-16T20:57:51Z

Also fix a long standing FIXME in the test framework.

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

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?

--arch option now causes the TARGETARCH and TARGETPLATFORM  build args to get modified

packit-as-a-service · 2024-04-16T21:05:33Z

Ephemeral COPR build failed. @containers/packit-build please check.

nalind · 2024-04-16T21:10:37Z

tests/bud.bats

-  fi
-
+  assert "$output" =~ "image platform .* does not match the expected platform" \
+         "With explicit --platform, buildah should warn about no variant"


This error description doesn't seem to match the error message it's checking against.

nalind · 2024-04-16T21:10:56Z

tests/bud.bats

-  assert "$output" !~ "missing .* build argument" \
-         "With explicit --platform, buildah should not warn"
+  assert "$output" =~ "image platform .* does not match the expected platform" \
+         "With explicit --platform, buildah should warn about no variant"


This error description doesn't seem to match the error message it's checking against.

nalind · 2024-04-16T21:15:16Z

pkg/parse/parse.go

 			return nil, fmt.Errorf("unable to parse platform: %w", err)
 		}
-		if os != "" || arch != "" || variant != "" {
+		if os != runtime.GOOS || arch != runtime.GOARCH || variant != "" {


Do the changes in this function affect test results?

We are now defaulting the OS and ARCH and only care if the user changes the value to something different then the default

before on an linux/amd64 machine --platform=linux/amd64 --os=linux --arch=amd64 blue up, now it is allowed since it ultimately does what the user wanted.

If the user does
--platform=linux/amd64 --os=linux --arch=arm64 then the command will fail

It doesn't check that the --platform and --arch/--os values agree. And what's the benefit of allowing some combinations of the flags? Why encourage people with "well, sometimes, it's allowed, in newer versions"?

It checks if the --arch or --os changed values from the default.
Similar to --arch="" and --os="" in the old version would have been ignored.

I think we are arguing about a nothing burger, and will relent to make you happy, even though this is the corner case of all corner cases.

nalind · 2024-04-16T21:15:55Z

tests/bud.bats

+
+  run_buildah build $WITH_POLICY_JSON --arch amd64 -t source -f $containerfile
+  assert "$output" !~ "WARNING" \
+         "With explicit --os (but no arch/variant), buildah should not warn about TARGETOS"


This error description doesn't seem to match the error message it's checking against.

nalind · 2024-04-22T13:52:58Z

pkg/parse/parse_test.go

 	assert.NoError(t, err)
 	assert.Equal(t, sc, &types.SystemContext{
 		BigFilesTemporaryDir:        GetTempDir(),
+		OSChoice:                    "linux",


Should this be runtime.GOOS, too?

nalind · 2024-04-22T13:53:04Z

tests/conformance/conformance_test.go

 				derivedBuilder, err := buildah.NewBuilder(ctx, store, buildah.BuilderOptions{
 					FromImage: buildahID,
 				})
+				assert.NoErrorf(t, err, "creating a new Builder")


Ah, good catch. The require.NoErrorf() after the defer can be removed.

nalind · 2024-04-23T13:53:56Z

tests/bud.bats

+         "With explicit --platform, buildah should warn about pulling difference in platform"
+  assert "$output" =~ "TARGETOS=linux" " --platform TARGETOS set correctly"
+  assert "$output" =~ "TARGETARCH=amd64" " --platform TARGETARCH set correctly"
+  assert "$output" =~ "TARGETVARIANT=" " --platform TARGETVARIANT set correctly"


Is TARGETVARIANT not being set to "v2", per "--platform"?

I think this reveals the fix just not being correct — the core issue is that the buildDockerfilesOnce code setting TARGET* expects SystemContext.*Choice to never be "", and that’s not how the values are set.

Compare more in #5543 (review)

nalind · 2024-04-23T13:55:20Z

tests/bud.bats

+  assert "$output" =~ "TARGETOS=linux" "--os TARGETOS set correctly"
+  assert "$output" =~ "TARGETARCH=amd64" "--os TARGETARCH set correctly"
+  assert "$output" =~ "TARGETVARIANT=" "--os TARGETVARIANT set correctly"
+  assert "$output" =~ "TARGETPLATFORM=linux/amd64" "--os TARGETPLATFORM set correctly"


Are these always going to be "amd64", or would "$(go env GOARCH)" be a better match?

TomSweeneyRedHat · 2024-04-30T23:46:15Z

Looks like this needs a refresh and test help @rhatdan

rhatdan · 2024-05-04T12:50:31Z

@TomSweeneyRedHat @nalind @flouthoc @giuseppe @vrothberg PTAL this finally passes, and is a pretty ugly bug

flouthoc

LGTM

openshift-ci · 2024-05-04T21:25:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [flouthoc,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Also fix a long standing FIXME in the test framework. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

TomSweeneyRedHat · 2024-05-06T23:49:22Z

LGTM
However, the branch is out of date, so I triggered a rebase of the branch.

openshift-ci bot added the approved label Apr 16, 2024

nalind self-assigned this Apr 16, 2024

nalind reviewed Apr 16, 2024

View reviewed changes

rhatdan force-pushed the arch branch 6 times, most recently from 1b80d3e to bf4ada5 Compare April 22, 2024 10:05

nalind reviewed Apr 22, 2024

View reviewed changes

rhatdan mentioned this pull request Apr 23, 2024

Use --platform / --arch etc., podman will not lookup image shortname containers/podman#22420

Closed

rhatdan force-pushed the arch branch from bf4ada5 to 4981946 Compare April 23, 2024 13:50

nalind reviewed Apr 23, 2024

View reviewed changes

rhatdan force-pushed the arch branch from 4981946 to adeb0f7 Compare April 24, 2024 15:27

rhatdan force-pushed the arch branch from adeb0f7 to 37877c4 Compare May 3, 2024 09:51

flouthoc approved these changes May 4, 2024

View reviewed changes

Setting --arch should set the TARGETARCH build arg

da4e2d7

Also fix a long standing FIXME in the test framework. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

TomSweeneyRedHat force-pushed the arch branch from 37877c4 to da4e2d7 Compare May 6, 2024 23:49

rhatdan added the lgtm label May 15, 2024

openshift-merge-bot bot merged commit b255971 into containers:main May 15, 2024

Luap99 mentioned this pull request May 22, 2024

DO NOT MERGE: buildah vendor treadmill containers/podman#13808

Draft

nalind mentioned this pull request May 23, 2024

build: be more selective about specifying the default OS #5543

Merged

stale-locking-app bot added the locked - please file new issue/PR label Aug 22, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 22, 2024

Conversation

rhatdan commented Apr 16, 2024

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?

Uh oh!

packit-as-a-service bot commented Apr 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented Apr 30, 2024

Uh oh!

rhatdan commented May 4, 2024

Uh oh!

flouthoc left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented May 4, 2024

Uh oh!

TomSweeneyRedHat commented May 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants