Merge pull request 'Pass a check's changed paths in a file, so a change of thousands of files can start and a check that cannot start says why (issue 1247)' (#396) from fix/1247-changed-paths-file into main

cmd/mesh-controller/merge_gate.go

@@ -154,6 +154,24 @@ type mergeWidthOf struct {
 // wideRebuild is how many modules a rebuild may take before the gate says so as a warning.
 var wideRebuild = 12
 
+// changedPaths are the paths --changed (comma-separated) and --changed-file (one per line) name together.
+func changedPaths(list, file string) ([]string, error) {
+	out := splitList(list)
+	if file == "" {
+		return out, nil
+	}
+	raw, err := os.ReadFile(file)
+	if err != nil {
+		return nil, fmt.Errorf("--changed-file: %w", err)
+	}
+	for _, line := range strings.Split(string(raw), "\n") {
+		if line = strings.TrimSpace(line); line != "" {
+			out = append(out, line)
+		}
+	}
+	return out, nil
+}
+
 // mergeGateCommand is `gate`:
 //
 //	merge-gate --facts <file> --store <postgres> [--repository owner/repo --tree <dir>] [--changed a,b] [--json]
@@ -167,6 +185,8 @@ func mergeGateCommand(ctx context.Context, args []string) error {
 	repository := set.String("repository", "", "owner/repository of the change")
 	tree := set.String("tree", "", "the change's checkout: every module.json in it replaces the mesh's")
 	changed := set.String("changed", "", "the paths the change touches, comma-separated, for its rebuild's width")
+	changedFile := set.String("changed-file", "", "a file of the paths the change touches, one per line; the same as "+
+		"--changed, for a list too long for an argument (novox/hq issue 1247)")
 	group := set.String("group", "", "a delivery group's other heads, as JSON [{repository, tree, changed}], composed "+
 		"with this one as one future state (novox/hq ADR 0239)")
 	asJSON := set.Bool("json", false, "print the verdict as JSON")
@@ -198,7 +218,11 @@ func mergeGateCommand(ctx context.Context, args []string) error {
 		// verdict at all. What is said goes beside the report.
 		os.Stdout = os.Stderr
 	}
-	v, err := judgeChange(ctx, mergeCheckInput{facts: f, repository: *repository, tree: *tree, changed: splitList(*changed),
+	changedList, err := changedPaths(*changed, *changedFile)
+	if err != nil {
+		return err
+	}
+	v, err := judgeChange(ctx, mergeCheckInput{facts: f, repository: *repository, tree: *tree, changed: changedList,
 		group: others, admin: *admin, say: out, beside: os.Getenv("MESH_CHECK_BESIDE")})
 	os.Stdout = stdout
 	if err != nil {

cmd/mesh-controller/merge_gate_changed_test.go

@@ -0,0 +1,31 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"reflect"
+	"strings"
+	"testing"
+)
+
+func TestTheGateJudgesAFileOfPathsAsItDoesTheSameListOfThem(t *testing.T) { // 1247/AC3
+	paths := []string{"modules/a/x.go", "modules/a/y.go", "modules/b/z.go", "README.md"}
+	file := filepath.Join(t.TempDir(), "changed.txt")
+	if err := os.WriteFile(file, []byte(strings.Join(paths, "\n")+"\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	fromFile, err := changedPaths("", file)
+	if err != nil {
+		t.Fatal(err)
+	}
+	fromList, err := changedPaths(strings.Join(paths, ","), "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !reflect.DeepEqual(fromFile, fromList) || len(fromFile) != len(paths) {
+		t.Errorf("the file gives %v, the list %v", fromFile, fromList)
+	}
+	if _, err := changedPaths("", filepath.Join(t.TempDir(), "none")); err == nil {
+		t.Error("a file that is not there is read as no paths")
+	}
+}

internal/builder/check.go

@@ -260,6 +260,12 @@ const (
 	EnvTestBus    = "MESH_TEST_NATS"
 	EnvRepository = "MESH_CHECK_REPOSITORY"
 	EnvChanged    = "MESH_CHECK_CHANGED"
+	// EnvChangedFile names a file holding every changed path, one per line, in the order given: the whole
+	// list however long, which one environment string cannot carry (novox/hq issue 1247).
+	EnvChangedFile = "MESH_CHECK_CHANGED_FILE"
+	// EnvChangedCut is 1 when MESH_CHECK_CHANGED was cut to directories, so the gate reads the file; unset
+	// otherwise, and the gate is given the list as before, which a judge older than --changed-file understands.
+	EnvChangedCut = "MESH_CHECK_CHANGED_CUT"
 	EnvVerdict    = "MESH_CHECK_VERDICT"
 	EnvBeside     = "MESH_CHECK_BESIDE"
 	EnvModules    = "MESH_CHECK_MODULES"
@@ -719,12 +725,17 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry
 	judged.JudgeFrom = siblingJudge.From
 	judged.Validator = siblingJudge.Validator
 	verdictFile := filepath.Join(root, "verdict.json")
+	changedEnv, err := changedEnvironment(root, paths, func(line string) { fmt.Fprintln(&out, line) })
+	if err != nil {
+		return CheckVerdict{}, err
+	}
 	env := []string{EnvFacts + "=" + factsFile, EnvGate + "=" + gate, EnvGateStore + "=" + storeURL,
 		EnvTestStore + "=" + storeURL, EnvTestBus + "=nats://" + bus, EnvRepository + "=" + spec.Owner + "/" + spec.Repo,
-		EnvChanged + "=" + strings.Join(paths, ","), EnvBeside + "=" + root,
+		EnvBeside + "=" + root,
 		EnvModules + "=" + strings.Join(append(append([]string{}, spec.Modules...), spec.New...), ","),
 		EnvGroup + "=" + groupFile,
 		"GOCACHE=" + filepath.Join(workspace, "go-cache"), "GOMODCACHE=" + filepath.Join(workspace, "go-modules")}
+	env = append(env, changedEnv...)
 	env = append(env, selection(spec)...)
 
 	v := CheckVerdict{}
@@ -919,6 +930,10 @@ func ownCheck(ctx context.Context, spec CheckSpec, parts []ScriptPart, tree stri
 			if said == "" {
 				said = whatFailed(own.String())
 			}
+			if said == "" {
+				// A command that could not start says nothing itself (novox/hq issue 1247): its error does.
+				said = err.Error()
+			}
 			return &Layer{Verdict: "fail", Summary: passedSoFar(ran) + "its " + part.Script + " failed: " + said,
 				Failed: picked.String()}
 		}
@@ -1084,7 +1099,7 @@ func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFil
 	// 2. Every machine composed with the change.
 	if err := running(inToolchain(tree, append(env, EnvVerdict+"="+verdictFile), "sh", "-c",
 		`"$MESH_GATE" merge-gate --facts "$MESH_FACTS" --store "$MESH_GATE_POSTGRES" `+
-			`--repository "$MESH_CHECK_REPOSITORY" --tree . --changed "$MESH_CHECK_CHANGED" `+
+			`--repository "$MESH_CHECK_REPOSITORY" --tree . `+gateChangedFlag(env)+` `+
 			`${MESH_CHECK_GROUP:+--group "$MESH_CHECK_GROUP"} --json > "$MESH_CHECK_VERDICT"`)); err != nil {
 		if ctx.Err() != nil {
 			return "error", "the check was ended during the merge gate"
@@ -1895,3 +1910,60 @@ func treeAt(ctx context.Context, run Runner, dir string) string {
 	}
 	return strings.TrimSpace(out)
 }
+
+// changedEnvLimit is how long MESH_CHECK_CHANGED may be: well under the kernel's 128 KiB for one environment
+// string, over which the container cannot start (novox/hq issue 1247).
+const changedEnvLimit = 64 << 10
+
+// changedEnvironment is the changed paths as a check reads them: every path, one per line, in a file beside the
+// check's checkouts that MESH_CHECK_CHANGED_FILE names; and MESH_CHECK_CHANGED, the same comma-joined while
+// that is under changedEnvLimit, otherwise each path's first two directories (a top-level file as itself) once
+// each, said in the check's output.
+func changedEnvironment(root string, paths []string, say func(string)) ([]string, error) {
+	file := filepath.Join(root, "changed.txt")
+	body := ""
+	if len(paths) > 0 {
+		body = strings.Join(paths, "\n") + "\n"
+	}
+	if err := os.WriteFile(file, []byte(body), 0o644); err != nil {
+		return nil, err
+	}
+	joined := strings.Join(paths, ",")
+	cut := false
+	if len(joined) >= changedEnvLimit {
+		var short []string
+		seen := map[string]bool{}
+		for _, p := range paths {
+			parts := strings.SplitN(p, "/", 3)
+			d := p
+			if len(parts) == 3 {
+				d = parts[0] + "/" + parts[1] + "/"
+			}
+			if !seen[d] {
+				seen[d] = true
+				short = append(short, d)
+			}
+		}
+		joined = strings.Join(short, ",")
+		cut = true
+		say(fmt.Sprintf("--- %d changed paths are too long for one environment string: %s holds all of them, one per "+
+			"line, and %s only each path's first two directories", len(paths), EnvChangedFile, EnvChanged))
+	}
+	env := []string{EnvChanged + "=" + joined, EnvChangedFile + "=" + file}
+	if cut {
+		env = append(env, EnvChangedCut+"=1")
+	}
+	return env, nil
+}
+
+// gateChangedFlag is how the gate is given the changed paths: as the argument it always was while the list was not
+// cut, so a judge from before --changed-file still runs; from the file only when it was cut, which that judge
+// could never have started anyway.
+func gateChangedFlag(env []string) string {
+	for _, e := range env {
+		if e == EnvChangedCut+"=1" {
+			return `--changed-file "$MESH_CHECK_CHANGED_FILE"`
+		}
+	}
+	return `--changed "$MESH_CHECK_CHANGED"`
+}

internal/builder/check_changed_test.go

@@ -0,0 +1,130 @@
+package builder
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// manyPaths is a list shaped like the real one of mesh-delivery feat/delivery-parts 9e9c2a7 (novox/hq issue
+// 1247): 2494 vendored files of nats-server across four new modules, about 208 KB comma-joined.
+func manyPaths() []string {
+	modules := []string{"mesh-delivery-forge-bridge", "mesh-delivery-indexer", "mesh-delivery-teller", "mesh-delivery-view"}
+	var paths []string
+	for i := 0; i < 2494; i++ {
+		paths = append(paths, fmt.Sprintf("modules/%s/vendor/github.com/nats-io/nats-server/v2/server/file_%04d.go",
+			modules[i%len(modules)], i))
+	}
+	return paths
+}
+
+func envValue(env []string, name string) string {
+	for _, e := range env {
+		if v, ok := strings.CutPrefix(e, name+"="); ok {
+			return v
+		}
+	}
+	return "\x00unset"
+}
+
+func TestALongListOfChangedPathsGoesInAFileAndTheStringHoldsTheModules(t *testing.T) { // 1247/AC1
+	paths := manyPaths()
+	if n := len(strings.Join(paths, ",")); n < 200<<10 {
+		t.Fatalf("the fixture is only %d bytes: not the real list's shape", n)
+	}
+	root := t.TempDir()
+	var said []string
+	env, err := changedEnvironment(root, paths, func(l string) { said = append(said, l) })
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, e := range env {
+		if len(e) > 64<<10 {
+			t.Errorf("an environment string of %d bytes is handed to docker: %.60s", len(e), e)
+		}
+	}
+	raw, err := os.ReadFile(envValue(env, EnvChangedFile))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := strings.Split(strings.TrimSuffix(string(raw), "\n"), "\n"); strings.Join(got, "\n") != strings.Join(paths, "\n") {
+		t.Errorf("the file holds %d lines, not the %d paths in the order given", len(got), len(paths))
+	}
+	want := "modules/mesh-delivery-forge-bridge/,modules/mesh-delivery-indexer/,modules/mesh-delivery-teller/,modules/mesh-delivery-view/"
+	if got := envValue(env, EnvChanged); got != want {
+		t.Errorf("%s is %q, want each module's directory once: %q", EnvChanged, got, want)
+	}
+	if len(said) != 1 || !strings.Contains(said[0], EnvChangedFile) {
+		t.Errorf("the check's output says %q, want one line naming %s", said, EnvChangedFile)
+	}
+}
+
+func TestAShortListOfChangedPathsStaysAsItWasAndIsInTheFileToo(t *testing.T) { // 1247/AC2
+	var said []string
+	env, err := changedEnvironment(t.TempDir(), []string{"a.go", "b.go"}, func(l string) { said = append(said, l) })
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := envValue(env, EnvChanged); got != "a.go,b.go" {
+		t.Errorf("%s is %q", EnvChanged, got)
+	}
+	raw, err := os.ReadFile(envValue(env, EnvChangedFile))
+	if err != nil || string(raw) != "a.go\nb.go\n" {
+		t.Errorf("the file holds %q, %v", raw, err)
+	}
+	if len(said) != 0 {
+		t.Errorf("a short list said %q", said)
+	}
+}
+
+// theGatesCommand is the command gateLayer hands the toolchain for the merge gate, given the environment of the check.
+func theGatesCommand(t *testing.T, env []string) string {
+	t.Helper()
+	var args []string
+	gateLayer(t.Context(), CheckSpec{}, t.TempDir(), t.TempDir(), "gate", filepath.Join(t.TempDir(), "verdict.json"), env,
+		func(dir string, env []string, command ...string) []string { return command },
+		func(a []string) error { args = a; return errors.New("stop here") }, &tail{}, "bus", "name", false, "", nil,
+		func(string, string, ...any) {})
+	return strings.Join(args, " ")
+}
+
+func TestTheGateIsGivenAShortListAsBeforeSoAnOlderJudgeStillRuns(t *testing.T) { // 1247/AC3
+	env, err := changedEnvironment(t.TempDir(), []string{"a.go", "b.go"}, func(string) {})
+	if err != nil {
+		t.Fatal(err)
+	}
+	command := theGatesCommand(t, env)
+	if !strings.Contains(command, `--changed "$MESH_CHECK_CHANGED"`) || strings.Contains(command, "--changed-file") {
+		t.Errorf("a short list is not given to the gate as --changed alone: %s", command)
+	}
+}
+
+func TestTheGateReadsACutListFromTheFile(t *testing.T) { // 1247/AC3
+	env, err := changedEnvironment(t.TempDir(), manyPaths(), func(string) {})
+	if err != nil {
+		t.Fatal(err)
+	}
+	command := theGatesCommand(t, env)
+	if !strings.Contains(command, `--changed-file "$MESH_CHECK_CHANGED_FILE"`) || strings.Contains(command, `--changed "`) {
+		t.Errorf("a cut list is not given to the gate through the file alone: %s", command)
+	}
+}
+
+func TestACommandThatCannotStartFailsWithItsError(t *testing.T) { // 1247/AC4
+	var out tail
+	layer := ownCheck(t.Context(), CheckSpec{Toolchain: "go-image"}, []ScriptPart{{Toolchain: "go", Script: CheckScript}}, t.TempDir(),
+		&out, func() bool { return false }, func(image, script string) *exec.Cmd {
+			return exec.CommandContext(t.Context(), "/nonexistent/docker", script)
+		}, func(string, string, ...any) {})
+	if layer == nil || layer.Verdict != "fail" {
+		t.Fatalf("a command that cannot start answered %+v", layer)
+	}
+	if !strings.Contains(layer.Summary, CheckScript) || !strings.Contains(layer.Summary, "no such file or directory") ||
+		strings.HasSuffix(strings.TrimSpace(layer.Summary), "failed:") {
+		t.Errorf("the summary does not say why: %q", layer.Summary)
+	}
+}

internal/builder/check_integration_test.go

@@ -114,6 +114,36 @@ echo checked
 	}
 }
 
+func TestACheckOfAChangeTouchingMoreFilesThanOneArgumentHoldsRunsWhole(t *testing.T) { // 1247/AC5
+	registry := checkEnvironment(t)
+	// 3000 paths of about 70 bytes: over 128 KiB comma-joined, which no environment string can carry.
+	var paths []string
+	for i := 0; i < 3000; i++ {
+		paths = append(paths, fmt.Sprintf("modules/m%d/vendor/github.com/nats-io/nats-server/v2/server/file_%05d.go", i%4, i))
+	}
+	script := `set -e
+test -s "$MESH_CHECK_CHANGED_FILE"
+test "$(wc -l < "$MESH_CHECK_CHANGED_FILE")" = 3000
+head -n 1 "$MESH_CHECK_CHANGED_FILE" | grep -q '^modules/m0/vendor/.*file_00000.go$'
+test "${#MESH_CHECK_CHANGED}" -lt 65536
+case "$MESH_CHECK_CHANGED" in modules/m0/,*) ;; *) echo "changed: $MESH_CHECK_CHANGED"; exit 1;; esac
+echo checked
+`
+	repo, head := aCheckedRepository(t, map[string]string{CheckScript: script})
+	v, err := Check(t.Context(), Command, CheckSpec{ID: fmt.Sprintf("check-many-%d", time.Now().UnixNano()), Repository: repo,
+		Ref: head, Owner: "novox", Repo: "hq", Number: 8, Paths: paths, Toolchain: goToolchain}, t.TempDir(), registry,
+		GitCredential{}, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if v.Repo == nil || v.Repo.Verdict != "pass" || !strings.Contains(v.Report, "checked") {
+		t.Fatalf("the repository's check answered %+v\n%s", v.Repo, v.Report)
+	}
+	if !strings.Contains(v.Report, EnvChangedFile) {
+		t.Errorf("the report does not say the list went into a file:\n%s", v.Report)
+	}
+}
+
 func TestAFailingCheckFailsAndOneThatCannotRunIsNeverAPass(t *testing.T) {
 	registry := checkEnvironment(t)
 	repo, head := aCheckedRepository(t, map[string]string{CheckScript: "echo 'resource \"x.service\": refused'; exit 3\n"})