Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: better error message on python fail #17298

Closed
wants to merge 3 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Nov 25, 2017

Fixes: #16864
Refs: #17293

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

@nodejs-github-bot 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
Copy link
Contributor Author

refack commented Nov 25, 2017

I suggest this as an alternative to #17293

@refack
Copy link
Contributor Author

refack commented Nov 25, 2017

@refack
Copy link
Contributor Author

refack commented Nov 25, 2017

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

@seishun
Copy link
Contributor

seishun commented Nov 25, 2017

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

  1. 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.
  2. 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
Copy link
Contributor Author

refack commented Nov 25, 2017

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

@@ -172,8 +181,6 @@ if "%target%"=="Clean" echo deleting %~dp0deps\icu
if "%target%"=="Clean" rmdir /S /Q %~dp0deps\icu
:no-depsicu

call :getnodeversion || exit /b 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next line uses FULLVERSION which is populated in getnodeversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by making the rmdir line more generic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%TAG% is used in project-gen too.

@refack
Copy link
Contributor Author

refack commented Nov 25, 2017

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.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't %FULLVERSION% set by getnodeversion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also in :package

@BridgeAR
Copy link
Member

Ping @refack

@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

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

@BridgeAR BridgeAR closed this Feb 7, 2018
@refack refack deleted the vcbuild-tweaks branch September 5, 2018 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vcbuild explodes is Python is not installed
5 participants