Skip to content
Draft
Show file tree
Hide file tree
Changes from 73 commits
Commits
Show all changes
95 commits
Select commit Hold shift + click to select a range
491cd0c
feat: search, load and validates manifests
upils Nov 26, 2025
a6e57f7
feat: adapt to revised strategy
upils Jan 29, 2026
5da0450
feat: implement upgrade
upils Jan 30, 2026
5703751
fix: deletion
upils Jan 30, 2026
26bb2b3
fix: revert inadvertent change
upils Jan 30, 2026
4a14230
fix: check slice collecting error
upils Jan 30, 2026
865c1b8
fix: simplify upgrade and improve deletion
upils Jan 30, 2026
0343ddc
refactor: cleaning
upils Jan 30, 2026
e116424
refactor: refine upgrade
upils Feb 2, 2026
5b9d408
refactor: fsutil handles moving and removing
upils Feb 3, 2026
7861c8d
refactor: improve consistency
upils Feb 3, 2026
27a0d22
style: fix lint error
upils Feb 3, 2026
5f377b0
tests: Move and Remove
upils Feb 3, 2026
f74265c
tests: FindPathsInRelease
upils Feb 3, 2026
799373c
tests: more test cases for Move
upils Feb 3, 2026
dbe3f30
tests: add spread test
upils Feb 3, 2026
becfe42
tests: fix recut spread test
upils Feb 4, 2026
3c994b8
tests: SelectValidManifest
upils Feb 4, 2026
6ebb893
fix: add missing deps
upils Feb 4, 2026
ceac217
tests: recut feature
upils Feb 4, 2026
7e34e32
fix: avoid duplicates in FindPathsInRelease
upils Feb 4, 2026
e1d4e17
ci: rerun
upils Feb 4, 2026
c3db9d4
fix: apply PR suggestions
upils Feb 5, 2026
e1e9c43
refactor: simplify FindPaths and FindPathsInRelease
upils Feb 5, 2026
da567ba
style: lint
upils Feb 5, 2026
2a6c891
fix: remove outdated comment
upils Feb 5, 2026
398e032
test: fix inaccurate tests
upils Feb 5, 2026
7c17f18
refactor: apply PR suggestions
upils Feb 6, 2026
7caf3ba
refactor: revert error message change
upils Feb 6, 2026
a259d1d
fix: gate recut feature behind env var
upils Feb 9, 2026
fad2cb7
refactor: split install from Run
upils Feb 9, 2026
d095953
tests: simplify recut spread test
upils Feb 9, 2026
d0b70cd
tests: rework recut spread tests
upils Feb 9, 2026
83e7af6
refactor: revert now useless changes
upils Feb 9, 2026
d217c43
refactor: simplify
upils Feb 9, 2026
8a7066b
refactor: more cleaning
upils Feb 9, 2026
87d1496
test: wip rework uprade tests
upils Feb 10, 2026
f8e2bd6
fix: ignore unknown manifest schema error
upils Feb 11, 2026
6dd701c
feat: improve upgrade and add tests
upils Feb 11, 2026
8523aa5
tests: simplify TestRunRecut
upils Feb 11, 2026
2ceb21a
fix: rename recut env var
upils Feb 12, 2026
10a1744
docs: add TODO on other file creation bug
upils Feb 12, 2026
6cf317c
refactor: simplify targetDir handling in Run
upils Feb 12, 2026
0d1103b
fix: improve workdir name
upils Feb 12, 2026
78e3f51
fix: clarify intent to remove existing content
upils Feb 12, 2026
d14c056
fix: apply various PR suggestions
upils Feb 13, 2026
4217131
fix: apply PR suggestions
upils Feb 13, 2026
a902099
fix: apply PR suggestions
upils Feb 13, 2026
08b3edd
test: improve regex precision
upils Feb 13, 2026
cf90f0d
refactor: extract and simplify mkParentAll
upils Feb 13, 2026
46f872c
fix: Error when only invalid manifests found
upils Feb 13, 2026
2052ee5
fix: lint
upils Feb 13, 2026
0fbc47a
fix: wrap add context on upgrade errors
upils Feb 13, 2026
30f1c32
style: improve error message
upils Feb 23, 2026
a675ad8
tests: previous package is in the new manifest
upils Feb 23, 2026
558b827
fix: properly replicate parent dirs when upgrading
upils Feb 25, 2026
ab11797
tests: Verify packages in manifest after recut
upils Feb 25, 2026
9ff9bcc
tests: check exactly the package list
upils Feb 25, 2026
f1324a8
fix: nitpicks and typos
upils Feb 26, 2026
78efc45
docs: Expose reasoning behind complex dir creation
upils Feb 26, 2026
b8d695f
refactor: clarify test intent
upils Feb 26, 2026
b811661
tests: test equivalence between a single and 2 cuts
upils Feb 26, 2026
acebd11
docs: outlines important aspects of the overall logic
upils Feb 27, 2026
6e6b09e
tests: simplify TestSelectValidManifest
upils Feb 27, 2026
76483c5
docs: Clarify intent on testing parent dir creation
upils Feb 27, 2026
fb41a11
fix: apply PR suggestions
upils Mar 3, 2026
9e87404
fix: Single predictable working dir name
upils Mar 9, 2026
fea8434
style: lint
upils Mar 9, 2026
aacc37e
feat: Reserve workdir and ban it from the release
upils Mar 9, 2026
cf9aa78
fix: move globs under subdirs
upils Mar 9, 2026
25be459
Revert "fix: Single predictable working dir name"
upils Mar 11, 2026
882c557
docs: Outline limits of workdir collisions handling
upils Mar 11, 2026
1ec1b4a
docs: clarify comment on collision with workdir
upils Mar 11, 2026
bdbba3c
style: fix typo in comment
upils Mar 23, 2026
b12185b
fix: properly return manifest
upils Mar 23, 2026
17d7935
Merge branch 'canonical:main' into recut
upils Mar 31, 2026
0c9b7bd
fix: track manifest path with bools
upils Mar 31, 2026
c5fa3b8
style: rename previous manifest
upils Mar 31, 2026
059422c
fix: simplify usage of contentHash
upils Mar 31, 2026
4f868da
fix: custom error type for unknown manifest schema
upils Mar 31, 2026
b44ef37
style: respect naming convention on manifest vars
upils Apr 1, 2026
85ea2b6
feat: stricter manifest path validation
upils Apr 1, 2026
91780f4
fix: allocate missingPaths slice at once
upils Apr 1, 2026
7bd6655
feat: log when keeping non empty dir
upils Apr 1, 2026
908b49b
fix: always return an error or non-nil valid manifest
upils Apr 1, 2026
e68f1f6
style: formating
upils Apr 1, 2026
3f19c2c
refactor: custom error type for no manifest errors
upils Apr 1, 2026
59a4d7f
docs: improve comment on SelectValidManifest
upils Apr 1, 2026
d1c3919
style: be more consistent with existing terminology
upils Apr 2, 2026
ac1be1b
fix: harmonize error message when applying recut
upils Apr 2, 2026
5cda705
fix: simplify contentHash
upils Apr 2, 2026
7dd133d
Revert "fix: simplify contentHash"
upils Apr 2, 2026
0e892f7
fix: rework strategy to apply recut
upils Apr 3, 2026
18a7b22
feat: refine upgrade process
upils Apr 3, 2026
4550747
fix: lint
upils Apr 3, 2026
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
24 changes: 24 additions & 0 deletions cmd/chisel/cmd_cut.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"fmt"
"os"
"slices"
"time"

