fix: do not require python2 to build node from source

[r3wretrhy]

Fixes #419.

build_node_from_src() always looked up python2 on Python 3, raised OSError if there wasn't one, and otherwise prepended a tmpbin dir whose python points at python2. Since nodeenv only runs on Python 3 now, nodeenv --source failed for every node.js version on machines without python2, which is most of them.

What node itself needs, by version:

• Before 8.8, gyp and make run python by name (deps/v8/gypfiles/toolchain.gypi during configure, the js2c actions in node.gyp during make), and node does not point it at Python 2 itself. 8.7.0 adds get_bin_override(), but it crashes when there is no python at all. 8.8.0 adds the None check.
• Up to 12.x, ./configure only accepts Python 2.7 (Please use Python 2.7), so 8.8-12.x still need python2, they just find it on their own.
• From 13.0, ./configure also accepts Python 3.

So this:

• links tmpbin/python to python2 and prepends it to PATH only for args.node < 8.8,
• raises an OSError naming the version when python2 is missing for args.node < 13, instead of a bare Command ./configure ... failed without --verbose,
• runs configure / make / make install with the inherited environment otherwise.

Tests:

• 8.8.0, 12.22.12 and 22.11.0 with python2 on PATH: no tmpbin, no extra_env passed to callit (positional or keyword).
• 13.0.0 and 22.11.0 without python2: the build runs, no tmpbin, no extra_env.
• 0.10.25, 8.5.0 and 8.7.0 with python2 on PATH: tmpbin/python is linked to python2 and PATH starts with tmpbin for all three build commands.
• 0.10.25, 8.7.0 and 12.22.12 without python2: OSError.
• pytest -m 'not integration' tests: 364 passed, 46 skipped.
• flake8 --extend-ignore=E127 nodeenv.py tests setup.py is clean.

Added a CHANGES entry under Version [unreleased].

[ekalinin]

Thanks for picking up #419! Not requiring python2 for current node.js versions is the right goal, but removing the shim entirely breaks source builds of older ones.

Older node.js versions still need python2

Before 8.5 (and 6.12 on the 6.x line), node's ./configure is a plain #!/usr/bin/env python script that only runs on Python 2. I checked v0.10.25, v6.0.0, v6.11.0, v7.10.1, v8.0.0 and v8.4.0. The tmpbin/python -> python2 link is what makes it work on hosts where python is missing or is Python 3. From 6.12 / 8.5 on, configure is a #!/bin/sh wrapper that finds python2 by itself, and current versions try python3 before python, so the shim doesn't get in their way.

With this change, the README example nodeenv --node=0.10.25 --source env-0.10.25 fails even on a host that has python2 installed. It works on master.

Could we keep the shim only for the versions that need it instead? args.node is already the resolved version here, so something like:

    if parse_version(args.node) < (8, 5):
        # before 8.5, ./configure is a `#!/usr/bin/env python` script
        # that only runs on Python 2, so point `python` at python2
        python2_path = shutil.which('python2')
        if not python2_path:
            raise OSError(
                'no python2 command (required for building '
                'node.js %s) was found' % args.node
            )
        ...  # the existing tmpbin / PATH code

This also keeps a clear error for old versions. Without the check, a user without --verbose only sees Command ./configure ... failed with error code N, because callit drops the command output on failure unless show_stdout is set.

Tests

The new test only covers shutil.which returning None. On master that path raises before tmpbin is created or PATH is set, so the "no PATH override" and "no tmpbin" assertions would pass there too. The test fails on master only because of the OSError. It would be good to cover:

• a current version with python2 on PATH: no tmpbin, no PATH override
• an old version (e.g. 0.10.25) with python2 on PATH: tmpbin/python is created and PATH starts with it
• an old version without python2: OSError

Please check the call kwargs as well, so an extra_env= keyword can't slip past the assertions.

CHANGES

Please add an entry for #419 under Version [unreleased] in CHANGES, like the other fixes there.

[ekalinin]

See #429 (comment) for the details.

[r3wretrhy]

Thanks for checking the old releases, you're right. I pushed 9bb0da6:

• The shim is back, but only when parse_version(args.node) < (8, 5), with the comment you suggested. Without python2 those versions now raise OSError: no python2 command (required for building node.js 0.10.25) was found before ./configure runs. 8.5 and later use the inherited environment.
• Tests replaced with the three cases: a current version (8.5.0 and 22.11.0) with python2 on PATH gets no tmpbin and no extra_env; 0.10.25 with python2 gets the tmpbin/python link and PATH starting with tmpbin for configure, make and make install; 0.10.25 without python2 raises OSError. extra_env is checked both positionally and as a keyword. Against master the current-version cases fail, and against my first commit the two old-version cases fail.
• Added a CHANGES entry for --source fails without python2 on Python 3 #419 under Version [unreleased].

pytest -m 'not integration' tests gives 357 passed, 46 skipped, and flake8 is clean. I also updated the PR description.

[ekalinin]

Thanks for the quick update, and sorry: the (8, 5) cutoff I suggested was wrong. I only looked at the first line of configure, but gyp and make also run python by name:

• 8.5.0 and 8.6.x have no python override of their own. During configure, deps/v8/gypfiles/toolchain.gypi runs <!(python -c "import sys; print sys.byteorder"), and during make the js2c actions in node.gyp run python. Both fail when python is Python 3 or missing.
• 8.7.0 adds get_bin_override() (a python -> sys.executable link in out/tools/bin), but it calls os.path.realpath(which('python')) without a None check, so configure crashes when there is no python at all.
• 8.8.0 adds that check, so from 8.8 on node points python at the right interpreter itself.

So the link is needed for < (8, 8), and the code comment should give that reason, e.g.:

    if parse_version(args.node) < (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

The test that uses 8.5.0 as a "no link" version would then need 8.8.0, and an 8.7.x case would cover the boundary.

The CHANGES entry also needs a fix: up to 12.x, ./configure only accepts Python 2.7 (Please use Python 2.7 in v12.22.12), so 8.8-12.x still need python2, they just find it on their own. Without the early check, a missing python2 on those versions shows up as Command ./configure ... failed with error code 1 unless --verbose is given. Could the "no python2" error cover < (13, 0), while the tmpbin link stays for < (8, 8) only?

[r3wretrhy]

Thanks for digging into the gyp and make side, I only checked configure too. I pushed 979d883:

• The tmpbin link to python2 is now only set up for < (8, 8), with your comment about gyp and make running python by name.
• The "no python2" OSError now covers < (13, 0), so 8.8-12.x still fail early with the version in the message, they just don't get the link.
• Tests: 8.8.0, 12.22.12 and 22.11.0 with python2 get no link and no extra_env. 13.0.0 and 22.11.0 build without python2. 0.10.25, 8.5.0 and 8.7.0 get the link with PATH starting at tmpbin. 0.10.25, 8.7.0 and 12.22.12 without python2 raise OSError. With 9bb0da6's nodeenv.py, the 8.5.0/8.7.0 link cases and the 8.7.0/12.22.12 error cases fail.
• The CHANGES entry now says 13+ no longer needs python2, 8.8-12.x still need it but point python at it themselves, and older versions get the link.

pytest -m 'not integration' tests gives 364 passed, 46 skipped, and flake8 --extend-ignore=E127 nodeenv.py tests setup.py is clean. I updated the PR description to match.

[ekalinin]

Thanks!