build: prefer python 3 over 2 for configure by sam-github · Pull Request #30091 · nodejs/node · GitHub

Change python search order to python3*, then python, then python2*.

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

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

could you do it also for windows? the logic is in https://github.com/nodejs/node/blob/master/tools/msvs/find_python.cmd

See the comment

Lines 6 to 7 in 5a042a6

    
           :: To remove the preference for Python 2, but still support it, just remove 
        
           :: the 5 blocks marked with "Python 2:" and the support warnings

Ok. The 5th block didn't have a clear end, I removed

node/tools/msvs/find_python.cmd

Lines 85 to 97 in 5a042a6

    
           :: Python 2: 
        
           :found-python2 
        
           echo Python 2 found in %p%\python.exe 
        
           set pyver=2 
        
           goto :done 
        
           :found-python 
        
           echo Python found in %p%\python.exe 
        
           echo WARNING: Python 3 is not yet fully supported, to avoid issues Python 2 should be installed. 
        
           set pyver=3 
        
           goto :done 
        
           :done

PTAL @targos

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

If someone can tell me exactly what this needs to be on Windows, I'll add it, but otherwise, I won't have the time for now, and I'll leave it (EDIT: meaning "abandon this PR") for others to pick it up.

cc: @MylesBorins

LGTM -- This is music to my ears!!! The mod to configure looks appropriate to me.

I will let others judge the validity of the Windows changes but my choice would have been to simplify by first trying py on Windows.

Given the various issues with macOS Catalina in Node and in Node-gyp and given the fact that Catalina is not yet testable on Travis or GitHub Actions or Jenkins, I am not in a rush that we land this PR.

@sam-github I pushed a fixup to your branch with the Windows changes, feel free to remove or change as needed. This won't prefer Python 3, but I don't think we need to do that. This uses Python from PATH if it is there, as has always(?) been the Windows behavior.

@cclauss adding a check for py after the PATH and before the registry would be nice (and also in node-gyp!). I might get to it, but since this works I might take a while. Would be happy to review a PR!

Change python search order to python3*, then python, then python2*.

vcbuild now searches for the first python.exe found, and uses it, where it used to look for Python 2 first.

@joaocgreis Thanks for the commit. I detangled it from my fixup, so the first commit is pure configure and yours is pure vcbuild. I made no changes (I hope!) to your code, but I rewrote the commit message to describe how it behaves on Windows, as its different from using configure.

Please take another look to ensure I got the commit description correct.

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

Landed in 11275dc...d1d571e

I think calling this major may be incorrect. If we support both py2 and py3, then it shouldn't matter which is found since they both work.

But, I'm not sure how we get that kind of confidence.

But, on the other hand, this only applies to builds of node from source, so perhaps its not so risky?

I'd like some folks from @nodejs/tsc to weigh in. Can we land this on 13.x?

Its easily revertible if it causes problems, and also probably easily fixable.

In the meantime, I labelled it semver-major so it doesn't get pulled into 13.x and below until consideration to the impact happens.

Change python search order to python3*, then python, then python2*. PR-URL: #30091 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

vcbuild now searches for the first python.exe found, and uses it, where it used to look for Python 2 first. PR-URL: #30091 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

If this is considered a semver major, does that mean it wouldn't go out in a release until after python 2 is EOL?

yes

But to be clear, WITHOUT this change, a build host that has only py3 would use py3. In other words, if someone builds 12.x and 13.x and is disturbed to find it used Py2 to run gyp... they should uninstall Py2.

I should also say: Unless any concerns are raised, I'm personally in favour of landing on 13.x after its been on master a week or so, just to make sure no one pulling master is affected. And if after its been on 13.x for a month or 2, it would be considered stable enough for 12.x by @nodejs/lts (I think).

I would prefer to try it in Node 13 as well due to the Python 2 EOL timing... in a way, that's kind of what the Current branch is for.

OK, 3 votes for 13.x, none against, so I removed semver-major, and added dont-land-on-12.x so it doesn't hit LTS until its baked a bit on 13.x

