Skip to content

Make the shell scripts runnable, and have them find a Python 3 - #137

Merged
dmccoystephenson merged 3 commits into
mainfrom
feature/shell-script-interpreter-resolution
Aug 22, 2026
Merged

Make the shell scripts runnable, and have them find a Python 3#137
dmccoystephenson merged 3 commits into
mainfrom
feature/shell-script-interpreter-resolution

Conversation

@dmccoystephenson

@dmccoystephenson dmccoystephenson commented Aug 20, 2026

Copy link
Copy Markdown
Member

Summary

  • test.sh, run.sh and format.sh are given a real #!/bin/bash shebang in place of the commented-out # /bin/bash, and the executable bit is set on all three, so the ./<name>.sh invocation each one documents in its own usage line works on a fresh clone instead of failing with Permission denied.
  • test.sh and run.sh no longer hardcode python. An interpreter is resolved first: the first of python3, then python, that reports 3.8 or newer — the minimum README states — is taken, and a PYTHON environment variable set beforehand overrides the choice so a virtualenv interpreter can be pointed at directly. When no candidate qualifies, a plain message is printed and the script exits 1, rather than the run proceeding under Python 2.7 and surfacing as a conftest ImportError for pygame.
  • The commands documented in README.md and in the comment at requirements.txt:2 are spelled python3, and the interpreter-resolution behaviour and the PYTHON override are documented in README's usage section.
  • The coverage artifacts test.sh writes (.coverage, cov.xml) are added to .gitignore. This is adjacent rather than named by the issue, and is included deliberately: with ./test.sh now runnable on a fresh clone, the project's only verification gate would otherwise leave two untracked files in the tree every time it is run — the same class of stray-artifact leak that commit ad85816 had to undo.
  • tests/test_shell_scripts.py is added as a regression guard, since none of the above is exercised by the existing suite. The resolver is run rather than pattern-matched: its function body is lifted out of the script under test and executed against fake python3/python executables that report whatever version a case needs, with PATH holding nothing else. Preference order, the fallback to python, the PYTHON override, the version floor and the failure exit are each observed. The floor is parsed out of README, so the scripts and the requirement they implement cannot drift apart quietly. The shebang, the executable bit, and the fact that every line running pytest or the game goes through "$PYTHON" are asserted statically alongside.

Test plan

  • python3 -m pytest — 306 passed, 0 failed (289 before this change, plus 17 new script tests). ./test.sh itself could not be invoked in the session this change was prepared in — the sandbox refuses to execute repository shell scripts — so the command test.sh issues was run directly instead.
  • Coverage was not regressed: no file under src/ is touched by this change, and total coverage is unchanged at 87%.
  • Regression evidence, empirically confirmed rather than reasoned: with test.sh, run.sh, format.sh, README.md and requirements.txt restored to their origin/main state and the new test file left in place, 16 of the 17 new tests FAIL (the seventeenth is a bash -n syntax check, which the old scripts also pass); with the change restored, all 17 PASS.
  • Two mutations were introduced to confirm the behavioural tests bite rather than merely accompany the change. Relaxing the floor to (3, 0) fails test_the_documented_minimum_version_is_the_one_enforced[test.sh] and nothing else; reversing the candidate order to python python3 fails test_the_newest_qualifying_candidate_is_preferred[test.sh] and nothing else. Both mutations were then reverted and the suite reconfirmed green.
  • Staged content was checked for stray artifacts: no __pycache__, .pyc, save.json, output.txt, .coverage, cov.xml or .claude/ path is included, and the three scripts are recorded in the index at mode 100755.
  • A one-off ./test.sh and ./run.sh on an ordinary machine is still worth doing before merge, since the end-to-end invocation is the one thing the sandbox could not cover and this repository has no CI workflow to cover it either.

Notes

  • No file under src/ is modified, so no player-facing behaviour changes and the UI-parity question between the pygame and text interfaces does not arise here.
  • The commented-out checkDependencies block in run.sh still mentions bare python; it is dead code and was left untouched to keep the diff to the issue's scope.
  • tests/test_shell_scripts.py does not mirror a module under src/ the way every other test file does, because the code it covers is not Python. It is placed at the top level of tests/ alongside the other non-package test files.

Deferred this cycle

The rest of the open backlog was not picked up, for these reasons:

Four pull requests were already open at triage — #59 (develop, conflicting), and Copilot drafts #98, #96 and #90 (two conflicting) — all between nine and fourteen months stale and authored elsewhere. None was opened by this loop, so none was adopted or closed here; a decision on them belongs to a human.

Closes #135

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

dmccoystephenson and others added 3 commits August 20, 2026 01:43
test.sh, run.sh and format.sh each documented themselves as ./<name>.sh
while being tracked non-executable with a commented-out shebang, so a
fresh clone got Permission denied from the one command a contributor is
told to run before opening a PR. All three now carry #!/bin/bash and the
executable bit.

test.sh and run.sh also hardcoded `python`, which is 2.7 on some systems
and absent on others; under 2.7 the failure surfaced as a conftest
ImportError for pygame rather than as a wrong interpreter. Both now pick
the first of python3/python that reports 3.8 or newer, honour a PYTHON
override for a virtualenv interpreter, and say so plainly when neither
candidate qualifies.

