build: prefer python 3 over 2 for configure #30091

sam-github · 2019-10-23T17:03:54Z

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

nodejs-github-bot · 2019-10-23T17:05:54Z

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

targos · 2019-10-23T17:07:21Z

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

targos · 2019-10-23T17:08:06Z

See the comment

node/tools/msvs/find_python.cmd

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

sam-github · 2019-10-23T17:38:47Z

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

nodejs-github-bot · 2019-10-23T17:39:54Z

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

tools/msvs/find_python.cmd

sam-github · 2019-10-23T19:28:17Z

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

cclauss

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.

joaocgreis · 2019-10-24T01:23:55Z

@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.

sam-github · 2019-10-24T20:24:36Z

@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.

nodejs-github-bot · 2019-10-24T20:25:08Z

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

sam-github · 2019-10-25T18:35:20Z

Landed in 11275dc...d1d571e

sam-github · 2019-10-25T18:39:30Z

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 <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

cjihrig · 2019-10-25T18:43:00Z

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

sam-github · 2019-10-25T18:44:13Z

yes

sam-github · 2019-10-25T18:48:45Z

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).

cjihrig · 2019-10-25T18:54:07Z

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.

sam-github · 2019-10-25T20:39:04Z

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

richardlau · 2019-10-25T20:45:28Z

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 <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 cclauss and MylesBorins 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

tools/msvs/find_python.cmd Show resolved Hide resolved

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

targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: prefer python 3 over 2 for configure #30091

build: prefer python 3 over 2 for configure #30091

sam-github commented Oct 23, 2019 •

edited

Loading

nodejs-github-bot commented Oct 23, 2019

targos commented Oct 23, 2019

targos commented Oct 23, 2019

sam-github commented Oct 23, 2019

nodejs-github-bot commented Oct 23, 2019

sam-github commented Oct 23, 2019 •

edited

Loading

cclauss left a comment

joaocgreis commented Oct 24, 2019

sam-github commented Oct 24, 2019

nodejs-github-bot commented Oct 24, 2019

sam-github commented Oct 25, 2019

sam-github commented Oct 25, 2019

cjihrig commented Oct 25, 2019

sam-github commented Oct 25, 2019

sam-github commented Oct 25, 2019

cjihrig commented Oct 25, 2019

sam-github commented Oct 25, 2019

richardlau commented Oct 25, 2019

build: prefer python 3 over 2 for configure #30091

build: prefer python 3 over 2 for configure #30091

Conversation

sam-github commented Oct 23, 2019 • edited Loading

Checklist

nodejs-github-bot commented Oct 23, 2019

targos commented Oct 23, 2019

targos commented Oct 23, 2019

sam-github commented Oct 23, 2019

nodejs-github-bot commented Oct 23, 2019

sam-github commented Oct 23, 2019 • edited Loading

cclauss left a comment

Choose a reason for hiding this comment

joaocgreis commented Oct 24, 2019

sam-github commented Oct 24, 2019

nodejs-github-bot commented Oct 24, 2019

sam-github commented Oct 25, 2019

sam-github commented Oct 25, 2019

cjihrig commented Oct 25, 2019

sam-github commented Oct 25, 2019

sam-github commented Oct 25, 2019

cjihrig commented Oct 25, 2019

sam-github commented Oct 25, 2019

richardlau commented Oct 25, 2019

sam-github commented Oct 23, 2019 •

edited

Loading

sam-github commented Oct 23, 2019 •

edited

Loading