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: ongoing list of actions for Python 3 compatibility #25789

Closed
57 of 63 tasks
cclauss opened this issue Jan 29, 2019 · 39 comments · Fixed by #36691
Closed
57 of 63 tasks

build: ongoing list of actions for Python 3 compatibility #25789

cclauss opened this issue Jan 29, 2019 · 39 comments · Fixed by #36691
Assignees
Labels
build Issues and PRs related to build files or the CI. gyp Issues and PRs related to the GYP tool and .gyp build files help wanted Issues that need assistance from volunteers or PRs that need help to proceed. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.

Comments

@cclauss
Copy link
Contributor

cclauss commented Jan 29, 2019

We have several threads in issues and pull requests related to Python 3 compatibility. We need a task list to keep in sync and to show our progress

QA:: Our CI machines are mix of machines that have Python 2, have Python 3, have both, have a python symlink, have it not, etc. By preferring Python 3 in configure/make/test some set of our current machines will start to test against Python 3, but many will continue to test against Python 2. The more modern machines will use 3, as will our users running more up-to-date operating systems in the next year.

Upstream V8 issues:

Neither Chromium or depot_tools are Python 3 compatible yet. This doesn't effect the main Node.js builds, but does affect our V8 integration builds, where v8 is built and tested.

Miscellaneous non-blocking cleanups

Please use the "python" label!

@ChALkeR ChALkeR added the build Issues and PRs related to build files or the CI. label Jan 29, 2019
@refack refack added python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. gyp Issues and PRs related to the GYP tool and .gyp build files labels Jan 29, 2019
@thefourtheye
Copy link
Contributor

@cclauss This is one neatly compiled list. Thanks for doing that 👍 I am just wondering if we can start dealing with GYP's Python 3 compatibility.

@targos
Copy link
Member

targos commented Jan 30, 2019

GYP got a few patches for Python 3 compatibility upstream: https://chromium.googlesource.com/external/gyp/+log

@thefourtheye
Copy link
Contributor

Oh, that's great. @refack and @cclauss have been working on @refack's fork of GYP at https://github.com/refack/GYP in this regard. Perhaps we can consolidate our efforts.

@gengjiawen
Copy link
Member

I will like to talk ci involved this. I know we are current introduce docker in node ci system.

We can introduce a docker image only with python3 to test our code.

@gengjiawen
Copy link
Member

gengjiawen commented Jan 30, 2019

One more thing, not sure this is the right place. We may need to pre-install some python3 package.
Package utils not include in python3.

import utils

@cclauss
Copy link
Contributor Author

cclauss commented Jan 30, 2019

node/tools/utils.py the file in question. Perhaps we need to start adding from __future__ import absolute_import.

https://stackoverflow.com/questions/33743880/what-does-from-future-import-absolute-import-actually-do

@refack refack mentioned this issue Feb 1, 2019
4 tasks
@gengjiawen
Copy link
Member

refack pushed a commit to cclauss/node that referenced this issue Mar 26, 2019
PR-URL: nodejs#26424
Refs: nodejs#25789 (comment)
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit to targos/node that referenced this issue Mar 27, 2019
PR-URL: nodejs#26424
Refs: nodejs#25789 (comment)
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this issue Mar 27, 2019
PR-URL: #26424
Refs: #25789 (comment)
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@sam-github
Copy link
Contributor

@cclauss I'm not sure how to automate a check for this. There are a huge number of machines.

One approach is this: https://gist.github.com/a2a2896f10c9f3e0814af1f60fc32d38

Its pretty ugly! Is it useful? Isn't there a pip version command I should run, too?

Also, its not clear what hosts we even care about.

It might be easier to try this:

  1. get a python3 build that works localy
  2. figure out what jobs need to be changed to support python2, or is it just a PR to nodejs/node that will switch us to using python3? I don't honestly know what the plan is here.
  3. clone jenkins jobs to make a python3 version
  4. run them.... observer what failed due to missing python3
  5. install python3 on those hosts

