scripts: make three silent tooling failures loud - #137
Merged
Conversation
All three cost a wasted run during the #134 work, and each failed in a way that looked like success. verify.sh: the valgrind smoke picks its scratch from ${DUPEREMOVE_TEST_DIR:-${TMPDIR:-/tmp}}. With the variable unset - the documented default - `make check` still passes, because the integration harness defaults to .itest-scratch next to the repo, but the smoke lands in /tmp, which is tmpfs on most distros. oans correctly refuses to dedupe there and exits 1, except its message went to the smoke's `>/dev/null`, so the script stopped mid-step with no output whatsoever. It now checks the scratch filesystem up front and says what to set, and the smoke keeps its output so a failure explains itself. perf-profile.sh --cold probed `sudo -n true`, but the sensible sudoers rule to grant is exactly `tee /proc/sys/vm/drop_caches` and nothing else - which is what scripts/bench.py relies on - and under that rule `sudo -n true` still demands a password. So --cold was refused on precisely the machines set up to support it. It now probes the real command. perf-profile.sh also resolved its binary as ./duperemove then PATH, never ./oans. Since `make install` puts a `duperemove` compatibility symlink on PATH, a bare invocation happily profiled the installed *upstream* duperemove 0.15.2 instead of the local build, and reported it in the header where it reads as normal. It now prefers ./oans and warns loudly when it falls all the way back to PATH. Verified each: verify.sh with DUPEREMOVE_TEST_DIR unset now exits 1 naming the tmpfs and the fix; --cold completes and records a real profile; a bare run reports `binary : ./oans`; and from a directory with no build it prints the fallback warning before profiling. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Three tooling bugs, all hit during the #134 work. What they have in common is that each one failed in a way that looked like success — no error, or an error nobody sees.
1.
verify.shsilently stops when the scratch is tmpfsThe valgrind smoke takes its scratch from
${DUPEREMOVE_TEST_DIR:-${TMPDIR:-/tmp}}. With the variable unset — the documented default —make checkstill passes, because the integration harness independently defaults to.itest-scratchnext to the repo. But the smoke lands in/tmp, which is tmpfs on most distros. oans correctly refuses to dedupe there and exits 1 — and its message went straight into the smoke's>/dev/null.The result: the script stopped mid-step with no output at all. My first v1.5.0 release attempt looked like it had simply ended, and the
| tailI'd piped it through hid the non-zero exit too.Now it checks the scratch filesystem before doing anything:
and the smoke retains its output, so any other failure explains itself instead of vanishing.
2.
perf-profile.sh --coldwas refused on machines that support itIt probed
sudo -n true. But the sensible sudoers rule to grant is exactlytee /proc/sys/vm/drop_cachesand nothing else — which is whatscripts/bench.pyalready relies on — and under such a rulesudo -n truestill demands a password.So
--coldwas disabled on precisely the boxes configured for it, whilebench.pydropped caches happily. It now probes the real command.3.
perf-profile.shprofiled upstream duperemove by defaultBinary resolution was
./duperemove, then PATH — never./oans, which is what this repo builds. And sincemake installputs aduperemovecompatibility symlink on PATH, a bare invocation cheerfully profiled the installed upstream duperemove 0.15.2, reporting it in the header where it reads as perfectly normal. My first profile of the O(n²) hotspot was of upstream, not the fork.It now prefers
./oans, and warns before falling back:This is the same trap
CLAUDE.mdalready warns about in prose ("Confirm you're testing this./oans, not a systemduperemove") — now enforced by the script rather than left to the reader.Verification
Each fix exercised against the failure it addresses:
verify.shwithDUPEREMOVE_TEST_DIRunset → exits 1, names the tmpfs and the fix (previously: silent stop).--cold→ completes and records a real profile (previously: bailed on the sudo probe).perf-profile.shin the repo →binary : ./oans(previously:/usr/bin/duperemove).scripts/verify.shpasses end to end.