build: don't pass python override to V8 build #38969

richardlau · 2021-06-08T13:41:01Z

If the configure.py script is executed by a Python binary that is
not the one on the PATH it will create a python symlink in
out/tools/bin and prefix that to the PATH so it is used instead of
the one that otherwise would have been found on the PATH. This is
done so that gyp scripts shelling out to python execute with the
same version of Python as used to run the configure script.

V8's build uses V8's build toolchain (i.e. not gyp) and currently that
is incompatible with Python 3. Prevent prefixing the PATH for the V8
build so that it picks up python from the unprefixed PATH. This will
allow us to build Node.js with Python 3 but still use Python 2 to build
V8 in the CI.

Refs: #38791 (comment)

If we land this I need to undo the change in #36691 (comment) to force configure to run with Python 2 to allow Node.js' to autodetect Python 3 again (and this PR should allow V8 to then build with python pointing to Python 2).

cc @targos @nodejs/v8-update

richardlau · 2021-06-08T14:08:25Z

If we land this I need to undo the change in #36691 (comment) to force configure to run with Python 2 to allow Node.js' to autodetect Python 3 again (and this PR should allow V8 to then build with python pointing to Python 2).

On second thought, since the V8 CI is currently broken anyway after #38867 I'll update the job now and we can verify that this PR fixes it.

targos

RSLGTM

targos · 2021-06-08T14:18:55Z

+1 to fast-track if V8 CI is fixed

richardlau · 2021-06-08T14:21:53Z

I've undone the previous change to the V8 CI that was running python ./configure.py to force building Node.js with Python 2 back to ./configure so that it autodetects Python 3 again.

Build against master (expected to fail as V8 will attempt to build with Python 3): https://ci.nodejs.org/job/node-test-commit-v8-linux/4032/

Build with this PR (expected to pass): https://ci.nodejs.org/job/node-test-commit-v8-linux/4033/

richardlau · 2021-06-08T14:52:34Z

Needs a bit more work. With this PR V8 does build successfully but then we end up back in our Makefile to run the V8 tests and the override written to config.mk is in effect which makes the V8 test runner run with Python 3 which then falls over.

node/Makefile

Lines 652 to 658 in 65a7fd3

    
           # Related CI job: node-test-commit-v8-linux 
        
           test-v8: v8  ## Runs the V8 test suite on deps/v8. 
        
           	deps/v8/tools/run-tests.py --gn --arch=$(V8_ARCH) $(V8_TEST_OPTIONS) \ 
        
           				mjsunit cctest debugger inspector message preparser \ 
        
           				$(TAP_V8) 
        
           	$(info Testing hash seed) 
        
           	$(MAKE) test-hash-seed

https://ci.nodejs.org/job/node-test-commit-v8-linux/4033/nodes=rhel7-s390x,v8test=v8test/console

15:36:35 Testing hash seed
15:36:35 deps/v8/tools/run-tests.py --gn --arch=s390x --progress=dots --timeout=120 \
15:36:35 			mjsunit cctest debugger inspector message preparser \
15:36:35 			--junitout /home/iojs/build/workspace/node-test-commit-v8-linux/v8-tap.xml
15:36:35 Traceback (most recent call last):
15:36:36   File "/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/tools/testrunner/base_runner.py", line 273, in execute
15:36:36     parser = self._create_parser()
15:36:36   File "/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/tools/testrunner/base_runner.py", line 319, in _create_parser
15:36:36     self._add_parser_default_options(parser)
15:36:36   File "/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/tools/testrunner/base_runner.py", line 355, in _add_parser_default_options
15:36:36     help="The style of progress indicator (verbose, dots, "
15:36:36   File "/usr/lib64/python3.6/optparse.py", line 1000, in add_option
15:36:36     option = self.option_class(*args, **kwargs)
15:36:36   File "/usr/lib64/python3.6/optparse.py", line 581, in __init__
15:36:36     checker(self)
15:36:36   File "/usr/lib64/python3.6/optparse.py", line 670, in _check_choice
15:36:36     % str(type(self.choices)).split("'")[1], self)
15:36:36 optparse.OptionError: option -p/--progress: choices must be a list of strings ('dict_keys' supplied)
15:36:36 make: *** [Makefile:654: test-v8] Error 5

(Note I cannot just add python in front of run-tests.py as config.mk has modified the PATH such that python is pointing to the Python 3 binary autodetected by configure.)

If the `configure.py` script is executed by a Python binary that is not the one on the PATH it will create a `python` symlink in `out/tools/bin` and prefix that to the PATH so it is used instead of the one that otherwise would have been found on the PATH. This is done so that gyp scripts shelling out to `python` execute with the same version of Python as used to run the configure script. V8's build uses V8's build toolchain (i.e. not gyp) and currently that is incompatible with Python 3. Prevent prefixing the PATH for the V8 build so that it picks up `python` from the unprefixed PATH. This will allow us to build Node.js with Python 3 but still use Python 2 to build V8 in the CI.

richardlau · 2021-06-08T15:59:12Z

Trying out doing the PATH manipulation in the Makefile.
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/4034/ (✔️)

nodejs-github-bot · 2021-06-08T16:27:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38536/ (✔️)

richardlau · 2021-06-08T16:31:46Z

Trying out doing the PATH manipulation in the Makefile.
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/4034/

V8 CI has passed with this PR (🎉).

@jasnell @targos FYI I've changed this PR to move the PATH manipulation to remove the bin override directory from the tools/make-v8.sh script to Makefile instead so we can also run V8's test runner from Makefile.

github-actions · 2021-06-09T01:40:12Z

Fast-track has been requested by @gengjiawen. Please 👍 to approve.

If the `configure.py` script is executed by a Python binary that is not the one on the PATH it will create a `python` symlink in `out/tools/bin` and prefix that to the PATH so it is used instead of the one that otherwise would have been found on the PATH. This is done so that gyp scripts shelling out to `python` execute with the same version of Python as used to run the configure script. V8's build uses V8's build toolchain (i.e. not gyp) and currently that is incompatible with Python 3. Prevent prefixing the PATH for the V8 build so that it picks up `python` from the unprefixed PATH. This will allow us to build Node.js with Python 3 but still use Python 2 to build V8 in the CI. PR-URL: #38969 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

gengjiawen · 2021-06-09T12:43:43Z

Landed in ff09d6d

If the `configure.py` script is executed by a Python binary that is not the one on the PATH it will create a `python` symlink in `out/tools/bin` and prefix that to the PATH so it is used instead of the one that otherwise would have been found on the PATH. This is done so that gyp scripts shelling out to `python` execute with the same version of Python as used to run the configure script. V8's build uses V8's build toolchain (i.e. not gyp) and currently that is incompatible with Python 3. Prevent prefixing the PATH for the V8 build so that it picks up `python` from the unprefixed PATH. This will allow us to build Node.js with Python 3 but still use Python 2 to build V8 in the CI. PR-URL: #38969 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

If the `configure.py` script is executed by a Python binary that is not the one on the PATH it will create a `python` symlink in `out/tools/bin` and prefix that to the PATH so it is used instead of the one that otherwise would have been found on the PATH. This is done so that gyp scripts shelling out to `python` execute with the same version of Python as used to run the configure script. V8's build uses V8's build toolchain (i.e. not gyp) and currently that is incompatible with Python 3. Prevent prefixing the PATH for the V8 build so that it picks up `python` from the unprefixed PATH. This will allow us to build Node.js with Python 3 but still use Python 2 to build V8 in the CI. PR-URL: nodejs#38969 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

github-actions bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Jun 8, 2021

richardlau mentioned this pull request Jun 8, 2021

tools: update gyp-next to v0.9.1 #38867

Closed

This comment has been minimized.

Sign in to view

jasnell approved these changes Jun 8, 2021

View reviewed changes

targos approved these changes Jun 8, 2021

View reviewed changes

richardlau added the wip Issues and PRs that are still a work in progress. label Jun 8, 2021

richardlau force-pushed the nooverridev8 branch from 774c8fe to 6dae709 Compare June 8, 2021 15:57

richardlau added build Issues and PRs related to build files or the CI. and removed wip Issues and PRs that are still a work in progress. tools Issues and PRs related to the tools directory. labels Jun 8, 2021

gengjiawen added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 9, 2021

targos approved these changes Jun 9, 2021

View reviewed changes

richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 9, 2021

gengjiawen closed this Jun 9, 2021

richardlau deleted the nooverridev8 branch June 9, 2021 15:14

bl-ue approved these changes Jun 9, 2021

View reviewed changes

richardlau mentioned this pull request Jun 11, 2021

v14.17.1 release proposal #38948

Merged

danielleadams mentioned this pull request Jun 14, 2021

v16.4.0 proposal #39031

Merged

richardlau mentioned this pull request Jul 20, 2021

v14.17.4 proposal #39466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: don't pass python override to V8 build #38969

build: don't pass python override to V8 build #38969

richardlau commented Jun 8, 2021

This comment has been minimized.

richardlau commented Jun 8, 2021

targos left a comment

targos commented Jun 8, 2021

richardlau commented Jun 8, 2021

richardlau commented Jun 8, 2021 •

edited

richardlau commented Jun 8, 2021 •

edited

nodejs-github-bot commented Jun 8, 2021 •

edited by richardlau

richardlau commented Jun 8, 2021 •

edited

github-actions bot commented Jun 9, 2021

gengjiawen commented Jun 9, 2021

build: don't pass python override to V8 build #38969

build: don't pass python override to V8 build #38969

Conversation

richardlau commented Jun 8, 2021

This comment has been minimized.

richardlau commented Jun 8, 2021

targos left a comment

Choose a reason for hiding this comment

targos commented Jun 8, 2021

richardlau commented Jun 8, 2021

richardlau commented Jun 8, 2021 • edited

richardlau commented Jun 8, 2021 • edited

nodejs-github-bot commented Jun 8, 2021 • edited by richardlau

richardlau commented Jun 8, 2021 • edited

github-actions bot commented Jun 9, 2021

gengjiawen commented Jun 9, 2021

richardlau commented Jun 8, 2021 •

edited

richardlau commented Jun 8, 2021 •

edited

nodejs-github-bot commented Jun 8, 2021 •

edited by richardlau

richardlau commented Jun 8, 2021 •

edited