Node: tools/test.py is not yet Python 3 compatible

[cclauss]

The other four Travis CI test runs now complete successfully on Python 3.6 but the __Test C++ Suites__ run fails. __HELP NEEDED:__ This blocks our progress on Python 3 compatibility and the root cause is hidden behind JavaScript code that I do not understand.

See #29451 to understand how the C++ tests fail on Travis CI. Any progress or hints would be much appreciated.

Note that https://github.com/nodejs/node/blob/master/tools/test.py#L32 does __import imp__ which raises a deprecation warning and sometime soon we need to modify the related code to use __import importlib__ (and possibly also __import importlib.util__) https://docs.python.org/3/library/importlib.html#module-importlib.util instead but I have tried a few times without success. I do not believe that this is the source of our inability to pass the __Test C++ Suites__.

As mentioned at:

1. https://github.com/nodejs/node/issues/25789#issuecomment-459001124
2. https://github.com/nodejs/node/issues/25789#issuecomment-522140095

• Version:
• Platform:
• Subsystem:

[cclauss]

@nodejs/python

[Trott]

@nodejs/testing

[sam-github]

@cclauss I'd like to help, but I don't know what C++ tests this refers to, or how to reproduce it.

make PYTHON=python3 test (which includes cctest) and the more explicit python3 tools/test.py -J --mode=release both pass on Linux, with python 3.7.3

Could you provide an example make or other CLI command that fails, so I can try running it?

Or, if its Travis specific, could you provide a link to a failing travis build, preferably with a PR branch that causes the failure (assuming some kind of Travis config change is required to cause the failure)?

[cclauss]

The other four Travis CI test runs now complete successfully on Python 3.6

This means that this issue is about __Travis CI__ tests on __Python 3.6__.
It is also trying to point out there are five tests runs that are made on Travis CI.

but the Test C++ Suites run fails.

This means that the other four tests can safely be ignored for now because they already pass.
__Test C++ Suites__ is in bold because it is the one test run that is failing and needs attention.

29360 made these test failures evident but unfortunately it had to be reverted. Now #29451 clarifies how to test Python 3 on Travis CI.

Results from one week ago: https://travis-ci.com/nodejs/node/builds/125061793

[richardlau]

The Test C++ Suites run node-gyp to build the addons tests. Is node-gyp expected to be Python 3.6 compatible?

[sam-github]

Thanks for #29451, I attempted to use it to reproduce this, but since compilation is blocked by https://github.com/nodejs/node/pull/29340, I'm not able to, yet.

Are there other fixes I can cherry-pick into #29451 to get a compile, so that I can get to the "Test C++ Suites" stage?

[cclauss]

29346

[sam-github]

The root cause of this is that the node-gyp.js in the npm that is included in node's deps/ ONLY works with python 2.7, at least by default. Poking around in deps/npm/node_modules/node-gyp/lib/find-python.js it appears node-gyp allows python3, if EXPERIMENTAL_NODE_GYP_PYTHON3=yes.

Retrying that, we get back to failures in python instead of javascript. Yay?

% make -j1 V=1 test/addons/.buildstamp EXPERIMENTAL_NODE_GYP_PYTHON3=yes
Building addon in /home/sam/w/core/lts/test/addons/01_function_arguments
Building addon in /home/sam/w/core/lts/test/addons/02_callbacks
Building addon in /home/sam/w/core/lts/test/addons/03_object_factory
Building addon in /home/sam/w/core/lts/test/addons/04_function_factory
/home/sam/w/core/lts/tools/build-addons.js:58
main(process.argv[3]).catch((err) => setImmediate(() => { throw err; }));
                                                          ^

Error: Command failed: /home/sam/w/core/lts/out/Release/node /home/sam/w/core/lts/deps/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --directory=/home/sam/w/core/lts/test/addons/01_function_arguments
Traceback (most recent call last):
  File "/home/sam/w/core/lts/deps/npm/node_modules/node-gyp/gyp/gyp_main.py", line 13, in <module>
    import gyp
  File "/home/sam/w/core/lts/deps/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 10, in <module>
    import gyp.input
  File "/home/sam/w/core/lts/deps/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 7, in <module>
    from compiler.ast import Const