Expand All @@ -11,6 +12,7 @@ import (
"github.com/canonical/chisel/internal/cache"
"github.com/canonical/chisel/internal/setup"
"github.com/canonical/chisel/internal/slicer"
"github.com/canonical/chisel/public/manifest"
)

var shortCutHelp = "Cut a tree with selected slices"
Expand Down Expand Up @@ -73,6 +75,27 @@ func (cmd *cmdCut) Execute(args []string) error {
}
}

var mfest *manifest.Manifest
// TODO: Remove this gating once the final upgrading strategy is in place.
if os.Getenv("CHISEL_RECUT_EXPERIMENTAL") != "" {
mfest, err := slicer.SelectValidManifest(cmd.RootDir, release)
if err != nil {
return err
}
if mfest != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This must never ever happen if err != nil. This is an important invariant to keep in mind in every Go project we work on. If we skip this rule, we're straight into billion Euro mistake territory. If you asked to select a valid manifest, and it did not return an error, it must have selected a valid manifest!

Let's please fix the function implementation and drop this verification from every call site.

cc @letFunny

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I completely agree. I brought the same point in the past out of muscle memory but I couldn't find any document to justify it properly. Maybe we should include it in the go style document that Ed wrote here.

@upils upils Apr 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I reworked it and added a custom error type. In cmd_cut.go the error message is not used but I wonder if it should be logged or not. If we are not planning on doing anything with these messages the implementation could be simplified by removing the message field or even by replacing the custom type by a named error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As a rule of thumb, if we don't have a use case for the error type we don't introduce the error type, but I'm not sure I understand what you're asking, as it sounds like you're both saying "I added a custom error type" and "I don't need to add an error type".

err = mfest.IterateSlices("", func(slice *manifest.Slice) error {
sk, err := setup.ParseSliceKey(slice.Name)
if err != nil {
return err
}
sliceKeys = append(sliceKeys, sk)
return nil
})
if err != nil {
return err
}
}
}
selection, err := setup.Select(release, sliceKeys, cmd.Arch)
if err != nil {
return err
Expand Down Expand Up @@ -125,6 +148,7 @@ func (cmd *cmdCut) Execute(args []string) error {
Selection: selection,
Archives: archives,
TargetDir: cmd.RootDir,
Manifest: mfest,
})
return err
}
7 changes: 7 additions & 0 deletions internal/fsutil/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,10 @@ func createDir(o *CreateOptions) error {
}
err = os.Mkdir(path, o.Mode)
if os.IsExist(err) {
// TODO: Detect if existing content is a file. ErrExist is also returned
// if a file exists at this path, so returning nil here creates an
// inconsistency between our view of the content and the real content on
// disk which is a bug that must be fixed.
return nil
}
return err
Expand All @@ -179,6 +183,9 @@ func createFile(o *CreateOptions) error {
if err != nil {
return err
}
// TODO: Detect if existing content is a symlink and remove it if so. The
// current implementation resolves the symlink and will override the target
// and not the symlink itself which is a bug.
file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, o.Mode)
if err != nil {
return err
Expand Down
19 changes: 19 additions & 0 deletions internal/manifestutil/manifestutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"io"
"io/fs"
"maps"
"path/filepath"
"slices"
"sort"
Expand Down Expand Up @@ -34,6 +35,24 @@ func FindPaths(slices []*setup.Slice) map[string][]*setup.Slice {
return manifestSlices
}

// FindPathsInRelease finds all the paths marked with "generate:manifest"
// for the given release.
func FindPathsInRelease(r *setup.Release) []string {
manifestPaths := make(map[string]struct{})
for _, pkg := range r.Packages {
for _, slice := range pkg.Slices {
for path, info := range slice.Contents {
if info.Generate == setup.GenerateManifest {
dir := strings.TrimSuffix(path, "**")
path = filepath.Join(dir, DefaultFilename)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is too loose. Imagine what happens if path is "/hmm/oops**", for instance, or just "/oops".

We need to be strict about how a manifest path looks like. Hopefully this is already the case elsewhere so there's already a pattern to follow. If not, please let me know.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Following our discussion offline, see 85ea2b6. The validation function is heavily inspired from validateGeneratePath in internal/setup/yaml.go.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please outline what was the outcome here? Many comments here saying "See XYZ.", but it's not practical to go to several different links trying to find out how you handled the issue that was outlined here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I added a validateManifestPath function verifying that the received path matches our definition of a valid manifest path before doing any manipulation on it. This function is now used in FindPaths and FindPathsInRelease. I also added a couple of tests to make sure the issue you pointed out was handled properly.

manifestPaths[path] = struct{}{}
Comment thread
upils marked this conversation as resolved.
Outdated
}
}
}
}
return slices.Sorted(maps.Keys(manifestPaths))
}

type WriteOptions struct {
PackageInfo []*archive.PackageInfo
Selection []*setup.Slice
Expand Down
111 changes: 111 additions & 0 deletions internal/manifestutil/manifestutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,121 @@ func (s *S) TestFindPaths(c *C) {
}
}

var findPathsInReleaseTests = []struct {
Comment thread
upils marked this conversation as resolved.
summary string
release *setup.Release
expected []string
}{{
summary: "Single package with single slice",
release: &setup.Release{
Packages: map[string]*setup.Package{
"package1": {
Name: "package1",
Slices: map[string]*setup.Slice{
"slice1": {
Name: "slice1",
Contents: map[string]setup.PathInfo{
"/folder/**": {
Kind: "generate",
Generate: "manifest",
},
},
},
},
},
},
},
expected: []string{"/folder/manifest.wall"},
}, {
summary: "No slices with generate:manifest",
release: &setup.Release{
Packages: map[string]*setup.Package{
"package1": {
Name: "package1",
Slices: map[string]*setup.Slice{
"slice1": {
Name: "slice1",
Contents: map[string]setup.PathInfo{},
},
},
},
},
},
Comment thread
upils marked this conversation as resolved.
expected: nil,
}, {
summary: "Multiple packages with multiple slices",
release: &setup.Release{
Packages: map[string]*setup.Package{
"package1": {
Name: "package1",
Slices: map[string]*setup.Slice{
"slice1": {
Name: "slice1",
Contents: map[string]setup.PathInfo{
"/folder/**": {
Kind: "generate",
Generate: "manifest",
},
},
},
"slice2": {
Name: "slice2",
Contents: map[string]setup.PathInfo{
"/folder/**": {
Kind: "generate",
Generate: "manifest",
},
},
},
},
},
"package2": {
Name: "package2",
Slices: map[string]*setup.Slice{
"slice3": {
Name: "slice3",
Contents: map[string]setup.PathInfo{},
},
"slice4": {
Name: "slice4",
Contents: map[string]setup.PathInfo{
"/other-folder/**": {
Kind: "generate",
Generate: "manifest",
},
},
},
},
},
},
},
expected: []string{"/folder/manifest.wall", "/other-folder/manifest.wall"},
}, {
summary: "Empty release",
release: &setup.Release{
Packages: map[string]*setup.Package{},
},
Comment thread
upils marked this conversation as resolved.
expected: nil,
}}

func (s *S) TestFindPathsInRelease(c *C) {
for _, test := range findPathsInReleaseTests {
c.Logf("Summary: %s", test.summary)

manifestPaths := manifestutil.FindPathsInRelease(test.release)

c.Assert(manifestPaths, HasLen, len(test.expected))
slices.Sort(manifestPaths)
slices.Sort(test.expected)
c.Assert(manifestPaths, DeepEquals, test.expected)
}
}

var slice1 = &setup.Slice{
Package: "package1",
Name: "slice1",
}

var slice2 = &setup.Slice{
Package: "package2",
Name: "slice2",
Expand Down
Loading
Loading