fix: handle default arch in Select by upils · Pull Request #256 · canonical/chisel

upils · 2025-11-21T10:27:11Z

This description is now wrong. The PR was changed to properly handle the default empty arch inside Select.

-- @niemeyer

Original description below.

This PR solves 2 distinct issues:

When the --arch option was not used when calling chisel cut, no architecture was passed down to the selector, resulting in slices using v3-essential with an Arch value to be ignored.
Slices using v3-essential with an Arch value where ignored during validation of the release, possibly leading to undetected conflicts (i.e. loops).

(cherry picked from commit 4f7f18d)

Signed-off-by: Paul Mars <[email protected]>

github-actions · 2025-11-21T10:31:08Z

Command	Mean [s]	Min [s]	Max [s]	Relative
`BASE`	11.460 ± 0.053	11.393	11.544	1.00
`HEAD`	11.502 ± 0.064	11.426	11.613	1.00 ± 0.01

letFunny · 2025-11-21T10:37:49Z

It is also useful to look at the processes to see how we failed to catch the first bug before releasing a new Chisel version.

How was the feature tested? Why was the bug not found?

For this feature, I did manual testing in my computer always using the --arch CLI option, which is why I did not see the bug. Because I wanted to see it tested against the real packages in case I missed anything, I asked the ROCKS team to create PR and test this feature in the CI. The CI did not find the issue because spread tests were not added to the PR and the logs were not manually inspected (they are in fact trimmed from the CI output). Lastly, there are also shortcomings in how we test Chisel because the CLI parsing and the glue is only tested when using spread with a limited set of inputs.

How was the bug found?

I only found the bug when reviewing the PR and seeing the logs where missing. I then tested it locally out of paranoia and found that the essentials were not installed.

How to prevent this from happening in the future?

In terms of processes, we should discuss adopting a strategy where new features are tested for a while before releasing a new version, similar to the "unstable" channel in snaps. At the same time, for every new feature, I think we should make QA part of the process and define the expectations more formally. For example, to create a PR using the new feature with the appropriate spread tests as a necessary step before tagging a new version from main.

Signed-off-by: Paul Mars <[email protected]>

niemeyer

Looks good, thanks for the fix.

Paul, please note that the summary for the PR and commit is not following our standard conventions.

niemeyer

Sorry, there's actually an issue worth talking about here.

niemeyer · 2025-11-21T16:01:34Z

 	logf("Selecting slices...")