ModuleNotFoundError: No module named 'compiler'

    at ChildProcess.exithandler (child_process.js:295:12)
    at ChildProcess.emit (events.js:209:13)
    at maybeClose (internal/child_process.js:1028:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5) {
  killed: false,
  code: 1,
  signal: null,
  cmd: '/home/sam/w/core/lts/out/Release/node /home/sam/w/core/lts/deps/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --directory=/home/sam/w/core/lts/test/addons/01_function_arguments',
  stdout: '',
  stderr: 'Traceback (most recent call last):\n' +
    '  File "/home/sam/w/core/lts/deps/npm/node_modules/node-gyp/gyp/gyp_main.py", line 13, in <module>\n' +
    '    import gyp\n' +
    '  File "/home/sam/w/core/lts/deps/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 10, in <module>\n' +
    '    import gyp.input\n' +
    '  File "/home/sam/w/core/lts/deps/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 7, in <module>\n' +
    '    from compiler.ast import Const\n' +
    "ModuleNotFoundError: No module named 'compiler'\n"
}
make: *** [Makefile:412: test/addons/.buildstamp] Error 1

[sam-github]

@nodejs/npm @nodejs/gyp --- I suspect the above is known.

Since node tests whether addons can be built with the node-gyp included with npm, that's going to block node's tests passing until its fixed in npm.

Of course, it was always going to be a blocker for the ecosystem.

py3 support does exist, https://github.com/nodejs/node-gyp/pull/1844, perhaps this conversation should move over there.

[bnoordhuis]

We could vendor node-gyp in deps/ as a temporary workaround until npm picks up a python3-capable node-gyp (and we pick up that version of npm), it only needs trivial changes to the Makefile:

diff --git a/Makefile b/Makefile
index 33d43798f5..d38b11e6cb 100644
--- a/Makefile
+++ b/Makefile
@@ -351,7 +351,7 @@ benchmark/napi/function_call/build/$(BUILDTYPE)/binding.node: \
                benchmark/napi/function_call/napi_binding.c \
                benchmark/napi/function_call/binding.cc \
                benchmark/napi/function_call/binding.gyp | all
-       $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
+       $(NODE) deps/node-gyp/bin/node-gyp rebuild \
                --python="$(PYTHON)" \
                --directory="$(shell pwd)/benchmark/napi/function_call" \
                --nodedir="$(shell pwd)"
@@ -360,7 +360,7 @@ benchmark/napi/function_args/build/$(BUILDTYPE)/binding.node: \
                benchmark/napi/function_args/napi_binding.c \
                benchmark/napi/function_args/binding.cc \
                benchmark/napi/function_args/binding.gyp | all
-       $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
+       $(NODE) deps/node-gyp/bin/node-gyp rebuild \
                --python="$(PYTHON)" \
                --directory="$(shell pwd)/benchmark/napi/function_args" \
                --nodedir="$(shell pwd)"
@@ -391,14 +391,14 @@ ADDONS_BINDING_SOURCES := \
        $(filter-out test/addons/??_*/*.h, $(wildcard test/addons/*/*.h))

 ADDONS_PREREQS := config.gypi \
-       deps/npm/node_modules/node-gyp/package.json tools/build-addons.js \
+       deps/node-gyp/package.json tools/build-addons.js \
        deps/uv/include/*.h deps/v8/include/*.h \
        src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h

 define run_build_addons
 env npm_config_loglevel=$(LOGLEVEL) npm_config_nodedir="$$PWD" \
        npm_config_python="$(PYTHON)" $(NODE) "$$PWD/tools/build-addons" \
-       "$$PWD/deps/npm/node_modules/node-gyp/bin/node-gyp.js" \
+       "$$PWD/deps/node-gyp/bin/node-gyp.js" \
        $1
 touch $2
 endef

[sam-github]

@bnoordhuis we could, but should we? If the test is trying to show addons can be built by npm (as people actually do, for the most part), then showing that upstream node-gyp works OK doesn't help so much. That said, I might give it a shot to see if updating node-gyp allows the build to continue and uncover a later problem.

[cclauss]

Perhaps we can modify __just two deps/npm files__ for now... https://github.com/nodejs/node/pull/29451#issuecomment-529076708

[rvagg]

do we need to get a release out? npm is pretty quick in picking up our releases these days, so if we have something in the queue that we want to get into the funnel then we should just push it. We have an awkward breaking-change list now so we might need to start maintaining two release lines for a little while, 5 & 6.

[cclauss]

Are there any objections to @rvagg's plan above and make a release and then encourage npm to adopt it so that we can cherrypick it back in?!?

[cclauss]

Closing this issue because the file __tools/test.py__ was not incompatible with Python 3. #29451 has proven that changes made to several _other_ files can enable __tools/test.py__ to pass on Python 3.