Gate prune's local-cleanup output on --json, and make make build build (closes #108)
All checks were successful
check / check (push) Successful in 2m16s
All checks were successful
check / check (push) Successful in 2m16s
Closes #110. CleanupLocalSnapshots wrote three prose lines to stdout with no --json awareness, covering every branch, so `vaultik prune --json | jq` failed on any input. -q never helped either: printlnStdout and stdoutf write straight to v.Stdout and never consult v.UI, which is what SetQuiet affects. It now takes *PruneOptions, symmetric with its sibling phase PruneBlobs, and gates all three writes. Threading opts.JSON was chosen over moving the lines to log.Info, because internal/log/log.go defaults the level to Warn: log.Info would not have relocated them to stderr, it would have deleted them from a plain `vaultik prune`, and "Removing stale local record" narrates the deletion of local index rows. The stale-record count is deliberately not added to PruneBlobsResult - every field there is blob-scoped and produced by the phase that runs after this reconciliation, so adding it would change a published --json schema as a side effect of a stream fix. Note for anyone reading the --json contract: under --json the stale-record removal now produces no signal in either stream. stdout is correctly gated, stderr is level-pinned to Warn because --json sets Quiet, and the count is not in the document. That is inherited behaviour - PruneBlobs' own log.Info calls are equally invisible under --json - not something this change introduced, and it is tracked separately. make build exited 0 and produced nothing: .PHONY listed build with no build: rule, and a phony target with no prerequisites and no recipe is considered already satisfied, which turns what would be a hard error into a silent success. In a repo where `make build` is the documented way to build, a caller checking the exit code concluded the build worked. Now `build: vaultik`, verified in both directions - a clean build produces the binary, a deliberately broken one exits non-zero and produces none. All 19 .PHONY names were audited; build was the only one lacking a rule. TestPhonyTargetsAllHaveRules keeps that true for names added later, so the class is closed rather than the instance.
This commit was merged in pull request #111.
This commit is contained in:
169
cmd/vaultik/makefile_test.go
Normal file
169
cmd/vaultik/makefile_test.go
Normal file
@@ -0,0 +1,169 @@
|
||||
package main_test
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"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
|
||||
// test binary runs with its package directory as the working directory,
|
||||
// so the root is found by walking up until the Makefile appears.
|
||||
func readMakefile(t *testing.T) string {
|
||||
t.Helper()
|
||||
|
||||
dir, err := os.Getwd()
|
||||
require.NoError(t, err)
|
||||
|
||||
for {
|
||||
//nolint:gosec // G304: the path is this test's own directory walk
|
||||
contents, err := os.ReadFile(filepath.Join(dir, "Makefile"))
|
||||
if err == nil {
|
||||
return string(contents)
|
||||
}
|
||||
|
||||
parent := filepath.Dir(dir)
|
||||
require.NotEqual(t, dir, parent,
|
||||
"walked to the filesystem root without finding a Makefile")
|
||||
|
||||
dir = parent
|
||||
}
|
||||
}
|
||||
|
||||
// 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
|
||||
}
|
||||
Reference in New Issue
Block a user