build,windows: Implement python detection #13900

refack · 2017-06-24T20:11:43Z

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

refack · 2017-06-24T20:20:03Z

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

refack · 2017-06-24T20:43:30Z

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

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

richardlau · 2017-06-25T12:06:37Z

vcbuild.bat

@@ -513,8 +519,9 @@ rem ***************
 set NODE_VERSION=
 set TAG=
 set FULLVERSION=
-
-for /F "usebackq tokens=*" %%i in (`python "%~dp0tools\getnodeversion.py"`) do set NODE_VERSION=%%i
+:: Call as subrutine for validation of python


refack · 2017-06-27T16:28:24Z

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

tniessen

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.

tniessen · 2017-06-27T18:01:29Z

tools/msvs/find_python.cmd

+:: Helper subroutine to handle quotes in %1
+:find-main-branch
+SET main_key="%~1\Python\PythonCore"
+REG QUERY %main_key% /s | findstr "2." | findstr InstallPath > NUL


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

tniessen · 2017-06-27T18:02:31Z

tools/msvs/find_python.cmd

+)
+EXIT /B 1
+
+:: Check if %p% hold a path to a real python2 executable


hold → holds?

refack · 2017-06-27T20:51:11Z

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 <[email protected]>

refack · 2017-06-28T15:53:02Z

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

joaocgreis · 2017-06-28T17:16:40Z

vcbuild.bat

@@ -550,7 +557,4 @@ if not "%DISTTYPE%"=="custom" (
  set TAG=%DISTTYPE%%DATESTRING%%COMMIT%
 )
 set FULLVERSION=%NODE_VERSION%-%TAG%
-
-:exit
-if not defined DISTTYPEDIR set DISTTYPEDIR=%DISTTYPE%


@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 <[email protected]>

* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

refack · 2017-06-30T20:14:53Z

Land with #13969

* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <[email protected]>

* rename :exit to :distexit PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <[email protected]>

* rename :exit to :distexit PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

MylesBorins · 2017-08-14T20:39:41Z

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 <[email protected]>

* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

refack · 2017-08-15T18:06:53Z

Backport PR #14842

Backport-PR-URL: #14842 PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <[email protected]>

* rename :exit to :distexit Backport-PR-URL: #14842 PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

Backport-PR-URL: #14842 PR-URL: #13900 Fixes: #13882 Reviewed-By: Tobias Nießen <[email protected]>

* rename :exit to :distexit Backport-PR-URL: #14842 PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,windows: Implement python detection #13900

build,windows: Implement python detection #13900

refack commented Jun 24, 2017 •

edited

Loading

refack commented Jun 24, 2017

refack commented Jun 24, 2017

richardlau Jun 25, 2017

refack Jun 25, 2017

refack commented Jun 27, 2017

tniessen left a comment

tniessen Jun 27, 2017

refack Jun 27, 2017

tniessen Jun 27, 2017

refack Jun 27, 2017

refack commented Jun 27, 2017 •

edited

Loading

refack commented Jun 28, 2017 •

edited

Loading

joaocgreis Jun 28, 2017

joaocgreis Jun 28, 2017

refack Jun 28, 2017

refack Jun 28, 2017

refack commented Jun 30, 2017

MylesBorins commented Aug 14, 2017

refack commented Aug 15, 2017

build,windows: Implement python detection #13900

build,windows: Implement python detection #13900

Conversation

refack commented Jun 24, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Jun 24, 2017

refack commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 27, 2017

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 27, 2017 • edited Loading

refack commented Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 30, 2017

MylesBorins commented Aug 14, 2017

refack commented Aug 15, 2017

refack commented Jun 24, 2017 •

edited

Loading

refack commented Jun 27, 2017 •

edited

Loading

refack commented Jun 28, 2017 •

edited

Loading