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.

Sign up for GitHub

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

Jump to bottom

build: find Python 3 or Python 2 in configure #25878

Closed
wants to merge 1 commit into from
Closed

build: find Python 3 or Python 2 in configure #25878

wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Feb 1, 2019 •
edited
Loading

Blocked by #28537, nodejs/build#1399, and/or 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 Feb 1, 2019
@cclauss cclauss force-pushed the find-python3-in-configure branch 2 times, most recently from 996d6df to 613bacd Compare February 1, 2019 20:00
richardlau
richardlau reviewed Feb 1, 2019
View reviewed changes
configure Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
@cclauss cclauss force-pushed the find-python3-in-configure branch 2 times, most recently from 2b5b7ac to e2b02a5 Compare February 1, 2019 21:29
This was referenced Feb 1, 2019
Closed
Closed
richardlau
richardlau reviewed Feb 1, 2019
View reviewed changes
configure Show resolved Hide resolved
lpinca
lpinca reviewed Feb 2, 2019
View reviewed changes
configure Outdated Show resolved Hide resolved
@addaleax addaleax added the python PRs and issues that require attention from people who are familiar with Python. label Feb 6, 2019
@lpinca lpinca added the blocked PRs that are blocked by other issues or PRs. label Feb 9, 2019
@lpinca
Copy link
Member

lpinca commented Feb 9, 2019

Added blocked label as per original description.

thefourtheye
thefourtheye suggested changes Feb 19, 2019
View reviewed changes
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

Apart from the reasons mentioned in the original description, all the dependencies should be Python 3 compatible for this to work. Blocking this PR by requesting changes (I am okay with the changes proposed actually) till we get there.

@BridgeAR
Copy link
Member

Ping @cclauss

@cclauss
Copy link
Contributor Author

cclauss commented Mar 10, 2019

@BridgeAR What do you want me to do?

@BridgeAR
Copy link
Member

I should have read the reasons why there was a -1. Sorry for the ping.

@richardlau richardlau mentioned this pull request Apr 10, 2019
Closed
2 tasks
cclauss added a commit to cclauss/node that referenced this pull request Apr 24, 2019
@cclauss
Remove the unsupported Python 2.6 from configure
45e3931 
A precursor to nodejs#25878
@cclauss cclauss mentioned this pull request Apr 24, 2019
Closed
4 tasks
@cclauss cclauss force-pushed the find-python3-in-configure branch from c0e2c18 to d4d7354 Compare July 4, 2019 07:03
@cclauss cclauss mentioned this pull request Jul 4, 2019
Closed
4 tasks
@cclauss cclauss force-pushed the find-python3-in-configure branch 2 times, most recently from 871393b to c380b43 Compare July 5, 2019 09:55
@cclauss
Copy link
Contributor Author

cclauss commented Jul 5, 2019 •
edited
Loading

Some experimentation here says that we will face an issue with Python 3.7 because async is a reserved word in Python >= 3.7 #29326 so using that word in the wrong context is a Python syntax error. So for initial experimentation, I plan to start with Python 3.6.7.

@sam-github
Copy link
Contributor

I don't think this is blocked by #25878, its blocked by using a python version that won't actually work for configure, isn't it?

The steps to python3 are
1a - node can be built, tested, etc using python2 if available, and python3 if it isn't or if python3 is deliberately requested. This can likely be done in a single PR, maybe #28537 is better for that? This step is backwards compatible, and allow local development and testing with python3.
1b - python3 needs to exist on all build infrastructure

--- these can be done in parallel, they don't block each other

2 - convert configure to use python3 when available, falling back on python2 if not. This is probably backwards compatible, and will cause the python3 installed in 1b to start to get used, so we can see how well it works across all our platforms.

3 - remove python2 support (after a while, possibly in the next major release post-python2.7 EOL)

@cclauss cclauss mentioned this pull request Jul 6, 2019
Closed
4 tasks
@cclauss cclauss requested a review from joaocgreis July 11, 2019 05:16
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25293/

@cclauss
Copy link
Contributor Author

cclauss commented Aug 29, 2019

Removing the blocked label based on #25878 (comment) and the lack of a response to #25878 (comment)

@cclauss cclauss removed the blocked PRs that are blocked by other issues or PRs. label Aug 29, 2019
@ryzokuken
Copy link
Contributor

