Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: test Python 3.7 on Travis CI #29451

Closed
wants to merge 2 commits into from
Closed

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Sep 5, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Sep 5, 2019
@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Sep 5, 2019
@sam-github sam-github added the wip Issues and PRs that are still a work in progress. label Sep 5, 2019
@sam-github
Copy link
Contributor

Requires #25878 or py3 will fail at configure, so I cherry-picked it in.

Took me a bit to get pyenv working locally on Ubuntu, but while waiting for Travis, I'm in process of reproducing the buildsteps travis should do.

Note, for the pyenv statements to work, anyone else doing this (and me) will need to do export TRAVIS=yes to cause pyenv's python to be chosen over a specific version.

Once I've done this, current state is:

Compile V8

Compile Node.js

  • make fails with same keyword problem as above
  • moving on

Test JS Suites

  • obviously impossible until node.js Compile works

Test C++ Suites

  • make cctest also has the async keyword problem, so this step is impossible as well

At this point, I think futher progress is blocked until #29340 completes and lands.

@cclauss
Copy link
Contributor Author

cclauss commented Sep 6, 2019

As noted in #29340, both the tools and deps version of that file need to be modified.

@sam-github
Copy link
Contributor

Thanks, I'll take another shot at this tomorrow.

@cclauss
Copy link
Contributor Author

cclauss commented Sep 6, 2019

I added the async change to the deps version of code_generator.py

@cclauss cclauss changed the title build: explain how to test Py3.6 & Py3.7 on Travis build: test Python 3.7 on Travis CI Sep 6, 2019
@sam-github
Copy link
Contributor

With the fixes merged in, I can now build and test locally without errors, which is unfortunate, because now I can't troubleshoot locally.

Travis error is:

Building addon in /home/travis/build/nodejs/node/test/addons/01_function_arguments
465Building addon in /home/travis/build/nodejs/node/test/addons/02_callbacks
466/home/travis/build/nodejs/node/tools/build-addons.js:58
467main(process.argv[3]).catch((err) => setImmediate(() => { throw err; }));
468                                                          ^
469
470Error: Command failed: /home/travis/build/nodejs/node/out/Release/node /home/travis/build/nodejs/node/deps/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --directory=/home/travis/build/nodejs/node/test/addons/01_function_arguments
471
472    at ChildProcess.exithandler (child_process.js:295:12)
473    at ChildProcess.emit (events.js:209:13)
474    at maybeClose (internal/child_process.js:1028:16)
475    at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5) {
476  killed: false,
477  code: 1,
478  signal: null,
479  cmd: '/home/travis/build/nodejs/node/out/Release/node /home/travis/build/nodejs/node/deps/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --directory=/home/travis/build/nodejs/node/test/addons/01_function_arguments',
480  stdout: '',
481  stderr: ''
482}
483Makefile:412: recipe for target 'test/addons/.buildstamp' failed

I'm trying to directly run the failing node-gyp command, and interestingly, I see that it uses python2... So, I removed /usr/bin/python2, and see something more interesting:

% PYTHON=/usr/bin/python3 ./out/Release/node deps/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --directory=/home/travis/build/no
dejs/node/test/addons/01_function_arguments --nodedir=/usr/local/stow/lts/include/
gyp info it worked if it ends with ok                                     
gyp info using [email protected]                             
gyp info using [email protected] | linux | x64              
gyp WARN chdir /home/travis/build/nodejs/node/test/addons/01_function_arguments is not a directory
gyp ERR! find Python                                                    
gyp ERR! find Python Python is not set from command line or npm configuration
gyp ERR! find Python checking Python explicitly set from environment variable PYTHON
gyp ERR! find Python - process.env.PYTHON is "/usr/bin/python3"                
gyp ERR! find Python - executable path is "/usr/bin/python3"
gyp ERR! find Python - version is "3.7.3"
gyp ERR! find Python - version is 3.7.3 - should be >=2.6.0 <3.0.0 
gyp ERR! find Python - THIS VERSION OF PYTHON IS NOT SUPPORTED                                                                                                               
gyp ERR! find Python checking if "python" can be used                                                                                                                        
gyp ERR! find Python - executable path is "/home/sam/.pyenv/versions/3.7.3/bin/python"                                                                                       
gyp ERR! find Python - version is "3.7.3"                                                                                                                                    
gyp ERR! find Python - version is 3.7.3 - should be >=2.6.0 <3.0.0
gyp ERR! find Python - THIS VERSION OF PYTHON IS NOT SUPPORTED          
gyp ERR! find Python checking if "python2" can be used    
gyp ERR! find Python - "python2" is not in PATH or produced an error                                                                                                         

