fix: do not require python2 to build node from source

CHANGES

@@ -4,6 +4,12 @@ Nodeenv changelog
 Version [unreleased]
 --------------------
 
+- `--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
+  versions, and versions before 13 still fail early with a clear error
+  when `python2` is missing
+  `#419 <https://github.com/ekalinin/nodeenv/issues/419>`_
 - The node.js archive download also retries when the connection is reset or
   times out in the middle of the body, pauses between the attempts and ends
   with an error message instead of a traceback when all of them fail; a

nodeenv.py

@@ -1078,24 +1078,28 @@ def build_node_from_src(env_dir, src_dir, node_src_dir, args):
         if value is not None
     ]
 
-    if getattr(sys.version_info, 'major', sys.version_info[0]) > 2:
-        # Currently, the node.js build scripts are using python2.*,
-        # therefore we need to temporarily point python exec to the
-        # python 2.* version in this case.
+    node_version = parse_version(args.node)
+    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')
         if not python2_path:
             raise OSError(
-                'Python >=3.0 virtualenv detected, but no python2 '
-                'command (required for building node.js) was found'
+                'no python2 command (required for building '
+                'node.js %s) was found' % args.node
             )
-        logger.debug(' * Temporarily pointing python to %s', python2_path)
-        node_tmpbin_dir = join(src_dir, 'tmpbin')
-        node_tmpbin_link = 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])
-        env['PATH'] = '{}:{}'.format(node_tmpbin_dir,
-                                     os.environ.get('PATH', ''))
+        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
+            logger.debug(' * Temporarily pointing python to %s',
+                         python2_path)
+            node_tmpbin_dir = join(src_dir, 'tmpbin')
+            node_tmpbin_link = 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])
+            env['PATH'] = '{}:{}'.format(node_tmpbin_dir,
+                                         os.environ.get('PATH', ''))
 
     conf_cmd = [
         './configure',

tests/nodeenv_test.py

@@ -2902,3 +2902,96 @@ def test_append_goes_before_a_powershell_signature(self, tmpdir):
                                '# SIG # Begin signature block\n'
                                '# MIIF...\n'
                                '# SIG # End signature block\n')
+
+
+def _build_node_from_src(tmpdir, node_version, python2_path):
+    args = mock.Mock(node=node_version, load_average=None, jobs='2',
+                     without_ssl=False, debug=False, profile=False,
+                     make_path='make', verbose=False)
+    src_dir = tmpdir.join('src')
+    node_src_dir = src_dir.join('node-v%s' % node_version)
+    with mock.patch.object(nodeenv.shutil, 'which',
+                           return_value=python2_path), \
+            mock.patch.object(nodeenv, 'callit') as mck:
+        nodeenv.build_node_from_src(
+            str(tmpdir.join('env')), str(src_dir), str(node_src_dir), args)
+    return src_dir, node_src_dir, mck.call_args_list
+
+
+def _build_env(call):
+    """extra_env passed to callit, positionally or as a keyword"""
+    args, kwargs = call
+    if 'extra_env' in kwargs:
+        return kwargs['extra_env']
+    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):
+    """
+    since 8.8, node points `python` at the right interpreter itself, so a
+    source build does not rewrite PATH, even when python2 is installed
+    """
+    src_dir, node_src_dir, calls = _build_node_from_src(
+        tmpdir, node_version, '/usr/bin/python2')
+
+    assert [c[0][0][0] for c in calls] == [
+        './configure', 'make', 'make install']
+    for c in calls:
+        assert c[0][3] == str(node_src_dir)
+        assert not _build_env(c)
+    assert not src_dir.join('tmpbin').exists()
+
+
+@pytest.mark.parametrize('node_version', ['13.0.0', '22.11.0'])
+def test_build_node_from_src_node_13_does_not_need_python2(tmpdir,
+                                                           node_version):
+    """
+    since 13, ./configure also runs on Python 3, so a source build works
+    without a python2 command
+    """
+    src_dir, node_src_dir, calls = _build_node_from_src(
+        tmpdir, node_version, None)
+
+    assert [c[0][0][0] for c in calls] == [
+        './configure', 'make', 'make install']
+    for c in calls:
+        assert not _build_env(c)
+    assert not src_dir.join('tmpbin').exists()
+
+
+@pytest.mark.parametrize('node_version', ['0.10.25', '8.5.0', '8.7.0'])
+def test_build_node_from_src_old_node_points_python_at_python2(tmpdir,
+                                                               node_version):
+    """
+    before 8.8, gyp and make run `python` by name, so `python` in PATH
+    is pointed at python2 for the build
+    """
+    src_dir, node_src_dir, calls = _build_node_from_src(
+        tmpdir, node_version, '/usr/bin/python2')
+
+    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)
+    assert [c[0][0][0] for c in build] == [
+        './configure', 'make', 'make install']
+    for c in build:
+        assert c[0][3] == str(node_src_dir)
+        assert _build_env(c)['PATH'].startswith(str(tmpbin) + ':')
+
+
+@pytest.mark.parametrize('node_version', ['0.10.25', '8.7.0', '12.22.12'])
+def test_build_node_from_src_old_node_without_python2(tmpdir, node_version):
+    """
+    building node.js before 13 without python2 fails early with a clear
+    error instead of a failed ./configure
+    """
+    with pytest.raises(OSError, match='python2.*' + node_version):
+        _build_node_from_src(tmpdir, node_version, None)