Merge pull request 'Let several modules declare one package, and refuse only present beside absent (hq ADR 0303)' (#237) from feat/303-shared-packages into main

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}
-	catalogue := map[string]Manifest{"pacman": pacman, "bar": bar, "other": other}
+	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"})
+func TestAPackageDeclaredPresentAndAbsentIsRefusedNamingBoth(t *testing.T) {
-	if err == nil || !strings.Contains(err.Error(), "pacman-contrib") || !strings.Contains(err.Error(), "bar") {
+	gone := res("package", "ufw")
-		t.Fatalf("the second owner of a package was not refused by name: %v", err)
+	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 {
+	if strings.Contains(err.Error(), "one owner per node") {
-		t.Errorf("a module declaring nothing shared is refused: %v", err)
+		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{"nftables"}, []string{"other"}); err != nil {
-	if err := CollisionRefusal(catalogue, "laptop", []string{"pacman", "bar"}, []string{"other"}); err != nil {
+		t.Errorf("a module declaring nothing contradictory is refused: %v", err)
-		t.Errorf("an unrelated assignment is refused for a collision already there: %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)
 	}
 }
 

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
+}

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)")
 	}