From b24bb030ec8dd47adf0ab133adccc2785213f1ed Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 6 Oct 2026 22:18:14 +0200 Subject: [PATCH] A changed file touches exactly the modules whose build reads it (hq issue 280, ADR 0237) The builder reads a module's own directory (the repository for one built from its root) and a repository its recipe packages, nothing else. A file in no module's directory was read as shared code and rebuilt everything built from the repository: 103 modules for a merge-check.sh added at the catalogue's root. It now touches nothing, in the merge handler, the release planner and the pull request's check alike, and the gate says so. --- cmd/mesh-controller/checks.go | 9 +-- cmd/mesh-controller/checks_test.go | 7 +- cmd/mesh-controller/merge_gate.go | 21 +++--- cmd/mesh-controller/merge_gate_test.go | 28 ++++---- cmd/mesh-controller/order_test.go | 27 ++++---- cmd/mesh-controller/upgrades.go | 91 +++++--------------------- internal/link/events.go | 4 +- 7 files changed, 71 insertions(+), 116 deletions(-) diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index 306c8e6..4dd4678 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -22,10 +22,11 @@ import ( // // **The mesh's module graph decides, not the repository.** The controller holds the graph — every module, // the repository and directory it is built from — and maps the pull request's changed paths onto it by -// the rule the merge handler uses (issue 278): a changed file inside a directory holding a module.json at -// the head is that module's, held or not; a file in no such directory, in a repository modules are built -// from into that branch, is shared code, and touches every module built from it there. A directory the -// change adds a module in, which the graph does not hold yet, is a new module and is checked too. +// the rule the merge handler and the release planner use (whatTheMergeTouched, issue 280): **a changed file +// touches exactly the modules whose build reads it** — a module's own directory (the whole repository for +// one built from its root), or a repository its recipe packages. A file no build reads — a script at the +// root, a README — touches no module. A directory the change adds a module.json in, which the graph does +// not hold yet (said by the head, issue 278), is a new module and is checked too. // // - touches a module: the build seat runs **the gate** — `mesh/merge-gate`, the touched manifests, every // machine composed with the change, the replays — and the repository's own merge-check.sh beside it; diff --git a/cmd/mesh-controller/checks_test.go b/cmd/mesh-controller/checks_test.go index c6fa582..f60f1b7 100644 --- a/cmd/mesh-controller/checks_test.go +++ b/cmd/mesh-controller/checks_test.go @@ -41,8 +41,11 @@ func TestThePullRequestIsMappedOntoTheModuleGraph(t *testing.T) { }{ {"one module's own files", pull("novox", "mesh-catalog", "main", []string{"modules/gitea/index.ts"}, "modules/gitea"), "gitea", "", "modules/gitea/module.json", true, ""}, - {"shared code touches every module built from the repository", - pull("novox", "mesh-catalog", "main", []string{"tsconfig.json"}), "gitea,keycloak,nats", "", + {"a file no module's build reads touches no module (issue 280)", + pull("novox", "mesh-catalog", "main", []string{"merge-check.sh", "README.md"}), "", "", "", true, ""}, + {"files the announcer could not list: everything built from it", + link.PullUpdated{Owner: "novox", Repo: "mesh-catalog", Base: "main", PathsTruncated: true, + Paths: []string{"README.md"}}, "gitea,keycloak,nats", "", "modules/gitea/module.json,modules/keycloak/module.json,modules/nats/module.json", true, ""}, {"a new module, said by the head", pull("novox", "mesh-catalog", "main", []string{"modules/newmod/index.ts", "modules/newmod/module.json"}, "modules/newmod"), diff --git a/cmd/mesh-controller/merge_gate.go b/cmd/mesh-controller/merge_gate.go index 2bd68c5..266fb5c 100644 --- a/cmd/mesh-controller/merge_gate.go +++ b/cmd/mesh-controller/merge_gate.go @@ -108,9 +108,9 @@ type mergeComposed struct { type mergeWidthOf struct { Modules []string `json:"modules"` Tiers int `json:"tiers"` - // Shared is the changed paths read as shared code: in no directory of a module the mesh holds, so - // everything built from the repository is rebuilt for them. - Shared []string `json:"shared,omitempty"` + // Unread is the changed paths no module's build reads — in no module's directory — which rebuild + // nothing (issue 280): said, so a reader sees why a change to them moves no module. + Unread []string `json:"unread,omitempty"` } // wideRebuild is how many modules a rebuild may take before the gate says so as a warning. @@ -312,11 +312,11 @@ func judgeChange(ctx context.Context, in mergeCheckInput) (mergeVerdict, error) v.Notes = append(v.Notes, "the width of the rebuild could not be worked out: "+err.Error()) } else { v.Width = &w - if len(w.Shared) > 0 && len(w.Modules) > wideRebuild { - v.Warnings = append(v.Warnings, fmt.Sprintf("a merge rebuilds %d module(s), because %s read as shared "+ - "code: in no directory of a module the mesh holds (issue 278)", len(w.Modules), - readableList(w.Shared))) - } else if len(w.Modules) > wideRebuild { + if len(w.Unread) > 0 { + v.Notes = append(v.Notes, fmt.Sprintf("%s read by no module's build, and rebuild nothing (issue 280)", + readableList(w.Unread))) + } + if len(w.Modules) > wideRebuild { v.Warnings = append(v.Warnings, fmt.Sprintf("a merge rebuilds %d module(s) in %d tier(s)", len(w.Modules), w.Tiers)) } @@ -1016,7 +1016,8 @@ func rebuildWidth(f snapshot.Facts, repository string, paths []string, tree stri } sort.Strings(w.Modules) } - // The paths read as shared: in no directory of a module the mesh holds from this repository. + // The paths no build reads: in no directory of a module the mesh holds from this repository, or one + // the tree holds. var dirs []string for _, e := range from { if d := strings.Trim(e.Source.Path, "/"); d != "" { @@ -1032,7 +1033,7 @@ func rebuildWidth(f snapshot.Facts, repository string, paths []string, tree stri inModule = inModule || strings.HasPrefix(p, d) } if !inModule && len(dirs) > 0 { - w.Shared = append(w.Shared, p) + w.Unread = append(w.Unread, p) } } return w, nil diff --git a/cmd/mesh-controller/merge_gate_test.go b/cmd/mesh-controller/merge_gate_test.go index c5280ee..95fc9b4 100644 --- a/cmd/mesh-controller/merge_gate_test.go +++ b/cmd/mesh-controller/merge_gate_test.go @@ -180,26 +180,32 @@ func TestAModuleAMachineRunsRemovedFromItsSourceFails(t *testing.T) { } } -// **Issue 278**: a file of a module nobody holds read as shared code, and the merge rebuilt the -// catalogue. The gate says how wide a merge's rebuild is, and warns when shared code makes it wide. +// **Issues 278 and 280**: a file in no held module's directory read as shared code, and a merge rebuilt the +// catalogue. A file is a module's only by being inside it — no build reads one outside — so it rebuilds +// nothing, and the gate says why; a merge that does rebuild widely is warned of. func TestIssue278AWideRebuildIsSaidBeforeTheMerge(t *testing.T) { f, manifests := catalogueMesh(t) was := wideRebuild wideRebuild = 2 t.Cleanup(func() { wideRebuild = was }) - v := gateJudged(t, f, aTree(t, manifests), "modules/showcase/index.ts") - if v.Width == nil || len(v.Width.Modules) < 5 || len(v.Width.Shared) != 1 { - t.Fatalf("the width reads %+v", v.Width) + for _, file := range []string{"modules/showcase/index.ts", "merge-check.sh", "README.md"} { + v := gateJudged(t, f, aTree(t, manifests), file) + if v.Width == nil || len(v.Width.Modules) != 0 || len(v.Width.Unread) != 1 || v.Verdict != "pass" { + t.Fatalf("%s, read by no build, reads %+v, %s", file, v.Width, v.Verdict) + } + if !strings.Contains(v.Report(), "read by no module's build") { + t.Errorf("why %s rebuilds nothing is not said:\n%s", file, v.Report()) + } } - if v.Verdict != "warning" || !strings.Contains(v.Report(), "shared") { - t.Fatalf("a rebuild of everything for one shared file is not said:\n%s", v.Report()) + v := gateJudged(t, f, aTree(t, manifests), "modules/album/x.ts", "modules/objects/x.go", "modules/resolver/x.go") + if v.Width == nil || len(v.Width.Modules) < 3 || v.Verdict != "warning" { + t.Fatalf("a rebuild wider than the bound is not warned of: %+v, %s", v.Width, v.Verdict) } - // The same file, with the reference module's definition in the tree: its directory is a module, held - // or not, and the file is its business alone (the fix of 278, read from the tree as the announcer does). + // With the reference module's definition in the tree, its directory is a module, held or not. withShowcase := withEdit(manifests, "showcase", `{"module":"showcase","version":"1"}`) v = gateJudged(t, f, aTree(t, withShowcase), "modules/showcase/index.ts") - if v.Width == nil || len(v.Width.Modules) != 0 || len(v.Width.Shared) != 0 { - t.Fatalf("a file of a module nobody holds still reads as shared: %+v", v.Width) + if v.Width == nil || len(v.Width.Modules) != 0 || len(v.Width.Unread) != 0 { + t.Fatalf("a file of a module nobody holds reads %+v", v.Width) } v = gateJudged(t, f, aTree(t, manifests), "modules/album/module.json") if v.Width == nil || strings.Join(v.Width.Modules, ",") != "album" || v.Verdict != "pass" { diff --git a/cmd/mesh-controller/order_test.go b/cmd/mesh-controller/order_test.go index 5fe7b12..7c6a901 100644 --- a/cmd/mesh-controller/order_test.go +++ b/cmd/mesh-controller/order_test.go @@ -144,7 +144,7 @@ func TestAMergeRebuildsTheModulesItChanged(t *testing.T) { }{ {"one module's own files", merge([]string{"modules/gitea/index.ts", "modules/gitea/client.ts"}, false), "gitea"}, {"two modules' files", merge([]string{"modules/gitea/index.ts", "modules/keycloak/module.json"}, false), "gitea,keycloak"}, - {"a file they share", merge([]string{"tsconfig.json"}, false), "gitea,keycloak"}, + {"a file at the root no build reads (issue 280)", merge([]string{"tsconfig.json"}, false), ""}, {"a module the mesh does not hold", merge([]string{"modules/plex/index.ts"}, false), ""}, {"nothing said about the files", merge(nil, false), "gitea,keycloak"}, {"more files than were listed", merge([]string{"modules/gitea/index.ts"}, true), "gitea,keycloak"}, @@ -153,9 +153,9 @@ func TestAMergeRebuildsTheModulesItChanged(t *testing.T) { {"a new module beside a held one", merge([]string{"modules/gitea/x", "modules/newmod/module.json"}, false), "gitea"}, {"a new module's other files", merge([]string{"modules/newmod/index.ts", "modules/newmod/module.json"}, false), ""}, {"a module removed", merge([]string{"modules/gone/module.json"}, false), ""}, - {"a directory with no manifest", merge([]string{"modules/lib/x.go"}, false), "gitea,keycloak"}, - {"a file directly among the modules", merge([]string{"modules/README.md"}, false), "gitea,keycloak"}, - {"the root's files still", merge([]string{"tsconfig.json"}, false), "gitea,keycloak"}, + {"a directory with no manifest", merge([]string{"modules/lib/x.go"}, false), ""}, + {"a file directly among the modules", merge([]string{"modules/README.md"}, false), ""}, + {"a root file beside a module's", merge([]string{"merge-check.sh", "modules/keycloak/x.ts"}, false), "keycloak"}, } { if got := named(whatTheMergeTouched(candidates, known, c.m)); got != c.want { t.Errorf("%s: rebuilt %q, wanted %q", c.what, got, c.want) @@ -245,12 +245,11 @@ func TestAModuleBuiltAtACommitStillFollowsItsBranch(t *testing.T) { } } -// novox/hq issue 278: whether a directory is a module is a fact of the repository at the merge commit, -// and the announcer says it. A merge touching the code of a module the mesh does not hold — the +// novox/hq issues 278 and 280: a merge touching the code of a module the mesh does not hold — the // catalogue's reference module, whose manifest it left alone — read as shared and rebuilt every module -// built from the repository (103 of them on 2026-10-06, 88 byte-identical). Said by the announcer, it -// rebuilds nothing; a directory holding no manifest is shared as before; and an announcer that does not -// say keeps the old rule. +// built from the repository (103 of them on 2026-10-06, 88 byte-identical); so did a merge-check.sh added at +// the root. No build reads a file outside its module's directory, so neither rebuilds anything, said by the +// announcer or not. func TestAChangeInsideAModuleIsThatModulesHeldOrNot(t *testing.T) { const repo = "http://forge.internal:20000/novox/mesh-catalog.git" gitea := fromRepo("gitea", repo, "modules/gitea") @@ -278,12 +277,12 @@ func TestAChangeInsideAModuleIsThatModulesHeldOrNot(t *testing.T) { {"beside a held one's change", merge(append([]string{"modules/gitea/index.ts"}, showcase...), []string{"modules/gitea", "modules/showcase"}, true), "gitea"}, {"deeper inside it", merge([]string{"modules/showcase/daemon/index.ts"}, []string{"modules/showcase"}, true), ""}, - {"a directory holding no manifest is still shared", merge([]string{"modules/lib/x.go"}, nil, true), "gitea,keycloak"}, + {"a directory holding no manifest, read by no build", merge([]string{"modules/lib/x.go"}, nil, true), ""}, {"one of two files in no module", merge([]string{"modules/showcase/index.ts", "modules/lib/x.go"}, - []string{"modules/showcase"}, true), "gitea,keycloak"}, - {"the root is never a module directory", merge([]string{"tsconfig.json"}, []string{"", "/", "."}, true), "gitea,keycloak"}, - {"not said: the old rule", merge(showcase, []string{"modules/showcase"}, false), "gitea,keycloak"}, - {"an old announcer saying nothing", merge(showcase, nil, false), "gitea,keycloak"}, + []string{"modules/showcase"}, true), ""}, + {"the root is never a module directory", merge([]string{"tsconfig.json"}, []string{"", "/", "."}, true), ""}, + {"not said: still no build reads it", merge(showcase, []string{"modules/showcase"}, false), ""}, + {"an old announcer saying nothing", merge(showcase, nil, false), ""}, {"a manifest the merge removed, said or not", merge([]string{"modules/gone/module.json", "modules/gone/x.ts"}, nil, true), ""}, } { if got := named(whatTheMergeTouched(candidates, known, c.m)); got != c.want { diff --git a/cmd/mesh-controller/upgrades.go b/cmd/mesh-controller/upgrades.go index b95a59e..f9094f9 100644 --- a/cmd/mesh-controller/upgrades.go +++ b/cmd/mesh-controller/upgrades.go @@ -5,7 +5,6 @@ import ( "errors" "flag" "fmt" - "path" "regexp" "sort" "strings" @@ -616,65 +615,33 @@ func lastLookAt(entries []inventory.Entry, m link.SourceMoved) time.Time { return newest } -// whatTheMergeTouched narrows the modules built from a repository to the ones the merge changed. +// whatTheMergeTouched narrows the modules built from a repository to the ones the merge changed: **a +// changed file touches exactly the modules whose build reads it** (novox/hq issue 280, ADR 0237 as amended). // -// **A change inside no module's own directory is a change to what they share.** The forge lists the -// files a merge changed; a module is affected when one of them is inside its own directory, when it -// is built from the repository's root — everything there is its source — or when some changed file -// belongs to no module's directory at all, which is how a shared file, a build recipe or a -// dependency at the root rebuilds everything built from that repository. +// What a build reads is the module's own directory — the builder clones the repository and builds within +// that directory alone: the manifest, the recipes, the bundles' sources, the Docker context — or the whole +// repository for a module built from its root. A second repository a recipe packages (an artifact's +// `context`) is read too, and that is the build record's `read`, answered by readsFrom beside this. So a +// changed file inside a module's directory is that module's; a file in no module's directory — a script +// at the root, a README, CI configuration, a directory of notes — is read by no build and touches nothing. // -// A change inside *another* module's directory is that module's business and not this one's, even -// when the mesh does not hold that module: `known` is every module this repository is known to hold, -// whatever branch it was registered from. +// **It used to be read as shared code**, rebuilding everything built from the repository, on the theory +// that a root file might be a build input (04-ISSUES/131). No build reads one: the theory cost a rebuild +// of 103 modules for a merge-check.sh added at the catalogue's root (issue 280), as it had for a module +// held by no machine (278) and a new module's directory (252), each patched as an exception to a rule that +// was wrong. Rebuilding too much is not the safe direction when every rebuild is a rollout. // -// **And a module the mesh has never seen is still a module** (novox/hq issue 252). A merge adding a -// new module to the catalogue repository — `modules/newmod/module.json` and its files — read as a -// change to shared code, because `modules/newmod` was nobody's known directory, and every module -// built from the repository was rebuilt and rolled out for a module none of them is. So the -// directories that hold modules are known too: the parents of the known modules' directories -// (`modules`, never the root). A changed path `//…` belongs to the module at -// `/` — held or not — and rebuilds nothing else, **provided it is shown to be a -// module**: its `module.json` is among the changed files (added, changed, or removed with it). A -// directory under the same parent whose manifest the merge did not touch may as well be a shared -// library (`modules/lib`), and that is still read as shared. Rebuilding too much remains the safe -// direction: the fault this whole path exists for is a mesh that believes it is current and is not -// (novox/hq 04-ISSUES/131). A file at the root, or directly in a parent, is shared as it always was. -// -// **Whether a directory is a module is a fact of the repository at the merge commit** (novox/hq issue -// 278), and the announcer now says it: a changed file inside a directory holding a `module.json` there -// is that module's, whatever the merge did to the manifest; only a file in no such directory is shared. -// From an announcer that does not say, the rule above stands. +// Nothing said about the files, or not all of them said, is still everything: what is not known cannot be +// narrowed. `known` is kept for the callers; which directories hold a module is no longer needed to read a +// change, because a file is a module's only by being inside it. func whatTheMergeTouched(candidates, known []inventory.Entry, m link.SourceMoved) []inventory.Entry { - // Nothing said about the files, or not all of them said: everything built from it is affected. + _ = known if len(m.Paths) == 0 || m.PathsTruncated { return candidates } - var dirs []string - parents := map[string]bool{} - for _, e := range known { - if e.Source.Path != "" && sameRepository(e.Source.Repository, m) { - dir := strings.Trim(e.Source.Path, "/") - dirs = append(dirs, dir) - if parent := path.Dir(dir); parent != "." && parent != "/" { - parents[parent] = true - } - } - } - changed := map[string]bool{} - for _, p := range m.Paths { - changed[strings.TrimPrefix(p, "/")] = true - } - modules := saidModuleDirs(m) - for _, p := range m.Paths { - if insideAny(p, dirs) || inAModuleOfItsOwn(p, parents, changed) || insideAny(p, modules) { - continue - } - return candidates - } var out []inventory.Entry for _, e := range candidates { - if e.Source.Path == "" || anyInside(m.Paths, e.Source.Path) { + if strings.Trim(e.Source.Path, "/") == "" || anyInside(m.Paths, e.Source.Path) { out = append(out, e) } } @@ -700,28 +667,6 @@ func saidModuleDirs(m link.SourceMoved) []string { return out } -// inAModuleOfItsOwn is whether a changed file is inside a module directory the mesh does not know — -// `//…` under a directory known to hold modules, whose `module.json` the same merge -// changed (novox/hq issue 252). Such a file is that module's business and nobody else's. -func inAModuleOfItsOwn(p string, parents, changed map[string]bool) bool { - p = strings.TrimPrefix(p, "/") - for parent := range parents { - rest, under := strings.CutPrefix(p, parent+"/") - if !under { - continue - } - name, _, inADirectory := strings.Cut(rest, "/") - if !inADirectory || name == "" { - // A file directly in the parent — `modules/README.md` — is about all of them. - continue - } - if changed[parent+"/"+name+"/module.json"] { - return true - } - } - return false -} - // inside is whether a changed file is in a directory: that directory itself, or under it. func inside(path, dir string) bool { dir = strings.Trim(dir, "/") diff --git a/internal/link/events.go b/internal/link/events.go index 0259f08..e97e50b 100644 --- a/internal/link/events.go +++ b/internal/link/events.go @@ -223,8 +223,8 @@ type PullUpdated struct { PathsTruncated bool `json:"paths_truncated,omitempty"` // ModuleDirs are the directories above the changed files that hold a `module.json` at the head, read // as the merge announcer reads them at a merge commit (issue 278); ModuleDirsSaid says it looked. - // What the controller maps a pull request onto the mesh's module graph with: a file inside one is - // that module's, held or not, and one inside none, in a repository modules are built from, is shared. + // What the controller finds a module the graph does not hold yet with: a directory the change adds a + // module.json in is a new module, checked before it merges. ModuleDirs []string `json:"module_dirs,omitempty"` ModuleDirsSaid bool `json:"module_dirs_said,omitempty"` // MergeCheck says the head holds a merge-check.sh at its root — the repository's own tests, the