FTR: Generated using grep '^Host ' ~/.ssh/config | sed -e 's/^Host //' > all (my ssh config was generated by nodejs/build tooling and contains setup for all (?) of inventory.yml), then doing parallel-ssh -i -h all 'which python2 && python2 --version; which python3 && python3 --version' > ../python.txt and gist < ../python.txt.

@cclauss
Copy link
Contributor Author

cclauss commented Aug 14, 2019

On each machine, I would just do:
$ python2 -m pip --version || true # This should give you the versions of both Python 2 and its pip
$ python3 -m pip --version || true # This should give you the versions of both Python 3 and its pip

Do we really need to hardcode paths to the executable?

My sense is that you can just $ PYTHON=python3 make lint # or other Makefile command.

@sam-github
Copy link
Contributor

Do we really need to hardcode paths to the executable?

Are you wondering why I ran which? Its helpful to me to know where the exe is, it gives a strong hint as to how it was installed. pkg managers will have it in /usr/bin, src builds with no special options will put it in /usr/local, anything else is pretty custom (perhaps excessively customized).

My goal on the machines I updated was that python2 and python3 be in the path. Its not clear to me what will happen when ci does git clone node; cd node; ./configure && make test (what it actually does is unfortunately more complex, but that is what it does conceptually). Will it choose python3 if found, python2 if not, and error out if neither exist?

@sam-github
Copy link
Contributor

I can't ssh into most of the machines, but of the ones I can, here are the python and pip versions: https://gist.github.com/sam-github/d9b8c40fc6001e3e75288da12e041737

@mhdawson mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Oct 23, 2019
@sam-github
Copy link
Contributor

sam-github commented Oct 23, 2019

@rvagg You can do make PYTHON=python3, but are you doing things backwards, calling make so that Makefile can call ./configure? If you do the more traditional ./configure; make then you give configure a chance to configure your build. I damaged my system to remove python, python2, and python2.7 and can repro your error, but if I do ./configure; make then the config.mk is written correctly, and it all works:

% rm -f config.mk config.status; ./configure && cat config.mk && make build-ci -n | head -n 10                                                  
Node configure: Found Python 3.7.3...

The config.mk records the python found by configure:

# Do not edit. Generated by the configure script.
...
PYTHON=/usr/bin/python3.7
...

The Makefile does an -include config.mk and picks up the right python version, and uses it:

/usr/bin/python3.7 ./configure --verbose
make
....

So I think ./configure; make is more robust.... I guess that's not how the builds work now, though?

@rvagg
Copy link
Member

rvagg commented Oct 23, 2019

Yeah, you're right @sam-github, our CI setup is artificial in the ordering it uses, but it's also intended to give us more control here over how it's run rather than embedding all of that into Jenkins.

I'm working on introducing more flexibility here for this, see https://github.com/nodejs/node/pull/30057/files#diff-da22d1a6aad00d641b775f4d963249b7R79 for this particular instance where I'm forcing PYTHON=python3 to deal with only having python3 on the container. I could expand this to introduce other passes through ./configure with different variations of Python versions existing or not or different PYTHON. I'm not really sure what we're preparing for in the real world wrt Python. It seems to me that python = Python 2 isn't going away any time soon even with Python 2 EOL. Is there a time in the future that python is ever going to be Python 3 or has everyone agreed that it's got to be python3 now and forever?

@rvagg
Copy link
Member

rvagg commented Oct 23, 2019

This python3-only machine is actually pretty interesting. Compile is fine and it returns green for the tests, but I just happened to look in the logs and it's full of this kind of thing, apparently not bubbling up as a non-zero exit code:

09:01:30 --- Logging error ---
09:01:30 Traceback (most recent call last):
09:01:30   File "/usr/lib64/python3.6/logging/__init__.py", line 996, in emit
09:01:30     stream.write(msg)
09:01:30 TypeError: a bytes-like object is required, not 'str'
09:01:30 Call stack:
09:01:30   File "tools/test.py", line 1709, in <module>
09:01:30     sys.exit(Main())
09:01:30   File "tools/test.py", line 1685, in Main
09:01:30     if RunTestCases(cases_to_run, options.progress, options.j, options.flaky_tests):
09:01:30   File "tools/test.py", line 908, in RunTestCases
09:01:30     return progress.Run(tasks)
09:01:30   File "tools/test.py", line 121, in Run
09:01:30     self.RunSingle(False, 0)
09:01:30   File "tools/test.py", line 182, in RunSingle
09:01:30     self.HasRun(output)
09:01:30   File "tools/test.py", line 347, in HasRun
09:01:30     (total_seconds, duration.microseconds / 1000))
09:01:30 Message: '  duration_ms: 0.213'
09:01:30 Arguments: ()

see https://ci.nodejs.org/job/rv-test-commit-linux-docker/43/label=docker-host-x64,linux_x64_container_suite=centos8/console for more

@sam-github
Copy link
Contributor

I think your container is realistic: it has python3 as Python 3, and doesn't have python or any Python 2 by any name, and there is a PEP that recommends that. IIUC that's what is going to happen when the new OS updates start coming out after Python is EOL.

Maybe the Makefile can be made to default to python3 if it exists, and python2 if it doesn't? But this fallback logic already exists, in configure... It looks like the Makefile is trying to work on systems where ./configure wouldn't work (Windows), by using python on it explicitly? Anyhow, above is more a description of how it works for users. I didn't even realize the python found was passed in a config.mk until I looked into it today! I'm glad the ./configure; make path is paved for them, if CI is doing make --> $(PYTHON) configure then it'll have to work something out. Since you know what's in the docker images, setting the env var seems pretty reasonable.

@sam-github
Copy link
Contributor

Good find on the py3 problems above, it really speaks to the usefulness of being able to have more containers with specific setups.

@targos
Copy link
Member

targos commented Oct 28, 2019

@sam-github @cclauss The switch to preferring Python 3 seems to have broken our V8 CI on centos7-ppcle.

See https://ci.nodejs.org/job/node-test-commit-v8-linux/2590/nodes=centos7-ppcle,v8test=v8test/console

07:58:31 + ./configure
07:58:32 Node configure: Found Python 3.7.3...
07:58:32 Traceback (most recent call last):
07:58:32   File "./configure", line 25, in <module>
07:58:32     import configure
07:58:32   File "/home/iojs/build/workspace/node-test-commit-v8-linux/nodes/centos7-ppcle/v8test/v8test/configure.py", line 14, in <module>
07:58:32     import bz2
07:58:32   File "/usr/local/lib/python3.7/bz2.py", line 19, in <module>
07:58:32     from _bz2 import BZ2Compressor, BZ2Decompressor
07:58:32 ModuleNotFoundError: No module named '_bz2'

@cclauss
Copy link
Contributor Author

cclauss commented Oct 28, 2019

@gengjiawen
Copy link
Member

This one related: nodejs/node-gyp#1947

# ./configure --debug -C

Node configure: Found Python 3.7.5...
INFO: Using floating patch "tools/icu/patches/64/source/common/putil.cpp" from "tools/icu"
INFO: Using floating patch "tools/icu/patches/64/source/i18n/dtptngen.cpp" from "tools/icu"
Traceback (most recent call last):
  File "./configure", line 25, in <module>
    import configure
  File "/root/node/configure.py", line 1755, in <module>
    run_gyp(gyp_args)
  File "tools/gyp_node.py", line 54, in run_gyp
    rc = gyp.main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 547, in main
    return gyp_main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 532, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File "tools/gyp/pylib/gyp/generator/compile_commands_json.py", line 95, in GenerateOutput
    for qualified_target, target in target_dicts.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'

@sam-github
Copy link
Contributor

@cclauss I haven't heard of anyone having troubles with Python 3, have you, or anyone else?

Are there next steps we can take now?

