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

CHANGES

@@ -4,14 +4,21 @@ Nodeenv changelog
 Version [unreleased]
 --------------------
 
+- `--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>`_
 - Require Python 3.10 or later, matching the minimum version tested in CI.
   Update package classifiers and wheel tags, and remove the unused Python 2
   compatibility paths. Old Node.js source builds still require Python 2.7
   as their build interpreter
   `#436 <https://github.com/ekalinin/nodeenv/issues/436>`_
 - `--source` builds of node.js 13 and later no longer need a `python2`
   command; 8.8 to 12.x still need one but point `python` at it on their
-  own, so the `python` to python2 link in `PATH` is only set up for older
+  own, so the `python` to python2 launcher in `PATH` is only set up for older
   versions, and versions before 13 still fail early with a clear error
   when `python2` is missing
   `#419 <https://github.com/ekalinin/nodeenv/issues/419>`_

README.rst

@@ -115,9 +115,9 @@ For node.js
 ^^^^^^^^^^^
 
 * libssl-dev
-* Python 2.7 as a ``python2`` command when building Node.js versions older
-  than 13 from source; this is separate from the Python 3.10+ interpreter
-  that runs nodeenv.
+* Python 2.7 as a ``python2`` or ``python2.7`` command when building Node.js
+  versions older than 13 from source; this is separate from the Python 3.10+
+  interpreter that runs nodeenv.
 
 Usage
 -----

nodeenv.py

@@ -1043,22 +1043,29 @@ def build_node_from_src(env_dir, src_dir, node_src_dir, args):
     if node_version < (13, 0):
         # up to 12.x, ./configure only accepts Python 2.7, so fail early
         # with a clear error instead of a failed ./configure
-        python2_path = shutil.which('python2')
+        python2_path = (shutil.which('python2') or
+                        shutil.which('python2.7'))
         if not python2_path:
             raise OSError(
-                'no python2 command (required for building '
+                'no python2 or python2.7 command (required for building '
                 'node.js %s) was found' % args.node
             )
         if node_version < (8, 8):
             # before 8.8, gyp and make run `python` by name and node does
-            # not point it at Python 2 itself, so point `python` at python2
+            # not point it at Python 2 itself. Invoke the selected command
+            # by its own name so pyenv shims do not dispatch to `python`.
             logger.debug(' * Temporarily pointing python to %s',
                          python2_path)
-            node_tmpbin_dir = join(src_dir, 'tmpbin')
-            node_tmpbin_link = join(node_tmpbin_dir, 'python')
+            node_tmpbin_dir = abspath(join(src_dir, 'tmpbin'))
+            node_tmpbin_python = join(node_tmpbin_dir, 'python')
             mkdir(node_tmpbin_dir)
-            if not os.path.exists(node_tmpbin_link):
-                callit(['ln', '-s', python2_path, node_tmpbin_link])
+            # Remove old links without following them, including dangling
+            # ones left by a failed build. Refresh the launcher on retries.
+            if os.path.islink(node_tmpbin_python):
+                os.unlink(node_tmpbin_python)
+            writefile(node_tmpbin_python, '#!/bin/sh\nexec %s "$@"\n' %
+                      _quote(abspath(python2_path)))
+            make_executable(node_tmpbin_python)
             env['PATH'] = '{}:{}'.format(node_tmpbin_dir,
                                          os.environ.get('PATH', ''))
 

tests/nodeenv_test.py

@@ -6,6 +6,7 @@
 import os.path
 import pathlib
 import shutil
+import shlex
 import socket
 import subprocess
 import sys
@@ -3063,10 +3064,6 @@ def _build_env(call):
     return args[4] if len(args) > 4 else None
 
 
-def _build_calls(calls):
-    return [c for c in calls if c[0][0][0] != 'ln']
-
-
 @pytest.mark.parametrize('node_version', ['8.8.0', '12.22.12', '22.11.0'])
 def test_build_node_from_src_current_node_does_not_link_python(tmpdir,
                                                                node_version):
