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: add Python 3 tests to Travis CI #29196

Closed

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Aug 18, 2019

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

Based on the lessons learned in #29193, this pull request adds Travis CI test suite named Python 3 is EXPERIMENTAL. These tests are run in allow_failures mode on Python 3.7.1 and they bypasses the Python version checks in ./configure by directly running ./configure.py.

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 18, 2019
@cclauss cclauss changed the title Add python 3 tests to travis ci @cclauss build: add Python 3 tests to Travis CI Aug 18, 2019
@cclauss cclauss changed the title @cclauss build: add Python 3 tests to Travis CI build: add Python 3 tests to Travis CI Aug 18, 2019
@Trott Trott added the python PRs and issues that require attention from people who are familiar with Python. label Aug 18, 2019
@Trott
Copy link
Member

Trott commented Aug 18, 2019

@nodejs/python @nodejs/build-files This seems like a very good idea to me: Add Python 3 to Travis but mark it as allowed-to-fail.

@Trott
Copy link
Member

Trott commented Aug 18, 2019

No need to run the full test suite in Jenkins for a change to .travis.yml but here's a lite CI just in case there's something weird and unexpected: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3835/

@richardlau
Copy link
Member

Is the intent (once configure.py is fixed for Python 3) to compile Node.js from source (using the output of configure/py)? If it is then there looks like there might be a few issues (e.g. compiler levels, builds taking too long (hence why we're using separate compile/test steps and ccache for the existing jobs), NODE=$(which node) make test would use node from the path and not the one built).

Those could possibly be fixed up later (which is why I'm not blocking this despites my concerns above).

@cclauss
Copy link
Contributor Author

cclauss commented Aug 19, 2019

once configure.py is fixed for Python 3

configure.py is already Python 3 compatible but it is the files that come afterwards that are not.

Those could possibly be fixed up later

Correct. Small moves. Let’s get the parts working separately before we try to get them working together.

cclauss added a commit to cclauss/node that referenced this pull request Aug 19, 2019
script:
- NODE=$(which node) make lint lint-py
- python3.7 ./configure.py
- NODE=$(which node) make test
Copy link
Member

Choose a reason for hiding this comment

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

You could also prefix this with PYTHON=python3.7 to run tools/gyp_node.py and tools/test.py with Python 3 too. There's also an install target that could exercise tools/install.py, but maybe that can be added later. It'll be important for making sure that release builds can be done with Python 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Will add these after make test goes green.

language: node_js
node_js: "node"
install:
- pyenv global 3.7.1
Copy link
Member

@gengjiawen gengjiawen Aug 20, 2019

Choose a reason for hiding this comment

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

Why not 3.7.4 since it's the latest ?

Copy link
Contributor Author

@cclauss cclauss Aug 20, 2019

Choose a reason for hiding this comment

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

3.7.4 is not available on the pyenv on Travis CI. pyenv list --install

We might even need to temporarily downgrade to Python 3.6 because async is a reserved word in Python >= 3.7 and that messes with inspector_protocol/code_generator.py

@gengjiawen

This comment has been minimized.

@gengjiawen gengjiawen added build Issues and PRs related to build files or the CI. and removed build Issues and PRs related to build files or the CI. labels Aug 20, 2019
@gengjiawen
Copy link
Member

Looks like same on travis, see https://travis-ci.com/nodejs/node/jobs/226154301#L573.

macOS is full compatible with python3 with the patch applied.

@cclauss
Copy link
Contributor Author

cclauss commented Aug 20, 2019

input.py is fixed in #29140 -- You need to pick from these to make progress.

@cclauss
Copy link
Contributor Author

cclauss commented Aug 20, 2019

This is how I do it. Make sure your master is up to date and then:

git checkout -b try_py3                                                          # Pull
git checkout Add_Python_3_tests_to_Travis_CI -- .travis.yml                      #29196
git checkout GN-scraper.decode -- tools/v8_gypfiles/GN-scraper.py                #29208
git checkout py3-gyp_generator_make.py -- tools/gyp/pylib/gyp/generator/make.py  #29214 
git checkout py3-icutrim.py -- tools/icu/icutrim.py                              #29213
git checkout py3_input.py-futurize-ast -- tools/gyp/pylib/gyp/input.py           #29140
git commit -am"Let's try Python 3"
git push --set-upstream origin try_py3

Go to your Travis CI page and watch the tests... Remember that Travis CI is running the Py3 tests in allow_failures mode so read the test logs carefully and don't be fooled by green results the Py3 run.
17 minutes later (async is a reserved word in Python >= 3.7)...

Failed to parse config file: Type names and field names cannot be a keyword: 'async'

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 20, 2019
@gengjiawen
Copy link
Member

Looks like this need to change. cc @cclauss

from cStringIO import StringIO

@Trott
Copy link
Member

Trott commented Aug 21, 2019

Looks like this need to change. cc @cclauss

from cStringIO import StringIO

I'm treating this as a helpful but non-blocking comment. Many things will need to change before this works which is why it's marked as "allowed to fail" for now. See https://github.com/nodejs/node/search?q=is%3Aopen+label%3Apython&type=Issues and in particular #25789, which includes a comment about ninja.py.

@Trott
Copy link
Member

Trott commented Aug 21, 2019

Landed in f70261f

@Trott Trott closed this Aug 21, 2019
Trott pushed a commit that referenced this pull request Aug 21, 2019
These tests are run in allow_failures mode on Python 3.7.1 and they
bypasses the Python version checks in ./configure by directly running
./configure.py.

PR-URL: #29196
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@cclauss
Copy link
Contributor Author

cclauss commented Aug 21, 2019

@gengjiawen @Trott Thanks much for this. I could do mass conversion of the codebase with tools like futurize or modernize but my approach on this codebase is different. I am deliberately taking a fix-as-I-go approach because I sense that many of our dependencies and much of our code is dead code. If once this port is complete, we can identify and strip away some of that dead code which would give us a smaller codebase to port over to JavaScript. So let’s convert code as our test break for a few more weeks so the code paths become crystal clear. For evidence of the dead code, see the scads of excludes in our .flake8 file.

@cclauss cclauss deleted the Add_Python_3_tests_to_Travis_CI branch August 21, 2019 05:14
@cclauss cclauss mentioned this pull request Aug 23, 2019
2 tasks
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
These tests are run in allow_failures mode on Python 3.7.1 and they
bypasses the Python version checks in ./configure by directly running
./configure.py.

PR-URL: #29196
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

7 participants