README and requirements.txt spell the run command python3, and README
documents the override. The coverage artifacts test.sh writes are
gitignored so running the gate no longer dirties the tree.

Closes #135

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first pass at these tests asserted that particular strings appeared in
test.sh and run.sh, which would have passed on a resolver that was worded
right and behaved wrong. The resolver's function body is now lifted out of
the script under test and run against fake python3/python executables that
report whatever version a case needs, so preference order, the fallback,
the PYTHON override, the version floor and the failure exit are each
observed rather than inferred.

The floor the tests hold the resolver to is parsed out of README, so the
scripts and the requirement they implement cannot drift apart quietly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Without bash on PATH the resolver cases handed subprocess.run a None
executable and died on a TypeError, which says nothing about the scripts.
They are marked to skip instead; the shebang and executable-bit checks need
no subprocess and still run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review

Scored against the local suite as the external anchor (python3 -m pytest — 306 passed, 0 failed), not against impression. This repository has no CI workflow, so that run is the whole of the anchor.

  • Scope: FAIL — six of the seven files are named by Shell scripts are not executable, have a commented-out shebang, and invoke a python that may be Python 2 #135, but .gitignore is not. It is kept rather than reverted, on the grounds that the two files it ignores are written by the very script this PR makes runnable on a fresh clone; the call is flagged here so a reviewer can overrule it, and it is disclosed in the PR body.
  • Tests-new: PASSresolvePython is the only new unit of behaviour introduced, and 12 of the 17 new tests execute it against fake interpreters rather than inspecting its text.
  • Tests-fix: PASS — confirmed empirically, not by reasoning. With the five changed scripts and documents restored to origin/main and the new test file left in place, 16 of 17 fail; restored, all 17 pass. The seventeenth is a bash -n syntax check that the old scripts also satisfy, which is expected. Two mutations were added on top: relaxing the floor to (3, 0) fails exactly test_the_documented_minimum_version_is_the_one_enforced[test.sh], and reversing the candidate order fails exactly test_the_newest_qualifying_candidate_is_preferred[test.sh].
  • Sibling structure: FAIL — every other file under tests/ mirrors a module under src/; tests/test_shell_scripts.py mirrors nothing, because the code it covers is not Python. It is placed at the top level of tests/ next to the other non-package test files, which is the closest available convention. Noted rather than resolved.
  • Sibling renames: PASStest.sh, run.sh and format.sh form a parallel series and all three receive the shebang and mode change in the same commit.
  • Docs: PASS — README's usage commands and the comment at requirements.txt:2 are updated, and README gains the interpreter-resolution and PYTHON-override description. README's Controls table and Libraries section are unaffected, since no keybinding, CLI flag or vendored dependency changes here.
  • Issue resolution: PASS — all three defects Shell scripts are not executable, have a commented-out shebang, and invoke a python that may be Python 2 #135 names are addressed: the executable bit, the commented-out shebang, and the assumption that python is Python 3.
  • Manual validation: PASS, with one gap — the command test.sh issues was run directly and is green. ./test.sh itself could not be invoked, because the sandbox this change was prepared in refuses to execute repository shell scripts. The end-to-end invocation is therefore the one claim in this PR resting on inspection rather than execution.
  • UI parity: PASS (not applicable) — no file under src/ is touched, so no behaviour lands in one interface and not the other.
  • No stray artifacts: PASS — nothing matching __pycache__, *.pyc, save.json, output.txt, .coverage, cov.xml or .claude/ is staged, and git ls-files -s records the three scripts at mode 100755.

Findings

  • test.sh:4-21 / run.sh:4-21 — the resolvePython function is duplicated verbatim between the two scripts, so the copies can drift apart. Extracting it into a sourced scripts/lib.sh would be the cleaner shape, but that was judged outside Shell scripts are not executable, have a commented-out shebang, and invoke a python that may be Python 2 #135's scope. The duplication is mitigated rather than removed: every resolver case is parametrised over both scripts, so a change made to one copy and not the other is caught by the suite.
  • run.sh:42-58 — the commented-out checkDependencies block still refers to bare python and bare pip. It is dead code and was deliberately left untouched, but anyone reviving it would revive the defect Shell scripts are not executable, have a commented-out shebang, and invoke a python that may be Python 2 #135 describes along with it.
  • run.sh:74-75resolvePython is called before getLatest, so a git pull that changes the resolver has no effect on the invocation already in flight. That ordering is correct for this script (bash reads ahead as it executes, so a mid-run rewrite is worth avoiding either way), and is called out only so it is not read as accidental.

Two rubric items are scored FAIL above, both judgment calls rather than defects, both left in place with the reasoning stated so a reviewer can decide against them.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit 47def1f into main Aug 22, 2026
@dmccoystephenson
dmccoystephenson deleted the feature/shell-script-interpreter-resolution branch August 22, 2026 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shell scripts are not executable, have a commented-out shebang, and invoke a python that may be Python 2

1 participant