@dylanaraps
Copy link

I've been building node with Python 3 for a while however the following workaround is needed for Python >= 3.8. It's a simple "hack" but it enables node to build on Python 3.8.

It seems to be limited to Python < 3.5(?). Not sure if this is true for Master or not.

sed -i 's/(3, 5)/(3, 8)/' configure

@sam-github
Copy link
Contributor

@dylanaraps with what node.js version do you do that?

master:

node/configure

Line 24 in 20fd123

acceptable_pythons = ((3, 8), (3, 7), (3, 6), (3, 5), (2, 7))

@dylanaraps
Copy link

Apologies, I see that this is fixed in the latest version. Disregard my prior comment. That being said, Python 3 support works flawlessly here. 👍

@sam-github
Copy link
Contributor

sam-github commented Jan 6, 2020

Talked to @cclauss, we came up with a plan. We'll open new issues for this, but for the moment, here's the notes.

Reminder:

1. Update npm to node-gyp@6.x on the next npm major, npm@7

This needs coordination with the npm team, @nodejs/npm

node-gyp@6 "prefers" py3 over py2 (node-gyp@5, what npm@6 uses, uses py3 only if py2 is not found). Changing the default is considered semver-major, which might be excessively paranoid, but nobody wants to break the ecosystem.

ATM, note that node 10.x and greater all have npm@6, which means that right now py3-only systems should build node.js addons with py3 on all currently supported node.js release lines.

2. node.js 12.x must support being built with py3

node@12.x will outlive py2 EOL by a long time, it must get non-experimental support for building with py3

We will identify the commits/PRs that need backporting.

3. node.js 10.x should support being built with py3

nodejs@10 outlives py2 EOL, so should be buildable with py3. Whether we can achieve that will take some analysis, maybe its easy, maybe its drifted too far from 12.x. If its way too hard/destabilizing, we might need to make an exception for it to our "unsupported platform" policy.

Note that lack of support for py3 means that https://github.com/felixrieseberg/windows-build-tools cannot yet install py3 instead of py2 - if it did that, users wouldn't be able to build node.js 10.x on Windows with using it. I mention, because its one example of the kind of downstream pain caused by not having py3 support. It might become a problem on non-Windows platforms, too, as py2 becomes less commonly packaged, or perhaps not packaged at all.

/cc @nodejs/tsc @nodejs/python @nodejs/node-gyp

@gengjiawen
Copy link
Member

gengjiawen commented Jan 29, 2020

#31562 after this patch.

python3 configure.py --debug --ninja failed with:

Traceback (most recent call last):
  File "configure.py", line 1770, in <module>
    run_gyp(gyp_args)
  File "tools/gyp_node.py", line 54, in run_gyp
    rc = gyp.main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 547, in main
    return gyp_main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 532, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 2426, in GenerateOutput
    config_name)
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 2317, in GenerateOutputForConfig
    hash_for_rules = hashlib.md5(qualified_target_for_hash).hexdigest()
TypeError: Unicode-objects must be encoded before hashing

@cclauss
Copy link
Contributor Author

cclauss commented Jan 29, 2020

On master this code is at line 2385 and the proper encoding is done on the line above.

Please see: https://github.com/nodejs/node/search?q=qualified_target_for_hash

@gengjiawen
Copy link
Member

gengjiawen commented Feb 29, 2020

Thanks to @targos for #30563, many compatibility issue should be resolved (like ninja issue).

@cclauss
Copy link
Contributor Author

cclauss commented Feb 29, 2020

Closing — Thanks to everyone who helped to make Node.js on Python 3 happen!

@cclauss cclauss closed this as completed Feb 29, 2020
richardlau pushed a commit to cclauss/node that referenced this issue Apr 16, 2021
PR-URL: nodejs#36691
Fixes: nodejs#25789
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
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. gyp Issues and PRs related to the GYP tool and .gyp build files help wanted Issues that need assistance from volunteers or PRs that need help to proceed. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging a pull request may close this issue.