Skip to content

fix: define default arch in cut command #256

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.

Sign up for GitHub

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

Open
upils wants to merge 8 commits into
base: main
Choose a base branch
from
+64 −4
Open

fix: define default arch in cut command #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a080409
bugfix: cut does not infer the architecture properly
letFunny Nov 19, 2025
2fb3c32
add test showcasing behavior
upils Nov 20, 2025
53fe075
fix: Detect cycles with v3-essential
upils Nov 21, 2025
c80bc7b
fix: arch specific slice installed if no arch provided
upils Nov 21, 2025
4ce1609
fix: Select error out if empty arch
upils Nov 21, 2025
7796bfc
style: improve error message and align with code conventions
upils Nov 21, 2025
842070c
style: improve quoting
upils Nov 21, 2025
f1ffee3
refactor: simplify setting default on test.arch
upils Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions cmd/chisel/cmd_cut.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/canonical/chisel/internal/archive"
"github.com/canonical/chisel/internal/cache"
"github.com/canonical/chisel/internal/deb"
"github.com/canonical/chisel/internal/setup"
"github.com/canonical/chisel/internal/slicer"
)
Expand Down Expand Up @@ -57,6 +58,14 @@ func (cmd *cmdCut) Execute(args []string) error {
}
sliceKeys[i] = sliceKey
}
arch := cmd.Arch
if arch == "" {
darch, err := deb.InferArch()
Copy link
Contributor

@lczyk lczyk Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.. 🤔

Copy link
Author

@upils upils Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@lczyk lczyk Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return err
}
arch = darch
}

release, err := obtainRelease(cmd.Release)
if err != nil {
Expand All @@ -73,7 +82,7 @@ func (cmd *cmdCut) Execute(args []string) error {
}
}

selection, err := setup.Select(release, sliceKeys, cmd.Arch)
selection, err := setup.Select(release, sliceKeys, arch)
if err != nil {
return err
}
Expand All @@ -83,7 +92,7 @@ func (cmd *cmdCut) Execute(args []string) error {
openArchive, err := archive.Open(&archive.Options{
Label: archiveName,
Version: archiveInfo.Version,
Arch: cmd.Arch,
Arch: arch,
Suites: archiveInfo.Suites,
Components: archiveInfo.Components,
Pro: archiveInfo.Pro,
Expand Down
6 changes: 5 additions & 1 deletion internal/setup/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ func order(pkgs map[string]*Package, keys []SliceKey, arch string) ([]SliceKey,
fqslice := slice.String()
predecessors := successors[fqslice]
for req, info := range slice.Essential {
if len(info.Arch) > 0 && !slices.Contains(info.Arch, arch) {
if len(info.Arch) > 0 && arch != "" && !slices.Contains(info.Arch, arch) {
continue
}
fqreq := req.String()
Expand Down Expand Up @@ -465,6 +465,10 @@ func stripBase(baseDir, path string) string {
func Select(release *Release, slices []SliceKey, arch string) (*Selection, error) {
logf("Selecting slices...")

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

@niemeyer niemeyer Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@letFunny letFunny Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@niemeyer niemeyer Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@upils upils Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@lczyk lczyk Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

selection := &Selection{
Release: release,
}
Expand Down
45 changes: 44 additions & 1 deletion internal/setup/setup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3548,6 +3548,44 @@ var setupTests = []setupTest{{
`,
},
relerror: `package "mypkg" repeats mypkg_myslice2 in essential fields`,
}, {
summary: "Cycles are detected with 'v3-essential'",
input: map[string]string{
"slices/mydir/mypkg1.yaml": `
package: mypkg1
slices:
myslice:
v3-essential:
mypkg2_myslice: {arch: [amd64, i386]}
`,
"slices/mydir/mypkg2.yaml": `
package: mypkg2
slices:
myslice:
v3-essential:
mypkg1_myslice: {arch: [amd64, i386]}
`,
},
relerror: "essential loop detected: mypkg1_myslice, mypkg2_myslice",
}, {
summary: "Cycles are detected with 'v3-essential' without architecture",
input: map[string]string{
"slices/mydir/mypkg1.yaml": `
package: mypkg1
slices:
myslice:
v3-essential:
mypkg2_myslice:
`,
"slices/mydir/mypkg2.yaml": `
package: mypkg2
slices:
myslice:
v3-essential:
mypkg1_myslice: {arch: [amd64, i386]}
`,
},
relerror: "essential loop detected: mypkg1_myslice, mypkg2_myslice",
}}

func (s *S) TestParseRelease(c *C) {
Expand Down Expand Up @@ -3625,7 +3663,7 @@ func runParseReleaseTests(c *C, tests []setupTest) {
}

if test.selslices != nil {
selection, err := setup.Select(release, test.selslices, "")
selection, err := setup.Select(release, test.selslices, "amd64")
Copy link
Contributor

@lczyk lczyk Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ??

Copy link
Author

@upils upils Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if test.selerror != "" {
c.Assert(err, ErrorMatches, test.selerror)
continue
Expand Down Expand Up @@ -3850,3 +3888,8 @@ func (s *S) TestYAMLPathGenerate(c *C) {
c.Assert(result, Equals, test.result)
}
}

func (s *S) TestSelectEmptyArch(c *C) {
_, err := setup.Select(nil, nil, "")
c.Assert(err, ErrorMatches, `cannot select slices: "arch" is unset`)
}
4 changes: 4 additions & 0 deletions internal/slicer/slicer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2078,6 +2078,10 @@ func runSlicerTests(s *S, c *C, tests []slicerTest) {
Slice: "manifest",
})

if test.arch == "" {
test.arch = "amd64"
Copy link
Contributor

@lczyk lczyk Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see this comment

}

selection, err := setup.Select(release, testSlices, test.arch)
c.Assert(err, IsNil)

Expand Down
Loading