fix: define default arch in cut command #256

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

Have you signed the CLA?

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 <paul.mars@canonical.com>

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

Command	Mean [s]	Min [s]	Max [s]	Relative
`BASE`	8.916 ± 0.052	8.843	9.013	1.00
`HEAD`	8.916 ± 0.053	8.856	9.011	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 <paul.mars@canonical.com>

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

internal/setup/setup.go

 	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

internal/setup/setup.go

 	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

cmd/chisel/cmd_cut.go

 	}
+	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

internal/setup/setup_test.go


 		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

internal/slicer/slicer_test.go

 			})

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


see this comment

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 <paul.mars@canonical.com>

fix: Detect cycles with v3-essential

53fe075

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: arch specific slice installed if no arch provided

c80bc7b

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: Select error out if empty arch

4ce1609

Signed-off-by: Paul Mars <paul.mars@canonical.com>

style: improve error message and align with code conventions

7796bfc

Signed-off-by: Paul Mars <paul.mars@canonical.com>

style: improve quoting

842070c

Signed-off-by: Paul Mars <paul.mars@canonical.com>

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 <paul.mars@canonical.com>

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

Open

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

fix: define default arch in cut command #256

Are you sure you want to change the base?

fix: define default arch in cut command #256

Uh oh!

Conversation

upils commented Nov 21, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Nov 21, 2025 •

edited

Loading

letFunny commented Nov 21, 2025 •

edited

Loading

letFunny Nov 21, 2025 •

edited

Loading