From 312b7083216a31020130002d4c294914200edc20 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Wed, 12 Feb 2025 16:49:29 +0100 Subject: [PATCH 1/8] feat: finish support for path conflicts with 'prefer' --- internal/setup/export_test.go | 8 +++ internal/setup/setup.go | 104 ++++++++++++++++++++++++++++----- internal/setup/setup_test.go | 100 ++++++++++++++++--------------- internal/setup/yaml.go | 7 ++- internal/slicer/slicer.go | 6 ++ internal/slicer/slicer_test.go | 61 +++++++++++++++++++ 6 files changed, 220 insertions(+), 66 deletions(-) diff --git a/internal/setup/export_test.go b/internal/setup/export_test.go index 35231e50..bfd83fd1 100644 --- a/internal/setup/export_test.go +++ b/internal/setup/export_test.go @@ -1,3 +1,11 @@ package setup type YAMLPath = yamlPath + +func (r *Release) SetPathOrdering(ordering map[string][]string) { + r.pathOrdering = ordering +} + +func (s *Selection) ClearCache() { + s.cachedSelectPackage = nil +} diff --git a/internal/setup/setup.go b/internal/setup/setup.go index e1e4d60e..eea50be6 100644 --- a/internal/setup/setup.go +++ b/internal/setup/setup.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "slices" "strings" "golang.org/x/crypto/openpgp/packet" @@ -19,6 +20,12 @@ type Release struct { Path string Packages map[string]*Package Archives map[string]*Archive + + // pathOrdering stores the sorted packages if there is a 'prefer' + // relationship. Otherwise, it will be nil. + // Given a selection of packages, the path should be extracted from the one + // that is found first on the list. + pathOrdering map[string][]string } // Archive is the location from which binary packages are obtained. @@ -118,8 +125,35 @@ func (s *Slice) String() string { return s.Package + "_" + s.Name } // the real information coming from packages is still unknown, so referenced // paths could potentially be missing, for example. type Selection struct { - Release *Release - Slices []*Slice + Release *Release + Slices []*Slice + cachedSelectPackage map[string]string +} + +// SelectPackage returns true if path should be extracted from pkg. +func (s *Selection) SelectPackage(path, pkg string) bool { + // If the path has no prefer relationships then it is always selected. + ordering, ok := s.Release.pathOrdering[path] + if !ok { + return true + } + + if cached, ok := s.cachedSelectPackage[path]; ok { + return cached == pkg + } + + var selected string + for _, pkg := range ordering { + i := slices.IndexFunc(s.Slices, func(s *Slice) bool { + return s.Package == pkg + }) + if i != -1 { + selected = s.Slices[i].Package + break + } + } + s.cachedSelectPackage[path] = selected + return selected == pkg } func ReadRelease(dir string) (*Release, error) { @@ -142,7 +176,7 @@ func ReadRelease(dir string) (*Release, error) { } func (r *Release) validate() error { - prefers, err := r.prefers() + prefers, pkgSampleSlice, err := r.prefers() if err != nil { return err } @@ -199,22 +233,57 @@ func (r *Release) validate() error { } } + for path, slice := range paths { + _, hasPrefers := prefers[preferKey{preferSource, path, ""}] + if !hasPrefers { + continue + } + pkg := slice.Package + for { + if _, ok := pkgSampleSlice[preferKey{path: path, pkg: pkg}]; !ok { + sourcePkg := prefers[preferKey{preferSource, path, pkg}] + sourceSlice := pkgSampleSlice[preferKey{path: path, pkg: sourcePkg}] + return fmt.Errorf("slice %s path %s 'prefer' refers to invalid package %q: package does not have path %s", sourceSlice, path, pkg, path) + } + r.pathOrdering[path] = append(r.pathOrdering[path], pkg) + pkg = prefers[preferKey{preferTarget, path, pkg}] + if pkg == "" { + break + } + } + slices.Reverse(r.pathOrdering[path]) + } + // Check for glob and generate conflicts. for oldPath, old := range globs { oldInfo := old.Contents[oldPath] for newPath, new := range paths { if oldPath == newPath { - // Identical paths have been filtered earlier. This must be the + // Identical globs have been filtered earlier. This must be the // exact same entry. continue } - newInfo := new.Contents[newPath] - if oldInfo.Kind == GlobPath && (newInfo.Kind == GlobPath || newInfo.Kind == CopyPath) { - if new.Package == old.Package { - continue + if !strdist.GlobPath(newPath, oldPath) { + continue + } + toCheck := []*Slice{new} + _, hasPrefers := prefers[preferKey{preferSource, newPath, ""}] + if hasPrefers { + toCheck = []*Slice{} + for _, pkg := range r.pathOrdering[newPath] { + s, _ := pkgSampleSlice[preferKey{path: newPath, pkg: pkg}] + toCheck = append(toCheck, s) } } - if strdist.GlobPath(newPath, oldPath) { + for _, new := range toCheck { + // It is okay to check only one slice per package because the + // content has been validated to be the same earlier. + newInfo := new.Contents[newPath] + if oldInfo.Kind == GlobPath && (newInfo.Kind == GlobPath || newInfo.Kind == CopyPath) { + if new.Package == old.Package { + continue + } + } if (old.Package > new.Package) || (old.Package == new.Package && old.Name > new.Name) || (old.Package == new.Package && old.Name == new.Name && oldPath > newPath) { old, new = new, old @@ -377,7 +446,8 @@ func Select(release *Release, slices []SliceKey) (*Selection, error) { logf("Selecting slices...") selection := &Selection{ - Release: release, + Release: release, + cachedSelectPackage: make(map[string]string), } sorted, err := order(release.Packages, slices) @@ -416,27 +486,31 @@ type preferKey struct { pkg string } -func (r *Release) prefers() (map[preferKey]string, error) { +func (r *Release) prefers() (map[preferKey]string, map[preferKey]*Slice, error) { prefers := make(map[preferKey]string) + pkgSampleSlice := make(map[preferKey]*Slice) for _, pkg := range r.Packages { for _, slice := range pkg.Slices { for path, info := range slice.Contents { + // Store a sample slice that contains the path. + pkgSampleSlice[preferKey{path: path, pkg: pkg.Name}] = slice + if info.Prefer != "" { if _, ok := r.Packages[info.Prefer]; !ok { - return nil, fmt.Errorf("slice %s path %s 'prefer' refers to undefined package %q", slice, path, info.Prefer) + return nil, nil, fmt.Errorf("slice %s path %s 'prefer' refers to undefined package %q", slice, path, info.Prefer) } tkey := preferKey{preferTarget, path, pkg.Name} skey := preferKey{preferSource, path, info.Prefer} if target, ok := prefers[tkey]; ok { if target != info.Prefer { pkg1, pkg2 := sortPair(target, info.Prefer) - return nil, fmt.Errorf("package %q has conflicting prefers for %s: %s != %s", + return nil, nil, fmt.Errorf("package %q has conflicting prefers for %s: %s != %s", pkg.Name, path, pkg1, pkg2) } } else if source, ok := prefers[skey]; ok { if source != pkg.Name { pkg1, pkg2 := sortPair(source, pkg.Name) - return nil, fmt.Errorf("packages %q and %q cannot both prefer %q for %s", + return nil, nil, fmt.Errorf("packages %q and %q cannot both prefer %q for %s", pkg1, pkg2, info.Prefer, path) } } else { @@ -449,7 +523,7 @@ func (r *Release) prefers() (map[preferKey]string, error) { } } } - return prefers, nil + return prefers, pkgSampleSlice, nil } func preferredPathPackage(path, pkg1, pkg2 string, prefers map[preferKey]string) (choice string, err error) { diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go index 9804678a..e608ca50 100644 --- a/internal/setup/setup_test.go +++ b/internal/setup/setup_test.go @@ -19,13 +19,14 @@ var ( ) type setupTest struct { - summary string - input map[string]string - release *setup.Release - relerror string - selslices []setup.SliceKey - selection *setup.Selection - selerror string + summary string + input map[string]string + release *setup.Release + relerror string + pathOrdering map[string][]string + selslices []setup.SliceKey + selection *setup.Selection + selerror string } var setupTests = []setupTest{{ @@ -2147,6 +2148,10 @@ var setupTests = []setupTest{{ }, }, }, + pathOrdering: map[string][]string{ + "/path": {"mypkg3", "mypkg2", "mypkg1"}, + "/link": {"mypkg1", "mypkg2"}, + }, }, { summary: "Cannot specify same package in 'prefer'", input: map[string]string{ @@ -2172,24 +2177,21 @@ var setupTests = []setupTest{{ }, relerror: `slice mypkg_myslice path /path 'prefer' refers to undefined package "non-existent"`, }, { - /* - TODO: handle this case: - summary: "Path prefers package, but package does not have path", - input: map[string]string{ - "slices/mydir/mypkg1.yaml": ` - package: mypkg1 - slices: - myslice: - contents: - /path: {prefer: mypkg2} - `, - "slices/mydir/mypkg2.yaml": ` - package: mypkg2 - `, - }, - relerror: `slice mypkg1_myslice path /path has invalid 'prefer' "mypkg2": package does not have path /path`, - }, { - */ + summary: "Path prefers package, but package does not have path", + input: map[string]string{ + "slices/mydir/mypkg1.yaml": ` + package: mypkg1 + slices: + myslice: + contents: + /path: {prefer: mypkg2} + `, + "slices/mydir/mypkg2.yaml": ` + package: mypkg2 + `, + }, + relerror: `slice mypkg1_myslice path /path 'prefer' refers to invalid package "mypkg2": package does not have path /path`, +}, { summary: "Path has 'prefer' cycle", input: map[string]string{ "slices/mydir/mypkg1.yaml": ` @@ -2327,29 +2329,26 @@ var setupTests = []setupTest{{ }, relerror: `packages "mypkg1" and "mypkg2" cannot both prefer "mypkg3" for /path`, }, { - /* - TODO: handle this case: - summary: "Glob paths can conflict with 'prefer' chain", - input: map[string]string{ - "slices/mydir/mypkg1.yaml": ` - package: mypkg1 - slices: - myslice: - contents: - /**: - /path: {prefer: mypkg2} - `, - "slices/mydir/mypkg2.yaml": ` - package: mypkg2 - slices: - myslice: - contents: - /path: - `, - }, - relerror: `slices mypkg1_myslice and mypkg2_myslice conflict on /\*\* and /path`, - }, { - */ + summary: "Glob paths can conflict with 'prefer' chain", + input: map[string]string{ + "slices/mydir/mypkg1.yaml": ` + package: mypkg1 + slices: + myslice: + contents: + /**: + /path: {prefer: mypkg2} + `, + "slices/mydir/mypkg2.yaml": ` + package: mypkg2 + slices: + myslice: + contents: + /path: + `, + }, + relerror: `slices mypkg1_myslice and mypkg2_myslice conflict on /\*\* and /path`, +}, { summary: "Slices of same package cannot have different 'prefer'", input: map[string]string{ "slices/mydir/mypkg1.yaml": ` @@ -2421,6 +2420,9 @@ func runParseReleaseTests(c *C, tests []setupTest) { if _, ok := test.input["chisel.yaml"]; !ok { test.input["chisel.yaml"] = string(defaultChiselYaml) } + if test.pathOrdering == nil { + test.pathOrdering = make(map[string][]string) + } dir := c.MkDir() for path, data := range test.input { @@ -2445,6 +2447,7 @@ func runParseReleaseTests(c *C, tests []setupTest) { release.Path = "" if test.release != nil { + test.release.SetPathOrdering(test.pathOrdering) c.Assert(release, DeepEquals, test.release) } @@ -2459,6 +2462,7 @@ func runParseReleaseTests(c *C, tests []setupTest) { c.Assert(selection.Release, Equals, release) selection.Release = nil if test.selection != nil { + selection.ClearCache() c.Assert(selection, DeepEquals, test.selection) } } diff --git a/internal/setup/yaml.go b/internal/setup/yaml.go index 651a8083..62d60083 100644 --- a/internal/setup/yaml.go +++ b/internal/setup/yaml.go @@ -151,9 +151,10 @@ type yamlPubKey struct { func parseRelease(baseDir, filePath string, data []byte) (*Release, error) { release := &Release{ - Path: baseDir, - Packages: make(map[string]*Package), - Archives: make(map[string]*Archive), + Path: baseDir, + Packages: make(map[string]*Package), + Archives: make(map[string]*Archive), + pathOrdering: make(map[string][]string), } fileName := stripBase(baseDir, filePath) diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index dca25a0b..aa0a7cd5 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -111,6 +111,9 @@ func Run(options *RunOptions) error { if len(pathInfo.Arch) > 0 && !slices.Contains(pathInfo.Arch, arch) { continue } + if !options.Selection.SelectPackage(targetPath, slice.Package) { + continue + } if pathInfo.Kind == setup.CopyPath || pathInfo.Kind == setup.GlobPath { sourcePath := pathInfo.Info @@ -254,6 +257,9 @@ func Run(options *RunOptions) error { pathInfo.Kind == setup.GeneratePath { continue } + if !options.Selection.SelectPackage(relPath, slice.Package) { + continue + } relPaths[relPath] = append(relPaths[relPath], slice) } } diff --git a/internal/slicer/slicer_test.go b/internal/slicer/slicer_test.go index e8ffecac..a2987fbf 100644 --- a/internal/slicer/slicer_test.go +++ b/internal/slicer/slicer_test.go @@ -1743,6 +1743,67 @@ var slicerTests = []slicerTest{{ "/file": "file 0644 2c26b46b <1> {test-package_myslice}", "/hardlink": "file 0644 2c26b46b <1> {test-package_myslice}", }, +}, { + summary: "Extract conflicting paths with prefer from proper package", + slices: []setup.SliceKey{ + {"test-package1", "myslice"}, + {"test-package2", "myslice"}, + }, + pkgs: []*testutil.TestPackage{{ + Name: "test-package1", + Data: testutil.MustMakeDeb([]testutil.TarEntry{ + testutil.Dir(0755, "./"), + testutil.Reg(0644, "./file", "foo"), + }), + }, { + Name: "test-package2", + Data: testutil.MustMakeDeb([]testutil.TarEntry{ + testutil.Dir(0755, "./"), + testutil.Reg(0644, "./file", "bar"), + }), + }, { + Name: "test-package3", + Data: testutil.MustMakeDeb([]testutil.TarEntry{ + testutil.Dir(0755, "./"), + }), + }}, + release: map[string]string{ + "slices/mydir/test-package1.yaml": ` + package: test-package1 + slices: + myslice: + contents: + /file: {prefer: test-package2} + /link: {symlink: /file1} + /text: {text: foo, prefer: test-package3} + `, + "slices/mydir/test-package2.yaml": ` + package: test-package2 + slices: + myslice: + contents: + /file: + /link: {symlink: /file2, prefer: test-package3} + `, + "slices/mydir/test-package3.yaml": ` + package: test-package3 + slices: + myslice: + contents: + /link: {symlink: /file2, prefer: test-package1} + /text: {text: bar} + `, + }, + filesystem: map[string]string{ + "/file": "file 0644 fcde2b2e", + "/link": "symlink /file1", + "/text": "file 0644 2c26b46b", + }, + manifestPaths: map[string]string{ + "/file": "file 0644 fcde2b2e {test-package2_myslice}", + "/link": "symlink /file1 {test-package1_myslice}", + "/text": "file 0644 2c26b46b {test-package1_myslice}", + }, }} var defaultChiselYaml = ` From 4d3ef9a4fdb630c013d19ad8d6230435ba3d4381 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Tue, 11 Mar 2025 17:08:14 +0100 Subject: [PATCH 2/8] refactor per Gustavo review --- internal/setup/export_test.go | 8 --- internal/setup/setup.go | 130 +++++++++++++++++----------------- internal/setup/setup_test.go | 41 +++++++---- internal/setup/yaml.go | 7 +- internal/slicer/slicer.go | 9 ++- 5 files changed, 100 insertions(+), 95 deletions(-) diff --git a/internal/setup/export_test.go b/internal/setup/export_test.go index bfd83fd1..35231e50 100644 --- a/internal/setup/export_test.go +++ b/internal/setup/export_test.go @@ -1,11 +1,3 @@ package setup type YAMLPath = yamlPath - -func (r *Release) SetPathOrdering(ordering map[string][]string) { - r.pathOrdering = ordering -} - -func (s *Selection) ClearCache() { - s.cachedSelectPackage = nil -} diff --git a/internal/setup/setup.go b/internal/setup/setup.go index eea50be6..72467a68 100644 --- a/internal/setup/setup.go +++ b/internal/setup/setup.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "path/filepath" - "slices" "strings" "golang.org/x/crypto/openpgp/packet" @@ -20,12 +19,6 @@ type Release struct { Path string Packages map[string]*Package Archives map[string]*Archive - - // pathOrdering stores the sorted packages if there is a 'prefer' - // relationship. Otherwise, it will be nil. - // Given a selection of packages, the path should be extracted from the one - // that is found first on the list. - pathOrdering map[string][]string } // Archive is the location from which binary packages are obtained. @@ -125,35 +118,48 @@ func (s *Slice) String() string { return s.Package + "_" + s.Name } // the real information coming from packages is still unknown, so referenced // paths could potentially be missing, for example. type Selection struct { - Release *Release - Slices []*Slice - cachedSelectPackage map[string]string + Release *Release + Slices []*Slice } -// SelectPackage returns true if path should be extracted from pkg. -func (s *Selection) SelectPackage(path, pkg string) bool { - // If the path has no prefer relationships then it is always selected. - ordering, ok := s.Release.pathOrdering[path] - if !ok { - return true - } - - if cached, ok := s.cachedSelectPackage[path]; ok { - return cached == pkg +// Perfers takes into account the prefer relationships and returns a map from +// path to the package where it should be extracted from. If there is no +// relationship for a given path then it will not be present on the map. +func (s *Selection) Prefers() (map[string]*Package, error) { + prefers, err := s.Release.prefers() + if err != nil { + return nil, err } - var selected string - for _, pkg := range ordering { - i := slices.IndexFunc(s.Slices, func(s *Slice) bool { - return s.Package == pkg - }) - if i != -1 { - selected = s.Slices[i].Package - break + pathPreferredPkg := make(map[string]*Package) + for _, slice := range s.Slices { + for path := range slice.Contents { + _, hasPrefers := prefers[preferKey{preferSource, path, ""}] + if !hasPrefers { + continue + } + old, ok := pathPreferredPkg[path] + if !ok { + pathPreferredPkg[path] = s.Release.Packages[slice.Package] + continue + } + if old.Name == slice.Package { + // Skip if the package was already recorded. + continue + } + preferred, err := preferredPathPackage(path, old.Name, slice.Package, prefers) + if err != nil { + // Note: we have checked above that the path has prefers and + // they are different packages so the error cannot be + // preferNone. + return nil, err + } + if preferred == old.Name { + pathPreferredPkg[path] = s.Release.Packages[slice.Package] + } } } - s.cachedSelectPackage[path] = selected - return selected == pkg + return pathPreferredPkg, nil } func ReadRelease(dir string) (*Release, error) { @@ -176,12 +182,13 @@ func ReadRelease(dir string) (*Release, error) { } func (r *Release) validate() error { - prefers, pkgSampleSlice, err := r.prefers() + prefers, err := r.prefers() if err != nil { return err } keys := []SliceKey(nil) + pathToSlice := make(map[preferKey]*Slice) // Check for info conflicts and prepare for following checks. A conflict // means that two slices attempt to extract different files or directories @@ -200,6 +207,8 @@ func (r *Release) validate() error { for _, new := range pkg.Slices { keys = append(keys, SliceKey{pkg.Name, new.Name}) for newPath, newInfo := range new.Contents { + // Store a sample slice that contains the path. + pathToSlice[preferKey{path: newPath, pkg: pkg.Name}] = new if old, ok := paths[newPath]; ok { if new.Package != old.Package { if p, err := preferredPathPackage(newPath, new.Package, old.Package, prefers); err == nil { @@ -233,25 +242,16 @@ func (r *Release) validate() error { } } - for path, slice := range paths { - _, hasPrefers := prefers[preferKey{preferSource, path, ""}] - if !hasPrefers { + // Check for invalid prefer relationships where the package does not have + // the path. + for skey, source := range prefers { + if skey.side != preferSource || skey.pkg == "" { continue } - pkg := slice.Package - for { - if _, ok := pkgSampleSlice[preferKey{path: path, pkg: pkg}]; !ok { - sourcePkg := prefers[preferKey{preferSource, path, pkg}] - sourceSlice := pkgSampleSlice[preferKey{path: path, pkg: sourcePkg}] - return fmt.Errorf("slice %s path %s 'prefer' refers to invalid package %q: package does not have path %s", sourceSlice, path, pkg, path) - } - r.pathOrdering[path] = append(r.pathOrdering[path], pkg) - pkg = prefers[preferKey{preferTarget, path, pkg}] - if pkg == "" { - break - } + if _, ok := pathToSlice[preferKey{path: skey.path, pkg: skey.pkg}]; !ok { + sourceSlice := pathToSlice[preferKey{path: skey.path, pkg: source}] + return fmt.Errorf("slice %s path %s 'prefer' refers to invalid package %q: package does not have path %s", sourceSlice, skey.path, skey.pkg, skey.path) } - slices.Reverse(r.pathOrdering[path]) } // Check for glob and generate conflicts. @@ -266,14 +266,13 @@ func (r *Release) validate() error { if !strdist.GlobPath(newPath, oldPath) { continue } - toCheck := []*Slice{new} - _, hasPrefers := prefers[preferKey{preferSource, newPath, ""}] - if hasPrefers { - toCheck = []*Slice{} - for _, pkg := range r.pathOrdering[newPath] { - s, _ := pkgSampleSlice[preferKey{path: newPath, pkg: pkg}] - toCheck = append(toCheck, s) - } + toCheck := []*Slice{} + pkg := new.Package + // Add all packages in the prefer relationship. + for pkg != "" { + slice, _ := pathToSlice[preferKey{path: newPath, pkg: pkg}] + toCheck = append(toCheck, slice) + pkg = prefers[preferKey{preferTarget, newPath, pkg}] } for _, new := range toCheck { // It is okay to check only one slice per package because the @@ -446,8 +445,7 @@ func Select(release *Release, slices []SliceKey) (*Selection, error) { logf("Selecting slices...") selection := &Selection{ - Release: release, - cachedSelectPackage: make(map[string]string), + Release: release, } sorted, err := order(release.Packages, slices) @@ -486,31 +484,27 @@ type preferKey struct { pkg string } -func (r *Release) prefers() (map[preferKey]string, map[preferKey]*Slice, error) { +func (r *Release) prefers() (map[preferKey]string, error) { prefers := make(map[preferKey]string) - pkgSampleSlice := make(map[preferKey]*Slice) for _, pkg := range r.Packages { for _, slice := range pkg.Slices { for path, info := range slice.Contents { - // Store a sample slice that contains the path. - pkgSampleSlice[preferKey{path: path, pkg: pkg.Name}] = slice - if info.Prefer != "" { if _, ok := r.Packages[info.Prefer]; !ok { - return nil, nil, fmt.Errorf("slice %s path %s 'prefer' refers to undefined package %q", slice, path, info.Prefer) + return nil, fmt.Errorf("slice %s path %s 'prefer' refers to undefined package %q", slice, path, info.Prefer) } tkey := preferKey{preferTarget, path, pkg.Name} skey := preferKey{preferSource, path, info.Prefer} if target, ok := prefers[tkey]; ok { if target != info.Prefer { pkg1, pkg2 := sortPair(target, info.Prefer) - return nil, nil, fmt.Errorf("package %q has conflicting prefers for %s: %s != %s", + return nil, fmt.Errorf("package %q has conflicting prefers for %s: %s != %s", pkg.Name, path, pkg1, pkg2) } } else if source, ok := prefers[skey]; ok { if source != pkg.Name { pkg1, pkg2 := sortPair(source, pkg.Name) - return nil, nil, fmt.Errorf("packages %q and %q cannot both prefer %q for %s", + return nil, fmt.Errorf("packages %q and %q cannot both prefer %q for %s", pkg1, pkg2, info.Prefer, path) } } else { @@ -523,9 +517,13 @@ func (r *Release) prefers() (map[preferKey]string, map[preferKey]*Slice, error) } } } - return prefers, pkgSampleSlice, nil + + return prefers, nil } +// preferredPathPackage returns pkg1 if one can reach pkg2 from pkg1 using +// prefer relationship, and conversely for pkg2. If none are reachable or there +// is a cycle, it returns an error. func preferredPathPackage(path, pkg1, pkg2 string, prefers map[preferKey]string) (choice string, err error) { pkg1, pkg2 = sortPair(pkg1, pkg2) prefer1, err := findPrefer(path, pkg1, pkg2, prefers) diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go index e608ca50..c634b8ce 100644 --- a/internal/setup/setup_test.go +++ b/internal/setup/setup_test.go @@ -19,14 +19,14 @@ var ( ) type setupTest struct { - summary string - input map[string]string - release *setup.Release - relerror string - pathOrdering map[string][]string - selslices []setup.SliceKey - selection *setup.Selection - selerror string + summary string + input map[string]string + release *setup.Release + relerror string + prefers map[string]string + selslices []setup.SliceKey + selection *setup.Selection + selerror string } var setupTests = []setupTest{{ @@ -2059,6 +2059,12 @@ var setupTests = []setupTest{{ relerror: "slice mypkg1_myslice1 cannot 'prefer' its own package for path /file", }, { summary: "Path conflicts with 'prefer'", + selslices: []setup.SliceKey{ + {"mypkg1", "myslice1"}, + {"mypkg1", "myslice2"}, + {"mypkg2", "myslice1"}, + {"mypkg3", "myslice1"}, + }, input: map[string]string{ "slices/mydir/mypkg1.yaml": ` package: mypkg1 @@ -2148,9 +2154,9 @@ var setupTests = []setupTest{{ }, }, }, - pathOrdering: map[string][]string{ - "/path": {"mypkg3", "mypkg2", "mypkg1"}, - "/link": {"mypkg1", "mypkg2"}, + prefers: map[string]string{ + "/path": "mypkg3", + "/link": "mypkg1", }, }, { summary: "Cannot specify same package in 'prefer'", @@ -2420,8 +2426,8 @@ func runParseReleaseTests(c *C, tests []setupTest) { if _, ok := test.input["chisel.yaml"]; !ok { test.input["chisel.yaml"] = string(defaultChiselYaml) } - if test.pathOrdering == nil { - test.pathOrdering = make(map[string][]string) + if test.prefers == nil { + test.prefers = make(map[string]string) } dir := c.MkDir() @@ -2447,7 +2453,6 @@ func runParseReleaseTests(c *C, tests []setupTest) { release.Path = "" if test.release != nil { - test.release.SetPathOrdering(test.pathOrdering) c.Assert(release, DeepEquals, test.release) } @@ -2460,9 +2465,15 @@ func runParseReleaseTests(c *C, tests []setupTest) { c.Assert(err, IsNil) } c.Assert(selection.Release, Equals, release) + rawPrefers, err := selection.Prefers() + c.Assert(err, IsNil) + prefers := make(map[string]string) + for path, pkg := range rawPrefers { + prefers[path] = pkg.Name + } + c.Assert(prefers, DeepEquals, test.prefers) selection.Release = nil if test.selection != nil { - selection.ClearCache() c.Assert(selection, DeepEquals, test.selection) } } diff --git a/internal/setup/yaml.go b/internal/setup/yaml.go index 62d60083..651a8083 100644 --- a/internal/setup/yaml.go +++ b/internal/setup/yaml.go @@ -151,10 +151,9 @@ type yamlPubKey struct { func parseRelease(baseDir, filePath string, data []byte) (*Release, error) { release := &Release{ - Path: baseDir, - Packages: make(map[string]*Package), - Archives: make(map[string]*Archive), - pathOrdering: make(map[string][]string), + Path: baseDir, + Packages: make(map[string]*Package), + Archives: make(map[string]*Archive), } fileName := stripBase(baseDir, filePath) diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index aa0a7cd5..36995222 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -95,6 +95,11 @@ func Run(options *RunOptions) error { return err } + prefers, err := options.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 { @@ -111,7 +116,7 @@ func Run(options *RunOptions) error { if len(pathInfo.Arch) > 0 && !slices.Contains(pathInfo.Arch, arch) { continue } - if !options.Selection.SelectPackage(targetPath, slice.Package) { + if preferredPkg, ok := prefers[targetPath]; ok && preferredPkg.Name != slice.Package { continue } @@ -257,7 +262,7 @@ func Run(options *RunOptions) error { pathInfo.Kind == setup.GeneratePath { continue } - if !options.Selection.SelectPackage(relPath, slice.Package) { + if preferredPkg, ok := prefers[relPath]; ok && preferredPkg.Name != slice.Package { continue } relPaths[relPath] = append(relPaths[relPath], slice) From 1d52f447a2b69075592ced2dc566797328fa2eb6 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Tue, 11 Mar 2025 17:12:35 +0100 Subject: [PATCH 3/8] remove extra newline --- internal/setup/setup.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/setup/setup.go b/internal/setup/setup.go index 72467a68..d356ebd0 100644 --- a/internal/setup/setup.go +++ b/internal/setup/setup.go @@ -517,7 +517,6 @@ func (r *Release) prefers() (map[preferKey]string, error) { } } } - return prefers, nil } From 19518d973f08301f303800d3c0ef37ad3da7a71e Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Tue, 11 Mar 2025 17:14:27 +0100 Subject: [PATCH 4/8] typo --- internal/setup/setup.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/setup/setup.go b/internal/setup/setup.go index d356ebd0..7002b470 100644 --- a/internal/setup/setup.go +++ b/internal/setup/setup.go @@ -521,8 +521,8 @@ func (r *Release) prefers() (map[preferKey]string, error) { } // preferredPathPackage returns pkg1 if one can reach pkg2 from pkg1 using -// prefer relationship, and conversely for pkg2. If none are reachable or there -// is a cycle, it returns an error. +// prefer relationships, and conversely for pkg2. If none are reachable or +// there is a cycle, it returns an error. func preferredPathPackage(path, pkg1, pkg2 string, prefers map[preferKey]string) (choice string, err error) { pkg1, pkg2 = sortPair(pkg1, pkg2) prefer1, err := findPrefer(path, pkg1, pkg2, prefers) From 688ae6cb740fe7892a773bf1e67122da84086d49 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Wed, 12 Mar 2025 12:33:15 +0100 Subject: [PATCH 5/8] finishing touches --- internal/setup/setup.go | 16 +++++++++------ internal/setup/setup_test.go | 39 ++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/internal/setup/setup.go b/internal/setup/setup.go index 7002b470..0730424c 100644 --- a/internal/setup/setup.go +++ b/internal/setup/setup.go @@ -122,9 +122,9 @@ type Selection struct { Slices []*Slice } -// Perfers takes into account the prefer relationships and returns a map from -// path to the package where it should be extracted from. If there is no -// relationship for a given path then it will not be present on the map. +// Perfers uses the prefer relationships and returns a map from each path to +// the package where it should be extracted from. If there is no relationship +// for a given path then it will not be present on the map. func (s *Selection) Prefers() (map[string]*Package, error) { prefers, err := s.Release.prefers() if err != nil { @@ -246,6 +246,7 @@ func (r *Release) validate() error { // the path. for skey, source := range prefers { if skey.side != preferSource || skey.pkg == "" { + // preferTarget packages have the path by construction. continue } if _, ok := pathToSlice[preferKey{path: skey.path, pkg: skey.pkg}]; !ok { @@ -268,7 +269,8 @@ func (r *Release) validate() error { } toCheck := []*Slice{} pkg := new.Package - // Add all packages in the prefer relationship. + // Add all packages in the prefer relationship. If there is no + // relationship only the current package gets added. for pkg != "" { slice, _ := pathToSlice[preferKey{path: newPath, pkg: pkg}] toCheck = append(toCheck, slice) @@ -521,8 +523,10 @@ func (r *Release) prefers() (map[preferKey]string, error) { } // preferredPathPackage returns pkg1 if one can reach pkg2 from pkg1 using -// prefer relationships, and conversely for pkg2. If none are reachable or -// there is a cycle, it returns an error. +// prefer relationships, and conversely for pkg2. If none are reachable it +// returns the preferNone error. +// +// If there is a cycle, an error is returned. func preferredPathPackage(path, pkg1, pkg2 string, prefers map[preferKey]string) (choice string, err error) { pkg1, pkg2 = sortPair(pkg1, pkg2) prefer1, err := findPrefer(path, pkg1, pkg2, prefers) diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go index c634b8ce..d8d7704c 100644 --- a/internal/setup/setup_test.go +++ b/internal/setup/setup_test.go @@ -2158,6 +2158,45 @@ var setupTests = []setupTest{{ "/path": "mypkg3", "/link": "mypkg1", }, +}, { + summary: "Path conflicts with 'prefer' depends on selection", + selslices: []setup.SliceKey{ + {"mypkg1", "myslice1"}, + {"mypkg1", "myslice2"}, + {"mypkg2", "myslice1"}, + }, + input: map[string]string{ + "slices/mydir/mypkg1.yaml": ` + package: mypkg1 + slices: + myslice1: + contents: + /path: {prefer: mypkg2} + /link: {symlink: /file1} + myslice2: + contents: + /path: {prefer: mypkg2} + `, + "slices/mydir/mypkg2.yaml": ` + package: mypkg2 + slices: + myslice1: + contents: + /path: {prefer: mypkg3} + /link: {symlink: /file2, prefer: mypkg1} + `, + "slices/mydir/mypkg3.yaml": ` + package: mypkg3 + slices: + myslice1: + contents: + /path: + `, + }, + prefers: map[string]string{ + "/path": "mypkg2", + "/link": "mypkg1", + }, }, { summary: "Cannot specify same package in 'prefer'", input: map[string]string{ From 1f398516dca7fc997522331328d7115203bde872 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Fri, 28 Mar 2025 17:44:35 +0100 Subject: [PATCH 6/8] only record slices when path is in prefer chain --- internal/setup/setup.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/setup/setup.go b/internal/setup/setup.go index 0730424c..40ee4a37 100644 --- a/internal/setup/setup.go +++ b/internal/setup/setup.go @@ -207,8 +207,11 @@ func (r *Release) validate() error { for _, new := range pkg.Slices { keys = append(keys, SliceKey{pkg.Name, new.Name}) for newPath, newInfo := range new.Contents { - // Store a sample slice that contains the path. - pathToSlice[preferKey{path: newPath, pkg: pkg.Name}] = new + if _, ok := prefers[preferKey{preferSource, newPath, ""}]; ok { + // If this is part of a prefer chain, store a sample slice that + // contains the path. + pathToSlice[preferKey{path: newPath, pkg: pkg.Name}] = new + } if old, ok := paths[newPath]; ok { if new.Package != old.Package { if p, err := preferredPathPackage(newPath, new.Package, old.Package, prefers); err == nil { @@ -267,14 +270,17 @@ func (r *Release) validate() error { if !strdist.GlobPath(newPath, oldPath) { continue } - toCheck := []*Slice{} + toCheck := []*Slice{new} pkg := new.Package // Add all packages in the prefer relationship. If there is no // relationship only the current package gets added. - for pkg != "" { - slice, _ := pathToSlice[preferKey{path: newPath, pkg: pkg}] - toCheck = append(toCheck, slice) + for { pkg = prefers[preferKey{preferTarget, newPath, pkg}] + if pkg == "" { + break + } + slice := pathToSlice[preferKey{path: newPath, pkg: pkg}] + toCheck = append(toCheck, slice) } for _, new := range toCheck { // It is okay to check only one slice per package because the From df70106d4939d68489464bdff020c7423d24f38c Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Fri, 28 Mar 2025 17:46:52 +0100 Subject: [PATCH 7/8] improve error message --- internal/setup/setup.go | 2 +- internal/setup/setup_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/setup/setup.go b/internal/setup/setup.go index 40ee4a37..5762fe48 100644 --- a/internal/setup/setup.go +++ b/internal/setup/setup.go @@ -254,7 +254,7 @@ func (r *Release) validate() error { } if _, ok := pathToSlice[preferKey{path: skey.path, pkg: skey.pkg}]; !ok { sourceSlice := pathToSlice[preferKey{path: skey.path, pkg: source}] - return fmt.Errorf("slice %s path %s 'prefer' refers to invalid package %q: package does not have path %s", sourceSlice, skey.path, skey.pkg, skey.path) + return fmt.Errorf("slice %s prefers package %q which does not contain path %s", sourceSlice, skey.pkg, skey.path) } } diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go index d8d7704c..f6e030fe 100644 --- a/internal/setup/setup_test.go +++ b/internal/setup/setup_test.go @@ -2235,7 +2235,7 @@ var setupTests = []setupTest{{ package: mypkg2 `, }, - relerror: `slice mypkg1_myslice path /path 'prefer' refers to invalid package "mypkg2": package does not have path /path`, + relerror: `slice mypkg1_myslice prefers package "mypkg2" which does not contain path /path`, }, { summary: "Path has 'prefer' cycle", input: map[string]string{ From 179f297fe5fb7713327c8e6c9ce20b70d9a98cd6 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Fri, 28 Mar 2025 17:54:40 +0100 Subject: [PATCH 8/8] better naming --- internal/setup/setup.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/setup/setup.go b/internal/setup/setup.go index 5762fe48..e5126ef2 100644 --- a/internal/setup/setup.go +++ b/internal/setup/setup.go @@ -207,7 +207,7 @@ func (r *Release) validate() error { for _, new := range pkg.Slices { keys = append(keys, SliceKey{pkg.Name, new.Name}) for newPath, newInfo := range new.Contents { - if _, ok := prefers[preferKey{preferSource, newPath, ""}]; ok { + if _, hasPrefers := prefers[preferKey{preferSource, newPath, ""}]; hasPrefers { // If this is part of a prefer chain, store a sample slice that // contains the path. pathToSlice[preferKey{path: newPath, pkg: pkg.Name}] = new