This sparked a theory that Travis doesn't have python2 installed when pyenv global 3.7.1 is used, which is why its failing, and by destroying my local python2, I can now see the same error Travis gets:

% make -j1 V=1 test/addons/.buildstamp
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

    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: ''
}
make: *** [Makefile:412: test/addons/.buildstamp] Error 1

@cclauss
Copy link
Contributor Author

cclauss commented Sep 7, 2019

cp tools/gyp/pylib/gyp/input.py deps/npm/node_modules/node-gyp/gyp/pylib/gyp/
cp tools/gyp/pylib/gyp/generator/make.py deps/npm/node_modules/node-gyp/gyp/pylib/gyp/generator/

@cclauss
Copy link
Contributor Author

cclauss commented Sep 7, 2019

https://travis-ci.com/nodejs/node/jobs/232041903#L1008

python tools/test.py -j 2 -p dots --report --mode=release --flaky-tests=dontcare addons js-native-api node-api
tools/test.py:32: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import imp
Total: 115 tests
 *    0 tests will be skipped
 *  115 tests are expected to pass
 *    0 tests are expected to fail that we won't fix
 *    0 tests are expected to fail that we should fix
Running 115 tests
...................C..............................
..................................................
...............
=== release test ===
Path: addons/dlopen-ping-pong/test
--- stderr ---
node: ../binding.cc:29: void {anonymous}::LoadLibrary(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `handle != nullptr' failed.
Command: out/Release/node /home/travis/build/nodejs/node/test/addons/dlopen-ping-pong/test.js
--- CRASHED (Signal: 6) ---
===
=== 1 tests failed
=== 1 tests CRASHED
===
The command "python tools/test.py -j 2 -p dots --report --mode=release --flaky-tests=dontcare addons js-native-api node-api" exited with 1.

@sam-github
Copy link
Contributor

That's interesting, I'll take a look Monday unless @bnoordhuis gets to it first.

@Trott
Copy link
Member

Trott commented Sep 9, 2019

I guess the deps changes should be a separate commit from the other ones so that it can be re-cherry-picked onto things if the deps get updated but still don't have these changes?

.travis.yml Outdated
@@ -6,9 +6,15 @@ x-ccache-setup-steps: &ccache-setup-steps

os: linux
language: cpp
# Currently this file can only support one PYTHON_VERSION.
# To experiment with Python 3, comment out Python 2.7 and uncomment one of the Python 3 versions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, with this change, Python 2.7 is already commented out, so this comment needs to be updated?

@Trott
Copy link
Member

Trott commented Sep 9, 2019

@cclauss Should the work-in-progress label be removed at this point? Or is it still work-in-progress?

@sam-github
Copy link
Contributor

Still WIP, it isn't green, and has local copies of changes that are pending in other PRs, or other repos (v8, node-gyp, npm). Its very useful to show what problems will remain even after the other in-progress work completes.

@cclauss
Copy link
Contributor Author

cclauss commented Sep 9, 2019

This is still a WIP. It is designed to pass the Travis CI tests on Python 3 but it still fails.

@cclauss
Copy link
Contributor Author

cclauss commented Sep 9, 2019

Does anyone know how to resolve the last failing test?

@sam-github
Copy link
Contributor

@bnoordhuis any ideas?

@bnoordhuis
Copy link
Member

I looked into it yesterday but it wasn't immediately obvious to me what went wrong. binding.node tries to dlopen ping.so but somehow it fails.

Time for some printf-style debugging, I suppose. Christian, can you try this?

diff --git a/test/addons/dlopen-ping-pong/binding.cc b/test/addons/dlopen-ping-pong/binding.cc
index 6a6c297b52..02ade757c0 100644
--- a/test/addons/dlopen-ping-pong/binding.cc
+++ b/test/addons/dlopen-ping-pong/binding.cc
@@ -26,6 +26,7 @@ static ping ping_func;
 void LoadLibrary(const FunctionCallbackInfo<Value>& args) {
   const String::Utf8Value filename(args.GetIsolate(), args[0]);
   void* handle = dlopen(*filename, RTLD_LAZY);
+  if (handle == nullptr) fprintf(stderr, "dlopen(%s): %s\n", *filename, dlerror());
   assert(handle != nullptr);
   ping_func = reinterpret_cast<ping>(dlsym(handle, "dlopen_ping"));
   assert(ping_func != nullptr);

(I tried to push it to your branch but you have collaborator edits disabled, it seems?)

@sam-github
Copy link
Contributor

Rebased onto master now that #29885 has landed.

.travis.yml Show resolved Hide resolved
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying my review comments from #29985 (duplicate?).

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor

I'll clean up the comments and up the python version, but would like to see travis pass. It seems really, really slow... is ccache not shared? Something else? Maybe ccache is reset when .travis.yml changes?

Anyhow, I'll give it overnight to run some more before touching the branch again.

@sam-github
Copy link
Contributor

@cclauss I don't think #29985 was needed, this just needed rebasing and pushing. Also, to pass. I can hope 🤞

@richardlau
Copy link
Member

I'll clean up the comments and up the python version, but would like to see travis pass. It seems really, really slow... is ccache not shared? Something else? Maybe ccache is reset when .travis.yml changes?

https://github.com/nodejs/node/compare/253137b5502a6732eac9f0f899c6577c04338023..2ff431b7a08fab7938a73fb6f10ed76f151035aa#diff-42f385dff7890b3c213d9699bcc350cc suggests v8.h was updated in the force push which is going to force a recompilation of most of V8.

@sam-github sam-github removed the wip Issues and PRs that are still a work in progress. label Oct 17, 2019
@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

@nodejs/python @nodejs/node-gyp PTAL

@sam-github
Copy link
Contributor

smartos out of memory:

tools/doc/generate.js --node-version=v13.0.0 --apilinks=out/doc/apilinks.json doc/api/url.md --output-directory=out/doc/api\""; exit 1; fi;
11:33:46 
11:33:46 
11:33:46 #
11:33:46 # Fatal error in , line 0
11:33:46 # Fatal process out of memory: Zone
11:33:46 #
11:33:46 #
11:33:46 #
11:33:46 #FailureMessage Object: fffffbffede7a500

Windows is an instance of #30011

Resuming the build.

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

@nodejs/platform-smartos FYI - #29451 (comment) I don't know how frequent that is, and hope it will be ephemral, but just to let you know.

sam-github pushed a commit that referenced this pull request Oct 18, 2019
Request Python 3 with pyenv and ensure that python3 is used by Makefile
to run Python scripts.

PR-URL: #29451
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@sam-github
Copy link
Contributor

Landed in d594a9a 🎉

@sam-github sam-github closed this Oct 18, 2019
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
Request Python 3 with pyenv and ensure that python3 is used by Makefile
to run Python scripts.

PR-URL: #29451
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
Request Python 3 with pyenv and ensure that python3 is used by Makefile
to run Python scripts.

PR-URL: #29451
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
targos pushed a commit that referenced this pull request Nov 8, 2019
Request Python 3 with pyenv and ensure that python3 is used by Makefile
to run Python scripts.

PR-URL: #29451
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
Request Python 3 with pyenv and ensure that python3 is used by Makefile
to run Python scripts.

PR-URL: #29451
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@devsnek devsnek deleted the Travis-test-Py36-or-Py37 branch May 21, 2020 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants