fix(scripts): share one Python-interpreter probe across the three scripts (closes #273) - #274
Merged
Conversation
…ipts Three shell scripts hand-rolled three probes with three different candidate lists and two validation techniques, and a fourth opted out of Python entirely because of that fragility. A Windows contributor's toolchain could satisfy two of them and fail the third, with nothing shared to fix. The concrete gap was ESP32-CAM/build.sh, which tried only python3 and python. On a stock Windows 11 + Git Bash box neither resolves and only the `py` launcher does, so its fallback path would abort on a machine with a working Python 3.12. It has never fired because build.sh prefers esptool.exe and skips the interpreter whenever it finds one -- but that mask lives in a different concern from the defect and could be removed by someone with no reason to look at the probe. scripts/lib/python.sh now holds one candidate list and one functional check. resolve_python() returns rather than exiting, because the callers genuinely differ on whether a missing interpreter is fatal (build.sh) or a skip (check-duckdb-bind-claims.sh), and that stays their decision. PYTHON is an array. `py -3` is two words, and a string would need `# shellcheck disable=SC2086` at every call site to stay splittable -- which the -S info job would otherwise require, so the array is what keeps the suppressions out. It also removes the one such directive that already existed. `py -3` goes first, for all three at once, which is the single-place-to-decide property this is for. The launcher is the one interpreter a python.org install on Windows always provides and it is never an alias stub. The shellcheck CI job gains -x and scripts/lib/*.sh. Without -x every new `source` line reports SC1091 at info level and the job goes red; without the glob the helper is unlinted, since scripts/*.sh does not match a subdirectory. Verified against shellcheck 0.11.0 with the exact new command line: clean at -S info, and the old command line goes red first, so the CI edit is load-bearing rather than tidying. .gitignore needed a third carve-out. `lib/` is ignored as the Python build-output convention, and it swallowed scripts/lib/python.sh exactly as it once swallowed homepage/src/lib/location.ts -- an ignored helper that three tracked scripts source would have failed on every machine but mine. Also removes the self-contradicting comment block in check-duckdb-bind-claims.sh: its pre-existing line claimed Windows Git Bash has "only the py launcher, never python3" and asserted parity with build.sh, while the paragraph schutera#272 added beneath it explains that a stock install does put a python3 stub on PATH. Both could not be true, and the parity claim was false when written -- build.sh has validated by execution since schutera#99 while that loop trusted `command -v` until schutera#270. Docs: a troubleshooting entry for the symptom, with the snippet to source for a fourth script; a scripts/README row; and a chapter 11 entry on the two lessons -- count near-duplicates of a platform workaround rather than reviewing them one at a time, and when a defect is unreachable, check what makes it unreachable.
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
2 participants
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.
Closes #273
One helper,
scripts/lib/python.sh, sourced by all three scripts. Shape as suggested in the issue, with the array sopy -3splits without# shellcheck disable=SC2086at every call site — that directive is now gone fromcheck-duckdb-bind-claims.shrather than multiplied.resolve_python()returns rather than exiting. The callers genuinely differ on whether a missing interpreter is fatal (build.sh) or a skip (check-duckdb-bind-claims.sh), and that should stay their decision, not the helper's.On the judgement call:
py -3goes in, for all three at once. That is the single-place-to-decide property the issue is actually about.The shellcheck job had to change, and I verified that it had to
This is the part I'd have got wrong by assuming. Installed shellcheck 0.11.0 and ran the exact CI command line:
So adding the helper would have turned the job red. Two changes were needed:
-xso shellcheck follows sourced files and analyses each in its caller's context;scripts/lib/*.shin the glob, becausescripts/*.shdoes not match a subdirectory.The second matters beyond the exit code: an unlinted helper sourced by three linted scripts would have re-created this issue's gap in a new shape. It also surfaced two things worth fixing rather than suppressing —
SC2148(the helper has no shebang, correctly, since it is sourced →# shellcheck shell=bash) andSC1091path resolution, which resolves from the CWD unless told →# shellcheck source-path=SCRIPTDIR..gitignoreneeded a third carve-outlib/is ignored as the Python build-output convention, and it silently swallowedscripts/lib/python.sh:This is the third time — the comment block already records
ESP32-CAM/lib/and the round-2 P0 wherehomepage/src/lib/location.tswas on the author's disk and invisible to git. Same failure here would have been worse: three tracked scripts sourcing a file that exists on no machine but mine, so every one of them breaks for everyone else. Added!scripts/lib/with a comment in the established style.The self-contradicting comment
Removed. Its pre-existing line claimed Windows Git Bash has "only the
pylauncher, neverpython3" and asserted parity withbuild.sh; the paragraph #272 added beneath it explains that a stock install does put apython3stub onPATH. Both cannot be true, and the parity claim was false when written. The rules now live in one place instead of being restated in three.(#271 no longer deletes that line — I narrowed it to docs-only after #272 landed the script fix — so it stayed in scope here.)
build.shkeeps its short-circuitThe
esptool.exe-first check stays ahead of the probe; only the fallback path changed. Its error message now lists what was actually tried, frompython_candidates_tried(), rather than a hardcodedpython3, python.Verification
The last two are the ones that matter: the duckdb gate and
ruff.shboth resolve their interpreter through the new helper on a box wherepython3andpythondo not resolve at all and onlypydoes — which is the machine the issue is about.resolve_pythonpickspy -3there and reports 3.12.4.Docs
docs/troubleshooting.md— the symptom, in the Windows-PATH cluster next toruff: command not found, with the exactcommand -vtranscript and the snippet to source for a fourth scriptscripts/README.md— a row for the helper, flagged as sourced-not-executeddocs/11-risks-and-technical-debt/— an entry on the two lessons: count near-duplicates of a platform workaround rather than reviewing them one at a time, and when a defect is unreachable, check what makes it unreachable — here the mask lived in a different concern from the defect and could be removed by someone with no reason to look at the probePer
CLAUDE.md, the issue reference is in the title only, not in the body or commit body.