OK, 3 votes for 13.x, none against, so I removed semver-major, and added dont-land-on-12.x so it doesn't hit LTS until its baked a bit on 13.x

We have a baking-for-lts label for risky things that should wait.

Change python search order to python3*, then python, then python2*. PR-URL: #30091 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

vcbuild now searches for the first python.exe found, and uses it, where it used to look for Python 2 first. PR-URL: #30091 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 23, 2019

cclauss mentioned this pull request Oct 23, 2019

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

Closed

63 tasks

sam-github requested review from MylesBorins and cclauss October 23, 2019 17:04

sam-github requested a review from targos October 23, 2019 17:40

targos reviewed Oct 23, 2019

View reviewed changes

Comment thread tools/msvs/find_python.cmd Outdated

cclauss approved these changes Oct 23, 2019

View reviewed changes

joaocgreis approved these changes Oct 24, 2019

View reviewed changes

gengjiawen approved these changes Oct 24, 2019

View reviewed changes

cjihrig approved these changes Oct 24, 2019

View reviewed changes

sam-github and others added 2 commits October 24, 2019 13:16

build: prefer python 3 over 2 for configure

c0624cf

Change python search order to python3*, then python, then python2*.

build: vcbuild uses default Python, not Py2

5cbec36

vcbuild now searches for the first python.exe found, and uses it, where it used to look for Python 2 first.

sam-github force-pushed the prefer-py3 branch from 0404d76 to 5cbec36 Compare October 24, 2019 20:21

gengjiawen approved these changes Oct 25, 2019

View reviewed changes

sam-github added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 25, 2019

sam-github closed this Oct 25, 2019

sam-github deleted the prefer-py3 branch October 25, 2019 18:39

sam-github added dont-land-on-v12.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 25, 2019

richardlau added the baking-for-lts PRs that need to wait before landing in a LTS release. label Oct 25, 2019

Artoria2e5 mentioned this pull request Oct 28, 2019

Support for Python 3 nexe/nexe#504

Closed

richardlau mentioned this pull request Oct 31, 2019

Build: No module named 'distutils.spawn' #30189

Closed

targos mentioned this pull request Nov 5, 2019

v13.1.0 release proposal #30262

Merged

richardlau mentioned this pull request Dec 21, 2019

Python 3 support in Node.js 12 nodejs/help#2363

Closed

build: prefer python 3 over 2 for configure by sam-github · Pull Request #30091 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: prefer python 3 over 2 for configure - #30091

build: prefer python 3 over 2 for configure#30091sam-github wants to merge 2 commits intonodejs:masternodejs/node:masterfrom sam-github:prefer-py3sam-github/node:prefer-py3Copy head branch name to clipboard

Conversation

sam-github commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Oct 23, 2019

Uh oh!

targos commented Oct 23, 2019

Uh oh!

targos commented Oct 23, 2019

Uh oh!

sam-github commented Oct 23, 2019

Uh oh!

nodejs-github-bot commented Oct 23, 2019

Uh oh!

Uh oh!

sam-github commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cclauss left a comment

Choose a reason for hiding this comment

Uh oh!

joaocgreis commented Oct 24, 2019

Uh oh!

sam-github commented Oct 24, 2019

Uh oh!

nodejs-github-bot commented Oct 24, 2019

Uh oh!

sam-github commented Oct 25, 2019

Uh oh!

sam-github commented Oct 25, 2019

Uh oh!

cjihrig commented Oct 25, 2019

Uh oh!

sam-github commented Oct 25, 2019

Uh oh!

sam-github commented Oct 25, 2019

Uh oh!

cjihrig commented Oct 25, 2019

Uh oh!

sam-github commented Oct 25, 2019

Uh oh!

richardlau commented Oct 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

build: prefer python 3 over 2 for configure#30091
sam-github wants to merge 2 commits into
nodejs:masterfrom
sam-github:prefer-py3

sam-github commented Oct 23, 2019 •
edited
Loading

sam-github commented Oct 23, 2019 •
edited
Loading