./configure --ninja still won't work on python3. Trying to make a patch soon.

@cclauss
Copy link
Contributor Author

cclauss commented Sep 3, 2019

#29414 allow legacy Python to continue to use cStringIO.StringIO

sam-github
sam-github suggested changes Sep 4, 2019
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

I'd like the comment about Travis to be clear to people who read it in the future, but other than that, I think this is ready to land.

configure
_=[ 'exec' '/bin/sh' '-c' '''
test ${TRAVIS} && exec python "$0" "$@" # workaround for pyenv on Travis CI
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this comment be more informative? I think it should be

# On Travis, pyenv arranges for the requested python version to be available as
# plain "python". In other environments, prefer the version specific executable names.

But I might misunderstand its purpose.

@sam-github
Copy link
Contributor

@thefourtheye I think your requested change was done, can you please re-review?

@sam-github
Copy link
Contributor

@thefourtheye Sorry, I found your requested change:

Apart from the reasons mentioned in the original description, all the dependencies should be Python 3 compatible for this to work.

I don't agree that this is a pre-req for this to land on master. Its blocked from 12.x, but on master it does no harm, it just changes a "no supported python 2.7 found" error to (possibly) an python3 incompatibility error. It causes no harm (or change) to a system that already would have built.

sam-github
sam-github approved these changes Sep 5, 2019
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

I rebased, squashed, etc.

@sam-github sam-github mentioned this pull request Sep 5, 2019
Closed
3 tasks
@sam-github
Copy link
Contributor

@thefourtheye , please re-review

@cclauss
Copy link
Contributor Author

cclauss commented Sep 9, 2019

Is something blocking us from landing this one?

@sam-github
Copy link
Contributor

@thefourtheye has a blocking review, but doesn't seem to be resonding to request to relook. @nodejs/tsc, is 5 days with no response enough to dismiss a review, or should we wait more? What is the convention here?

@Trott
Copy link
Member

Trott commented Sep 10, 2019

@thefourtheye has a blocking review, but doesn't seem to be resonding to request to relook. @nodejs/tsc, is 5 days with no response enough to dismiss a review, or should we wait more? What is the convention here?

@sam-github There's no set rule, but 5 days of non-responsiveness strikes me as a reasonable amount of time to wait before dismissing the request-for-changes.

@Trott
Copy link
Member

Trott commented Sep 10, 2019

@thefourtheye has a blocking review, but doesn't seem to be resonding to request to relook. @nodejs/tsc, is 5 days with no response enough to dismiss a review, or should we wait more? What is the convention here?

@sam-github There's no set rule, but 5 days of non-responsiveness strikes me as a reasonable amount of time to wait before dismissing the request-for-changes.

Not necessary, but it may be a reasonable courtesy to also email the Collaborator who hasn't responded and then wait, say, 48 hours. Sometimes people just don't see stuff because of overwhelming GitHub notifications.

@Trott
Copy link
Member

Trott commented Sep 10, 2019

@thefourtheye has a blocking review, but doesn't seem to be resonding to request to relook. @nodejs/tsc, is 5 days with no response enough to dismiss a review, or should we wait more? What is the convention here?

@sam-github There's no set rule, but 5 days of non-responsiveness strikes me as a reasonable amount of time to wait before dismissing the request-for-changes.

Not necessary, but it may be a reasonable courtesy to also email the Collaborator who hasn't responded and then wait, say, 48 hours. Sometimes people just don't see stuff because of overwhelming GitHub notifications.

I just emailed @thefourtheye now.

@thefourtheye
Copy link
Contributor

Sorry, this was blocked because of me. I just re-reviewd now and LGTM.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25416/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25418/

@cclauss cclauss mentioned this pull request Sep 11, 2019
Closed
@Trott
Copy link
Member

Trott commented Sep 11, 2019

Landed in be926c7

@Trott Trott closed this Sep 11, 2019
Trott pushed a commit that referenced this pull request Sep 11, 2019
@cclauss @Trott
build: find Python 3 or Python 2 in configure
be926c7 
PR-URL: #25878
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 21, 2019
Merged
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
@cclauss @richardlau
build: find Python 3 or Python 2 in configure
a5774d8 
PR-URL: nodejs#25878
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@sam-github sam-github sam-github approved these changes

@joaocgreis joaocgreis Awaiting requested review from joaocgreis

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.