~bigbes/sr-ht-compare

19f0585714695aa711b0a70e601ded7770118bd6 — bigbes 9 days ago f828386
ci: drop the 'all' from go mod download, which rewrote go.sum

The cache warm-up was 'go mod download all'. That resolves the entire
module graph, test dependencies of dependencies included, and appends
their hashes to go.sum — 145 lines in this repository, measured in a
fresh clone of this branch.

Which makes it the bug this wave exists to remove. A modified tracked
file in the checkout when go build runs is what stamps vcs.modified into
the binary, the same failure the version task avoids by not sed-ing the
APKBUILD; the warm-up would have reintroduced it one task later. The
bench sibling proved it first: build #359 stopped at check-version with
a -dirty binary and 'M go.sum' named as the thing in the way, 228 lines
there.

Without 'all' go.sum is untouched, go mod verify still passes, and what
is fetched is what the main module builds, which is all a warm-up needs.

Also removes the sentence that let this through. Both the manifest and
docs/ci.md claimed -mod=readonly is the default so neither command can
rewrite go.mod or go.sum. It is false, and it sat directly above the
line that disproved it. readonly governs updates to the module
requirements, not writes to go.sum: measured here with the flag set
explicitly on the command line, 'go mod download all' still appends the
same 145 lines and still exits 0, and go mod verify passes after it, so
verification guards nothing either.

Setting the flag on the command line is the point. This machine carries
go env -w GOFLAGS=-mod=mod, as bench's did, so a run that omits it
proves nothing about the builder.

What remains is the git status --porcelain at the end of the task, and
it is now documented as the check rather than as belt-and-braces: it is
the only thing that would notice a go.sum rewrite before the compile,
and on bench it is what did. docs/ci.md warns against restoring the
'all' and against replacing the print with an appeal to a flag.

compare's README makes no claim about module hygiene in CI, so nothing
to correct there
2 files changed, 76 insertions(+), 11 deletions(-)

M .build.yml
M docs/ci.md
M .build.yml => .build.yml +17 -5
@@ 115,12 115,24 @@ tasks:
        echo "restored module cache did not verify — discarding it"
        rm -rf ~/go/pkg/mod
      fi
      go mod download all
      # `go mod download`, and NOT `go mod download all` — do not add the `all`
      # back as an optimization. It resolves the whole module graph, including
      # test dependencies of dependencies, and APPENDS their hashes to go.sum:
      # 145 lines in this repository, 228 in the bench sibling, whose build #359
      # packaged a "-dirty" binary because of it. A modified tracked file in the
      # checkout at `go build` time is the exact stamp this pipeline exists to
      # prevent. Without `all` go.sum is untouched and `go mod verify` still
      # passes. See docs/ci.md#not-go-mod-download-all.
      go mod download
      go mod verify
      # -mod=readonly is the default, so neither line above can rewrite go.mod
      # or go.sum — but an untracked or modified file here is a "+dirty" stamp
      # in the packaged binary, so say so out loud rather than trusting the
      # flag. docs/ci.md#cache_restore.
      # This print is NOT belt-and-braces. It is the ONLY thing here that
      # catches a go.sum rewrite, and no flag stands behind it. Do not replace
      # it with an appeal to -mod=readonly: readonly governs updates to the
      # module REQUIREMENTS, it does not stop writes to go.sum. Measured with
      # GOFLAGS=-mod=readonly explicitly on the command line — `go mod download
      # all` still appended the same 145 lines and still exited 0. A modified or
      # untracked file here is a "+dirty" stamp in the packaged binary.
      # docs/ci.md#not-go-mod-download-all.
      git status --porcelain
  - test: |
      cd "$REPO"

M docs/ci.md => docs/ci.md +59 -6
@@ 166,16 166,69 @@ in the code under test.

So: make the tree writable (the module cache is mode 555 and `rm -rf` cannot
remove it otherwise), ask `go mod verify` whether what came back is intact, and
throw the whole thing away if it is not. Then `go mod download all` and verify
for real, and let *that* failure be fatal.
throw the whole thing away if it is not. Then `go mod download` and verify for
real, and let *that* failure be fatal.

Do not soften the final `go mod verify` to `|| true`. A build that proceeds with
a module cache it could not verify is a build whose result means nothing.

The second `git status --porcelain` is there because `-mod=readonly` is the
default and neither command *can* rewrite `go.mod` or `go.sum` — but a dirty
tree at this point is a `+dirty` apk, so it is said out loud rather than trusted
to a flag.
### Not `go mod download all`

The warm-up is `go mod download`. **The `all` must not come back**, and it is
worth knowing why, because `all` is exactly the shape a future reader adds as an
obvious improvement — warm *everything*, surely, so nothing is fetched later.

`go mod download all` resolves the entire module graph, test dependencies of
dependencies included, and **appends the hashes of everything it resolved to
`go.sum`**. Measured on this tree: 145 lines added. In the bench sibling, 228 —
and that is not a hypothetical, it is what failed its build #359, which stopped
at `check-version` with a `-dirty` binary and `M go.sum` named as the thing in
the way.

A modified tracked file in the checkout when `go build` runs is precisely the
stamp this whole pipeline is arranged to avoid — the same failure the `version`
task refuses to cause by not `sed`-ing the APKBUILD. Warming the cache with a
command that edits the tree would have reintroduced it one task later.

Without `all`, `go.sum` is untouched, `go mod verify` still passes afterwards,
and what gets fetched is what the main module actually builds. That is all a
cache warm-up needs; the packages `all` would have added are ones no build in
this pipeline compiles.

Verified in a fresh clone of this branch — `go mod download` followed by
`git status --porcelain` leaves the tree clean. compare has the smallest
dependency graph of the six services, so this is where `all` came closest to
being harmless, and it still rewrote 145 lines.

### `-mod=readonly` does not save you

There used to be a sentence here, and another above the `git status` line in the
manifest, saying `-mod=readonly` is the default so neither command *can* rewrite
`go.mod` or `go.sum`. **It was false**, and it is a large part of why nobody
looked at `go mod download all` for as long as they did: it sat directly above
the line that disproved it and invited the reader to skip both.

`readonly` governs updates to the module **requirements**. It does not stop
writes to `go.sum`. Measured on this tree rather than inherited, with the flag
set explicitly on the command line:

| command | `go.sum` | exit |
|---|---|---|
| `GOFLAGS=-mod=readonly go mod download all` | **+145 lines** | 0 |
| `GOFLAGS=-mod=readonly go mod download` | untouched | 0 |

Both still pass `go mod verify` afterwards, so verification is no guard either.

Set the flag **on the command line** when reproducing this. `go env -w` writes a
persistent `GOFLAGS`, and a machine carrying `go env -w GOFLAGS=-mod=mod` — this
one does, and bench's did too — makes a run that omits the flag prove nothing
about what the builder would do. Check with `go env GOFLAGS` first.

So the `git status --porcelain` at the end of the task is not belt-and-braces,
and no flag stands behind it: **it is the check**. It is the only thing in this
pipeline that would notice a `go.sum` rewrite before the compile, and on the
bench sibling it is what did. Do not delete it as redundant and do not replace
it with an appeal to a flag.

## test