Skip to content

fix(nodeenv): select legacy Python by its original command name - #437

Open
jihtsan wants to merge 4 commits into
ekalinin:masterfrom
jihtsan:codex/435-source-python-launcher
Open

jihtsan wants to merge 4 commits into
ekalinin:masterfrom
jihtsan:codex/435-source-python-launcher

Conversation

@jihtsan

@jihtsan jihtsan commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Legacy source builds fail when Python 2 is installed only as python2.7, when a previous build leaves a stale/dangling tmpbin/python link, or when a pyenv-style shim dispatches using its command name. Relative PATH entries also produce broken links after changing into the Node source directory.

Find python2.7 when python2 is absent. For Node before 8.8, write an executable launcher that invokes the selected command by its original absolute, shell-quoted path and forwards arguments. Remove previous symlinks without changing their targets, and refresh the launcher on retries. Preserve #429's version boundaries: 8.8–12.x get no PATH override; 13+ do not require Python 2.

The subprocess tests now limit PATH to their selected fake tools; a host Python 2 executable cannot shadow the python2.7 fixture. The changelog describes the absolute command path, launcher script, and replacement of stale links accurately. Current master's Python 3 compatibility cleanup is integrated; the existing #435 launcher behavior is preserved. The README now accurately permits either python2 or python2.7.

Validation on macOS arm64:

  • Simulated a system python2 in the OS search path without modifying OS directories: the prior tests produce 1 failure and 5 passes; the isolated tests pass all 6 cases under the same simulation. Scripts use shell builtins and the make command is absolute.
  • CPython 3.10.21: full non-integration suite under coverage run -p -m pytest, 404 passed, 11 skipped, 11 integration cases deselected, 2 existing expected failures.
  • CPython 3.13.12: full non-integration suite under coverage run -p -m pytest, 405 passed, 10 skipped, 11 deselected, 2 existing expected failures.
  • Full flake8 --extend-ignore=E127 nodeenv.py tests setup.py and git diff --check passed.
  • The original six behavior regressions failed on the unchanged base before the initial implementation. Earlier validation encountered an HTTP 503 also reproduced on that base; that historical failure remains recorded separately and did not recur in the current complete runs.

Actual old Node compilation with a real Python 2 installation, other operating systems, the remaining Python versions, and hosted CI for this new head were not run locally. Prepared and independently reviewed for Standards and Spec with Codex assistance.

Closes #435.

Contribution checklist for the integrated head:

  • Existing regression tests cover the issue and this repair.
  • Exact unit-test/coverage and flake8 commands from tox.ini pass on Python 3.10 and 3.13.
  • A tox environment itself was not invoked; the guide's equivalent commands were run directly.
  • CHANGES and README match the resulting behavior.
  • The PR title and new own commit subjects use type(scope): subject.

Published history is preserved by merging current master; no force push is used.

@ekalinin ekalinin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the patch! Two things before merge.

  • The CHANGES text doesn't quite match the code. The new entry says the launcher "preserves ... relative PATH entries" and "refreshes stale or dangling links", but it turns the python2 path into an absolute one and replaces the link with a script. In the #419 entry, "link" became "shim", which is easy to confuse with the pyenv shims the new entry talks about and with nodeenv's own bin/shim; "launcher", as in the new entry, would fit. Something like this for the new entry:

    - `--source` builds of node.js before 13 also accept `python2.7` when there
      is no `python2`. For node.js before 8.8, `python` in `PATH` is now a small
      launcher script instead of a link: it runs the selected python2 by its
      absolute path and its own name, so a pyenv shim or a relative `PATH` entry
      works, and a stale or dangling link left by an earlier run is replaced
      instead of reused
      `#435 <https://github.com/ekalinin/nodeenv/issues/435>`_
  • The python2.7 case in tests/test_build_python.py depends on the host. PATH ends with /usr/bin:/bin, so on a machine with /usr/bin/python2 (for example Ubuntu 20.04 or CentOS 7 with python2 installed) shutil.which('python2') finds that one before python2.7, the launcher runs it, and the test fails with ./configure ... failed with error code 2. It passes in CI only because the runners have no python2. I reproduced it by putting a directory with a python2 in front of /usr/bin. The fake scripts use only shell builtins and make_path is absolute, so PATH can be just the tools directory: with + os.pathsep + '/usr/bin:/bin' dropped in both tests, all six pass.

@jihtsan jihtsan changed the title fix: preserve Python command identity in legacy source builds fix(nodeenv): select legacy Python by its original command name Oct 26, 2026

This branch has not been deployed

No deployments
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.

--source: the tmpbin python -> python2 link breaks in several setups

2 participants