docs(contributing): the make vermin trap is the opposite of what was recorded - #659
Merged
Merged
Conversation
…s recorded #656 said `make vermin`'s exit status is the viewer's, so a run that found violations still looks like a success. That is what `Makefile:152-157` and `lint.yml:150-152` both assert, and it is wrong: the redirect into `temp/` is its own recipe line, so make gives up there and never reaches the line that would open the report. Measured on this tree -- `make vermin RUN=` exits 2 with `*** [vermin] Error 1`, vermin itself exits 1 on `Target versions not met: 3.6` against a real floor of 3.11, and the 365-line report is left unread in `temp/vermin.txt`. A control run with a succeeding command does reach the viewer line, which is what makes the redirect the deciding factor rather than the shell semantics of `A && B || C`. So the trap is real but inverted: the failure propagates and the *findings* are what get hidden. A contributor told the old version would ignore a red `make vermin` as a known false pass. Three precision fixes in the same section, from a cross-review of #656: * The CI paragraph claimed the job runs "the same `Makefile` targets listed above". It runs `vermin-ci` for vermin, which is not one of them, and the paragraph four lines later said so -- the file contradicted itself. It now names which target each tool takes, and rests the no-drift claim on the shared flag variables, which is what actually holds. * "every step writes its count" was wrong for vermin, whose summary line is a version verdict rather than a count. * `Changelog drift` also runs on every `workflow_call` into `unit-tests.yml` -- the vendor and conda updates, the pages deploy and the release all gate on it -- because that job carries no `gate-only` guard. The two stale comments in `Makefile` and `lint.yml` are deliberately left alone: a workflow edit ahead of the 1.5.0b5 release is not worth the risk, and they want their own change. No changelog entry, deliberately: none of this is user-visible, and a bullet here would collide with the changelog consolidation currently in flight.
1 task
Owner
Author
|
The cross-review verdict that produced this PR is on #656: #656 (comment) It opened NEEDS CHANGES against #656 and raised items 1-3 of the section above. Item 1 here — the Unpublished-equivalent status: this PR is open and awaiting your review. Nothing here has been merged, tagged or released. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request?
docs— documentation onlyFollow-up to #656, which merged while the cross-review of it was still running. Four corrections to
the section #656 rewrote: one is a claim that is actively wrong and inverts what a contributor
should do, three are precision fixes the cross-review raised.
1.
make verminfails on violations — the opposite of what #656 said#656 shipped this, and it is false:
That sentence was not invented for #656 — it is what
Makefile:152-157andlint.yml:150-152bothassert, and it was carried across in good faith. The reasoning behind it looks at the shell semantics
of
command -v code >/dev/null && code temp/vermin.txt || cat temp/vermin.txtand correctly concludesthat line always exits 0. What it misses is that the redirect is a separate recipe line, and make
abandons a target at the first line that exits non-zero, so the viewer line is never reached at all.
Measured on this tree, not reasoned about:
temp/vermin.txtis written (365 lines) but never displayed, andREACHED_VIEWER_LINEin a minimalreproduction of the same three-line recipe is never printed. A control where the tool succeeds does
reach the viewer line and exits 0 — which is what isolates the redirect, rather than the
A && B || Cchain, as the deciding factor. GNU Make 3.82, no
.ONESHELL:anywhere in theMakefile.vermin exits 1 today because
vermin.inisetstargets = 3.6while the code's real floor is 3.11.Why this matters more than the wording: a contributor who believed the old text would see a red
make vermin, remember being told it cannot fail on violations, and dismiss a genuine failure as aknown false pass. The trap is real but inverted — the failure propagates and the findings are what
get hidden, in a file the target never opens.
2. Three precision fixes from the cross-review of #656
same
Makefiletargets listed above", butlint.yml:166runsmake vermin-ci, which is not inthat block — and the paragraph further down admitted it. It now says which target each tool takes,
and rests the no-drift claim on
$(VERMIN_FLAGS)and friends being shared betweenverminandvermin-ci, which is the thing that actually holds.lint.yml:173emitsMinimum required versions:/Incompatible versions:/Target versions not met:, which areversion verdicts, not counts. Now "each step writes its own verdict there".
Changelog drifthas a third trigger. The job carries nogate-onlyguard, unlike its siblingjobs, so it also runs on every
workflow_callintounit-tests.yml—cron-vendor.yml:27,cron-conda.yml:29,deploy-pages.yml:37andcreate-release.yml:40all gate on it.Notes
Makefile:152-157andlint.yml:150-152still carry the wrong claim. Deliberately not fixedhere:
.github/workflows/**should not be touched ahead of the1.5.0b5release, and the twocomments want their own change. Worth doing, because they are what this documentation was derived
from in the first place — the error will propagate again otherwise.
changelog consolidation in flight.
CONTRIBUTING.mdis modified. The test suite was not run; nothing here executeslibrary code.
Jarry Shaw <jarryshaw@icloud.com>, on top of7b0df4a9c.