diff --git a/internal/catalogue/contribution_dependencies_test.go b/internal/catalogue/contribution_dependencies_test.go index a1dc1b16..e56c9579 100644 --- a/internal/catalogue/contribution_dependencies_test.go +++ b/internal/catalogue/contribution_dependencies_test.go @@ -79,23 +79,38 @@ func TestAHolderMeetsItsOwnContribution(t *testing.T) { } } -func TestTwoModulesDeclaringOnePackageAreRefusedBeforeAnythingIsRecorded(t *testing.T) { +func TestTwoModulesMayDeclareOnePackage(t *testing.T) { pacman := withResources(mod("pacman", nil, nil, nil, Claim{Name: PackageManagerSeat}), res("package", "pacman-contrib")) bar := withResources(mod("bar", nil, nil, nil), res("package", "bar"), res("package", "pacman-contrib")) - other := withResources(mod("other", nil, nil, nil), res("package", "other")) - catalogue := map[string]Manifest{"pacman": pacman, "bar": bar, "other": other} + catalogue := map[string]Manifest{"pacman": pacman, "bar": bar} + if err := CollisionRefusal(catalogue, "laptop", []string{"pacman"}, []string{"bar"}); err != nil { + t.Fatalf("a second declaration of a package was refused (novox/hq ADR 0303): %v", err) + } +} - err := CollisionRefusal(catalogue, "laptop", []string{"pacman"}, []string{"bar"}) - if err == nil || !strings.Contains(err.Error(), "pacman-contrib") || !strings.Contains(err.Error(), "bar") { - t.Fatalf("the second owner of a package was not refused by name: %v", err) +func TestAPackageDeclaredPresentAndAbsentIsRefusedNamingBoth(t *testing.T) { + gone := res("package", "ufw") + gone["absent"] = true + filter := withResources(mod("nftables", nil, nil, nil), gone) + wants := withResources(mod("legacy", nil, nil, nil), res("package", "ufw")) + other := withResources(mod("other", nil, nil, nil), res("package", "other")) + catalogue := map[string]Manifest{"nftables": filter, "legacy": wants, "other": other} + + err := CollisionRefusal(catalogue, "laptop", []string{"nftables"}, []string{"legacy"}) + if err == nil || !strings.Contains(err.Error(), "ufw") || !strings.Contains(err.Error(), "nftables") || + !strings.Contains(err.Error(), "legacy") || !strings.Contains(err.Error(), "ADR 0303") { + t.Fatalf("a package declared present and absent was not refused naming both: %v", err) } - if err := CollisionRefusal(catalogue, "laptop", []string{"pacman"}, []string{"other"}); err != nil { - t.Errorf("a module declaring nothing shared is refused: %v", err) + if strings.Contains(err.Error(), "one owner per node") { + t.Errorf("the refusal gives the one-owner rule, which no longer holds for packages: %v", err) } - // A collision already on the node is status's, not a reason to refuse an unrelated assignment. - if err := CollisionRefusal(catalogue, "laptop", []string{"pacman", "bar"}, []string{"other"}); err != nil { - t.Errorf("an unrelated assignment is refused for a collision already there: %v", err) + if err := CollisionRefusal(catalogue, "laptop", []string{"nftables"}, []string{"other"}); err != nil { + t.Errorf("a module declaring nothing contradictory is refused: %v", err) + } + // A contradiction already on the node is status's, not a reason to refuse an unrelated assignment. + if err := CollisionRefusal(catalogue, "laptop", []string{"nftables", "legacy"}, []string{"other"}); err != nil { + t.Errorf("an unrelated assignment is refused for a contradiction already there: %v", err) } } diff --git a/internal/catalogue/resolve.go b/internal/catalogue/resolve.go index 6ed02150..5cd3aecf 100644 --- a/internal/catalogue/resolve.go +++ b/internal/catalogue/resolve.go @@ -960,6 +960,7 @@ func checkResources(modules []Manifest) []string { var problems []string owner := map[string]string{} ownedPath := map[string]string{} // path → owning module, for the access check below + packagePresent, packageAbsent := map[string][]string{}, map[string][]string{} for _, m := range modules { // Compared placed, not as written (novox/hq ADR 0112): ${dir:state} is the same six @@ -992,7 +993,22 @@ func checkResources(modules []Manifest) []string { } continue } - for _, field := range []string{"path", "unit", "name", "package"} { + if fmt.Sprint(r["type"]) == "package" { + // **A package may be declared by several modules** (novox/hq ADR 0303): declaring it says + // it is needed, and the host installs a missing one, never removes one a declaration + // stopped naming, so a second declaration changes nothing on the machine. What two + // modules can contradict is presence: one declaring it present and another absent + // (ADR 0180) would have the host install and remove it in turn, so that pair is refused. + if name, _ := r["package"].(string); name != "" { + if r["absent"] == true { + packageAbsent[name] = append(packageAbsent[name], m.Module) + } else { + packagePresent[name] = append(packagePresent[name], m.Module) + } + } + continue + } + for _, field := range []string{"path", "unit", "name"} { value, ok := r[field].(string) if !ok || value == "" { if field == "path" && fmt.Sprint(r["type"]) == "directory" { @@ -1038,6 +1054,12 @@ func checkResources(modules []Manifest) []string { ownedPath[fact.Path] = m.Module } } + for _, name := range packageNames(packageAbsent) { + if wanted := packagePresent[name]; len(wanted) > 0 { + problems = append(problems, fmt.Sprintf("%s declares the package %q absent and %s declare it present (novox/hq ADR 0303)", + strings.Join(packageAbsent[name], ", "), name, strings.Join(wanted, ", "))) + } + } // An accessed path is the operator's, so no module may declare it as one of its own // (novox/hq ADR 0051). Refused here rather than silently tolerated: an owner would create and @@ -1308,3 +1330,13 @@ func answeredElsewhere(want string, node Node, world World, brokered, keeps map[ holder, held := HolderAmong(want, world.Offered[want], world.Held) return held && holder.Node != node.Name } + +// packageNames are the packages a map names, in order. +func packageNames(m map[string][]string) []string { + names := make([]string, 0, len(m)) + for n := range m { + names = append(names, n) + } + sort.Strings(names) + return names +} diff --git a/internal/catalogue/seat_dependencies.go b/internal/catalogue/seat_dependencies.go index 2acb6ec5..088d83fb 100644 --- a/internal/catalogue/seat_dependencies.go +++ b/internal/catalogue/seat_dependencies.go @@ -349,6 +349,10 @@ func CollisionRefusal(catalogue map[string]Manifest, node string, assigned, addi if before[p] { continue // on the node already; not this assignment's doing } + if strings.Contains(p, "ADR 0303") { + problems = append(problems, p) // a package declared present and absent says its own rule + continue + } problems = append(problems, p+" (novox/hq ADR 0210: one owner per node; the other module "+ "depends on the owner's seat instead)") }