Skip to content

fix(makefile): quote RUN_PYTHON so make works on paths containing spaces - #610

Merged
calesthio merged 1 commit into
OpenMontageInc:mainfrom
CarlosBakerUAi:fix/makefile-quote-run-python
Nov 8, 2026
Merged

calesthio merged 1 commit into
OpenMontageInc:mainfrom
CarlosBakerUAi:fix/makefile-quote-run-python

Conversation

@CarlosBakerUAi

Copy link
Copy Markdown
Contributor

Summary

RUN_PYTHON expands to the interpreter path, and that path may contain spaces.
Unquoted, sh word-splits it, so on such a checkout every make target fails and
ensure-venv reports the wrong cause:

/bin/sh: /Volumes/Some Drive/repo/.venv/bin/python: not found
ERROR: OpenMontage requires Python 3.10+.
Current interpreter is unavailable: /Volumes/Some Drive/repo/.venv/bin/python

The interpreter is fine and already the right version. Nothing about the
message points at the space, so the natural response is to go install a Python
that was never the problem. make setup, make test, make preflight,
make demo and the rest are all unusable until you work it out.

Spaces in a checkout path are not exotic: an external volume, a synced folder,
or any user directory with a space in the name will do it.

Related issue

Not filed separately; happy to open one if you would prefer that first.

Changes

  • Quote every command-position use of $(RUN_PYTHON) in the Makefile: the
    PIP recipe variable, recipe line starts, uses after ||, and the one
    inside a $$(...) command substitution.
  • Display-only uses inside echo strings are deliberately left unquoted, since
    quoting them would just put quote marks in the printed output.
  • No behavior change on paths without spaces.

Testing

New tests/tools/test_makefile_run_python_quoting.py, two tests:

  1. A static check that no command-position $(RUN_PYTHON) is unquoted.
  2. An end-to-end check that copies the Makefile into a temporary directory
    whose name contains a space, points VIRTUAL_ENV at it, and runs
    make ensure-venv for real.

Both fail on current main and pass with this change. Test 2 reproduces the
exact word-split error above.

Full suite on macOS: 965 passed, 11 skipped (963 before this PR, plus the two
tests added here).

Checklist

  • The change is focused on a single logical concern.
  • I ran the relevant tests locally (make test).
  • I updated docs/README if behavior or usage changed. (No user-facing
    behavior or usage change; the same commands simply work now.)
  • No unrelated files (build artifacts, local config) are included in the diff.

RUN_PYTHON expands to the interpreter path, and that path may contain
spaces. Unquoted, sh word-splits it, so every make target fails on such
a checkout and ensure-venv blames the wrong thing:

    /bin/sh: /Volumes/Some Drive/repo/.venv/bin/python: not found
    ERROR: OpenMontage requires Python 3.10+.
    Current interpreter is unavailable: .../.venv/bin/python

The interpreter is fine. The diagnosis sends people off to reinstall a
Python they do not need, and `make setup`, `make test`, `make preflight`
and the rest are all unusable until they work out that the space is the
problem.

Quote every command-position use. Display-only uses inside echo strings
are left alone.

Tests: a static check that no command-position use is unquoted, and an
end-to-end check that runs `make ensure-venv` from a temporary directory
whose name contains a space. Both fail on the current Makefile and pass
with this change.

@qinghuanandejiangshi qinghuanandejiangshi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the focused Makefile change and its regression coverage. I found no blocking issues.

I verified git diff --check, inspected every $(RUN_PYTHON) command-position use, and independently ran the test's static matcher against the proposed Makefile (zero unquoted command-position matches). The quoting is applied consistently to recipe starts, the PIP variable, the || fallback, and the nested command substitution, while display-only uses remain unchanged.

I could not exercise the GNU Make end-to-end test on this Windows host because make is not installed here; the test correctly marks that environment as skipped. The author's macOS end-to-end run covers the shell behavior this patch targets.

@calesthio calesthio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the focused Makefile quoting repair. Reviewed every changed command-position use of RUN_PYTHON, including PIP, the fallback command and nested substitution, against docs/PR_REVIEW_GUIDE.md. A clean integration with current main passed the static regression (1 passed); the GNU Make end-to-end case was correctly skipped because this Windows environment lacks make. The shell quoting is applied consistently without changing the display-only text, and the author's reported native run supplies the platform-specific execution evidence. No blocking issue found in this diff.

@calesthio calesthio closed this Nov 7, 2026
@calesthio calesthio reopened this Nov 7, 2026
calesthio pushed a commit that referenced this pull request Nov 8, 2026
@calesthio
calesthio merged commit 9540329 into OpenMontageInc:main Nov 8, 2026
1 check passed
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.

3 participants