All checks were successful
check / check (pull_request) Successful in 4m0s
Every lint run now happens inside its own container, invoked through script/lint, and linting is a build step rather than a container command: a successful build of the new root Dockerfile.lint IS a clean lint. That shape also works where the docker daemon is remote and bind mounts are impossible. Its FROM line -- golangci/golangci-lint:v2.12.2, pinned by digest -- is now the only pin of the linter version in this repo. A container per run has its own lint cache and its own golangci-lint lock, both discarded with it, so neither cross-worktree contamination nor lock contention exists any more. The machinery that defended against them is therefore gone: the per-worktree cache directories, the lock-retry loop, and script/lint-audit, which existed to catch findings replayed from a cache that no longer exists. So is the host lint path in its entirety -- the native escape hatch, its version detection, and VAULTIK_LINT_IN_CONTAINER in both script/lint and the Dockerfile. Nothing lints on the host, at any version. A cached build lints nothing, so the CHECK_EPOCH mechanism the product Dockerfile already used is what makes a green mean something: ARG CHECK_EPOCH with no default, placed below the module layers so dependency caching survives, a `RUN [ -n "$CHECK_EPOCH" ] || exit 1` guard so a build that withholds the arg fails instead of replaying, and the value expanded into the lint command itself. script/lint computes `epoch="$(date +%s%N)$$"` as a bare assignment on its own line, because inline in the argument a failing substitution does not abort under `set -eu` and yields a constant empty epoch -- which is exactly the false green being prevented. The product Dockerfile loses its lint stage rather than gaining a second linter pin. That stage ran `make lint`, which is now `docker build`: docker-in-docker inside a BuildKit step with no daemon. Calling golangci-lint directly there instead would have meant two independently bumpable digests for one tool. `make fmt-check` moves beside `make test` in the builder stage, and script/cibuild now builds Dockerfile.lint and then Dockerfile, each with its own fresh epoch, failing on either. Consequence, stated in comments rather than left to be discovered: script/docker builds the product image only and no longer lints; script/check and script/cibuild are the gates. Two decisions taken deliberately and documented where they apply. `golangci-lint config verify` is omitted: it fetches its JSON schema over an unpinned live HTTPS call, which would make the gate depend on a remote resource outside this repo's hash-pinning discipline and turn an upstream outage or an egress-less runner into a red that is not a lint verdict. script/lint-fix is kept, reimplemented as a bind-mounted docker run against the image parsed out of Dockerfile.lint -- a build step cannot write fixes back to the worktree -- and its header states outright that it is a developer convenience, never a gate, and needs a local daemon. cmd/vaultik/lintdocker_test.go parses both Dockerfiles and both scripts and fails if any part of the mechanism is dropped: the digest pin, the defaultless ARG below `go mod download`, the emptiness guard, the expansion of the epoch into each check command, the bare per-invocation epoch assignment in both scripts, cibuild building both files, and the absence of any host-lint escape hatch. Every one of those losses is silent -- the build still exits 0 and nothing is checked -- which is why they are asserted rather than trusted. script/lint takes no arguments now, and says so instead of dropping them: a build step has no command line to pass linter flags to.
152 lines
5.0 KiB
Go
152 lines
5.0 KiB
Go
package main_test
|
|
|
|
import (
|
|
"regexp"
|
|
"slices"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
// This file guards the Makefile that builds this program, which is why
|
|
// it lives beside it rather than in a package of its own.
|
|
//
|
|
// Issue #110: `build` was listed in .PHONY with no `build:` rule
|
|
// anywhere in the file. That combination is silently successful — make
|
|
// considers a phony target with no prerequisites and no recipe already
|
|
// satisfied, so `rm -f vaultik && make build` printed "Nothing to be
|
|
// done for 'build'" and exited 0 with no binary produced. Declaring the
|
|
// name phony is precisely what converts the "No rule to make target"
|
|
// error into a green.
|
|
//
|
|
// The guard is a parse of the Makefile rather than an invocation of
|
|
// make. `make test` is what runs these tests, so shelling back into
|
|
// `make build` here would nest a build inside the test run and drop a
|
|
// binary into the tree as a side effect of testing. The one property a
|
|
// parse cannot establish — that the recipe still fails when the build
|
|
// fails — is not testable from inside the build either; it is verified
|
|
// by hand against a deliberately broken tree.
|
|
|
|
// phonyDirective introduces the list of phony target names.
|
|
const phonyDirective = ".PHONY:"
|
|
|
|
// ruleLine matches a rule's target list: a target starts in column
|
|
// zero, so recipe lines (tab-indented) and the continuation lines of a
|
|
// variable assignment (space-indented) are excluded by construction.
|
|
//
|
|
// The trailing (?:[^=]|$) rejects `:=` assignments such as
|
|
// `VERSION := $(shell script/version)`, which are not rules. Directives
|
|
// and function calls (`.PHONY:`, `ifeq`, `$(error ...)`) do not match
|
|
// because a target here must begin with a letter, digit or underscore.
|
|
var ruleLine = regexp.MustCompile(`^([A-Za-z0-9_][A-Za-z0-9_./ -]*):(?:[^=]|$)`)
|
|
|
|
// TestPhonyTargetsAllHaveRules fails on any name in .PHONY that has no
|
|
// rule in the Makefile. Such a name is not a build target at all: it is
|
|
// a command that reports success without doing anything, which is worse
|
|
// than one that does not exist, because a caller checking the exit code
|
|
// cannot tell the difference.
|
|
func TestPhonyTargetsAllHaveRules(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
makefile := readMakefile(t)
|
|
|
|
phony := phonyTargets(makefile)
|
|
require.NotEmpty(t, phony, "no .PHONY names found; the parser is broken")
|
|
|
|
rules := declaredRules(makefile)
|
|
|
|
// Sanity check on the rule parser before trusting its verdict: a
|
|
// parser that found nothing would pass this test by accident.
|
|
require.Contains(t, rules, "vaultik",
|
|
"the file rule that builds the binary must be recognized")
|
|
|
|
for _, target := range phony {
|
|
assert.Contains(t, rules, target,
|
|
"`.PHONY` lists %q but the Makefile declares no %q rule, so "+
|
|
"`make %s` exits 0 without doing anything", target, target, target)
|
|
}
|
|
}
|
|
|
|
// TestBuildTargetBuildsTheBinary pins the specific shape of issue #110:
|
|
// `make build` has to reach the rule that produces the binary. The test
|
|
// above would also pass if `build:` were given an empty recipe of its
|
|
// own, which would be the same silent success under a different
|
|
// spelling.
|
|
func TestBuildTargetBuildsTheBinary(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
prerequisites := rulePrerequisites(readMakefile(t), "build")
|
|
require.NotNil(t, prerequisites, "the Makefile declares no `build` rule")
|
|
|
|
assert.Contains(t, prerequisites, "vaultik",
|
|
"`make build` must depend on the rule that builds the binary")
|
|
}
|
|
|
|
// readMakefile returns the contents of the repository's Makefile. The
|
|
// root is located by the shared walk in lintdocker_test.go.
|
|
func readMakefile(t *testing.T) string {
|
|
t.Helper()
|
|
|
|
return readRepoFile(t, "Makefile")
|
|
}
|
|
|
|
// phonyTargets returns every name declared phony, across all .PHONY
|
|
// lines.
|
|
func phonyTargets(makefile string) []string {
|
|
var targets []string
|
|
|
|
for line := range strings.SplitSeq(makefile, "\n") {
|
|
if !strings.HasPrefix(line, phonyDirective) {
|
|
continue
|
|
}
|
|
|
|
targets = append(targets,
|
|
strings.Fields(strings.TrimPrefix(line, phonyDirective))...)
|
|
}
|
|
|
|
return targets
|
|
}
|
|
|
|
// declaredRules returns the set of target names that have a rule.
|
|
func declaredRules(makefile string) map[string]bool {
|
|
rules := make(map[string]bool)
|
|
|
|
for line := range strings.SplitSeq(makefile, "\n") {
|
|
match := ruleLine.FindStringSubmatch(line)
|
|
if match == nil {
|
|
continue
|
|
}
|
|
|
|
// One rule may name several targets: `a b: prereq`.
|
|
for target := range strings.FieldsSeq(match[1]) {
|
|
rules[target] = true
|
|
}
|
|
}
|
|
|
|
return rules
|
|
}
|
|
|
|
// rulePrerequisites returns the prerequisites of the named rule, or nil
|
|
// if no such rule exists. A rule with none returns an empty slice, so
|
|
// "declared with nothing to do" is distinguishable from "not declared".
|
|
func rulePrerequisites(makefile, target string) []string {
|
|
for line := range strings.SplitSeq(makefile, "\n") {
|
|
match := ruleLine.FindStringSubmatch(line)
|
|
if match == nil {
|
|
continue
|
|
}
|
|
|
|
if !slices.Contains(strings.Fields(match[1]), target) {
|
|
continue
|
|
}
|
|
|
|
_, after, _ := strings.Cut(line, ":")
|
|
|
|
return append([]string{}, strings.Fields(after)...)
|
|
}
|
|
|
|
return nil
|
|
}
|