From a8e7ee5f9e2fe61159c4d207864b261748bceda5 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Mon, 20 Apr 2026 15:18:36 +0200 Subject: [PATCH 1/9] feat: split from branch --- cmd/chisel/cmd_cut.go | 7 +- internal/fsutil/create.go | 7 + internal/slicer/slicer.go | 272 +++++++++++++++++++-- internal/slicer/slicer_test.go | 419 +++++++++++++++++++++++++++++++++ 4 files changed, 684 insertions(+), 21 deletions(-) diff --git a/cmd/chisel/cmd_cut.go b/cmd/chisel/cmd_cut.go index 104c3b84..8cc75b22 100644 --- a/cmd/chisel/cmd_cut.go +++ b/cmd/chisel/cmd_cut.go @@ -155,9 +155,10 @@ func (cmd *cmdCut) Execute(args []string) error { } err = slicer.Run(&slicer.RunOptions{ - Selection: selection, - Archives: archives, - TargetDir: targetDir, + Selection: selection, + Archives: archives, + TargetDir: targetDir, + PreviousManifest: mfest, }) return err } diff --git a/internal/fsutil/create.go b/internal/fsutil/create.go index 0503c96f..08e58828 100644 --- a/internal/fsutil/create.go +++ b/internal/fsutil/create.go @@ -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 @@ -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 diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index 24c17bf7..268b3e5b 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "io/fs" + "maps" "os" "path/filepath" "slices" @@ -30,9 +31,10 @@ import ( const manifestMode fs.FileMode = 0644 type RunOptions struct { - Selection *setup.Selection - Archives map[string]archive.Archive - TargetDir string + Selection *setup.Selection + Archives map[string]archive.Archive + TargetDir string + PreviousManifest *manifest.Manifest } type pathData struct { @@ -90,14 +92,50 @@ func Run(options *RunOptions) error { return fmt.Errorf("internal error: cannot use a relative target directory %s", targetDir) } - pkgArchive, err := selectPkgArchives(options.Archives, options.Selection) + optsCopy := *options + cutOpts := &optsCopy + cutOpts.TargetDir = targetDir + if options.PreviousManifest != nil { + // There is a very unlikely case that a package contains a file or directory + // with the same name as the one generated by os.MkdirTemp. That would lead + // to a collision when upgrading content from the workdir to the target + // directory. + // TODO: Use a predetermined name and reserve it. The release validation + // would ensure it cannot be used in chisel-releases. + tmpWorkDir, err := os.MkdirTemp(targetDir, "chisel-workdir-*") + if err != nil { + return fmt.Errorf("cannot create temporary working directory: %w", err) + } + cutOpts.TargetDir = tmpWorkDir + defer func() { + os.RemoveAll(tmpWorkDir) + }() + } + + report, err := cut(cutOpts) if err != nil { return err } + if options.PreviousManifest != nil { + err = applyRecut(targetDir, cutOpts.TargetDir, report, options.PreviousManifest) + if err != nil { + return err + } + } + + return nil +} + +func cut(options *RunOptions) (*manifestutil.Report, error) { + pkgArchive, err := selectPkgArchives(options.Archives, options.Selection) + if err != nil { + return nil, err + } + prefers, err := options.Selection.Prefers() if err != nil { - return err + return nil, err } // Build information to process the selection. @@ -154,7 +192,7 @@ func Run(options *RunOptions) error { } reader, info, err := pkgArchive[slice.Package].Fetch(slice.Package) if err != nil { - return err + return nil, err } defer reader.Close() packages[slice.Package] = reader @@ -165,9 +203,9 @@ func Run(options *RunOptions) error { // listed as until: mutate in all the slices that reference them. knownPaths := map[string]pathData{} addKnownPath(knownPaths, "/", pathData{}) - report, err := manifestutil.NewReport(targetDir) + report, err := manifestutil.NewReport(options.TargetDir) if err != nil { - return fmt.Errorf("internal error: cannot create report: %w", err) + return nil, fmt.Errorf("internal error: cannot create report: %w", err) } // Record directories which are created but where not listed in the slice @@ -183,7 +221,7 @@ func Run(options *RunOptions) error { return err } - relPath := filepath.Clean("/" + strings.TrimPrefix(o.Path, targetDir)) + relPath := filepath.Clean("/" + strings.TrimPrefix(o.Path, options.TargetDir)) if o.Mode.IsDir() { relPath = relPath + "/" } @@ -241,13 +279,13 @@ func Run(options *RunOptions) error { err := deb.Extract(reader, &deb.ExtractOptions{ Package: slice.Package, Extract: extract[slice.Package], - TargetDir: targetDir, + TargetDir: options.TargetDir, Create: create, }) reader.Close() packages[slice.Package] = nil if err != nil { - return err + return nil, err } } @@ -303,9 +341,9 @@ func Run(options *RunOptions) error { mutable: pathInfo.Mutable, } addKnownPath(knownPaths, relPath, data) - entry, err := createFile(targetDir, relPath, pathInfo) + entry, err := createFile(options.TargetDir, relPath, pathInfo) if err != nil { - return err + return nil, err } // Do not add paths with "until: mutate". @@ -313,7 +351,7 @@ func Run(options *RunOptions) error { for _, slice := range slices { err = report.Add(slice, entry) if err != nil { - return err + return nil, err } } } @@ -323,7 +361,7 @@ func Run(options *RunOptions) error { // dependencies must run before dependents. checker := contentChecker{knownPaths} content := &scripts.ContentValue{ - RootDir: targetDir, + RootDir: options.TargetDir, CheckWrite: checker.checkMutable, CheckRead: checker.checkKnown, OnWrite: report.Mutate, @@ -338,16 +376,214 @@ func Run(options *RunOptions) error { } err := scripts.Run(&opts) if err != nil { - return fmt.Errorf("slice %s: %w", slice, err) + return nil, fmt.Errorf("slice %s: %w", slice, err) } } - err = removeAfterMutate(targetDir, knownPaths) + err = removeAfterMutate(options.TargetDir, knownPaths) + if err != nil { + return nil, err + } + + err = generateManifests(options.TargetDir, options.Selection, report, pkgInfos) + if err != nil { + return nil, err + } + + return report, nil +} + +// applyRecut applies the content from the tempDir to the targetDir. This +// process assumes the targetDir was verified with prevManifest, and so +// prevManifest is an accurate representation of files and directories +// previously installed by Chisel in the targetDir. +func applyRecut(targetDir string, tempDir string, report *manifestutil.Report, prevManifest *manifest.Manifest) error { + logf("Applying cut on %s...", targetDir) + missingPaths := make([]string, 0, len(report.Entries)) + newEntries := maps.Clone(report.Entries) + err := prevManifest.IteratePaths("", func(path *manifest.Path) error { + _, ok := report.Entries[path.Path] + if ok { + delete(newEntries, path.Path) + } else { + missingPaths = append(missingPaths, path.Path) + } + return nil + }) if err != nil { return err } - return generateManifests(targetDir, options.Selection, report, pkgInfos) + // Step 1: Verify no new path will collide with user content. + newPaths := slices.Sorted(maps.Keys(newEntries)) + for _, path := range newPaths { + dstPath := filepath.Clean(filepath.Join(targetDir, path)) + fileInfo, err := os.Lstat(dstPath) + if err != nil { + if os.IsNotExist(err) { + continue + } + return err + } + if !fileInfo.IsDir() { + return fmt.Errorf("cannot override user content: %s exists", path) + } + } + + // Step 2: Remove content removed from packages/slices. + // An entry listed in the previous manifest but missing from the new report + // means it is not part of the package/slice anymore, so remove it. + // Doing the removing before updating content prevents from collisions in the + // next step if a path changed type in the package (ex. a dir became a file). + // This works because we can differentiate files and directories in + // the manifest due to the trailing slash on directories. + // These files/directories are safe to remove because the targetDir + // verification ensured they were unmodified. + sort.Sort(sort.Reverse(sort.StringSlice(missingPaths))) + // Go through the list in reverse order to empty directories before removing + // them. Any ENOTEMPTY error encountered means user content is in the directory + // and Chisel does not manage it anymore. + for _, relPath := range missingPaths { + path := filepath.Clean(filepath.Join(targetDir, relPath)) + err = os.Remove(path) + if err != nil && !os.IsNotExist(err) { + if errors.Is(err, syscall.ENOTEMPTY) { + logf("Keep %s as not empty after package content removal", relPath) + continue + } + return err + } + } + + // Step 3: Apply tmpDir content to targetDir. + paths := slices.Sorted(maps.Keys(report.Entries)) + for _, path := range paths { + var prevEntry *manifest.Path + err := prevManifest.IteratePaths(path, func(prevPath *manifest.Path) error { + if path == prevPath.Path { + prevEntry = prevPath + } + return nil + }) + if err != nil { + return err + } + entry := report.Entries[path] + if prevEntry != nil { + entryMode := fmt.Sprintf("0%o", unixPerm(entry.Mode)) + // Skip the entry if both previous and new one are identical, except for + // manifests. No Size/SHA256/FinalSH2A56 are recorded for manifests, so make + // sure they are NOT skipped. + if prevEntry.Mode == entryMode && + int(prevEntry.Size) == entry.Size && + prevEntry.Link == entry.Link && + prevEntry.SHA256 == entry.SHA256 && + prevEntry.FinalSHA256 == entry.FinalSHA256 && + (prevEntry.Inode != 0) == (entry.Inode != 0) && + // Do not skip manifests. + (filepath.Base(prevEntry.Path) != manifestutil.DefaultFilename && + prevEntry.Size == 0 && + prevEntry.SHA256 == "") { + // The entry did not change, nothing to do. + continue + } + } + // When extracting the content, a great care is taken to create parent + // directories respecting the tarball permissions. However this approach + // can create implicit parents, not recorded in the report. Make sure + // to replicate these directories in the targetDir to sustain the same + // guarantees as a normal cut. + // Even if unlikely, this can operation can fail if a user file has the + // same path as one of the implicit parent directory replicated here. + if err := replicateParentDirs(tempDir, targetDir, path); err != nil { + return fmt.Errorf("cannot create parent directory for %q: %s", path, err) + } + // The removal done at step 2 ensures that if the destination path exists + // it is of the same type (dir or file/symlink) as the source. So any error + // here is considered a failure because it can only mean one of these things: + // - An OS error happened, we cannot do anything about it; + // - There is a collision with a directory containing user content and not + // removed at step 1. This content must not be deleted; + // - Content was modified in the rootfs between its verification and this + // step. This process does not try to solve this case. + srcPath := filepath.Clean(filepath.Join(tempDir, path)) + dstPath := filepath.Clean(filepath.Join(targetDir, path)) + switch entry.Mode & fs.ModeType { + case 0, fs.ModeSymlink: + err = os.Rename(srcPath, dstPath) + if err != nil { + err = fmt.Errorf("cannot move file at %q: %s", path, err) + } + case fs.ModeDir: + mkdirErr := os.Mkdir(dstPath, entry.Mode) + if mkdirErr != nil { + if os.IsExist(mkdirErr) { + err = os.Chmod(dstPath, entry.Mode) + } else { + err = mkdirErr + } + } + default: + err = fmt.Errorf("unsupported file type: %s", path) + } + if err != nil { + return err + } + } + return nil +} + +func unixPerm(mode fs.FileMode) (perm uint32) { + perm = uint32(mode.Perm()) + if mode&fs.ModeSticky != 0 { + perm |= 0o1000 + } + return perm +} + +// replicateParentDirs replicates the parent directories of targetPath in dstRoot. +// Fails if any non-directory is on the way. +func replicateParentDirs(srcRoot string, dstRoot string, targetPath string) error { + parents := parentDirs(targetPath) + for _, path := range parents { + if path == "/" { + continue + } + srcPath := filepath.Clean(filepath.Join(srcRoot, path)) + srcInfo, err := os.Stat(srcPath) + if err != nil { + return err + } + dstPath := filepath.Clean(filepath.Join(dstRoot, path)) + err = os.Mkdir(dstPath, srcInfo.Mode()) + if err != nil { + if !os.IsExist(err) { + return err + } + fileinfo, err := os.Lstat(dstPath) + if err != nil { + return err + } + if !fileinfo.IsDir() { + return fmt.Errorf("cannot create directory, found a non-directory at %s", dstPath) + } + return os.Chmod(dstPath, srcInfo.Mode()) + } + } + return nil +} + +func parentDirs(path string) []string { + path = filepath.Clean(path) + parents := make([]string, strings.Count(path, "/")) + count := 0 + for i, c := range path { + if c == '/' { + parents[count] = path[:i+1] + count++ + } + } + return parents } func generateManifests(targetDir string, selection *setup.Selection, diff --git a/internal/slicer/slicer_test.go b/internal/slicer/slicer_test.go index 3552ee4b..cd1cd68e 100644 --- a/internal/slicer/slicer_test.go +++ b/internal/slicer/slicer_test.go @@ -2167,6 +2167,425 @@ func runSlicerTests(s *S, c *C, tests []slicerTest) { } } +type slicerRecutTest struct { + summary string + arch string + release map[string]string + pkgs []*testutil.TestPackage + cutSlices []setup.SliceKey + recutSlices []setup.SliceKey + // Modifies the filesystem built after the first execution and before the + // second one. + alterFilesystem func(c *C, targetDir string) + filesystem map[string]string + manifestPaths map[string]string + manifestPkgs map[string]string + error string +} + +var slicerRecutTests = []slicerRecutTest{{ + summary: "Basic upgrade", + cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + recutSlices: []setup.SliceKey{{"test-package", "slice1"}, {"test-package", "slice2"}, {"other-package", "slice1"}}, + pkgs: []*testutil.TestPackage{{ + Name: "test-package", + Hash: "h1", + Version: "v1", + Arch: "a1", + Data: testutil.PackageData["test-package"], + }, { + Name: "other-package", + Hash: "h2", + Version: "v2", + Arch: "a2", + Data: testutil.PackageData["other-package"], + }}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + slice1: + contents: + /dir/file: + /dir/file-copy: {copy: /dir/file} + /other-dir/file: {symlink: ../dir/file} + slice2: + contents: + /dir/other-file: + /parent/permissions/file: + `, + "slices/mydir/other-package.yaml": ` + package: other-package + slices: + slice1: + contents: + /file: + `, + }, + filesystem: map[string]string{ + "/dir/": "dir 0755", + "/dir/file": "file 0644 cc55e2ec", + "/dir/file-copy": "file 0644 cc55e2ec", + "/dir/other-file": "file 0644 63d5dd49", + "/file": "file 0644 fc02ca0e", + "/other-dir/": "dir 0755", + "/other-dir/file": "symlink ../dir/file", + "/parent/": "dir 01777", // Permissions from the tarball preserved. + "/parent/permissions/": "dir 0764", // Permissions from the tarball preserved. + "/parent/permissions/file": "file 0755 722c14b3", + }, + manifestPaths: map[string]string{ + "/dir/file": "file 0644 cc55e2ec {test-package_slice1}", + "/dir/file-copy": "file 0644 cc55e2ec {test-package_slice1}", + "/dir/other-file": "file 0644 63d5dd49 {test-package_slice2}", + "/other-dir/file": "symlink ../dir/file {test-package_slice1}", + "/parent/permissions/file": "file 0755 722c14b3 {test-package_slice2}", + "/file": "file 0644 fc02ca0e {other-package_slice1}", + }, + manifestPkgs: map[string]string{ + "test-package": "test-package v1 a1 h1", + "other-package": "other-package v2 a2 h2", + }, +}, { + summary: "Upgrade removes obsolete paths when selection shrinks", + cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + recutSlices: []setup.SliceKey{{"test-package", "slice2"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + slice1: + contents: + /dir/file: + slice2: + contents: + /dir/other-file: + `, + }, + filesystem: map[string]string{ + "/dir/": "dir 0755", + "/dir/other-file": "file 0644 63d5dd49", + }, + manifestPaths: map[string]string{ + "/dir/other-file": "file 0644 63d5dd49 {test-package_slice2}", + }, +}, { + summary: "Upgrade overrides modified content and mode", + cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + recutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + slice1: + contents: + /dir/file: + `, + }, + alterFilesystem: func(c *C, targetDir string) { + modifiedPath := filepath.Join(targetDir, "dir/file") + err := os.WriteFile(modifiedPath, []byte("data2"), 0o700) + c.Assert(err, IsNil) + }, + filesystem: map[string]string{ + "/dir/": "dir 0755", + "/dir/file": "file 0644 cc55e2ec", + }, + manifestPaths: map[string]string{ + "/dir/file": "file 0644 cc55e2ec {test-package_slice1}", + }, +}, { + summary: "Upgrade keeps untracked files", + cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + recutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + slice1: + contents: + /dir/file: + `, + }, + alterFilesystem: func(c *C, targetDir string) { + err := os.Mkdir(filepath.Join(targetDir, "extra"), 0o755) + c.Assert(err, IsNil) + err = os.WriteFile(filepath.Join(targetDir, "extra", "untracked"), []byte("data"), 0o644) + c.Assert(err, IsNil) + }, + filesystem: map[string]string{ + "/extra/": "dir 0755", + "/extra/untracked": "file 0644 3a6eb079", + "/dir/": "dir 0755", + "/dir/file": "file 0644 cc55e2ec", + }, + manifestPaths: map[string]string{ + "/dir/file": "file 0644 cc55e2ec {test-package_slice1}", + }, +}, { + summary: "Upgrade overrides existing mode on directories", + cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + recutSlices: []setup.SliceKey{{"test-package", "slice1"}, {"test-package", "slice2"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + slice1: + contents: + /dir/: + slice2: + contents: + /dir/file: + /other-dir/: + `, + }, + alterFilesystem: func(c *C, targetDir string) { + err := os.Mkdir(filepath.Join(targetDir, "other-dir"), 0o777) + c.Assert(err, IsNil) + }, + filesystem: map[string]string{ + "/dir/": "dir 0755", + "/other-dir/": "dir 0755", + "/dir/file": "file 0644 cc55e2ec", + }, + manifestPaths: map[string]string{ + "/dir/": "dir 0755 {test-package_slice1}", + "/other-dir/": "dir 0755 {test-package_slice2}", + "/dir/file": "file 0644 cc55e2ec {test-package_slice2}", + }, +}, { + summary: "Upgrade fails to override existing file", + cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + recutSlices: []setup.SliceKey{{"test-package", "slice1"}, {"test-package", "slice2"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + slice1: + contents: + /dir/: + slice2: + contents: + /dir/file: + `, + }, + alterFilesystem: func(c *C, targetDir string) { + err := os.WriteFile(filepath.Join(targetDir, "dir", "file"), []byte("data"), 0o644) + c.Assert(err, IsNil) + }, + error: "cannot override user content: /dir/file exists", +}, { + summary: "Upgrade relicate implicit parent dirs", + cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + recutSlices: []setup.SliceKey{{"test-package", "slice1"}, {"test-package", "slice2"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + slice1: + contents: + /dir/file: {text: data} + slice2: + contents: + /parent/permissions/file: + `, + }, + filesystem: map[string]string{ + "/dir/": "dir 0755", + "/dir/file": "file 0644 3a6eb079", + "/parent/": "dir 01777", + "/parent/permissions/": "dir 0764", + "/parent/permissions/file": "file 0755 722c14b3", + }, + manifestPaths: map[string]string{ + "/dir/file": "file 0644 3a6eb079 {test-package_slice1}", + "/parent/permissions/file": "file 0755 722c14b3 {test-package_slice2}", + }, +}, { + summary: "Upgrade removes obsolete content but keeps non-empty directories", + cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, + recutSlices: []setup.SliceKey{{"test-package", "slice2"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + slice1: + contents: + /old-dir/: {make: true} + /link: {symlink: target} + /baz: {text: data} + /foo: {symlink: baz} + slice2: + contents: + /new-file: {text: data1} + `, + }, + alterFilesystem: func(c *C, targetDir string) { + err := os.WriteFile(filepath.Join(targetDir, "old-dir", "file"), []byte("data"), 0o644) + c.Assert(err, IsNil) + }, + filesystem: map[string]string{ + "/new-file": "file 0644 5b41362b", + "/old-dir/": "dir 0755", + "/old-dir/file": "file 0644 3a6eb079", + }, + manifestPaths: map[string]string{ + "/new-file": "file 0644 5b41362b {test-package_slice2}", + }, +}} + +func (s *S) TestRunRecut(c *C) { + for _, test := range slicerRecutTests { + c.Logf("Summary: %s", test.summary) + + if _, ok := test.release["chisel.yaml"]; !ok { + test.release["chisel.yaml"] = testutil.DefaultChiselYaml + } + if test.pkgs == nil { + test.pkgs = []*testutil.TestPackage{{ + Name: "test-package", + Data: testutil.PackageData["test-package"], + }} + } + for _, pkg := range test.pkgs { + // We need to set these fields for manifest validation. + if pkg.Arch == "" { + pkg.Arch = "arch" + } + if pkg.Hash == "" { + pkg.Hash = "hash" + } + if pkg.Version == "" { + pkg.Version = "version" + } + } + + releaseDir := c.MkDir() + for path, data := range test.release { + fpath := filepath.Join(releaseDir, path) + err := os.MkdirAll(filepath.Dir(fpath), 0o755) + c.Assert(err, IsNil) + err = os.WriteFile(fpath, testutil.Reindent(data), 0o644) + c.Assert(err, IsNil) + } + + release, err := setup.ReadRelease(releaseDir) + c.Assert(err, IsNil) + + // Create a manifest slice and add it to the selection. + manifestPackage := test.cutSlices[0].Package + manifestPath := "/chisel-data/manifest.wall" + release.Packages[manifestPackage].Slices["manifest"] = &setup.Slice{ + Package: manifestPackage, + Name: "manifest", + Essential: nil, + Contents: map[string]setup.PathInfo{ + "/chisel-data/**": { + Kind: "generate", + Generate: "manifest", + }, + }, + Scripts: setup.SliceScripts{}, + } + test.cutSlices = append(test.cutSlices, setup.SliceKey{ + Package: manifestPackage, + Slice: "manifest", + }) + + selection, err := setup.Select(release, test.cutSlices, test.arch) + c.Assert(err, IsNil) + + archives := map[string]archive.Archive{} + for name, setupArchive := range release.Archives { + pkgs := make(map[string]*testutil.TestPackage) + for _, pkg := range test.pkgs { + if len(pkg.Archives) == 0 || slices.Contains(pkg.Archives, name) { + pkgs[pkg.Name] = pkg + } + } + archive := &testutil.TestArchive{ + Opts: archive.Options{ + Label: setupArchive.Name, + Version: setupArchive.Version, + Suites: setupArchive.Suites, + Components: setupArchive.Components, + Pro: setupArchive.Pro, + Arch: test.arch, + }, + Packages: pkgs, + } + archives[name] = archive + } + + targetDir := c.MkDir() + options := slicer.RunOptions{ + Selection: selection, + Archives: archives, + TargetDir: targetDir, + } + // First run. + err = slicer.Run(&options) + c.Assert(err, IsNil) + + if test.alterFilesystem != nil { + test.alterFilesystem(c, targetDir) + } + mfest := readManifest(c, options.TargetDir, manifestPath) + + test.recutSlices = append(test.recutSlices, setup.SliceKey{ + Package: manifestPackage, + Slice: "manifest", + }) + selection, err = setup.Select(release, test.recutSlices, test.arch) + c.Assert(err, IsNil) + + options = slicer.RunOptions{ + Selection: selection, + Archives: archives, + TargetDir: targetDir, + PreviousManifest: mfest, + } + // Second run. + err = slicer.Run(&options) + if test.error != "" { + c.Assert(err, ErrorMatches, test.error) + continue + } + c.Assert(err, IsNil) + + if test.filesystem == nil && test.manifestPaths == nil && test.manifestPkgs == nil { + continue + } + mfest = readManifest(c, options.TargetDir, manifestPath) + + // Assert state of final filesystem. + if test.filesystem != nil { + filesystem := testutil.TreeDump(options.TargetDir) + c.Assert(filesystem["/chisel-data/"], Not(HasLen), 0) + c.Assert(filesystem[manifestPath], Not(HasLen), 0) + delete(filesystem, "/chisel-data/") + delete(filesystem, manifestPath) + c.Assert(filesystem, DeepEquals, test.filesystem) + } + + // Assert state of the files recorded in the manifest. + if test.manifestPaths != nil { + pathsDump, err := treeDumpManifestPaths(mfest) + c.Assert(err, IsNil) + c.Assert(pathsDump[manifestPath], Not(HasLen), 0) + delete(pathsDump, manifestPath) + c.Assert(pathsDump, DeepEquals, test.manifestPaths) + } + + // Assert state of the packages recorded in the manifest. + if test.manifestPkgs != nil { + pkgsDump, err := dumpManifestPkgs(mfest) + c.Assert(err, IsNil) + c.Assert(pkgsDump, DeepEquals, test.manifestPkgs) + } + } +} + type selectValidManifestTest struct { summary string setup func(c *C, targetDir string, release *setup.Release) From 94763fa606aff3e5ae4ae4603d93091319a236c8 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Mon, 20 Apr 2026 15:19:24 +0200 Subject: [PATCH 2/9] feat: add tests from previous branch --- .../chisel-releases/chisel.yaml | 49 +++++++++++++++++++ .../chisel-releases/slices/base-files.yaml | 13 +++++ .../chisel-releases/slices/hello.yaml | 13 +++++ tests/recut-equivalence/task.yaml | 35 +++++++++++++ tests/recut/chisel-releases/chisel.yaml | 49 +++++++++++++++++++ .../chisel-releases/slices/base-files.yaml | 13 +++++ tests/recut/chisel-releases/slices/hello.yaml | 13 +++++ tests/recut/task.yaml | 48 ++++++++++++++++++ 8 files changed, 233 insertions(+) create mode 100644 tests/recut-equivalence/chisel-releases/chisel.yaml create mode 100644 tests/recut-equivalence/chisel-releases/slices/base-files.yaml create mode 100644 tests/recut-equivalence/chisel-releases/slices/hello.yaml create mode 100644 tests/recut-equivalence/task.yaml create mode 100644 tests/recut/chisel-releases/chisel.yaml create mode 100644 tests/recut/chisel-releases/slices/base-files.yaml create mode 100644 tests/recut/chisel-releases/slices/hello.yaml create mode 100644 tests/recut/task.yaml diff --git a/tests/recut-equivalence/chisel-releases/chisel.yaml b/tests/recut-equivalence/chisel-releases/chisel.yaml new file mode 100644 index 00000000..c137ff40 --- /dev/null +++ b/tests/recut-equivalence/chisel-releases/chisel.yaml @@ -0,0 +1,49 @@ +format: v2 + + +archives: + # archive.ubuntu.com/ubuntu/ (amd64, i386) + # ports.ubuntu.com/ubuntu-ports/ (other arch) + ubuntu: + priority: 10 + version: 24.04 + components: [main, universe] + suites: [noble, noble-security, noble-updates] + public-keys: [ubuntu-archive-key-2018] + + +public-keys: + # Ubuntu Archive Automatic Signing Key (2018) + # rsa4096/f6ecb3762474eda9d21b7022871920d1991bc93c 2018-09-17T15:01:46Z + ubuntu-archive-key-2018: + id: "871920D1991BC93C" + armor: | + -----BEGIN PGP PUBLIC KEY BLOCK----- + + mQINBFufwdoBEADv/Gxytx/LcSXYuM0MwKojbBye81s0G1nEx+lz6VAUpIUZnbkq + dXBHC+dwrGS/CeeLuAjPRLU8AoxE/jjvZVp8xFGEWHYdklqXGZ/gJfP5d3fIUBtZ + HZEJl8B8m9pMHf/AQQdsC+YzizSG5t5Mhnotw044LXtdEEkx2t6Jz0OGrh+5Ioxq + X7pZiq6Cv19BohaUioKMdp7ES6RYfN7ol6HSLFlrMXtVfh/ijpN9j3ZhVGVeRC8k + KHQsJ5PkIbmvxBiUh7SJmfZUx0IQhNMaDHXfdZAGNtnhzzNReb1FqNLSVkrS/Pns + AQzMhG1BDm2VOSF64jebKXffFqM5LXRQTeqTLsjUbbrqR6s/GCO8UF7jfUj6I7ta + LygmsHO/JD4jpKRC0gbpUBfaiJyLvuepx3kWoqL3sN0LhlMI80+fA7GTvoOx4tpq + VlzlE6TajYu+jfW3QpOFS5ewEMdL26hzxsZg/geZvTbArcP+OsJKRmhv4kNo6Ayd + yHQ/3ZV/f3X9mT3/SPLbJaumkgp3Yzd6t5PeBu+ZQk/mN5WNNuaihNEV7llb1Zhv + Y0Fxu9BVd/BNl0rzuxp3rIinB2TX2SCg7wE5xXkwXuQ/2eTDE0v0HlGntkuZjGow + DZkxHZQSxZVOzdZCRVaX/WEFLpKa2AQpw5RJrQ4oZ/OfifXyJzP27o03wQARAQAB + tEJVYnVudHUgQXJjaGl2ZSBBdXRvbWF0aWMgU2lnbmluZyBLZXkgKDIwMTgpIDxm + dHBtYXN0ZXJAdWJ1bnR1LmNvbT6JAjgEEwEKACIFAlufwdoCGwMGCwkIBwMCBhUI + AgkKCwQWAgMBAh4BAheAAAoJEIcZINGZG8k8LHMQAKS2cnxz/5WaoCOWArf5g6UH + beOCgc5DBm0hCuFDZWWv427aGei3CPuLw0DGLCXZdyc5dqE8mvjMlOmmAKKlj1uG + g3TYCbQWjWPeMnBPZbkFgkZoXJ7/6CB7bWRht1sHzpt1LTZ+SYDwOwJ68QRp7DRa + Zl9Y6QiUbeuhq2DUcTofVbBxbhrckN4ZteLvm+/nG9m/ciopc66LwRdkxqfJ32Cy + q+1TS5VaIJDG7DWziG+Kbu6qCDM4QNlg3LH7p14CrRxAbc4lvohRgsV4eQqsIcdF + kuVY5HPPj2K8TqpY6STe8Gh0aprG1RV8ZKay3KSMpnyV1fAKn4fM9byiLzQAovC0 + LZ9MMMsrAS/45AvC3IEKSShjLFn1X1dRCiO6/7jmZEoZtAp53hkf8SMBsi78hVNr + BumZwfIdBA1v22+LY4xQK8q4XCoRcA9G+pvzU9YVW7cRnDZZGl0uwOw7z9PkQBF5 + KFKjWDz4fCk+K6+YtGpovGKekGBb8I7EA6UpvPgqA/QdI0t1IBP0N06RQcs1fUaA + QEtz6DGy5zkRhR4pGSZn+dFET7PdAjEK84y7BdY4t+U1jcSIvBj0F2B7LwRL7xGp + SpIKi/ekAXLs117bvFHaCvmUYN7JVp1GMmVFxhIdx6CFm3fxG8QjNb5tere/YqK+ + uOgcXny1UlwtCUzlrSaP + =9AdM + -----END PGP PUBLIC KEY BLOCK----- diff --git a/tests/recut-equivalence/chisel-releases/slices/base-files.yaml b/tests/recut-equivalence/chisel-releases/slices/base-files.yaml new file mode 100644 index 00000000..84c30016 --- /dev/null +++ b/tests/recut-equivalence/chisel-releases/slices/base-files.yaml @@ -0,0 +1,13 @@ +package: base-files +slices: + slice-a: + contents: + /etc/debian_version: + /etc/foo: + text: bar + slice-b: + contents: + /etc/issue: + manifest: + contents: + /chisel/**: {generate: manifest} diff --git a/tests/recut-equivalence/chisel-releases/slices/hello.yaml b/tests/recut-equivalence/chisel-releases/slices/hello.yaml new file mode 100644 index 00000000..7bd1a393 --- /dev/null +++ b/tests/recut-equivalence/chisel-releases/slices/hello.yaml @@ -0,0 +1,13 @@ +package: hello + +essential: + - hello_copyright + +slices: + bins: + contents: + /usr/bin/hello: + + copyright: + contents: + /usr/share/doc/hello/copyright: diff --git a/tests/recut-equivalence/task.yaml b/tests/recut-equivalence/task.yaml new file mode 100644 index 00000000..764dd9bf --- /dev/null +++ b/tests/recut-equivalence/task.yaml @@ -0,0 +1,35 @@ +summary: Recut produce the exact same result as two consecutive cuts. + +variants: + - noble + +environment: + ROOTFS: rootfs + ROOTFS_RECUT: rootfs-recut + +execute: | + mkdir -p ${ROOTFS} ${ROOTFS_RECUT} + + # TODO: remove this env var when final upgrade strategy is in place. + export CHISEL_RECUT_EXPERIMENTAL=1 + + # First cut single cut + chisel cut --release ./chisel-releases/ \ + --root ${ROOTFS} \ + hello_bins \ + base-files_manifest \ + base-files_slice-a \ + base-files_slice-b + + # Cut a rootfs twice. + chisel cut --release ./chisel-releases/ \ + --root ${ROOTFS_RECUT} \ + hello_bins \ + base-files_manifest \ + base-files_slice-a + chisel cut --release ./chisel-releases/ \ + --root ${ROOTFS_RECUT} \ + base-files_slice-b + + # Compares both rootfs. + diff -q -r ${ROOTFS} ${ROOTFS_RECUT} diff --git a/tests/recut/chisel-releases/chisel.yaml b/tests/recut/chisel-releases/chisel.yaml new file mode 100644 index 00000000..c137ff40 --- /dev/null +++ b/tests/recut/chisel-releases/chisel.yaml @@ -0,0 +1,49 @@ +format: v2 + + +archives: + # archive.ubuntu.com/ubuntu/ (amd64, i386) + # ports.ubuntu.com/ubuntu-ports/ (other arch) + ubuntu: + priority: 10 + version: 24.04 + components: [main, universe] + suites: [noble, noble-security, noble-updates] + public-keys: [ubuntu-archive-key-2018] + + +public-keys: + # Ubuntu Archive Automatic Signing Key (2018) + # rsa4096/f6ecb3762474eda9d21b7022871920d1991bc93c 2018-09-17T15:01:46Z + ubuntu-archive-key-2018: + id: "871920D1991BC93C" + armor: | + -----BEGIN PGP PUBLIC KEY BLOCK----- + + mQINBFufwdoBEADv/Gxytx/LcSXYuM0MwKojbBye81s0G1nEx+lz6VAUpIUZnbkq + dXBHC+dwrGS/CeeLuAjPRLU8AoxE/jjvZVp8xFGEWHYdklqXGZ/gJfP5d3fIUBtZ + HZEJl8B8m9pMHf/AQQdsC+YzizSG5t5Mhnotw044LXtdEEkx2t6Jz0OGrh+5Ioxq + X7pZiq6Cv19BohaUioKMdp7ES6RYfN7ol6HSLFlrMXtVfh/ijpN9j3ZhVGVeRC8k + KHQsJ5PkIbmvxBiUh7SJmfZUx0IQhNMaDHXfdZAGNtnhzzNReb1FqNLSVkrS/Pns + AQzMhG1BDm2VOSF64jebKXffFqM5LXRQTeqTLsjUbbrqR6s/GCO8UF7jfUj6I7ta + LygmsHO/JD4jpKRC0gbpUBfaiJyLvuepx3kWoqL3sN0LhlMI80+fA7GTvoOx4tpq + VlzlE6TajYu+jfW3QpOFS5ewEMdL26hzxsZg/geZvTbArcP+OsJKRmhv4kNo6Ayd + yHQ/3ZV/f3X9mT3/SPLbJaumkgp3Yzd6t5PeBu+ZQk/mN5WNNuaihNEV7llb1Zhv + Y0Fxu9BVd/BNl0rzuxp3rIinB2TX2SCg7wE5xXkwXuQ/2eTDE0v0HlGntkuZjGow + DZkxHZQSxZVOzdZCRVaX/WEFLpKa2AQpw5RJrQ4oZ/OfifXyJzP27o03wQARAQAB + tEJVYnVudHUgQXJjaGl2ZSBBdXRvbWF0aWMgU2lnbmluZyBLZXkgKDIwMTgpIDxm + dHBtYXN0ZXJAdWJ1bnR1LmNvbT6JAjgEEwEKACIFAlufwdoCGwMGCwkIBwMCBhUI + AgkKCwQWAgMBAh4BAheAAAoJEIcZINGZG8k8LHMQAKS2cnxz/5WaoCOWArf5g6UH + beOCgc5DBm0hCuFDZWWv427aGei3CPuLw0DGLCXZdyc5dqE8mvjMlOmmAKKlj1uG + g3TYCbQWjWPeMnBPZbkFgkZoXJ7/6CB7bWRht1sHzpt1LTZ+SYDwOwJ68QRp7DRa + Zl9Y6QiUbeuhq2DUcTofVbBxbhrckN4ZteLvm+/nG9m/ciopc66LwRdkxqfJ32Cy + q+1TS5VaIJDG7DWziG+Kbu6qCDM4QNlg3LH7p14CrRxAbc4lvohRgsV4eQqsIcdF + kuVY5HPPj2K8TqpY6STe8Gh0aprG1RV8ZKay3KSMpnyV1fAKn4fM9byiLzQAovC0 + LZ9MMMsrAS/45AvC3IEKSShjLFn1X1dRCiO6/7jmZEoZtAp53hkf8SMBsi78hVNr + BumZwfIdBA1v22+LY4xQK8q4XCoRcA9G+pvzU9YVW7cRnDZZGl0uwOw7z9PkQBF5 + KFKjWDz4fCk+K6+YtGpovGKekGBb8I7EA6UpvPgqA/QdI0t1IBP0N06RQcs1fUaA + QEtz6DGy5zkRhR4pGSZn+dFET7PdAjEK84y7BdY4t+U1jcSIvBj0F2B7LwRL7xGp + SpIKi/ekAXLs117bvFHaCvmUYN7JVp1GMmVFxhIdx6CFm3fxG8QjNb5tere/YqK+ + uOgcXny1UlwtCUzlrSaP + =9AdM + -----END PGP PUBLIC KEY BLOCK----- diff --git a/tests/recut/chisel-releases/slices/base-files.yaml b/tests/recut/chisel-releases/slices/base-files.yaml new file mode 100644 index 00000000..84c30016 --- /dev/null +++ b/tests/recut/chisel-releases/slices/base-files.yaml @@ -0,0 +1,13 @@ +package: base-files +slices: + slice-a: + contents: + /etc/debian_version: + /etc/foo: + text: bar + slice-b: + contents: + /etc/issue: + manifest: + contents: + /chisel/**: {generate: manifest} diff --git a/tests/recut/chisel-releases/slices/hello.yaml b/tests/recut/chisel-releases/slices/hello.yaml new file mode 100644 index 00000000..7bd1a393 --- /dev/null +++ b/tests/recut/chisel-releases/slices/hello.yaml @@ -0,0 +1,13 @@ +package: hello + +essential: + - hello_copyright + +slices: + bins: + contents: + /usr/bin/hello: + + copyright: + contents: + /usr/share/doc/hello/copyright: diff --git a/tests/recut/task.yaml b/tests/recut/task.yaml new file mode 100644 index 00000000..fc426ef5 --- /dev/null +++ b/tests/recut/task.yaml @@ -0,0 +1,48 @@ +summary: Recut relies on manifest to upgrade existing content + +variants: + - noble + +environment: + ROOTFS: rootfs + +execute: | + # Install yq. + wget -q https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64 -O /usr/bin/yq &&\ + chmod +x /usr/bin/yq + + mkdir -p ${ROOTFS} + + # TODO: remove this env var when final upgrade strategy is in place. + export CHISEL_RECUT_EXPERIMENTAL=1 + # First cut generates manifest and installs slice-a. + chisel cut --release ./chisel-releases/ \ + --root ${ROOTFS} \ + base-files_slice-a hello_bins base-files_manifest + + test -s ${ROOTFS}/etc/debian_version + test -s ${ROOTFS}/etc/foo + test -s ${ROOTFS}/usr/bin/hello + cat ${ROOTFS}/etc/foo | grep "bar" + test -f ${ROOTFS}/chisel/manifest.wall + + # Update slice-a definition. + yq -i '.slices.slice-a.contents./etc/foo.text = "qux"' ./chisel-releases/slices/base-files.yaml + + # Second cut, only requesting slice-b. + chisel cut --release ./chisel-releases/ \ + --root ${ROOTFS} \ + base-files_slice-b + + test -s ${ROOTFS}/etc/debian_version + test -s ${ROOTFS}/etc/issue + cat ${ROOTFS}/etc/foo | grep "qux" + test -f ${ROOTFS}/chisel/manifest.wall + zstd -d ${ROOTFS}/chisel/manifest.wall -o ${ROOTFS}/chisel/manifest.jsonwall + packages=$(jq -r 'select(.kind == "package") | .name' ${ROOTFS}/chisel/manifest.jsonwall) + expected=$(cat < Date: Wed, 22 Apr 2026 08:44:46 +0200 Subject: [PATCH 3/9] feat: investigate cleaner implementation --- cmd/chisel/cmd_cut.go | 1 + internal/slicer/slicer.go | 150 ++++++++++++++++++--------------- internal/slicer/slicer_test.go | 2 + 3 files changed, 85 insertions(+), 68 deletions(-) diff --git a/cmd/chisel/cmd_cut.go b/cmd/chisel/cmd_cut.go index 8cc75b22..634eea0a 100644 --- a/cmd/chisel/cmd_cut.go +++ b/cmd/chisel/cmd_cut.go @@ -159,6 +159,7 @@ func (cmd *cmdCut) Execute(args []string) error { Archives: archives, TargetDir: targetDir, PreviousManifest: mfest, + Release: release, }) return err } diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index 268b3e5b..b825a778 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -14,6 +14,7 @@ import ( "path/filepath" "slices" "sort" + "strconv" "strings" "syscall" @@ -35,6 +36,7 @@ type RunOptions struct { Archives map[string]archive.Archive TargetDir string PreviousManifest *manifest.Manifest + Release *setup.Release } type pathData struct { @@ -112,30 +114,31 @@ func Run(options *RunOptions) error { }() } - report, err := cut(cutOpts) + err := cut(cutOpts) if err != nil { return err } if options.PreviousManifest != nil { - err = applyRecut(targetDir, cutOpts.TargetDir, report, options.PreviousManifest) - if err != nil { - return err - } + return merge(targetDir, cutOpts.TargetDir, options.PreviousManifest, options.Release) } return nil } -func cut(options *RunOptions) (*manifestutil.Report, error) { +func cut(options *RunOptions) error { + if !filepath.IsAbs(options.TargetDir) { + return fmt.Errorf("internal error: cannot operate on a relative target directory") + } + pkgArchive, err := selectPkgArchives(options.Archives, options.Selection) if err != nil { - return nil, err + return err } prefers, err := options.Selection.Prefers() if err != nil { - return nil, err + return err } // Build information to process the selection. @@ -192,7 +195,7 @@ func cut(options *RunOptions) (*manifestutil.Report, error) { } reader, info, err := pkgArchive[slice.Package].Fetch(slice.Package) if err != nil { - return nil, err + return err } defer reader.Close() packages[slice.Package] = reader @@ -205,7 +208,7 @@ func cut(options *RunOptions) (*manifestutil.Report, error) { addKnownPath(knownPaths, "/", pathData{}) report, err := manifestutil.NewReport(options.TargetDir) if err != nil { - return nil, fmt.Errorf("internal error: cannot create report: %w", err) + return fmt.Errorf("internal error: cannot create report: %w", err) } // Record directories which are created but where not listed in the slice @@ -285,7 +288,7 @@ func cut(options *RunOptions) (*manifestutil.Report, error) { reader.Close() packages[slice.Package] = nil if err != nil { - return nil, err + return err } } @@ -343,7 +346,7 @@ func cut(options *RunOptions) (*manifestutil.Report, error) { addKnownPath(knownPaths, relPath, data) entry, err := createFile(options.TargetDir, relPath, pathInfo) if err != nil { - return nil, err + return err } // Do not add paths with "until: mutate". @@ -351,7 +354,7 @@ func cut(options *RunOptions) (*manifestutil.Report, error) { for _, slice := range slices { err = report.Add(slice, entry) if err != nil { - return nil, err + return err } } } @@ -376,33 +379,49 @@ func cut(options *RunOptions) (*manifestutil.Report, error) { } err := scripts.Run(&opts) if err != nil { - return nil, fmt.Errorf("slice %s: %w", slice, err) + return fmt.Errorf("slice %s: %w", slice, err) } } err = removeAfterMutate(options.TargetDir, knownPaths) if err != nil { - return nil, err - } - - err = generateManifests(options.TargetDir, options.Selection, report, pkgInfos) - if err != nil { - return nil, err + return err } - return report, nil + return generateManifests(options.TargetDir, options.Selection, report, pkgInfos) } -// applyRecut applies the content from the tempDir to the targetDir. This +// merge applies the content from the tempDir to the targetDir. This // process assumes the targetDir was verified with prevManifest, and so // prevManifest is an accurate representation of files and directories // previously installed by Chisel in the targetDir. -func applyRecut(targetDir string, tempDir string, report *manifestutil.Report, prevManifest *manifest.Manifest) error { - logf("Applying cut on %s...", targetDir) - missingPaths := make([]string, 0, len(report.Entries)) - newEntries := maps.Clone(report.Entries) - err := prevManifest.IteratePaths("", func(path *manifest.Path) error { - _, ok := report.Entries[path.Path] +func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, release *setup.Release) error { + logf("Merging cut in %s...", targetDir) + if !filepath.IsAbs(targetDir) { + return fmt.Errorf("internal error: cannot operate on a relative target directory %s", targetDir) + } + if !filepath.IsAbs(tempDir) { + return fmt.Errorf("internal error: cannot operate on a relative working directory %s", tempDir) + } + + newManifest, err := SelectValidManifest(tempDir, release) + if err != nil { + return fmt.Errorf("internal error: cannot select manifest from working directory: %s", err) + } + + // Step 1: Identify new and missing entries. + entries := make(map[string]*manifest.Path) + err = newManifest.IteratePaths("", func(path *manifest.Path) error { + entries[path.Path] = path + return nil + }) + if err != nil { + return err + } + missingPaths := make([]string, 0, len(entries)) + newEntries := maps.Clone(entries) + err = prevManifest.IteratePaths("", func(path *manifest.Path) error { + _, ok := entries[path.Path] if ok { delete(newEntries, path.Path) } else { @@ -414,11 +433,12 @@ func applyRecut(targetDir string, tempDir string, report *manifestutil.Report, p return err } - // Step 1: Verify no new path will collide with user content. + // Step 2: Verify no new path will collide with user content. + // Existing directories are accepted. newPaths := slices.Sorted(maps.Keys(newEntries)) for _, path := range newPaths { - dstPath := filepath.Clean(filepath.Join(targetDir, path)) - fileInfo, err := os.Lstat(dstPath) + absPath := filepath.Clean(filepath.Join(targetDir, path)) + fileInfo, err := os.Lstat(absPath) if err != nil { if os.IsNotExist(err) { continue @@ -430,8 +450,8 @@ func applyRecut(targetDir string, tempDir string, report *manifestutil.Report, p } } - // Step 2: Remove content removed from packages/slices. - // An entry listed in the previous manifest but missing from the new report + // Step 3: Remove content removed from packages/slices. + // An entry listed in the previous manifest but missing from the new one // means it is not part of the package/slice anymore, so remove it. // Doing the removing before updating content prevents from collisions in the // next step if a path changed type in the package (ex. a dir became a file). @@ -439,24 +459,25 @@ func applyRecut(targetDir string, tempDir string, report *manifestutil.Report, p // the manifest due to the trailing slash on directories. // These files/directories are safe to remove because the targetDir // verification ensured they were unmodified. - sort.Sort(sort.Reverse(sort.StringSlice(missingPaths))) + slices.Sort(missingPaths) + slices.Reverse(missingPaths) // Go through the list in reverse order to empty directories before removing // them. Any ENOTEMPTY error encountered means user content is in the directory // and Chisel does not manage it anymore. - for _, relPath := range missingPaths { - path := filepath.Clean(filepath.Join(targetDir, relPath)) - err = os.Remove(path) + for _, path := range missingPaths { + absPath := filepath.Clean(filepath.Join(targetDir, path)) + err = os.Remove(absPath) if err != nil && !os.IsNotExist(err) { if errors.Is(err, syscall.ENOTEMPTY) { - logf("Keep %s as not empty after package content removal", relPath) + logf("Keep %s as not empty after package content removal", path) continue } return err } } - // Step 3: Apply tmpDir content to targetDir. - paths := slices.Sorted(maps.Keys(report.Entries)) + // Step 4: Apply tempDir content to targetDir. + paths := slices.Sorted(maps.Keys(entries)) for _, path := range paths { var prevEntry *manifest.Path err := prevManifest.IteratePaths(path, func(prevPath *manifest.Path) error { @@ -468,14 +489,13 @@ func applyRecut(targetDir string, tempDir string, report *manifestutil.Report, p if err != nil { return err } - entry := report.Entries[path] + entry := entries[path] if prevEntry != nil { - entryMode := fmt.Sprintf("0%o", unixPerm(entry.Mode)) // Skip the entry if both previous and new one are identical, except for // manifests. No Size/SHA256/FinalSH2A56 are recorded for manifests, so make // sure they are NOT skipped. - if prevEntry.Mode == entryMode && - int(prevEntry.Size) == entry.Size && + if prevEntry.Mode == entry.Mode && + prevEntry.Size == entry.Size && prevEntry.Link == entry.Link && prevEntry.SHA256 == entry.SHA256 && prevEntry.FinalSHA256 == entry.FinalSHA256 && @@ -490,57 +510,51 @@ func applyRecut(targetDir string, tempDir string, report *manifestutil.Report, p } // When extracting the content, a great care is taken to create parent // directories respecting the tarball permissions. However this approach - // can create implicit parents, not recorded in the report. Make sure + // can create implicit parents, not recorded in the manifest. Make sure // to replicate these directories in the targetDir to sustain the same // guarantees as a normal cut. - // Even if unlikely, this can operation can fail if a user file has the + // Even if unlikely, this operation can fail if a user file has the // same path as one of the implicit parent directory replicated here. if err := replicateParentDirs(tempDir, targetDir, path); err != nil { return fmt.Errorf("cannot create parent directory for %q: %s", path, err) } - // The removal done at step 2 ensures that if the destination path exists + // The removal done at step 3 ensures that if the destination path exists // it is of the same type (dir or file/symlink) as the source. So any error // here is considered a failure because it can only mean one of these things: // - An OS error happened, we cannot do anything about it; // - There is a collision with a directory containing user content and not - // removed at step 1. This content must not be deleted; + // removed at step 3. This content must not be deleted; // - Content was modified in the rootfs between its verification and this // step. This process does not try to solve this case. srcPath := filepath.Clean(filepath.Join(tempDir, path)) dstPath := filepath.Clean(filepath.Join(targetDir, path)) - switch entry.Mode & fs.ModeType { - case 0, fs.ModeSymlink: - err = os.Rename(srcPath, dstPath) + if strings.HasSuffix(path, "/") { + permissions, err := strconv.ParseUint(entry.Mode, 8, 32) if err != nil { - err = fmt.Errorf("cannot move file at %q: %s", path, err) + return fmt.Errorf("cannot parse mode %q: %w", entry.Mode, err) } - case fs.ModeDir: - mkdirErr := os.Mkdir(dstPath, entry.Mode) + mode := fs.FileMode(permissions) + mkdirErr := os.Mkdir(dstPath, mode) if mkdirErr != nil { if os.IsExist(mkdirErr) { - err = os.Chmod(dstPath, entry.Mode) + err = os.Chmod(dstPath, mode) } else { err = mkdirErr } + if err != nil { + return err + } + } + } else { + err = os.Rename(srcPath, dstPath) + if err != nil { + return fmt.Errorf("cannot move file at %q: %s", path, err) } - default: - err = fmt.Errorf("unsupported file type: %s", path) - } - if err != nil { - return err } } return nil } -func unixPerm(mode fs.FileMode) (perm uint32) { - perm = uint32(mode.Perm()) - if mode&fs.ModeSticky != 0 { - perm |= 0o1000 - } - return perm -} - // replicateParentDirs replicates the parent directories of targetPath in dstRoot. // Fails if any non-directory is on the way. func replicateParentDirs(srcRoot string, dstRoot string, targetPath string) error { diff --git a/internal/slicer/slicer_test.go b/internal/slicer/slicer_test.go index cd1cd68e..1be306ef 100644 --- a/internal/slicer/slicer_test.go +++ b/internal/slicer/slicer_test.go @@ -2522,6 +2522,7 @@ func (s *S) TestRunRecut(c *C) { Selection: selection, Archives: archives, TargetDir: targetDir, + Release: release, } // First run. err = slicer.Run(&options) @@ -2544,6 +2545,7 @@ func (s *S) TestRunRecut(c *C) { Archives: archives, TargetDir: targetDir, PreviousManifest: mfest, + Release: release, } // Second run. err = slicer.Run(&options) From e1a912506a4138a58cd16af24a8745183022d7ba Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Wed, 22 Apr 2026 11:46:02 +0200 Subject: [PATCH 4/9] feat: CutOptions and MergeOptions --- cmd/chisel/cmd_cut.go | 19 +++++++++ internal/slicer/slicer.go | 86 +++++++++++++++++++-------------------- 2 files changed, 60 insertions(+), 45 deletions(-) diff --git a/cmd/chisel/cmd_cut.go b/cmd/chisel/cmd_cut.go index 634eea0a..46ef03ca 100644 --- a/cmd/chisel/cmd_cut.go +++ b/cmd/chisel/cmd_cut.go @@ -154,10 +154,29 @@ func (cmd *cmdCut) Execute(args []string) error { } } + var tmpWorkDir string + if mfest != nil { + // There is a very unlikely case that a package contains a file or directory + // with the same name as the one generated by os.MkdirTemp. That would lead + // to a collision when upgrading content from the workdir to the target + // directory. + // TODO: Use a predetermined name and reserve it. The release validation + // would ensure it cannot be used in chisel-releases. + tmpWorkDir, err := os.MkdirTemp(targetDir, "chisel-workdir-*") + if err != nil { + return fmt.Errorf("cannot create temporary working directory: %w", err) + } + // targetDir = tmpWorkDir + defer func() { + os.RemoveAll(tmpWorkDir) + }() + } + err = slicer.Run(&slicer.RunOptions{ Selection: selection, Archives: archives, TargetDir: targetDir, + WorkDir: tmpWorkDir, PreviousManifest: mfest, Release: release, }) diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index b825a778..93c1e319 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -29,12 +29,26 @@ import ( "github.com/canonical/chisel/public/manifest" ) -const manifestMode fs.FileMode = 0644 +const manifestMode fs.FileMode = 0o644 type RunOptions struct { Selection *setup.Selection Archives map[string]archive.Archive TargetDir string + WorkDir string + PreviousManifest *manifest.Manifest + Release *setup.Release +} + +type CutOptions struct { + Selection *setup.Selection + Archives map[string]archive.Archive + TargetDir string +} + +type MergeOptions struct { + TargetDir string + WorkDir string PreviousManifest *manifest.Manifest Release *setup.Release } @@ -94,43 +108,32 @@ func Run(options *RunOptions) error { return fmt.Errorf("internal error: cannot use a relative target directory %s", targetDir) } - optsCopy := *options - cutOpts := &optsCopy - cutOpts.TargetDir = targetDir + cutOpts := &CutOptions{ + Selection: options.Selection, + Archives: options.Archives, + TargetDir: options.TargetDir, + } if options.PreviousManifest != nil { - // There is a very unlikely case that a package contains a file or directory - // with the same name as the one generated by os.MkdirTemp. That would lead - // to a collision when upgrading content from the workdir to the target - // directory. - // TODO: Use a predetermined name and reserve it. The release validation - // would ensure it cannot be used in chisel-releases. - tmpWorkDir, err := os.MkdirTemp(targetDir, "chisel-workdir-*") - if err != nil { - return fmt.Errorf("cannot create temporary working directory: %w", err) - } - cutOpts.TargetDir = tmpWorkDir - defer func() { - os.RemoveAll(tmpWorkDir) - }() + cutOpts.TargetDir = options.WorkDir } - err := cut(cutOpts) if err != nil { return err } if options.PreviousManifest != nil { - return merge(targetDir, cutOpts.TargetDir, options.PreviousManifest, options.Release) + return merge(&MergeOptions{ + TargetDir: options.TargetDir, + WorkDir: options.WorkDir, + PreviousManifest: options.PreviousManifest, + Release: options.Release, + }) } return nil } -func cut(options *RunOptions) error { - if !filepath.IsAbs(options.TargetDir) { - return fmt.Errorf("internal error: cannot operate on a relative target directory") - } - +func cut(options *CutOptions) error { pkgArchive, err := selectPkgArchives(options.Archives, options.Selection) if err != nil { return err @@ -395,16 +398,9 @@ func cut(options *RunOptions) error { // process assumes the targetDir was verified with prevManifest, and so // prevManifest is an accurate representation of files and directories // previously installed by Chisel in the targetDir. -func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, release *setup.Release) error { - logf("Merging cut in %s...", targetDir) - if !filepath.IsAbs(targetDir) { - return fmt.Errorf("internal error: cannot operate on a relative target directory %s", targetDir) - } - if !filepath.IsAbs(tempDir) { - return fmt.Errorf("internal error: cannot operate on a relative working directory %s", tempDir) - } - - newManifest, err := SelectValidManifest(tempDir, release) +func merge(options *MergeOptions) error { + logf("Merging cut in %s...", options.TargetDir) + newManifest, err := SelectValidManifest(options.WorkDir, options.Release) if err != nil { return fmt.Errorf("internal error: cannot select manifest from working directory: %s", err) } @@ -420,7 +416,7 @@ func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, re } missingPaths := make([]string, 0, len(entries)) newEntries := maps.Clone(entries) - err = prevManifest.IteratePaths("", func(path *manifest.Path) error { + err = options.PreviousManifest.IteratePaths("", func(path *manifest.Path) error { _, ok := entries[path.Path] if ok { delete(newEntries, path.Path) @@ -437,7 +433,7 @@ func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, re // Existing directories are accepted. newPaths := slices.Sorted(maps.Keys(newEntries)) for _, path := range newPaths { - absPath := filepath.Clean(filepath.Join(targetDir, path)) + absPath := filepath.Clean(filepath.Join(options.TargetDir, path)) fileInfo, err := os.Lstat(absPath) if err != nil { if os.IsNotExist(err) { @@ -457,7 +453,7 @@ func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, re // next step if a path changed type in the package (ex. a dir became a file). // This works because we can differentiate files and directories in // the manifest due to the trailing slash on directories. - // These files/directories are safe to remove because the targetDir + // These files/directories are safe to remove because the TargetDir // verification ensured they were unmodified. slices.Sort(missingPaths) slices.Reverse(missingPaths) @@ -465,7 +461,7 @@ func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, re // them. Any ENOTEMPTY error encountered means user content is in the directory // and Chisel does not manage it anymore. for _, path := range missingPaths { - absPath := filepath.Clean(filepath.Join(targetDir, path)) + absPath := filepath.Clean(filepath.Join(options.TargetDir, path)) err = os.Remove(absPath) if err != nil && !os.IsNotExist(err) { if errors.Is(err, syscall.ENOTEMPTY) { @@ -476,11 +472,11 @@ func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, re } } - // Step 4: Apply tempDir content to targetDir. + // Step 4: Apply WorkDir content to TargetDir. paths := slices.Sorted(maps.Keys(entries)) for _, path := range paths { var prevEntry *manifest.Path - err := prevManifest.IteratePaths(path, func(prevPath *manifest.Path) error { + err := options.PreviousManifest.IteratePaths(path, func(prevPath *manifest.Path) error { if path == prevPath.Path { prevEntry = prevPath } @@ -511,11 +507,11 @@ func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, re // When extracting the content, a great care is taken to create parent // directories respecting the tarball permissions. However this approach // can create implicit parents, not recorded in the manifest. Make sure - // to replicate these directories in the targetDir to sustain the same + // to replicate these directories in the TargetDir to sustain the same // guarantees as a normal cut. // Even if unlikely, this operation can fail if a user file has the // same path as one of the implicit parent directory replicated here. - if err := replicateParentDirs(tempDir, targetDir, path); err != nil { + if err := replicateParentDirs(options.WorkDir, options.TargetDir, path); err != nil { return fmt.Errorf("cannot create parent directory for %q: %s", path, err) } // The removal done at step 3 ensures that if the destination path exists @@ -526,8 +522,8 @@ func merge(targetDir string, tempDir string, prevManifest *manifest.Manifest, re // removed at step 3. This content must not be deleted; // - Content was modified in the rootfs between its verification and this // step. This process does not try to solve this case. - srcPath := filepath.Clean(filepath.Join(tempDir, path)) - dstPath := filepath.Clean(filepath.Join(targetDir, path)) + srcPath := filepath.Clean(filepath.Join(options.WorkDir, path)) + dstPath := filepath.Clean(filepath.Join(options.TargetDir, path)) if strings.HasSuffix(path, "/") { permissions, err := strconv.ParseUint(entry.Mode, 8, 32) if err != nil { From 2e46c34ec2311f9987fa6d75f5f7a278623e63e9 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Wed, 22 Apr 2026 13:57:17 +0200 Subject: [PATCH 5/9] feat: multiple parameters to cut & merge --- cmd/chisel/cmd_cut.go | 1 - internal/slicer/slicer.go | 80 ++++++++++++++------------------------- 2 files changed, 29 insertions(+), 52 deletions(-) diff --git a/cmd/chisel/cmd_cut.go b/cmd/chisel/cmd_cut.go index 46ef03ca..18081eab 100644 --- a/cmd/chisel/cmd_cut.go +++ b/cmd/chisel/cmd_cut.go @@ -166,7 +166,6 @@ func (cmd *cmdCut) Execute(args []string) error { if err != nil { return fmt.Errorf("cannot create temporary working directory: %w", err) } - // targetDir = tmpWorkDir defer func() { os.RemoveAll(tmpWorkDir) }() diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index 93c1e319..26e2dbba 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -40,19 +40,6 @@ type RunOptions struct { Release *setup.Release } -type CutOptions struct { - Selection *setup.Selection - Archives map[string]archive.Archive - TargetDir string -} - -type MergeOptions struct { - TargetDir string - WorkDir string - PreviousManifest *manifest.Manifest - Release *setup.Release -} - type pathData struct { until setup.PathUntil mutable bool @@ -108,45 +95,36 @@ func Run(options *RunOptions) error { return fmt.Errorf("internal error: cannot use a relative target directory %s", targetDir) } - cutOpts := &CutOptions{ - Selection: options.Selection, - Archives: options.Archives, - TargetDir: options.TargetDir, - } + cutTargetDir := options.TargetDir if options.PreviousManifest != nil { - cutOpts.TargetDir = options.WorkDir + cutTargetDir = options.WorkDir } - err := cut(cutOpts) + err := cut(cutTargetDir, options.Selection, options.Archives) if err != nil { return err } if options.PreviousManifest != nil { - return merge(&MergeOptions{ - TargetDir: options.TargetDir, - WorkDir: options.WorkDir, - PreviousManifest: options.PreviousManifest, - Release: options.Release, - }) + return merge(options.TargetDir, options.WorkDir, options.PreviousManifest, options.Release) } return nil } -func cut(options *CutOptions) error { - pkgArchive, err := selectPkgArchives(options.Archives, options.Selection) +func cut(targetDir string, selection *setup.Selection, archives map[string]archive.Archive) error { + pkgArchive, err := selectPkgArchives(archives, selection) if err != nil { return err } - prefers, err := options.Selection.Prefers() + prefers, err := selection.Prefers() if err != nil { return err } // Build information to process the selection. extract := make(map[string]map[string][]deb.ExtractInfo) - for _, slice := range options.Selection.Slices { + for _, slice := range selection.Slices { extractPackage := extract[slice.Package] if extractPackage == nil { extractPackage = make(map[string][]deb.ExtractInfo) @@ -192,7 +170,7 @@ func cut(options *CutOptions) error { // Fetch all packages, using the selection order. packages := make(map[string]io.ReadSeekCloser) var pkgInfos []*archive.PackageInfo - for _, slice := range options.Selection.Slices { + for _, slice := range selection.Slices { if packages[slice.Package] != nil { continue } @@ -209,7 +187,7 @@ func cut(options *CutOptions) error { // listed as until: mutate in all the slices that reference them. knownPaths := map[string]pathData{} addKnownPath(knownPaths, "/", pathData{}) - report, err := manifestutil.NewReport(options.TargetDir) + report, err := manifestutil.NewReport(targetDir) if err != nil { return fmt.Errorf("internal error: cannot create report: %w", err) } @@ -227,7 +205,7 @@ func cut(options *CutOptions) error { return err } - relPath := filepath.Clean("/" + strings.TrimPrefix(o.Path, options.TargetDir)) + relPath := filepath.Clean("/" + strings.TrimPrefix(o.Path, targetDir)) if o.Mode.IsDir() { relPath = relPath + "/" } @@ -277,7 +255,7 @@ func cut(options *CutOptions) error { } // Extract all packages, also using the selection order. - for _, slice := range options.Selection.Slices { + for _, slice := range selection.Slices { reader := packages[slice.Package] if reader == nil { continue @@ -285,7 +263,7 @@ func cut(options *CutOptions) error { err := deb.Extract(reader, &deb.ExtractOptions{ Package: slice.Package, Extract: extract[slice.Package], - TargetDir: options.TargetDir, + TargetDir: targetDir, Create: create, }) reader.Close() @@ -312,7 +290,7 @@ func cut(options *CutOptions) error { // First group them by their relative path. Then create them and attribute // them to the appropriate slices. relPaths := map[string][]*setup.Slice{} - for _, slice := range options.Selection.Slices { + for _, slice := range selection.Slices { arch := pkgArchive[slice.Package].Options().Arch for relPath, pathInfo := range slice.Contents { if len(pathInfo.Arch) > 0 && !slices.Contains(pathInfo.Arch, arch) { @@ -347,7 +325,7 @@ func cut(options *CutOptions) error { mutable: pathInfo.Mutable, } addKnownPath(knownPaths, relPath, data) - entry, err := createFile(options.TargetDir, relPath, pathInfo) + entry, err := createFile(targetDir, relPath, pathInfo) if err != nil { return err } @@ -367,12 +345,12 @@ func cut(options *CutOptions) error { // dependencies must run before dependents. checker := contentChecker{knownPaths} content := &scripts.ContentValue{ - RootDir: options.TargetDir, + RootDir: targetDir, CheckWrite: checker.checkMutable, CheckRead: checker.checkKnown, OnWrite: report.Mutate, } - for _, slice := range options.Selection.Slices { + for _, slice := range selection.Slices { opts := scripts.RunOptions{ Label: "mutate", Script: slice.Scripts.Mutate, @@ -386,21 +364,21 @@ func cut(options *CutOptions) error { } } - err = removeAfterMutate(options.TargetDir, knownPaths) + err = removeAfterMutate(targetDir, knownPaths) if err != nil { return err } - return generateManifests(options.TargetDir, options.Selection, report, pkgInfos) + return generateManifests(targetDir, selection, report, pkgInfos) } // merge applies the content from the tempDir to the targetDir. This // process assumes the targetDir was verified with prevManifest, and so // prevManifest is an accurate representation of files and directories // previously installed by Chisel in the targetDir. -func merge(options *MergeOptions) error { - logf("Merging cut in %s...", options.TargetDir) - newManifest, err := SelectValidManifest(options.WorkDir, options.Release) +func merge(targetDir string, workDir string, prevManifest *manifest.Manifest, release *setup.Release) error { + logf("Merging cut in %s...", targetDir) + newManifest, err := SelectValidManifest(workDir, release) if err != nil { return fmt.Errorf("internal error: cannot select manifest from working directory: %s", err) } @@ -416,7 +394,7 @@ func merge(options *MergeOptions) error { } missingPaths := make([]string, 0, len(entries)) newEntries := maps.Clone(entries) - err = options.PreviousManifest.IteratePaths("", func(path *manifest.Path) error { + err = prevManifest.IteratePaths("", func(path *manifest.Path) error { _, ok := entries[path.Path] if ok { delete(newEntries, path.Path) @@ -433,7 +411,7 @@ func merge(options *MergeOptions) error { // Existing directories are accepted. newPaths := slices.Sorted(maps.Keys(newEntries)) for _, path := range newPaths { - absPath := filepath.Clean(filepath.Join(options.TargetDir, path)) + absPath := filepath.Clean(filepath.Join(targetDir, path)) fileInfo, err := os.Lstat(absPath) if err != nil { if os.IsNotExist(err) { @@ -461,7 +439,7 @@ func merge(options *MergeOptions) error { // them. Any ENOTEMPTY error encountered means user content is in the directory // and Chisel does not manage it anymore. for _, path := range missingPaths { - absPath := filepath.Clean(filepath.Join(options.TargetDir, path)) + absPath := filepath.Clean(filepath.Join(targetDir, path)) err = os.Remove(absPath) if err != nil && !os.IsNotExist(err) { if errors.Is(err, syscall.ENOTEMPTY) { @@ -476,7 +454,7 @@ func merge(options *MergeOptions) error { paths := slices.Sorted(maps.Keys(entries)) for _, path := range paths { var prevEntry *manifest.Path - err := options.PreviousManifest.IteratePaths(path, func(prevPath *manifest.Path) error { + err := prevManifest.IteratePaths(path, func(prevPath *manifest.Path) error { if path == prevPath.Path { prevEntry = prevPath } @@ -511,7 +489,7 @@ func merge(options *MergeOptions) error { // guarantees as a normal cut. // Even if unlikely, this operation can fail if a user file has the // same path as one of the implicit parent directory replicated here. - if err := replicateParentDirs(options.WorkDir, options.TargetDir, path); err != nil { + if err := replicateParentDirs(workDir, targetDir, path); err != nil { return fmt.Errorf("cannot create parent directory for %q: %s", path, err) } // The removal done at step 3 ensures that if the destination path exists @@ -522,8 +500,8 @@ func merge(options *MergeOptions) error { // removed at step 3. This content must not be deleted; // - Content was modified in the rootfs between its verification and this // step. This process does not try to solve this case. - srcPath := filepath.Clean(filepath.Join(options.WorkDir, path)) - dstPath := filepath.Clean(filepath.Join(options.TargetDir, path)) + srcPath := filepath.Clean(filepath.Join(workDir, path)) + dstPath := filepath.Clean(filepath.Join(targetDir, path)) if strings.HasSuffix(path, "/") { permissions, err := strconv.ParseUint(entry.Mode, 8, 32) if err != nil { From 8bc670359c1adee6813049c503b182dbc99a267e Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Thu, 23 Apr 2026 09:07:30 +0200 Subject: [PATCH 6/9] feat: add stateDir --- cmd/chisel/cmd_cut.go | 31 +++-- internal/slicer/merge.go | 220 +++++++++++++++++++++++++++++++++ internal/slicer/slicer.go | 219 ++------------------------------ internal/slicer/slicer_test.go | 8 +- internal/slicer/state.go | 41 ++++++ 5 files changed, 294 insertions(+), 225 deletions(-) create mode 100644 internal/slicer/merge.go create mode 100644 internal/slicer/state.go diff --git a/cmd/chisel/cmd_cut.go b/cmd/chisel/cmd_cut.go index 18081eab..5932f8ae 100644 --- a/cmd/chisel/cmd_cut.go +++ b/cmd/chisel/cmd_cut.go @@ -154,28 +154,27 @@ func (cmd *cmdCut) Execute(args []string) error { } } - var tmpWorkDir string - if mfest != nil { - // There is a very unlikely case that a package contains a file or directory - // with the same name as the one generated by os.MkdirTemp. That would lead - // to a collision when upgrading content from the workdir to the target - // directory. - // TODO: Use a predetermined name and reserve it. The release validation - // would ensure it cannot be used in chisel-releases. - tmpWorkDir, err := os.MkdirTemp(targetDir, "chisel-workdir-*") - if err != nil { - return fmt.Errorf("cannot create temporary working directory: %w", err) - } - defer func() { - os.RemoveAll(tmpWorkDir) - }() + // Prepare state directory in target directory. + // There is an unlikely case that a package contains a file or directory + // with this name. That would lead to a collision when upgrading content + // from the temporary rootfs in the state directory to the target directory. + // TODO: Reserve this name. The release validation would then ensure it + // cannot be used in the release. + stateDir, err := slicer.MkStateDir(targetDir, 0o755) + if err != nil { + return fmt.Errorf("cannot create temporary working directory: %w", err) } + defer func() { + // The state directory must only be removed if empty. This call will do so + // or silently fail. + os.Remove(stateDir) + }() err = slicer.Run(&slicer.RunOptions{ Selection: selection, Archives: archives, TargetDir: targetDir, - WorkDir: tmpWorkDir, + StateDir: stateDir, PreviousManifest: mfest, Release: release, }) diff --git a/internal/slicer/merge.go b/internal/slicer/merge.go new file mode 100644 index 00000000..f47a7f50 --- /dev/null +++ b/internal/slicer/merge.go @@ -0,0 +1,220 @@ +package slicer + +import ( + "errors" + "fmt" + "io/fs" + "maps" + "os" + "path/filepath" + "slices" + "strconv" + "strings" + "syscall" + + "github.com/canonical/chisel/internal/manifestutil" + "github.com/canonical/chisel/internal/setup" + "github.com/canonical/chisel/public/manifest" +) + +// merge applies the content from the workDir to the targetDir. This +// process assumes the targetDir was verified with prevManifest, and so +// prevManifest is an accurate representation of files and directories +// previously cut by Chisel in the targetDir. +func merge(targetDir string, workDir string, prevManifest *manifest.Manifest, release *setup.Release) error { + logf("Merging cut in %s...", targetDir) + newManifest, err := SelectValidManifest(workDir, release) + if err != nil { + return fmt.Errorf("internal error: cannot select manifest from working directory: %s", err) + } + + // Step 1: Identify new and missing entries. + entries := make(map[string]*manifest.Path) + err = newManifest.IteratePaths("", func(path *manifest.Path) error { + entries[path.Path] = path + return nil + }) + if err != nil { + return err + } + missingPaths := make([]string, 0, len(entries)) + newEntries := maps.Clone(entries) + err = prevManifest.IteratePaths("", func(path *manifest.Path) error { + _, ok := entries[path.Path] + if ok { + delete(newEntries, path.Path) + } else { + missingPaths = append(missingPaths, path.Path) + } + return nil + }) + if err != nil { + return err + } + + // Step 2: Verify no new path will collide with user content. + // Existing directories are accepted. + newPaths := slices.Sorted(maps.Keys(newEntries)) + for _, path := range newPaths { + absPath := filepath.Clean(filepath.Join(targetDir, path)) + fileInfo, err := os.Lstat(absPath) + if err != nil { + if os.IsNotExist(err) { + continue + } + return err + } + if !fileInfo.IsDir() { + return fmt.Errorf("cannot override user content: %s exists", path) + } + } + + // Step 3: Remove content removed from packages/slices. + // An entry listed in the previous manifest but missing from the new one + // means it is not part of the package/slice anymore, so remove it. + // Doing the removing before updating content prevents from collisions in the + // next step if a path changed type in the package (ex. a dir became a file). + // This works because we can differentiate files and directories in + // the manifest due to the trailing slash on directories. + // These files/directories are safe to remove because the TargetDir + // verification ensured they were unmodified. + slices.Sort(missingPaths) + slices.Reverse(missingPaths) + // Go through the list in reverse order to empty directories before removing + // them. Any ENOTEMPTY error encountered means user content is in the directory + // and Chisel does not manage it anymore. + for _, path := range missingPaths { + absPath := filepath.Clean(filepath.Join(targetDir, path)) + err = os.Remove(absPath) + if err != nil && !os.IsNotExist(err) { + if errors.Is(err, syscall.ENOTEMPTY) { + logf("Keep %s as not empty after package content removal", path) + continue + } + return err + } + } + + // Step 4: Apply WorkDir content to TargetDir. + paths := slices.Sorted(maps.Keys(entries)) + for _, path := range paths { + var prevEntry *manifest.Path + err := prevManifest.IteratePaths(path, func(prevPath *manifest.Path) error { + if path == prevPath.Path { + prevEntry = prevPath + } + return nil + }) + if err != nil { + return err + } + entry := entries[path] + if prevEntry != nil { + // Skip the entry if both previous and new one are identical, except for + // manifests. No Size/SHA256/FinalSH2A56 are recorded for manifests, so make + // sure they are NOT skipped. + if prevEntry.Mode == entry.Mode && + prevEntry.Size == entry.Size && + prevEntry.Link == entry.Link && + prevEntry.SHA256 == entry.SHA256 && + prevEntry.FinalSHA256 == entry.FinalSHA256 && + (prevEntry.Inode != 0) == (entry.Inode != 0) && + // Do not skip manifests. + (filepath.Base(prevEntry.Path) != manifestutil.DefaultFilename && + prevEntry.Size == 0 && + prevEntry.SHA256 == "") { + // The entry did not change, nothing to do. + continue + } + } + // When extracting the content, a great care is taken to create parent + // directories respecting the tarball permissions. However this approach + // can create implicit parents, not recorded in the manifest. Make sure + // to replicate these directories in the TargetDir to sustain the same + // guarantees as a normal cut. + // Even if unlikely, this operation can fail if a user file has the + // same path as one of the implicit parent directory replicated here. + if err := replicateParentDirs(workDir, targetDir, path); err != nil { + return fmt.Errorf("cannot create parent directory for %q: %s", path, err) + } + // The removal done at step 3 ensures that if the destination path exists + // it is of the same type (dir or file/symlink) as the source. So any error + // here is considered a failure because it can only mean one of these things: + // - An OS error happened, we cannot do anything about it; + // - There is a collision with a directory containing user content and not + // removed at step 3. This content must not be deleted; + // - Content was modified in the rootfs between its verification and this + // step. This process does not try to solve this case. + srcPath := filepath.Clean(filepath.Join(workDir, path)) + dstPath := filepath.Clean(filepath.Join(targetDir, path)) + if strings.HasSuffix(path, "/") { + permissions, err := strconv.ParseUint(entry.Mode, 8, 32) + if err != nil { + return fmt.Errorf("cannot parse mode %q: %w", entry.Mode, err) + } + mode := fs.FileMode(permissions) + mkdirErr := os.Mkdir(dstPath, mode) + if mkdirErr != nil { + if os.IsExist(mkdirErr) { + err = os.Chmod(dstPath, mode) + } else { + err = mkdirErr + } + if err != nil { + return err + } + } + } else { + err = os.Rename(srcPath, dstPath) + if err != nil { + return fmt.Errorf("cannot move file at %q: %s", path, err) + } + } + } + return nil +} + +// replicateParentDirs replicates the parent directories of targetPath in dstRoot. +// Fails if any non-directory is on the way. +func replicateParentDirs(srcRoot string, dstRoot string, targetPath string) error { + parents := parentDirs(targetPath) + for _, path := range parents { + if path == "/" { + continue + } + srcPath := filepath.Clean(filepath.Join(srcRoot, path)) + srcInfo, err := os.Stat(srcPath) + if err != nil { + return err + } + dstPath := filepath.Clean(filepath.Join(dstRoot, path)) + err = os.Mkdir(dstPath, srcInfo.Mode()) + if err != nil { + if !os.IsExist(err) { + return err + } + fileinfo, err := os.Lstat(dstPath) + if err != nil { + return err + } + if !fileinfo.IsDir() { + return fmt.Errorf("cannot create directory, found a non-directory at %s", dstPath) + } + return os.Chmod(dstPath, srcInfo.Mode()) + } + } + return nil +} + +func parentDirs(path string) []string { + path = filepath.Clean(path) + parents := make([]string, strings.Count(path, "/")) + count := 0 + for i, c := range path { + if c == '/' { + parents[count] = path[:i+1] + count++ + } + } + return parents +} diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index 26e2dbba..944ed273 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -9,12 +9,10 @@ import ( "fmt" "io" "io/fs" - "maps" "os" "path/filepath" "slices" "sort" - "strconv" "strings" "syscall" @@ -35,7 +33,7 @@ type RunOptions struct { Selection *setup.Selection Archives map[string]archive.Archive TargetDir string - WorkDir string + StateDir string PreviousManifest *manifest.Manifest Release *setup.Release } @@ -97,15 +95,24 @@ func Run(options *RunOptions) error { cutTargetDir := options.TargetDir if options.PreviousManifest != nil { - cutTargetDir = options.WorkDir + // When recuting, cut inside a temporary workdir. + tmpWorkDir, err := os.MkdirTemp(options.StateDir, "chisel-workdir-*") + if err != nil { + return fmt.Errorf("cannot create temporary working directory: %w", err) + } + cutTargetDir = tmpWorkDir + defer func() { + os.RemoveAll(tmpWorkDir) + }() } + err := cut(cutTargetDir, options.Selection, options.Archives) if err != nil { return err } if options.PreviousManifest != nil { - return merge(options.TargetDir, options.WorkDir, options.PreviousManifest, options.Release) + return merge(options.TargetDir, cutTargetDir, options.PreviousManifest, options.Release) } return nil @@ -372,208 +379,6 @@ func cut(targetDir string, selection *setup.Selection, archives map[string]archi return generateManifests(targetDir, selection, report, pkgInfos) } -// merge applies the content from the tempDir to the targetDir. This -// process assumes the targetDir was verified with prevManifest, and so -// prevManifest is an accurate representation of files and directories -// previously installed by Chisel in the targetDir. -func merge(targetDir string, workDir string, prevManifest *manifest.Manifest, release *setup.Release) error { - logf("Merging cut in %s...", targetDir) - newManifest, err := SelectValidManifest(workDir, release) - if err != nil { - return fmt.Errorf("internal error: cannot select manifest from working directory: %s", err) - } - - // Step 1: Identify new and missing entries. - entries := make(map[string]*manifest.Path) - err = newManifest.IteratePaths("", func(path *manifest.Path) error { - entries[path.Path] = path - return nil - }) - if err != nil { - return err - } - missingPaths := make([]string, 0, len(entries)) - newEntries := maps.Clone(entries) - err = prevManifest.IteratePaths("", func(path *manifest.Path) error { - _, ok := entries[path.Path] - if ok { - delete(newEntries, path.Path) - } else { - missingPaths = append(missingPaths, path.Path) - } - return nil - }) - if err != nil { - return err - } - - // Step 2: Verify no new path will collide with user content. - // Existing directories are accepted. - newPaths := slices.Sorted(maps.Keys(newEntries)) - for _, path := range newPaths { - absPath := filepath.Clean(filepath.Join(targetDir, path)) - fileInfo, err := os.Lstat(absPath) - if err != nil { - if os.IsNotExist(err) { - continue - } - return err - } - if !fileInfo.IsDir() { - return fmt.Errorf("cannot override user content: %s exists", path) - } - } - - // Step 3: Remove content removed from packages/slices. - // An entry listed in the previous manifest but missing from the new one - // means it is not part of the package/slice anymore, so remove it. - // Doing the removing before updating content prevents from collisions in the - // next step if a path changed type in the package (ex. a dir became a file). - // This works because we can differentiate files and directories in - // the manifest due to the trailing slash on directories. - // These files/directories are safe to remove because the TargetDir - // verification ensured they were unmodified. - slices.Sort(missingPaths) - slices.Reverse(missingPaths) - // Go through the list in reverse order to empty directories before removing - // them. Any ENOTEMPTY error encountered means user content is in the directory - // and Chisel does not manage it anymore. - for _, path := range missingPaths { - absPath := filepath.Clean(filepath.Join(targetDir, path)) - err = os.Remove(absPath) - if err != nil && !os.IsNotExist(err) { - if errors.Is(err, syscall.ENOTEMPTY) { - logf("Keep %s as not empty after package content removal", path) - continue - } - return err - } - } - - // Step 4: Apply WorkDir content to TargetDir. - paths := slices.Sorted(maps.Keys(entries)) - for _, path := range paths { - var prevEntry *manifest.Path - err := prevManifest.IteratePaths(path, func(prevPath *manifest.Path) error { - if path == prevPath.Path { - prevEntry = prevPath - } - return nil - }) - if err != nil { - return err - } - entry := entries[path] - if prevEntry != nil { - // Skip the entry if both previous and new one are identical, except for - // manifests. No Size/SHA256/FinalSH2A56 are recorded for manifests, so make - // sure they are NOT skipped. - if prevEntry.Mode == entry.Mode && - prevEntry.Size == entry.Size && - prevEntry.Link == entry.Link && - prevEntry.SHA256 == entry.SHA256 && - prevEntry.FinalSHA256 == entry.FinalSHA256 && - (prevEntry.Inode != 0) == (entry.Inode != 0) && - // Do not skip manifests. - (filepath.Base(prevEntry.Path) != manifestutil.DefaultFilename && - prevEntry.Size == 0 && - prevEntry.SHA256 == "") { - // The entry did not change, nothing to do. - continue - } - } - // When extracting the content, a great care is taken to create parent - // directories respecting the tarball permissions. However this approach - // can create implicit parents, not recorded in the manifest. Make sure - // to replicate these directories in the TargetDir to sustain the same - // guarantees as a normal cut. - // Even if unlikely, this operation can fail if a user file has the - // same path as one of the implicit parent directory replicated here. - if err := replicateParentDirs(workDir, targetDir, path); err != nil { - return fmt.Errorf("cannot create parent directory for %q: %s", path, err) - } - // The removal done at step 3 ensures that if the destination path exists - // it is of the same type (dir or file/symlink) as the source. So any error - // here is considered a failure because it can only mean one of these things: - // - An OS error happened, we cannot do anything about it; - // - There is a collision with a directory containing user content and not - // removed at step 3. This content must not be deleted; - // - Content was modified in the rootfs between its verification and this - // step. This process does not try to solve this case. - srcPath := filepath.Clean(filepath.Join(workDir, path)) - dstPath := filepath.Clean(filepath.Join(targetDir, path)) - if strings.HasSuffix(path, "/") { - permissions, err := strconv.ParseUint(entry.Mode, 8, 32) - if err != nil { - return fmt.Errorf("cannot parse mode %q: %w", entry.Mode, err) - } - mode := fs.FileMode(permissions) - mkdirErr := os.Mkdir(dstPath, mode) - if mkdirErr != nil { - if os.IsExist(mkdirErr) { - err = os.Chmod(dstPath, mode) - } else { - err = mkdirErr - } - if err != nil { - return err - } - } - } else { - err = os.Rename(srcPath, dstPath) - if err != nil { - return fmt.Errorf("cannot move file at %q: %s", path, err) - } - } - } - return nil -} - -// replicateParentDirs replicates the parent directories of targetPath in dstRoot. -// Fails if any non-directory is on the way. -func replicateParentDirs(srcRoot string, dstRoot string, targetPath string) error { - parents := parentDirs(targetPath) - for _, path := range parents { - if path == "/" { - continue - } - srcPath := filepath.Clean(filepath.Join(srcRoot, path)) - srcInfo, err := os.Stat(srcPath) - if err != nil { - return err - } - dstPath := filepath.Clean(filepath.Join(dstRoot, path)) - err = os.Mkdir(dstPath, srcInfo.Mode()) - if err != nil { - if !os.IsExist(err) { - return err - } - fileinfo, err := os.Lstat(dstPath) - if err != nil { - return err - } - if !fileinfo.IsDir() { - return fmt.Errorf("cannot create directory, found a non-directory at %s", dstPath) - } - return os.Chmod(dstPath, srcInfo.Mode()) - } - } - return nil -} - -func parentDirs(path string) []string { - path = filepath.Clean(path) - parents := make([]string, strings.Count(path, "/")) - count := 0 - for i, c := range path { - if c == '/' { - parents[count] = path[:i+1] - count++ - } - } - return parents -} - func generateManifests(targetDir string, selection *setup.Selection, report *manifestutil.Report, pkgInfos []*archive.PackageInfo) error { manifestSlices := manifestutil.FindPaths(selection.Slices) diff --git a/internal/slicer/slicer_test.go b/internal/slicer/slicer_test.go index 1be306ef..99706656 100644 --- a/internal/slicer/slicer_test.go +++ b/internal/slicer/slicer_test.go @@ -2109,6 +2109,7 @@ func runSlicerTests(s *S, c *C, tests []slicerTest) { Selection: selection, Archives: archives, TargetDir: c.MkDir(), + StateDir: c.MkDir(), } if test.hackopt != nil { test.hackopt(c, &options) @@ -2518,11 +2519,13 @@ func (s *S) TestRunRecut(c *C) { } targetDir := c.MkDir() + stateDir := c.MkDir() options := slicer.RunOptions{ Selection: selection, Archives: archives, TargetDir: targetDir, - Release: release, + StateDir: stateDir, + Release: release, } // First run. err = slicer.Run(&options) @@ -2544,8 +2547,9 @@ func (s *S) TestRunRecut(c *C) { Selection: selection, Archives: archives, TargetDir: targetDir, + StateDir: stateDir, PreviousManifest: mfest, - Release: release, + Release: release, } // Second run. err = slicer.Run(&options) diff --git a/internal/slicer/state.go b/internal/slicer/state.go new file mode 100644 index 00000000..d84c63d8 --- /dev/null +++ b/internal/slicer/state.go @@ -0,0 +1,41 @@ +package slicer + +import ( + "fmt" + "os" + "path/filepath" +) + +const stateDir = ".chisel" + +// MkStateDir ensures the state dir exists under the given directory, with the proper +// permissions. +func MkStateDir(targetDir string, mode os.FileMode) (string, error) { + var err error + dir := filepath.Join(targetDir, stateDir) + defer func() { + if err != nil { + err = fmt.Errorf("cannot create state directory: %w", err) + } + }() + err = os.Mkdir(dir, mode) + if err != nil { + if !os.IsExist(err) { + return "", err + } + fileinfo, err := os.Lstat(dir) + if err != nil { + return "", err + } + if !fileinfo.IsDir() { + return "", fmt.Errorf("existing entry at %s is not a directory", dir) + } + // The needed mode might change between Chisel versions. Reset it to ensure + // backward compatibility. + err = os.Chmod(dir, mode) + if err != nil { + return "", err + } + } + return dir, nil +} From d18dd5b51ee5babc2816ce74e8f439015043cdc2 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Thu, 23 Apr 2026 09:21:34 +0200 Subject: [PATCH 7/9] fix: fmt and comments --- cmd/chisel/cmd_cut.go | 2 +- internal/slicer/slicer_test.go | 2 +- tests/recut-equivalence/task.yaml | 2 +- tests/recut/task.yaml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/chisel/cmd_cut.go b/cmd/chisel/cmd_cut.go index 5932f8ae..86f8d821 100644 --- a/cmd/chisel/cmd_cut.go +++ b/cmd/chisel/cmd_cut.go @@ -87,7 +87,7 @@ func (cmd *cmdCut) Execute(args []string) error { } var mfest *manifest.Manifest - // TODO: Remove this gating once the final upgrading strategy is in place. + // TODO: Remove this gating once the rootfs verification is in place. if os.Getenv("CHISEL_RECUT_EXPERIMENTAL") != "" { mfest, err = slicer.SelectValidManifest(targetDir, release) if err == nil { diff --git a/internal/slicer/slicer_test.go b/internal/slicer/slicer_test.go index 99706656..34d2d196 100644 --- a/internal/slicer/slicer_test.go +++ b/internal/slicer/slicer_test.go @@ -2519,7 +2519,7 @@ func (s *S) TestRunRecut(c *C) { } targetDir := c.MkDir() - stateDir := c.MkDir() + stateDir := c.MkDir() options := slicer.RunOptions{ Selection: selection, Archives: archives, diff --git a/tests/recut-equivalence/task.yaml b/tests/recut-equivalence/task.yaml index 764dd9bf..b3ad106c 100644 --- a/tests/recut-equivalence/task.yaml +++ b/tests/recut-equivalence/task.yaml @@ -10,7 +10,7 @@ environment: execute: | mkdir -p ${ROOTFS} ${ROOTFS_RECUT} - # TODO: remove this env var when final upgrade strategy is in place. + # TODO: Remove this env var once the rootfs verification is in place. export CHISEL_RECUT_EXPERIMENTAL=1 # First cut single cut diff --git a/tests/recut/task.yaml b/tests/recut/task.yaml index fc426ef5..e12b4bc0 100644 --- a/tests/recut/task.yaml +++ b/tests/recut/task.yaml @@ -13,7 +13,7 @@ execute: | mkdir -p ${ROOTFS} - # TODO: remove this env var when final upgrade strategy is in place. + # TODO: remove this env var once the rootfs verification is in place. export CHISEL_RECUT_EXPERIMENTAL=1 # First cut generates manifest and installs slice-a. chisel cut --release ./chisel-releases/ \ From 63ce01457ceae55485f4d5a5508df3f2ee150eaf Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Thu, 23 Apr 2026 10:08:59 +0200 Subject: [PATCH 8/9] tests: add tests for MkStateDir Signed-off-by: Paul Mars --- cmd/chisel/cmd_cut.go | 2 +- internal/slicer/slicer.go | 2 +- internal/slicer/state.go | 5 +-- internal/slicer/state_test.go | 77 +++++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 internal/slicer/state_test.go diff --git a/cmd/chisel/cmd_cut.go b/cmd/chisel/cmd_cut.go index 86f8d821..1ee5c414 100644 --- a/cmd/chisel/cmd_cut.go +++ b/cmd/chisel/cmd_cut.go @@ -162,7 +162,7 @@ func (cmd *cmdCut) Execute(args []string) error { // cannot be used in the release. stateDir, err := slicer.MkStateDir(targetDir, 0o755) if err != nil { - return fmt.Errorf("cannot create temporary working directory: %w", err) + return fmt.Errorf("cannot create temporary working directory: %s", err) } defer func() { // The state directory must only be removed if empty. This call will do so diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index 944ed273..76d592f3 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -27,7 +27,7 @@ import ( "github.com/canonical/chisel/public/manifest" ) -const manifestMode fs.FileMode = 0o644 +const manifestMode fs.FileMode = 0644 type RunOptions struct { Selection *setup.Selection diff --git a/internal/slicer/state.go b/internal/slicer/state.go index d84c63d8..2e3c90c3 100644 --- a/internal/slicer/state.go +++ b/internal/slicer/state.go @@ -10,9 +10,8 @@ const stateDir = ".chisel" // MkStateDir ensures the state dir exists under the given directory, with the proper // permissions. -func MkStateDir(targetDir string, mode os.FileMode) (string, error) { - var err error - dir := filepath.Join(targetDir, stateDir) +func MkStateDir(targetDir string, mode os.FileMode) (dir string, err error) { + dir = filepath.Join(targetDir, stateDir) defer func() { if err != nil { err = fmt.Errorf("cannot create state directory: %w", err) diff --git a/internal/slicer/state_test.go b/internal/slicer/state_test.go new file mode 100644 index 00000000..61547578 --- /dev/null +++ b/internal/slicer/state_test.go @@ -0,0 +1,77 @@ +package slicer_test + +import ( + "os" + "path/filepath" + + . "gopkg.in/check.v1" + + "github.com/canonical/chisel/internal/slicer" +) + +type mkStateDirTest struct { + summary string + mode os.FileMode + prepareTarget func(c *C, targetDir string) + dir string + error string +} + +var mkStateDirTests = []mkStateDirTest{{ + summary: "Create state dir from scratch", + mode: 0o755, + dir: ".chisel", +}, { + summary: "Create state dir with different mode", + mode: 0o700, + dir: ".chisel", +}, { + summary: "Existing state dir gets mode reset", + mode: 0o755, + prepareTarget: func(c *C, targetDir string) { + err := os.Mkdir(filepath.Join(targetDir, ".chisel"), 0o700) + c.Assert(err, IsNil) + }, + dir: ".chisel", +}, { + summary: "Existing non-directory entry at state dir path", + mode: 0o755, + prepareTarget: func(c *C, targetDir string) { + err := os.WriteFile(filepath.Join(targetDir, ".chisel"), []byte("data"), 0o644) + c.Assert(err, IsNil) + }, + error: `cannot create state directory: existing entry at .*/\.chisel is not a directory`, +}, { + summary: "Target dir does not exist", + mode: 0o755, + prepareTarget: func(c *C, targetDir string) { + err := os.RemoveAll(targetDir) + c.Assert(err, IsNil) + }, + error: `cannot create state directory: mkdir .*/\.chisel: no such file or directory`, +}} + +func (s *S) TestMkStateDir(c *C) { + for _, test := range mkStateDirTests { + c.Logf("Summary: %s", test.summary) + + targetDir := c.MkDir() + if test.prepareTarget != nil { + test.prepareTarget(c, targetDir) + } + + dir, err := slicer.MkStateDir(targetDir, test.mode) + if test.error != "" { + c.Assert(err, ErrorMatches, test.error) + continue + } + + c.Assert(err, IsNil) + c.Assert(dir, Equals, filepath.Join(targetDir, test.dir)) + + info, err := os.Lstat(dir) + c.Assert(err, IsNil) + c.Assert(info.IsDir(), Equals, true) + c.Assert(info.Mode().Perm(), Equals, test.mode) + } +} From 68da572312ca2f455c9390c13604a7c250280e2d Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Wed, 29 Apr 2026 08:30:26 +0200 Subject: [PATCH 9/9] fix: simplify state dir handling --- cmd/chisel/cmd_cut.go | 17 ------ internal/slicer/slicer.go | 56 ++++++++++++++++++-- internal/slicer/slicer_test.go | 95 ++++++++++++++++++++++++++++++++-- internal/slicer/state.go | 40 -------------- internal/slicer/state_test.go | 77 --------------------------- 5 files changed, 143 insertions(+), 142 deletions(-) delete mode 100644 internal/slicer/state.go delete mode 100644 internal/slicer/state_test.go diff --git a/cmd/chisel/cmd_cut.go b/cmd/chisel/cmd_cut.go index 1ee5c414..524da45d 100644 --- a/cmd/chisel/cmd_cut.go +++ b/cmd/chisel/cmd_cut.go @@ -154,27 +154,10 @@ func (cmd *cmdCut) Execute(args []string) error { } } - // Prepare state directory in target directory. - // There is an unlikely case that a package contains a file or directory - // with this name. That would lead to a collision when upgrading content - // from the temporary rootfs in the state directory to the target directory. - // TODO: Reserve this name. The release validation would then ensure it - // cannot be used in the release. - stateDir, err := slicer.MkStateDir(targetDir, 0o755) - if err != nil { - return fmt.Errorf("cannot create temporary working directory: %s", err) - } - defer func() { - // The state directory must only be removed if empty. This call will do so - // or silently fail. - os.Remove(stateDir) - }() - err = slicer.Run(&slicer.RunOptions{ Selection: selection, Archives: archives, TargetDir: targetDir, - StateDir: stateDir, PreviousManifest: mfest, Release: release, }) diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index 76d592f3..902e76cc 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -33,7 +33,6 @@ type RunOptions struct { Selection *setup.Selection Archives map[string]archive.Archive TargetDir string - StateDir string PreviousManifest *manifest.Manifest Release *setup.Release } @@ -95,10 +94,25 @@ func Run(options *RunOptions) error { cutTargetDir := options.TargetDir if options.PreviousManifest != nil { + // Prepare state directory in target directory. + // There is an unlikely case that a package contains a file or directory + // with this name. That would lead to a collision when upgrading content + // from the temporary rootfs in the state directory to the target directory. + // TODO: Reserve this name. The release validation would then ensure it + // cannot be used in the release. + stateDir, err := mkStateDir(targetDir) + if err != nil { + return err + } + defer func() { + // The state directory must only be removed if empty. This call will do so + // or silently fail. + os.Remove(stateDir) + }() // When recuting, cut inside a temporary workdir. - tmpWorkDir, err := os.MkdirTemp(options.StateDir, "chisel-workdir-*") + tmpWorkDir, err := os.MkdirTemp(stateDir, "workdir-*") if err != nil { - return fmt.Errorf("cannot create temporary working directory: %w", err) + return fmt.Errorf("cannot create working directory: %w", err) } cutTargetDir = tmpWorkDir defer func() { @@ -118,6 +132,42 @@ func Run(options *RunOptions) error { return nil } +const ( + stateDir = ".chisel" + stateMode os.FileMode = 0o755 +) + +// mkStateDir ensures the state dir exists under the given directory, with the proper +// permissions. +func mkStateDir(targetDir string) (dir string, err error) { + dir = filepath.Join(targetDir, stateDir) + defer func() { + if err != nil { + err = fmt.Errorf("cannot create state directory: %w", err) + } + }() + err = os.Mkdir(dir, stateMode) + if err != nil { + if !os.IsExist(err) { + return "", err + } + fileinfo, err := os.Lstat(dir) + if err != nil { + return "", err + } + if !fileinfo.IsDir() { + return "", fmt.Errorf("existing entry at %s is not a directory", dir) + } + // The needed mode might change between Chisel versions. Reset it to ensure + // backward compatibility. + err = os.Chmod(dir, stateMode) + if err != nil { + return "", err + } + } + return dir, nil +} + func cut(targetDir string, selection *setup.Selection, archives map[string]archive.Archive) error { pkgArchive, err := selectPkgArchives(archives, selection) if err != nil { diff --git a/internal/slicer/slicer_test.go b/internal/slicer/slicer_test.go index 34d2d196..c14fc618 100644 --- a/internal/slicer/slicer_test.go +++ b/internal/slicer/slicer_test.go @@ -2109,7 +2109,6 @@ func runSlicerTests(s *S, c *C, tests []slicerTest) { Selection: selection, Archives: archives, TargetDir: c.MkDir(), - StateDir: c.MkDir(), } if test.hackopt != nil { test.hackopt(c, &options) @@ -2185,7 +2184,7 @@ type slicerRecutTest struct { } var slicerRecutTests = []slicerRecutTest{{ - summary: "Basic upgrade", + summary: "Basic recut", cutSlices: []setup.SliceKey{{"test-package", "slice1"}}, recutSlices: []setup.SliceKey{{"test-package", "slice1"}, {"test-package", "slice2"}, {"other-package", "slice1"}}, pkgs: []*testutil.TestPackage{{ @@ -2433,6 +2432,95 @@ var slicerRecutTests = []slicerRecutTest{{ manifestPaths: map[string]string{ "/new-file": "file 0644 5b41362b {test-package_slice2}", }, +}, { + summary: "Recut fixes mode of existing .chisel directory", + cutSlices: []setup.SliceKey{{"test-package", "myslice"}}, + recutSlices: []setup.SliceKey{{"test-package", "myslice"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + myslice: + contents: + /file: {text: data1} + `, + }, + alterFilesystem: func(c *C, targetDir string) { + chiselPath := filepath.Join(targetDir, ".chisel") + err := os.Mkdir(chiselPath, 0o700) + c.Assert(err, IsNil) + }, + filesystem: map[string]string{ + "/file": "file 0644 5b41362b", + }, + manifestPaths: map[string]string{ + "/file": "file 0644 5b41362b {test-package_myslice}", + }, +}, { + summary: "Recut fails when .chisel path is not a dir", + cutSlices: []setup.SliceKey{{"test-package", "myslice"}}, + recutSlices: []setup.SliceKey{{"test-package", "myslice"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + myslice: + contents: + /file: {text: data1} + `, + }, + alterFilesystem: func(c *C, targetDir string) { + chiselPath := filepath.Join(targetDir, ".chisel") + err := os.Symlink("/nonexistent", chiselPath) + c.Assert(err, IsNil) + }, + error: `cannot create state directory: existing entry at .*/\.chisel is not a directory`, +}, { + summary: "Recut keeps non-empty .chisel directory", + cutSlices: []setup.SliceKey{{"test-package", "myslice"}}, + recutSlices: []setup.SliceKey{{"test-package", "myslice"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + myslice: + contents: + /file: {text: data1} + `, + }, + alterFilesystem: func(c *C, targetDir string) { + chiselPath := filepath.Join(targetDir, ".chisel") + err := os.Mkdir(chiselPath, 0o755) + c.Assert(err, IsNil) + err = os.WriteFile(filepath.Join(chiselPath, "keep"), []byte("keep"), 0o644) + c.Assert(err, IsNil) + }, + filesystem: map[string]string{ + "/.chisel/": "dir 0755", + "/.chisel/keep": "file 0644 6ca7ea2f", + "/file": "file 0644 5b41362b", + }, + manifestPaths: map[string]string{ + "/file": "file 0644 5b41362b {test-package_myslice}", + }, +}, { + summary: "Recut fails when target dir is not writable", + cutSlices: []setup.SliceKey{{"test-package", "myslice"}}, + recutSlices: []setup.SliceKey{{"test-package", "myslice"}}, + release: map[string]string{ + "slices/mydir/test-package.yaml": ` + package: test-package + slices: + myslice: + contents: + /file: {text: data1} + `, + }, + alterFilesystem: func(c *C, targetDir string) { + err := os.Chmod(targetDir, 0o555) + c.Assert(err, IsNil) + }, + error: `cannot create state directory: mkdir .*/\.chisel: permission denied`, }} func (s *S) TestRunRecut(c *C) { @@ -2519,12 +2607,10 @@ func (s *S) TestRunRecut(c *C) { } targetDir := c.MkDir() - stateDir := c.MkDir() options := slicer.RunOptions{ Selection: selection, Archives: archives, TargetDir: targetDir, - StateDir: stateDir, Release: release, } // First run. @@ -2547,7 +2633,6 @@ func (s *S) TestRunRecut(c *C) { Selection: selection, Archives: archives, TargetDir: targetDir, - StateDir: stateDir, PreviousManifest: mfest, Release: release, } diff --git a/internal/slicer/state.go b/internal/slicer/state.go deleted file mode 100644 index 2e3c90c3..00000000 --- a/internal/slicer/state.go +++ /dev/null @@ -1,40 +0,0 @@ -package slicer - -import ( - "fmt" - "os" - "path/filepath" -) - -const stateDir = ".chisel" - -// MkStateDir ensures the state dir exists under the given directory, with the proper -// permissions. -func MkStateDir(targetDir string, mode os.FileMode) (dir string, err error) { - dir = filepath.Join(targetDir, stateDir) - defer func() { - if err != nil { - err = fmt.Errorf("cannot create state directory: %w", err) - } - }() - err = os.Mkdir(dir, mode) - if err != nil { - if !os.IsExist(err) { - return "", err - } - fileinfo, err := os.Lstat(dir) - if err != nil { - return "", err - } - if !fileinfo.IsDir() { - return "", fmt.Errorf("existing entry at %s is not a directory", dir) - } - // The needed mode might change between Chisel versions. Reset it to ensure - // backward compatibility. - err = os.Chmod(dir, mode) - if err != nil { - return "", err - } - } - return dir, nil -} diff --git a/internal/slicer/state_test.go b/internal/slicer/state_test.go deleted file mode 100644 index 61547578..00000000 --- a/internal/slicer/state_test.go +++ /dev/null @@ -1,77 +0,0 @@ -package slicer_test - -import ( - "os" - "path/filepath" - - . "gopkg.in/check.v1" - - "github.com/canonical/chisel/internal/slicer" -) - -type mkStateDirTest struct { - summary string - mode os.FileMode - prepareTarget func(c *C, targetDir string) - dir string - error string -} - -var mkStateDirTests = []mkStateDirTest{{ - summary: "Create state dir from scratch", - mode: 0o755, - dir: ".chisel", -}, { - summary: "Create state dir with different mode", - mode: 0o700, - dir: ".chisel", -}, { - summary: "Existing state dir gets mode reset", - mode: 0o755, - prepareTarget: func(c *C, targetDir string) { - err := os.Mkdir(filepath.Join(targetDir, ".chisel"), 0o700) - c.Assert(err, IsNil) - }, - dir: ".chisel", -}, { - summary: "Existing non-directory entry at state dir path", - mode: 0o755, - prepareTarget: func(c *C, targetDir string) { - err := os.WriteFile(filepath.Join(targetDir, ".chisel"), []byte("data"), 0o644) - c.Assert(err, IsNil) - }, - error: `cannot create state directory: existing entry at .*/\.chisel is not a directory`, -}, { - summary: "Target dir does not exist", - mode: 0o755, - prepareTarget: func(c *C, targetDir string) { - err := os.RemoveAll(targetDir) - c.Assert(err, IsNil) - }, - error: `cannot create state directory: mkdir .*/\.chisel: no such file or directory`, -}} - -func (s *S) TestMkStateDir(c *C) { - for _, test := range mkStateDirTests { - c.Logf("Summary: %s", test.summary) - - targetDir := c.MkDir() - if test.prepareTarget != nil { - test.prepareTarget(c, targetDir) - } - - dir, err := slicer.MkStateDir(targetDir, test.mode) - if test.error != "" { - c.Assert(err, ErrorMatches, test.error) - continue - } - - c.Assert(err, IsNil) - c.Assert(dir, Equals, filepath.Join(targetDir, test.dir)) - - info, err := os.Lstat(dir) - c.Assert(err, IsNil) - c.Assert(info.IsDir(), Equals, true) - c.Assert(info.Mode().Perm(), Equals, test.mode) - } -}