+	if arch == "" {
+		return nil, fmt.Errorf(`cannot select slices: "arch" is unset`)


Sorry, I actually have a question here: there's something suspect about this PR, which is the whole logic was actually handling the empty arch just fine until now, and suddenly because we introduced a v3-essential feature, the behavior is changing and we're unable to let the architecture empty, while before we could. It looks like there's something being missed here. Clearly we had a default architecture before. Where was it, and why is this PR not touching that, and also why isn't that place collaborating with the existing fix?

The arch was not needed prior to v3-essential because the dependency set that Select would generate would not depend on it. When we introduced v3-essential we changed the signature of this function so the user needs to supply an arch. Paul and me discussed today whether selecting slices with an empty arch makes sense or not and we think it doesn't.

We also discussed calling deb.InferArch here which might be the default you are hinting to. In general we thought it was better to return an error and let the upper layer either infer the arch or to supply something else.

The arch was not needed prior to v3-essential because

If that was true we would not have an arch parameter in select. It's hard to believe it's taken in, and not taken into account for anything. If it is taken into account for something, that something is now working differently from before.

Maybe that's okay, but this is ignoring the point I made instead of addressing it.

Prior to v3-essential, Select was not taking in the arch parameter (see https://github.com/canonical/chisel/pull/246/files#diff-f61b5f82e446ae57f46ae7c811a56addc74662f37f0dd6440c99c5d869b651d1R465), because the order function was not either.

This parameter was added so the arch-specific essentials could be ignored if the arch was not matching. However the implementation of #246 did not consider the case of an empty arch parameter, which happens during the release validation. And this led to not considering slices with v3-essential with a Arch entry. The fix is also addressing this issue (see second bullet point of the PR description).

previously the default arch was "", and now its the result of InferArch which returns non-empty string or error: https://github.com/canonical/chisel/pull/256/changes#diff-8867bd2aaddbb2642dd6d8fee78a80c836af5eaabbd059da7fe716a378603203R61-R68

having said that, this feels like a great place/use of enums

lczyk · 2025-12-12T11:09:18Z

 	logf("Selecting slices...")

+	if arch == "" {
+		return nil, fmt.Errorf(`cannot select slices: "arch" is unset`)


previously the default arch was "", and now its the result of InferArch which returns non-empty string or error: https://github.com/canonical/chisel/pull/256/changes#diff-8867bd2aaddbb2642dd6d8fee78a80c836af5eaabbd059da7fe716a378603203R61-R68

having said that, this feels like a great place/use of enums

lczyk · 2025-12-12T11:10:59Z

 	}
+	arch := cmd.Arch
+	if arch == "" {
+		darch, err := deb.InferArch()


NOTE: this is returning the debArch field of archPair is that the correct one to use? maybe in the future we should do an internal refactor for goArch and debArch to be enums to make it clear which one we're using where.. 🤔

This debArch is then used as a filter for values defined in path so this is the one we want. The purpose of deb.InferArch() is to return the current runtime deb architecture. The fact it is inferred from the GOARCH is an implementation details I think.

What do you think if the source of confusion?

oh, just when reading the code there is both goArch and debArch, which are both strings so its not explicit which one should go where, and the name of InferArch does not say which one it returns, so i could imagine someone making a mistake of thinking it returns the other one -- something which would not be caught by the compiler

lczyk · 2025-12-12T11:11:22Z


 		if test.selslices != nil {
-			selection, err := setup.Select(release, test.selslices, "")
+			selection, err := setup.Select(release, test.selslices, "amd64")


magic value. should be a constant imo.

these are tests, so its maybe less important here, but nonetheless it does introduce a notion of a default architecture. maybe we should define that and the return it here:
https://github.com/canonical/chisel/pull/256/changes#diff-8867bd2aaddbb2642dd6d8fee78a80c836af5eaabbd059da7fe716a378603203R65 ??

I do not think it introduces the notion of a default architecture from the point of view of setup.Slice().
Here in the test we use a value coherent with our tests cases. We set amd64 because that was previously the implicit value for the test cases.

The behavior of falling back to inferring the arch is consistent with how the release is inferred in obtainRelease. There is no "default value" but a heuristic to get a value from the building environment if none was explicitly provided (I see a nuance here, let me know if this is not clear).

lczyk · 2025-12-12T11:17:23Z

 			})

+			if test.arch == "" {
+				test.arch = "amd64"


see this comment

Let Select handle a received empty arch as archive.Open() does. This respects the existing logic handling it in other places of the code.

Also set a default value for release parsing tests to make them independent of the testing machine architecture.

lczyk

i confirm this works correctly without the --arch flag for ubuntu-26.04/binutils, and changes + tests look good to me 👍

Make the test robust to execution on different archs.

letFunny

Thanks for working on this Paul, some comments about how to make the test a bit cleaner.

letFunny

Thanks for working on this Paul, I think now it is perfect!

niemeyer

Thanks for the updates. I've updated the summary and description to reflect the changes in the PR.

* feat(26.04): add cpp-13 slices for s390x and ppc64le Forward port of #1091 from ubuntu-24.04, rewritten for chisel format v3. The /usr/share/doc entry of a cpp-13-<triplet> package is a symlink whose target depends on how that package was built: gcc-13-base for the native build, gcc-13-<triplet>-base for the cross builds. The copyright slices are keyed by architecture accordingly, and the four gcc-13-<triplet>-base packages get copyright slices of their own. * test(26.04): add integration tests for cpp-13 on s390x and ppc64le The existing cpp-13 tests also learn about the two new spread runners, on which they are cross-compilation cases. * docs(26.04): mark cpp-13 copyright paths as symlinks Matches the marker the binutils and cpp-15 slices carry, and makes it clear the entry is not the directory it looks like. * test(26.04): fix dynamic linker lookup in the cpp-13 tests The ld-linux-*.so.* glob matches nothing on ppc64el and s390x, whose loaders are ld64.so.2 and ld64.so.1. Use the same lookup #1090 gave the binutils tests, so all four cpp-13 suites agree. Nothing was failing: the value is only consumed on the native-architecture path, which never runs on those two runners. * fix(26.04): dangling gcc-13 copyright on cross arches Same bug and same fix as the cpp-13 slices this branch builds on: the /usr/share/doc entry of a gcc-13-<triplet> package is a symlink to gcc-13-base for the native build and to gcc-13-<triplet>-base for the cross builds, so pulling gcc-13-base unconditionally left it dangling everywhere but on the native architecture. Depends on the gcc-13-<triplet>-base slices added by the cpp-13 branch. * test(26.04): run the gcc-13 tests on the s390x and ppc64le runners Both suites bailed out with "Unsupported architecture" there, which fails outright now that a change to these slices schedules them on all four runners. The arch lists match what gcc-15 already carries. * docs(26.04): align the gcc-13 slice notes with gcc-15 Three generations of these slices each spelled the same guidance differently. Adopt the wording gcc-15 carries. * feat(26.04): add gcc-13 slices for s390x and ppc64le Forward port of the remaining half of #1092. The two new triplet slices follow gcc-15's, including the per-architecture copyright split the rest of this branch applies to the existing two. gcc-13.yaml gains the matching cc1 and gcc-13 entries, so gcc-13_gcc-13 -- the architecture-independent slice that carries /usr/bin/gcc-13 along with an assembler, a linker and cc1 -- resolves on all four architectures rather than only amd64 and arm64. * test(26.04): add integration tests for gcc-13 on s390x and ppc64le Copies of the gcc-15 suites, which already cover these two runners. * test(26.04): drop the chisel_arch workaround from the gcc-13 test Obsolete since canonical/chisel#256, and its Unsupported architecture fallback fails outright on the s390x and ppc64le runners -- which a change to gcc-13.yaml now schedules. Same shape the gcc-15 test already has.

* feat(26.04): add cpp-14 slices for s390x and ppc64le Same shape as the cpp-13 slices, one toolchain generation up. The /usr/share/doc entry of a cpp-14-<triplet> package is a symlink whose target depends on how that package was built: gcc-14-base for the native build, gcc-14-<triplet>-base for the cross builds. The copyright slices are keyed by architecture accordingly, and the four gcc-14-<triplet>-base packages get copyright slices of their own. * test(26.04): add integration tests for cpp-14 on s390x and ppc64le The existing cpp-14 tests also learn about the two new spread runners, on which they are cross-compilation cases. * docs(26.04): mark cpp-14 copyright paths as symlinks Matches the marker the binutils and cpp-15 slices carry, and makes it clear the entry is not the directory it looks like. * test(26.04): fix dynamic linker lookup in the cpp-14 tests The ld-linux-*.so.* glob matches nothing on ppc64el and s390x, whose loaders are ld64.so.2 and ld64.so.1. Use the same lookup #1090 gave the binutils tests, so all four cpp-14 suites agree. Nothing was failing: the value is only consumed on the native-architecture path, which never runs on those two runners. * fix(26.04): dangling gcc-14 copyright on cross arches Same bug and same fix as the cpp-14 slices this branch builds on: the /usr/share/doc entry of a gcc-14-<triplet> package is a symlink to gcc-14-base for the native build and to gcc-14-<triplet>-base for the cross builds, so pulling gcc-14-base unconditionally left it dangling everywhere but on the native architecture. Depends on the gcc-14-<triplet>-base slices added by the cpp-14 branch. * test(26.04): run the gcc-14 tests on the s390x and ppc64le runners Both suites bailed out with "Unsupported architecture" there, which fails outright now that a change to these slices schedules them on all four runners. The arch lists match what gcc-15 already carries. * docs(26.04): correct and align the gcc-14 slice notes The notes pointed at cpp-12-<triplet> for the cc1 compiler, two generations off. They also carried a TODO saying the architecture- independent gcc-14 slices do not exist yet, which slices/gcc-14.yaml has since made untrue. Replaced with the wording gcc-15 already carries, so the three generations read the same. * feat(26.04): add gcc-14 slices for s390x and ppc64le The two new triplet slices follow gcc-15's, including the per-architecture copyright split the rest of this branch applies to the existing two. Noble has no gcc-14, so there is nothing to forward port from -- this is #1092 applied to a generation that release never carried. gcc-14.yaml gains the matching cc1 and gcc-14 entries, so gcc-14_gcc-14 -- the architecture-independent slice that carries /usr/bin/gcc-14 along with an assembler, a linker and cc1 -- resolves on all four architectures rather than only amd64 and arm64. * test(26.04): add integration tests for gcc-14 on s390x and ppc64le Copies of the gcc-15 suites, which already cover these two runners. * test(26.04): drop the chisel_arch workaround from the gcc-14 test Obsolete since canonical/chisel#256, and its Unsupported architecture fallback fails outright on the s390x and ppc64le runners -- which a change to gcc-14.yaml now schedules. Same shape the gcc-15 test already has.

letFunny and others added 7 commits November 20, 2025 13:41

bugfix: cut does not infer the architecture properly

a080409

(cherry picked from commit 4f7f18d)

add test showcasing behavior

2fb3c32

Signed-off-by: Paul Mars <[email protected]>

fix: Detect cycles with v3-essential

53fe075

Signed-off-by: Paul Mars <[email protected]>

fix: arch specific slice installed if no arch provided

c80bc7b

Signed-off-by: Paul Mars <[email protected]>

fix: Select error out if empty arch

4ce1609

Signed-off-by: Paul Mars <[email protected]>

style: improve error message and align with code conventions

7796bfc

Signed-off-by: Paul Mars <[email protected]>

style: improve quoting

842070c

Signed-off-by: Paul Mars <[email protected]>

letFunny added Priority Look at me first Bug An undesired feature ;-) labels Nov 21, 2025

letFunny requested a review from niemeyer November 21, 2025 10:41

letFunny approved these changes Nov 21, 2025

View reviewed changes

refactor: simplify setting default on test.arch

f1ffee3

Signed-off-by: Paul Mars <[email protected]>

niemeyer approved these changes Nov 21, 2025

View reviewed changes

niemeyer changed the title ~~fix: Properly taking into account the architecture in v3-essential~~ fix: define default arch in cut command Nov 21, 2025

niemeyer requested changes Nov 21, 2025

View reviewed changes

lczyk mentioned this pull request Dec 11, 2025

bug: chisel 1.3.0 --arch requirement canonical/chisel-releases#803

Closed

lczyk reviewed Dec 12, 2025

View reviewed changes

alesancor1 mentioned this pull request Dec 16, 2025

feat(24.04): rust 1.75 canonical/rust-rock#8

Merged

upils mentioned this pull request Jan 5, 2026

Slice definition components under 'v3-essential' are not fetched and extracted into the container image #262

Closed

upils added 3 commits January 7, 2026 09:22

refactor: handle empty arch in Select

9b8ace6

Let Select handle a received empty arch as archive.Open() does. This respects the existing logic handling it in other places of the code.

test: Invalid arch received by Select

79e047d

test: invalid and empty arch for Select

2b506e8

Also set a default value for release parsing tests to make them independent of the testing machine architecture.

upils requested a review from letFunny January 7, 2026 10:33

lczyk approved these changes Jan 7, 2026

View reviewed changes

test: check Select output

032536a

Make the test robust to execution on different archs.

letFunny reviewed Jan 9, 2026

View reviewed changes

Comment thread internal/setup/setup_test.go Resolved

Comment thread internal/setup/setup_test.go Resolved

Comment thread internal/setup/setup_test.go Resolved

test: split test to clarify intent

1863441

letFunny approved these changes Jan 9, 2026

View reviewed changes

niemeyer changed the title ~~fix: define default arch in cut command~~ fix: handle default arch in Select Jan 12, 2026

niemeyer approved these changes Jan 12, 2026

View reviewed changes

niemeyer merged commit d4fa479 into canonical:main Jan 12, 2026
18 checks passed

lczyk mentioned this pull request Jun 25, 2026

feat(26.04): add rustfmt and rustfmt-1.93 canonical/chisel-releases#1028

Merged

3 tasks done

lczyk mentioned this pull request Jul 15, 2026

chore(testing): remove leftover --arch workaround from 26.04+ integration tests canonical/chisel-releases#1078

Closed

Conversation

upils commented Nov 21, 2025 • edited by niemeyer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

letFunny commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How was the feature tested? Why was the bug not found?

How was the bug found?

How to prevent this from happening in the future?

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

letFunny Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lczyk left a comment

Choose a reason for hiding this comment

Uh oh!

letFunny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

letFunny left a comment

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

upils commented Nov 21, 2025 •

edited by niemeyer

Loading

github-actions Bot commented Nov 21, 2025 •

edited

Loading

letFunny commented Nov 21, 2025 •

edited

Loading

letFunny Nov 21, 2025 •

edited

Loading