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: PYTHON ?= python3 #25759

Closed
wants to merge 1 commit into from
Closed

build: PYTHON ?= python3 #25759

wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Jan 28, 2019

Blocked by nodejs/build#1674

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 28, 2019
@gengjiawen
Copy link
Member

There many thing need to fix in tools\test.py like dict.keys.

@thefourtheye
Copy link
Contributor

I don't think we are there yet. If we have to switch to Python 3 here, we need to get the word out and explicitly mention this in our requirements to build.

@cclauss cclauss force-pushed the patch-1 branch 2 times, most recently from 84aafbb to 243eff0 Compare January 28, 2019 13:58
@cclauss
Copy link
Contributor Author

cclauss commented Jan 28, 2019

338 days until Python 2 end of life... Perhaps we need to get the word out. #25766

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I don't think we are there yet. If we have to switch to Python 3 here, we need to get the word out and explicitly mention this in our requirements to build.

Is that an issue, assuming that we still have Python 2 builds in our CI?

I’m wondering whether this should be labelled semver-major, though.

@addaleax addaleax added the python PRs and issues that require attention from people who are familiar with Python. label Jan 28, 2019
@thefourtheye
Copy link
Contributor

Is that an issue, assuming that we still have Python 2 builds in our CI?

As of now, only one of our build machines is using Python 3 (that too kind of experimental, @refack please correct me if I am wrong here), rest of them are still using Python 2 and most of them don't have Python 3.

I’m wondering whether this should be labelled semver-major, though.

I also thought the same. If we do this as part of a major release, people might not miss to notice this important change.

cclauss added a commit to cclauss/node that referenced this pull request Jan 28, 2019
As recommended at nodejs#25759 (comment)
* Python 2.6 end of life statement in 2013: https://www.python.org/dev/peps/pep-0361/#release-lifespan
* Python 2.7 end of life statement in 2019: https://www.python.org/dev/peps/pep-0373/#update
* Python 3.4 reaches it end of life in < 50 days so it should not be a target: https://devguide.python.org/#branchstatus
@thefourtheye
Copy link
Contributor

338 days until Python 2 end of life... Perhaps we need to get the word out.

I understand that we are racing against the clock here, but we wouldn't want to break people's build infrastructure without proper notice. I believe our first step would be to make all the Python modules compatible with Python 3 and then have all our linters use both Python 2 and Python 3. Once we achieve that we can think about totally dropping Python 2 support, I believe.

@gengjiawen
Copy link
Member

Currently the code is not fully python3 compatible yet.

@thefourtheye
Copy link
Contributor

There many thing need to fix in tools\test.py like dict.keys.

@gengjiawen I have a PR now to fix them #25767.

@addaleax
Copy link
Member

Actually, @cclauss, can you update the commit message to start with build: rather than test: makefile:?

@cclauss cclauss changed the title test: makefile: PYTHON ?= python3 build: PYTHON ?= python3 Jan 28, 2019
@refack
Copy link
Contributor

refack commented Jan 28, 2019

FYI, ./configure overrides the default var PYTHON ?= in config.mk, via:

-include config.mk

with

node/configure

Lines 16 to 17 in 4814987

if sys.version_info[0] != 2 or sys.version_info[1] not in (6, 7):
sys.stderr.write('Please use either Python 2.6 or 2.7')

and

node/configure.py

Lines 1658 to 1662 in 4814987

config = {
'BUILDTYPE': 'Debug' if options.debug else 'Release',
'PYTHON': sys.executable,
'NODE_TARGET_TYPE': variables['node_target_type'],
}

So this change is mostly symbolic... (it only affects vanilla repositories, when using a target that does not depend on all or $(NODE_EXE), or when called with python3 configure.py)

@cclauss
Copy link
Contributor Author

cclauss commented Jan 28, 2019

@refack But this change does effect Travis --> Jenkins automated builds because there is no config.mk in this repo and node/configure does not get run. Thus in the automated CI builds we get tools/cpp_lint.py complaining about Python 3 compatibility --> #25760

@refack
Copy link
Contributor

refack commented Jan 28, 2019

But this change does effect Travis --> Jenkins

Yes, but AFAICT only for the lint jobs (they don't try to build, hence don't call configure).
BTW that Travis fail did raise incompatibilities in tools/cpplint.py that I'm looking at now.

@cclauss
Copy link
Contributor Author

cclauss commented Jan 28, 2019

flake8 is passing on both Py2 and Py3 and with #25771, cpp_lint.py should be in the same boat. Are there other linters in the build that rely on Python?

@refack
Copy link
Contributor

refack commented Jan 29, 2019

Are there other linters in the build that rely on Python?

I tested and make lint PYTHON=python3 works as a whole after #25771.

@gengjiawen
Copy link
Member

Also we need to make windows ready for Python3. https://github.com/nodejs/node/blob/master/tools/msvs/find_python.cmd.

cc @refack @cclauss

addaleax pushed a commit that referenced this pull request Jan 30, 2019
As recommended at #25759 (comment)
* Python 2.6 end of life statement in 2013: https://www.python.org/dev/peps/pep-0361/#release-lifespan
* Python 2.7 end of life statement in 2019: https://www.python.org/dev/peps/pep-0373/#update
* Python 3.4 reaches it end of life in < 50 days so it should not be a target: https://devguide.python.org/#branchstatus

PR-URL: #25766
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@refack
Copy link
Contributor

refack commented Feb 1, 2019

DI: https://ci.nodejs.org/job/node-test-pull-request/20509/

@refack
Copy link
Contributor

refack commented Feb 1, 2019

Also we need to make windows ready for Python3. /tools/msvs/find_python.cmd@master.

Refs: #25789
I'll make sure that's done when the tree is ready for python3.

@addaleax
Copy link
Member

addaleax commented Feb 9, 2019

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

@addaleax
Copy link
Member

Looks like this still needs support from the build WG?

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2019
@cclauss
Copy link
Contributor Author

cclauss commented Feb 17, 2019

Blocked by nodejs/build#1674

@richardlau richardlau added the blocked PRs that are blocked by other issues or PRs. label Feb 17, 2019
@cclauss
Copy link
Contributor Author

cclauss commented Jul 5, 2019

Closing in favor of #28537

@cclauss cclauss closed this Jul 5, 2019
@cclauss cclauss deleted the patch-1 branch July 5, 2019 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. 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