build,windows: Implement python detection by refack · Pull Request #13900 · nodejs/node · GitHub

According to PEP 514

Fixes: #13882

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build,tools,windows

CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/9990/

New CI: https://ci.nodejs.org/job/node-compile-windows/10007/

/cc @nodejs/build @nodejs/platform-windows

subroutine

Ack.

ping @nodejs/build @nodejs/platform-windows @nodejs/python @benjamingr @tniessen
Anyone wants to review

LGTM with the suggested changes. I think we should just deploy it and wait for people to complain about build problems, it is the only way to find out if there are any incompatibilities. It won't make things worse than they are as python will still be used if it is in the PATH.

This prints error messages to my console before finding the correct key... Maybe add 2>&1?

Ack.

hold → holds?

Ack.

LGTM with the suggested changes. I think we should just deploy it and wait for people to complain about build problems, it is the only way to find out if there are any incompatibilities. It won't make things worse than they are as python will still be used if it is in the PATH.

Just an FYI, there are quite a few downstream builders, all sort of projects that build their own node.exe. Just one (not favorable) example — http://blog.sec-consult.com/2017/04/application-whitelisting-application.html
I agree this should only make things better, since first thing we look in the path (the current status quo)

PR-URL: nodejs#13900 Fixes: nodejs#13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Windows sanity: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10045/ ✔️

@refack I was not able to do a careful review of this PR, but this line caught my eye. Is there a good reason for completely removing this? I believe this will completely break or release process, if DISTTYPEDIR is not set.

Confirmed this breaks. Will try a fix in the build script instead.

Woops. Opening a reversion PR.

So I removed it because of :exit is redefined from L511
Fix PR #13969

PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Land with #13969

* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

* rename :exit to :distexit PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

* rename :exit to :distexit PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Please include all patches needed to make this atomic change work

PR-URL: nodejs#13900 Fixes: nodejs#13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Backport PR #14842

Backport-PR-URL: #14842 PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

* rename :exit to :distexit Backport-PR-URL: #14842 PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Backport-PR-URL: #14842 PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

* rename :exit to :distexit Backport-PR-URL: #14842 PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

nodejs-github-bot added build Issues and PRs related to build files or the CI. install Issues and PRs related to the installers. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Jun 24, 2017

refack self-assigned this Jun 24, 2017

refack force-pushed the vcbuild-detect-python branch from 0a2c33e to 50bebc7 Compare June 24, 2017 20:42

richardlau reviewed Jun 25, 2017

View reviewed changes

refack force-pushed the vcbuild-detect-python branch from a3cbff7 to 5e8e829 Compare June 25, 2017 12:45

tniessen approved these changes Jun 27, 2017

View reviewed changes

build,windows: implement PEP514 python detection

614dbbd

PR-URL: nodejs#13900 Fixes: nodejs#13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

refack force-pushed the vcbuild-detect-python branch from 606d52b to 614dbbd Compare June 28, 2017 15:49

refack merged commit 614dbbd into nodejs:master Jun 28, 2017

refack mentioned this pull request Jun 28, 2017

doc: add note about python installer #13881

Closed

2 tasks

refack deleted the vcbuild-detect-python branch June 28, 2017 15:51

joaocgreis reviewed Jun 28, 2017

View reviewed changes

refack mentioned this pull request Jun 28, 2017

build,windows: fix vcbuild.bat #13969

Merged

2 tasks

joaocgreis mentioned this pull request Jun 28, 2017

Moving DISTTYPEDIR to CI-Release script for Windows nodejs/build#776

Closed

addaleax pushed a commit that referenced this pull request Jun 29, 2017

build,windows: implement PEP514 python detection

00a0b39

PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax mentioned this pull request Jun 29, 2017

v8.2.0 proposal #13744

Merged

addaleax pushed a commit that referenced this pull request Jul 11, 2017

build,windows: implement PEP514 python detection

49d542d

PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax pushed a commit that referenced this pull request Jul 18, 2017

build,windows: implement PEP514 python detection

949f7be

PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins added the backport-requested-v6.x label Aug 14, 2017

MylesBorins mentioned this pull request Aug 14, 2017

backport updates to windows build script to v6.x nodejs/Release#238

Closed

3 tasks

refack added a commit to refack/node that referenced this pull request Aug 15, 2017

build,windows: implement PEP514 python detection

5663961

PR-URL: nodejs#13900 Fixes: nodejs#13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

refack mentioned this pull request Aug 15, 2017

[v6.x backport] build,windows: implement PEP514 python detection #14842

Closed

2 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Aug 16, 2017

MylesBorins pushed a commit that referenced this pull request Aug 16, 2017

build,windows: implement PEP514 python detection

162bc81

Backport-PR-URL: #14842 PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

MylesBorins pushed a commit that referenced this pull request Sep 5, 2017

build,windows: implement PEP514 python detection

2a1a93d

Backport-PR-URL: #14842 PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

bzoz mentioned this pull request Jan 31, 2018

build,win: replace find-python subroutine with PYTHON variable #17804

Closed

1 task

refack mentioned this pull request Jun 10, 2018

Sub teams for build? nodejs/build#1331

Closed

refack removed their assignment Oct 20, 2018

               set FULLVERSION=%NODE_VERSION%-%TAG%
-              :exit
-              if not defined DISTTYPEDIR set DISTTYPEDIR=%DISTTYPE%

build,windows: Implement python detection by refack · Pull Request #13900 · nodejs/node · GitHub

Uh oh!

Uh oh!

build,windows: Implement python detection - #13900

build,windows: Implement python detection#13900refack merged 1 commit intonodejs:masternodejs/node:masterfrom refack:vcbuild-detect-pythonrefack/node:vcbuild-detect-pythonCopy head branch name to clipboard

Conversation

refack commented Jun 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Jun 24, 2017

Uh oh!

refack commented Jun 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 27, 2017

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 30, 2017

Uh oh!

MylesBorins commented Aug 14, 2017

Uh oh!

refack commented Aug 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

build,windows: Implement python detection#13900
refack merged 1 commit into
nodejs:masterfrom
refack:vcbuild-detect-python

refack commented Jun 24, 2017 •
edited
Loading

refack commented Jun 27, 2017 •
edited
Loading

refack commented Jun 28, 2017 •
edited
Loading