diff --git a/internal/builder/check.go b/internal/builder/check.go index 0e10a85f..ee320858 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -406,13 +406,16 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry // **What the merge would move is built from a checkout nothing of the check touches** (novox/hq issue 703): the // gate and the repository's own check run in containers that mount the workspace read-write, so they can change // its tree, its objects and its caches. The builds are made from a copy of the merge's objects, cloned before - // anything of the change runs, outside the workspace — so what is kept under a key is what the key describes. - building, buildingTree := "", "" + // anything of the change runs, outside the workspace — so what is kept under a key is what the key describes. Each + // build's working copy is cloned from it into the workspace only after the check has ended (forTheMerge): the + // host's container engine sees no other directory of the holder's (novox/hq issue 713). + buildingTree := "" if len(spec.Builds) > 0 && spec.BuildWith != nil && len(spec.Group) == 0 { at := judged.Merge if at == "" { at = spec.Ref } + var building string building, buildingTree, err = checkoutForTheBuilds(ctx, run, tree, at, workspace) if err != nil { return CheckVerdict{}, err @@ -750,23 +753,35 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry // **What the merge would move, built once, here** (novox/hq ADR 0350 §1): after the judging, from the merge it // judged, into the scratch namespace — so the walk of the merge commit takes it rather than building again. if len(spec.Builds) > 0 && spec.BuildWith != nil && len(spec.Group) == 0 { - if why := skipsTheBuilds(v); why != "" { - say("check", "%s", why) - } else { - began := time.Now() - v.Built = buildForTheMerge(ctx, run, spec, *spec.BuildWith, building, buildingTree, judged.Merge, judged, forge, - func(step, message string) { - if log != nil { - log(step, redact.redact(message)) - } - }, say) - say("check", "built for the merge: %d of %d module(s) (%s)", builtCount(v.Built), len(v.Built), - time.Since(began).Round(time.Second)) - } + v.Built = forTheMerge(ctx, run, spec, v, workspace, buildingTree, judged, forge, func(step, message string) { + if log != nil { + log(step, redact.redact(message)) + } + }, say) } return named(v), nil } +// forTheMerge builds what the merge moves once the check has judged it, from merged — the copy of the merge +// checkoutForTheBuilds made outside the workspace — **in the builder's own workspace** (novox/hq issue 713). A build's +// containers are run by the host's container engine, which mounts a path as the host names it: the build seat's +// holder runs in a container that shares only its workspace with the host, at the same path, so a working copy in +// its own temporary directory reaches the toolchain as an empty directory ("go.mod file not found"), every check +// build failed, and every walk built again. The copy stays outside the workspace, so nothing the tests ran reaches it; +// the working copy cloned from it is made only now, after every container of the check has ended. +func forTheMerge(ctx context.Context, run Runner, spec CheckSpec, verdict CheckVerdict, workspace, merged string, + judged CheckVerdict, forge GitCredential, log Log, say func(step, format string, args ...any)) []CheckBuilt { + if why := skipsTheBuilds(verdict); why != "" { + say("check", "%s", why) + return nil + } + began := time.Now() + built := buildForTheMerge(ctx, run, spec, *spec.BuildWith, workspace, merged, judged.Merge, judged, forge, log, say) + say("check", "built for the merge: %d of %d module(s) (%s)", builtCount(built), len(built), + time.Since(began).Round(time.Second)) + return built +} + // checkoutForTheBuilds is a clone of the commit a check builds for its merge, made before anything of the check runs, // outside the workspace its containers mount (novox/hq issue 703): the directory to remove, and the clone in it. // Cloned over git's own transport, never by linking the objects: what the check's containers later write into the diff --git a/internal/builder/check_builds.go b/internal/builder/check_builds.go index 7c0223ca..0540d0e2 100644 --- a/internal/builder/check_builds.go +++ b/internal/builder/check_builds.go @@ -70,7 +70,9 @@ func skipsTheBuilds(v CheckVerdict) string { // buildForTheMerge builds each module the plan moves, tier by tier, into the scratch namespace, and keeps what it // built under its keys. A module standing after one not built is not built either: what it stands on in the walk -// is not what it would stand on here. tree is the check's clone and merge the merge commit it made there. +// is not what it would stand on here. tree is the check's clone and merge the merge commit it made there; workspace +// is where each build's working copy is made, a directory the host's container engine sees at the same path — the +// builder's own workspace, never a temporary directory of its container (novox/hq issue 713). func buildForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with CheckBuilding, workspace, tree, merge string, judged CheckVerdict, forge GitCredential, log Log, say func(step, format string, args ...any)) []CheckBuilt { if len(spec.Builds) == 0 || with.Publish == nil || with.Scratch == nil { @@ -80,6 +82,8 @@ func buildForTheMerge(ctx context.Context, run Runner, spec CheckSpec, with Chec if ref == "" { ref = spec.Ref } + // What an earlier check left, ended before it could remove it, is not built on. + _ = os.RemoveAll(filepath.Join(workspace, "for-the-merge")) held := map[string]string{} for k, v := range with.Held { held[k] = v diff --git a/internal/builder/replay713_test.go b/internal/builder/replay713_test.go new file mode 100644 index 00000000..214ae2a2 --- /dev/null +++ b/internal/builder/replay713_test.go @@ -0,0 +1,125 @@ +package builder + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" +) + +// novox/hq issue 713: after ADR 0350 the walk of novox/mesh-controller#331 built mesh-controller, build-agent and +// route-proxy again, each "built: no check of novox/mesh-controller#331 built it". The check had built none: its +// build log said "built for the merge: 0 of 3 module(s)", mesh-controller's go bundle failing with "go.mod file not +// found". The build seat's holder runs in a container sharing only its workspace with the host, and a build's +// toolchain container is run by the host's container engine, which mounts a path as the host names it. The check +// built in a temporary directory of the holder's own (issue 703's copy), which the engine mounted as an empty one. + +// aHostsEngine runs as the host's container engine does for a holder in a container: a mount of a path outside the +// workspace the holder shares with the host is an empty directory, and a Go toolchain finds no go.mod in it. +type aHostsEngine struct { + onTrunk + shared string +} + +func (h aHostsEngine) run(ctx context.Context, dir, name string, args ...string) (string, error) { + if name == "docker" && len(args) > 0 && args[0] == "run" { + for i, a := range args { + if a != "--volume" || i+1 >= len(args) { + continue + } + from, _, _ := strings.Cut(args[i+1], ":") + if !withinDir(h.shared, from) { + return "", errors.New("go: go.mod file not found in current directory or any parent directory") + } + } + } + return h.onTrunk.run(ctx, dir, name, args...) +} + +// aGoModule is a module of one Go bundle built from a repository's root. +func aGoModule(module string) map[string]string { + files := aSharedRepository("one", "1") + files[ManifestName] = `{"module":"` + module + `","version":"1","build":{"artifacts":[ + {"name":"server","kind":"bundle","language":"go","system":"arch","from":"cmd/ctl","binary":"ctl"}]}, + "resources":[{"id":"server","type":"process","name":"` + module + `","artifact":"server","run":["./ctl"]}]}` + return files +} + +// A controller change whose walk moves mesh-controller, build-agent and route-proxy (#331's) has its check build all +// three, in the workspace the host's engine sees, and the walk of its merge takes each from the check by its key. +func TestReplay713TheWalkOfAControllerChangeTakesAllThreeFromItsCheck(t *testing.T) { + withVersion(t, "builder-1") + pull := PullRef{Owner: "novox", Repo: "mesh-controller", Number: 331} + held := map[string]string{"mesh-tools-go/build": "registry.invalid/mesh-tools-go/build@sha256:" + strings.Repeat("b", 64)} + repositories := map[string]string{"build-agent": "https://forge.invalid/mesh-catalog-build-agent.git", + "route-proxy": "https://forge.invalid/mesh-catalog-route-proxy.git"} + checked := func(shared string) (*recorded, aHostsEngine) { + r := &recorded{tree: "tree-of-the-merge", contents: aGoModule("mesh-controller"), + secondary: map[string]map[string]string{}} + for module, repository := range repositories { + r.secondary[repository] = aGoModule(module) + } + return r, aHostsEngine{onTrunk: onTrunk{recorded: r}, shared: shared} + } + spec := CheckSpec{ID: "build-331", Owner: pull.Owner, Repo: pull.Repo, Number: pull.Number, + Builds: []CheckBuildSpec{{Module: "mesh-controller", Merged: true, Tier: 0}, + {Module: "build-agent", Repository: repositories["build-agent"], Ref: "main", Tier: 1}, + {Module: "route-proxy", Repository: repositories["route-proxy"], Ref: "main", Tier: 2}}} + judged := CheckVerdict{Head: "98e856874913", Trunk: "c975dbf7b053", Merge: "2d22279b1d07"} + passed := CheckVerdict{Gate: &Layer{Verdict: "pass"}, Repo: &Layer{Verdict: "pass"}} + + // As it ran: the builds made in the holder's own temporary directory, which the host's engine does not see. + shared, outside := t.TempDir(), t.TempDir() + merged := filepath.Join(outside, "merged") + r, engine := checked(shared) + asItRan := buildForTheMerge(t.Context(), engine.run, spec, CheckBuilding{Publish: r, Scratch: newScratch(), Held: held}, + outside, merged, judged.Merge, judged, GitCredential{}, nil, logging(nil)) + if builtCount(asItRan) != 0 { + t.Fatalf("the engine of this test sees the holder's temporary directory: %+v", asItRan) + } + + // The check as it builds now: every module its merge moves, kept under its key. + scratch := newScratch() + spec.BuildWith = &CheckBuilding{Publish: r, Scratch: scratch, Held: held} + built := forTheMerge(t.Context(), engine.run, spec, passed, shared, merged, judged, GitCredential{}, nil, + logging(nil)) + if len(built) != 3 || builtCount(built) != 3 { + t.Fatalf("the check built %d of %d module(s) its merge moves: %+v", builtCount(built), len(built), built) + } + var names []NamedBuild + for _, b := range built { + n := NamedBuild{Pull: pull, Check: spec.ID, Module: b.Module, Key: b.Key, Digests: map[string]string{}} + for _, m := range b.Made { + d := m.Digest + if d == "" { + d = DigestOf(m.Reference) + } + n.Digests[m.Name] = d + } + names = append(names, n) + } + + // The walk of the merge commit, tier by tier: each module taken from the check, none built. + walked := map[string]string{"mesh-controller": "https://forge.invalid/mesh-controller.git"} + for module, repository := range repositories { + walked[module] = repository + } + for _, module := range []string{"mesh-controller", "build-agent", "route-proxy"} { + w, walk := checked(shared) + w.secondary[walked["mesh-controller"]] = aGoModule("mesh-controller") + got, err := BuildReusing(t.Context(), walk.run, w, Reuse{Scratch: scratch, Pulls: []PullRef{pull}, Named: names}, + walked[module], "", "main", t.TempDir(), held, Npmrc{}, GitCredential{}, nil) + if err != nil { + t.Fatalf("%s: %v", module, err) + } + if got.Reused == nil || got.Why != "" { + t.Fatalf("the walk built %s again: %s", module, got.Why) + } + for _, line := range w.ran { + if strings.HasPrefix(line, "docker run") && strings.Contains(line, "go build") { + t.Fatalf("the walk compiled %s: %s", module, line) + } + } + } +}