Repository navigation
Conversation
ekalinin
left a comment
There was a problem hiding this comment.
Thanks for the patch! Two things before merge.
-
The
CHANGEStext 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 ownbin/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.7case intests/test_build_python.pydepends on the host.PATHends 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 beforepython2.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 apython2in front of/usr/bin. The fake scripts use only shell builtins andmake_pathis absolute, soPATHcan be just the tools directory: with+ os.pathsep + '/usr/bin:/bin'dropped in both tests, all six pass.
Legacy source builds fail when Python 2 is installed only as
python2.7, when a previous build leaves a stale/danglingtmpbin/pythonlink, 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.7whenpython2is 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.7fixture. 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 eitherpython2orpython2.7.Validation on macOS arm64:
python2in 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.coverage run -p -m pytest, 404 passed, 11 skipped, 11 integration cases deselected, 2 existing expected failures.coverage run -p -m pytest, 405 passed, 10 skipped, 11 deselected, 2 existing expected failures.flake8 --extend-ignore=E127 nodeenv.py tests setup.pyandgit diff --checkpassed.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:
tox.inipass on Python 3.10 and 3.13.type(scope): subject.Published history is preserved by merging current master; no force push is used.