build: better error message on python fail by refack · Pull Request #17298 · nodejs/node

refack · 2017-11-25T19:21:28Z

On a machine without python installed:

d:\code\node$ vcbuild
Could not find python2. More information can be found at
https://github.com/nodejs/node/blob/master/BUILDING.md#windows-1

Checklist

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

Affected core subsystem(s)

build,windows,tools

refack · 2017-11-25T19:22:25Z

I suggest this as an alternative to #17293

refack · 2017-11-25T19:34:38Z

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

refack · 2017-11-25T19:35:31Z

/CC @nodejs/build @nodejs/platform-windows

seishun · 2017-11-25T20:02:27Z

This still has the issues I mentioned in #17015 (comment). Namely:

run-python is used inconsistently. Most of vcbuild.bat uses :run-python as a replacement for python, but one subroutine uses the VCBUILD_PYTHON_LOCATION variable which it assumes is populated.
To make sure the VCBUILD_PYTHON_LOCATION variable is populated (and I guess also to get early exit?), :run-python is called once in the beginning. Now whenever significant changes are introduced into vcbuild.bat, one has to be careful to make sure it's called before the first usage of Python. In fact, this PR already makes a mistake by assuming linting doesn't require Python although it actually does.

refack · 2017-11-25T20:16:28Z

(2) bug fixed, and made the "validation" section explicit.

What I suggest is that we are gaining more then we're losing. Using run-python gives mostly consistent way to find and call python, and a lower threshold for occasional builders, and building on a new system.
Now that getnodeversion has been moved next to it's usage, it has become an edge case that is run only when building an MSI or doing a release.

seishun · 2017-11-25T20:20:07Z

vcbuild.bat

 if "%target%"=="Clean" rmdir /S /Q %~dp0deps\icu
 :no-depsicu

-call :getnodeversion || exit /b 1


The next line uses FULLVERSION which is populated in getnodeversion.

Fixed by making the rmdir line more generic.

%TAG% is used in project-gen too.

refack · 2017-11-25T20:22:35Z

I'll continue working on refactoring the MSI & release bits to a separate batch file, that way we can wrap python, and other dependency resolution in a consistent way, and handle that section differently since is not used by occasional builders.

bzoz · 2017-11-28T08:33:11Z

vcbuild.bat

@@ -354,6 +363,7 @@ if not defined msi goto run

 :msibuild
 echo Building node-v%FULLVERSION%-%target_arch%.msi


Isn't %FULLVERSION% set by getnodeversion?

Also in :package

BridgeAR · 2018-01-31T02:12:07Z

Ping @refack

BridgeAR · 2018-02-07T03:38:48Z

Closing due to long inactivity. @refack please feel free to reopen if you would like to continue working on this!

build: better error message on python fail

6a10eb5

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 Nov 25, 2017

refack requested review from gibfahn and seishun November 25, 2017 19:21

refack mentioned this pull request Nov 25, 2017

Revert "build,windows: implement PEP514 python detection" #17293

Closed

2 tasks

refack force-pushed the vcbuild-tweaks branch from 31ff552 to 6a10eb5 Compare November 25, 2017 19:25

[fixup] explicit validation section

eb496c4

seishun reviewed Nov 25, 2017

View reviewed changes

[fixup] improve cleaning of Release artifacts

aa15c27

refack force-pushed the vcbuild-tweaks branch from 46ec09d to aa15c27 Compare November 25, 2017 20:39

bzoz reviewed Nov 28, 2017

View reviewed changes

refack added the wip Issues and PRs that are still a work in progress. label Dec 2, 2017

seishun mentioned this pull request Dec 4, 2017

build, win: vcbuild improvements #17015

Closed

2 tasks

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

seishun mentioned this pull request Dec 21, 2017

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

Closed

1 task

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR closed this Feb 7, 2018

refack deleted the vcbuild-tweaks branch September 5, 2018 22:29

		@@ -354,6 +363,7 @@ if not defined msi goto run

		:msibuild
		echo Building node-v%FULLVERSION%-%target_arch%.msi

Uh oh!

Conversation

refack commented Nov 25, 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 Nov 25, 2017

Uh oh!

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

Uh oh!

Uh oh!

refack commented Nov 25, 2017

Uh oh!

seishun commented Nov 25, 2017

Uh oh!

refack commented Nov 25, 2017

Uh oh!

seishun Nov 25, 2017

Choose a reason for hiding this comment

Uh oh!

refack Nov 25, 2017

Choose a reason for hiding this comment

Uh oh!

seishun Nov 25, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 25, 2017

Uh oh!

bzoz Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

bzoz Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 31, 2018

Uh oh!

BridgeAR commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

refack commented Nov 25, 2017 •

edited

Loading

refack commented Nov 25, 2017 •

edited

Loading