@@ -3109,14 +3106,16 @@ def test_build_node_from_src_old_node_points_python_at_python2(tmpdir,
     before 8.8, gyp and make run `python` by name, so `python` in PATH
     is pointed at python2 for the build
     """
+    python2_path = str(tmpdir.join('selected tools', 'python2'))
     src_dir, node_src_dir, calls = _build_node_from_src(
-        tmpdir, node_version, '/usr/bin/python2')
+        tmpdir, node_version, python2_path)
 
     tmpbin = src_dir.join('tmpbin')
     assert tmpbin.isdir()
-    assert calls[0][0][0] == [
-        'ln', '-s', '/usr/bin/python2', str(tmpbin.join('python'))]
-    build = _build_calls(calls)
+    shebang, command = tmpbin.join('python').read().splitlines()
+    assert shebang == '#!/bin/sh'
+    assert shlex.split(command) == ['exec', python2_path, '$@']
+    build = calls
     assert [c[0][0][0] for c in build] == [
         './configure', 'make', 'make install']
     for c in build:

tests/test_build_python.py

@@ -0,0 +1,73 @@
+from types import SimpleNamespace
+
+import pytest
+
+import nodeenv
+
+
+pytestmark = pytest.mark.skipif(
+    nodeenv.is_WIN, reason='source builds use POSIX configure scripts')
+
+
+def _script(path, contents):
+    path.parent.mkdir(parents=True, exist_ok=True)
+    path.write_text('#!/bin/sh\n' + contents)
+    path.chmod(0o755)
+    return path
+
+
+def _build(tmp_path, node='8.7.0'):
+    source = tmp_path / 'src' / ('node-v' + node)
+    _script(source / 'configure',
+            'exec python configure-argument "argument with spaces"\n')
+    make = _script(tmp_path / 'make', 'exit 0\n')
+    args = SimpleNamespace(
+        node=node, load_average=None, jobs='2', without_ssl=False,
+        debug=False, profile=False, make_path=str(make), verbose=False)
+    nodeenv.build_node_from_src(
+        str(tmp_path / 'env'), str(source.parent), str(source), args)
+
+
+@pytest.mark.parametrize('case', [
+    'python2.7', 'dangling-link', 'stale-link', 'shim', 'relative-path',
+])
+def test_legacy_build_selects_python_without_reusing_a_link(
+        tmp_path, monkeypatch, case):
+    tools = tmp_path / 'selected tools'
+    name = 'python2.7' if case == 'python2.7' else 'python2'
+    python = _script(
+        tools / name,
+        '[ "${0##*/}" = "' + name + '" ] || exit 19\n'
+        'printf "%s\\n" "$0" "$@" >> "$PYTHON_LOG"\n')
+    log = tmp_path / 'python.log'
+    monkeypatch.setenv('PYTHON_LOG', str(log))
+    monkeypatch.chdir(tmp_path)
+    path = str(tools) if case != 'relative-path' else tools.name
+    monkeypatch.setenv('PATH', path)
+
+    launcher = tmp_path / 'src' / 'tmpbin' / 'python'
+    old = tmp_path / 'old' / 'python2'
+    if case in ('stale-link', 'dangling-link'):
+        launcher.parent.mkdir(parents=True)
+        if case == 'stale-link':
+            _script(old, 'exit 23\n')
+        launcher.symlink_to(old)
+
+    _build(tmp_path)
+
+    assert log.read_text().splitlines() == [
+        str(python), 'configure-argument', 'argument with spaces']
+    if case == 'stale-link':
+        assert old.read_text() == '#!/bin/sh\nexit 23\n'
+
+
+def test_legacy_build_refreshes_launcher_on_retry(tmp_path, monkeypatch):
+    log = tmp_path / 'python.log'
+    monkeypatch.setenv('PYTHON_LOG', str(log))
+    for label in ('first', 'second'):
+        tools = tmp_path / label
+        _script(tools / 'python2',
+                'printf "%s\\n" "' + label + '" >> "$PYTHON_LOG"\n')
+        monkeypatch.setenv('PATH', str(tools))
+        _build(tmp_path)
+    assert log.read_text().splitlines() == ['first', 'second']