Compare commits
1 Commits
d9be89c339
...
62b31af5bd
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
62b31af5bd |
@@ -774,11 +774,38 @@ style conventions are in separate documents:
|
|||||||
# was closed. That case is not hypothetical here: two runs in the same
|
# was closed. That case is not hypothetical here: two runs in the same
|
||||||
# checkout is exactly what --allow-serial-runners below exists to support,
|
# checkout is exactly what --allow-serial-runners below exists to support,
|
||||||
# and serialising the linter does not serialise the shell's redirections
|
# and serialising the linter does not serialise the shell's redirections
|
||||||
# or the grep and cat that read them. $$ is the same idiom CHECK_EPOCH
|
# or the grep and cat that read them. mktemp rather than $$: two
|
||||||
# uses, for the same reason.
|
# containerised runs over one bind-mounted checkout are in separate PID
|
||||||
LINT_OUT="$LINT_STATE/run.$$.stdout"
|
# namespaces and can both be PID 7, which puts the collision back.
|
||||||
LINT_ERR="$LINT_STATE/run.$$.stderr"
|
LINT_OUT="$(mktemp "$LINT_STATE/run.out.XXXXXX")"
|
||||||
trap 'rm -f "$LINT_OUT" "$LINT_ERR"' EXIT HUP INT TERM
|
LINT_ERR="$(mktemp "$LINT_STATE/run.err.XXXXXX")"
|
||||||
|
|
||||||
|
# Print whatever the linter had written before the interruption, then exit
|
||||||
|
# 128+signal. The exit is what makes this handler TERMINATING, and that is
|
||||||
|
# the point: a signal-trap handler that returns RESUMES the script. With
|
||||||
|
# the capture files already deleted, execution would fall into the grep
|
||||||
|
# below against a missing file, take the not-a-collision branch, and report
|
||||||
|
# the FINDINGS exit status with empty output for a run that was killed —
|
||||||
|
# after deleting the findings it was about to print. That is why cleanup
|
||||||
|
# is on EXIT only. Killing a run is not hypothetical here: it is the stated
|
||||||
|
# mitigation for the unbounded wait --allow-serial-runners can produce, and
|
||||||
|
# it is what Ctrl-C on a make check does.
|
||||||
|
lint_interrupted() {
|
||||||
|
if [ -f "$LINT_ERR" ]; then cat "$LINT_ERR" >&2; fi
|
||||||
|
if [ -f "$LINT_OUT" ]; then cat "$LINT_OUT"; fi
|
||||||
|
echo "lint: interrupted by a signal, so nothing was completed." \
|
||||||
|
"This is NOT a lint result." >&2
|
||||||
|
exit "$1"
|
||||||
|
}
|
||||||
|
|
||||||
|
# The EXIT trap still fires on the way out of a terminating handler, so
|
||||||
|
# cleanup happens exactly once on every path. `|| :` because a failing rm
|
||||||
|
# — an unwritable state directory is enough — would otherwise change the
|
||||||
|
# exit status of an otherwise clean run under set -e.
|
||||||
|
trap 'rm -f "$LINT_OUT" "$LINT_ERR" || :' EXIT
|
||||||
|
trap 'lint_interrupted 129' HUP
|
||||||
|
trap 'lint_interrupted 130' INT
|
||||||
|
trap 'lint_interrupted 143' TERM
|
||||||
|
|
||||||
# Backstop for a caller that reached the linter without the environment
|
# Backstop for a caller that reached the linter without the environment
|
||||||
# above. With it set, this should never fire.
|
# above. With it set, this should never fire.
|
||||||
@@ -806,10 +833,12 @@ style conventions are in separate documents:
|
|||||||
# this failure only on stderr, so a finding that quotes the string
|
# this failure only on stderr, so a finding that quotes the string
|
||||||
# from source cannot be mistaken for a collision and retried away —
|
# from source cannot be mistaken for a collision and retried away —
|
||||||
# that direction would be a false green. The exit status is not a
|
# that direction would be a false green. The exit status is not a
|
||||||
# usable discriminator: the collision exits 3 (exitcodes.Failure)
|
# usable discriminator: the collision exits 3 (exitcodes.Failure),
|
||||||
# while findings exit 1, and the other codes in pkg/exitcodes
|
# which does separate it from findings at 1, but run.go returns it
|
||||||
# carry meanings of their own, so no exit status tells a collision
|
# as a plain error that Execute maps to Failure like every other
|
||||||
# apart from a result.
|
# error at that level, so 3 cannot separate a collision from a
|
||||||
|
# genuine linter failure — a broken config, an unparseable file.
|
||||||
|
# Retrying on 3 would retry real failures into a void.
|
||||||
if ! grep -q 'parallel golangci-lint is running' "$LINT_ERR"; then
|
if ! grep -q 'parallel golangci-lint is running' "$LINT_ERR"; then
|
||||||
cat "$LINT_ERR" >&2
|
cat "$LINT_ERR" >&2
|
||||||
cat "$LINT_OUT"
|
cat "$LINT_OUT"
|
||||||
@@ -882,10 +911,27 @@ style conventions are in separate documents:
|
|||||||
found something, and a lock error erased before `grep` reads it, so the
|
found something, and a lock error erased before `grep` reads it, so the
|
||||||
retry never fires and the void run returns as a result. Both are a run
|
retry never fires and the void run returns as a result. Both are a run
|
||||||
reporting a result that is not its own, which is this bullet's entire
|
reporting a result that is not its own, which is this bullet's entire
|
||||||
subject reintroduced one layer above where it was fixed. `$$` is
|
subject reintroduced one layer above where it was fixed. Use `mktemp`
|
||||||
sufficient and is the same idiom the `CHECK_EPOCH` rule uses; `mktemp`
|
under the state directory rather than `$$`: two containerised runs over
|
||||||
under the state directory is equally fine. The `trap` matters as much as
|
one bind-mounted checkout sit in separate PID namespaces and can hold the
|
||||||
the paths, or the directory accumulates a pair per run forever.
|
same low PID, which puts the collision back on exactly the fleet's
|
||||||
|
arrangement. `$$` is acceptable only where `mktemp` is unavailable and
|
||||||
|
that arrangement is ruled out.
|
||||||
|
- **Clean up on `EXIT` only, and give each signal a TERMINATING handler.** A
|
||||||
|
signal-trap handler that does not exit **resumes** the script: with the
|
||||||
|
capture files already deleted, the run falls into the `grep` against a
|
||||||
|
missing file, takes the not-a-collision branch, and reports the
|
||||||
|
**findings** exit status with empty output for a run that was killed —
|
||||||
|
having deleted the findings it was about to print. Measured: `TERM`, `INT`
|
||||||
|
and `HUP` all returning 1 with empty stdout, against 143, 130 and 129 for
|
||||||
|
the same block without the handler. Exit `128+signal` instead, after
|
||||||
|
printing what the linter had already written, and let the `EXIT` trap do
|
||||||
|
the cleanup on the way out. Killing a run is not a corner case here — it
|
||||||
|
is the stated mitigation for the unbounded wait `--allow-serial-runners`
|
||||||
|
can produce, and it is what Ctrl-C on a `make check` does. Put `|| :` on
|
||||||
|
the `rm`: an unwritable state directory makes it fail, and a failing
|
||||||
|
`EXIT` trap under `set -e` changes the exit status of an otherwise clean
|
||||||
|
run to 1, which was measured in both `dash` and `bash`.
|
||||||
|
|
||||||
Adopting repos must add `.lint-cache/` to both `.gitignore` and
|
Adopting repos must add `.lint-cache/` to both `.gitignore` and
|
||||||
`.dockerignore`. The second matters as much as the first: the directory
|
`.dockerignore`. The second matters as much as the first: the directory
|
||||||
|
|||||||
Reference in New Issue
Block a user