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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Dec 21, 2017

A subroutine does not work as a replacement for the python command
since one cannot use a subroutine call in a for /F loop.

Similarly to #17298 and #17015, this introduces a maintenance burden: namely, whenever significant changes are introduced in vcbuild.bat, one has to make sure find_python.cmd is called before the first Python usage. Consequently, I would prefer #17293 to be landed instead.

Checklist
Affected core subsystem(s)

build

@seishun
build,win: replace find-python subroutine with PYTHON variable
53c10bc 
A subroutine does not work as a replacement for the `python` command
since one cannot use a subroutine call in a `for /F` loop.
@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 Dec 21, 2017
@seishun
Copy link
Contributor Author

seishun commented Dec 21, 2017

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

cc @nodejs/build @nodejs/platform-windows

@benjamingr
Copy link
Member

@refack

rvagg
rvagg reviewed Dec 28, 2017
View reviewed changes
vcbuild.bat
@@ -232,7 +235,8 @@ goto run
if defined noprojgen goto msbuild

@rem Generate the VS project.
call :run-python configure %configure_flags%
echo configure %configure_flags%
Copy link
Member

Choose a reason for hiding this comment

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

%PYTHON% in here would be nice

rvagg
rvagg reviewed Dec 28, 2017
View reviewed changes
vcbuild.bat
@@ -448,7 +452,8 @@ if defined no_cctest echo Skipping cctest because no-cctest was specified && got
echo running 'cctest %cctest_args%'
"%config%\cctest" %cctest_args%
:run-test-py
call :run-python tools\test.py %test_args%
echo running 'python tools\test.py %test_args%'
Copy link
Member

Choose a reason for hiding this comment

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

ditto, running '%PYTHON% ...

@rvagg
Copy link
Member

rvagg commented Dec 28, 2017

This approach sgtm, I don't really see the problem here though @seishun:

whenever significant changes are introduced in vcbuild.bat, one has to make sure find_python.cmd is called before the first Python usage

You have it at the top of the file, just under arg parsing, so any major changes are going to have to be above that line and it's not highly likely those changes would have a use for python above there anyway. To be extra sure you could just add a comment in the file to explain why it's being called so future committers have a warning at least.

@seishun seishun mentioned this pull request Dec 28, 2017
Closed
2 tasks
@BridgeAR
Copy link
Member

Ping @seishun

@seishun
Copy link
Contributor Author

seishun commented Jan 19, 2018

@nodejs/collaborators we need to decide whether to merge this PR or #17293. The reasons why I prefer that one are mentioned in OP. That PR has 3 approvals and 1 change request (not sure if it's still relevant? cc @refack), this one has 2 approvals, so the opinions seem divided.

@bzoz
Copy link
Contributor

bzoz commented Jan 30, 2018

@seishun I think we should land this PR, since #17293 would break some user scenarios that currently work.

@seishun
Copy link
Contributor Author

seishun commented Jan 30, 2018

@bzoz If you mean the scenario where Python is installed but not added to PATH, then it doesn't work too well - it prints some error messages before proceeding with the build, then leaks PATH modifications to the cmd session:

C:\node>python
'python' is not recognized as an internal or external command,
operable program or batch file.

C:\node>vcbuild
ERROR: The system was unable to find the specified registry key or value.
ERROR: The system was unable to find the specified registry key or value.
Looking for Visual Studio 2017
calling: "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\\Auxiliary\Build\vcvarsall.bat" amd64
^CTerminate batch job (Y/N)? y

C:\node>python
Python 2.7.14 (v2.7.14:84471935ed, Sep 16 2017, 20:19:30) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.

Since no one has reported these issues, I suspect very few people, if any, rely on this scenario.

@bzoz
Copy link
Contributor

bzoz commented Jan 30, 2018

#17015 will fix the error messages, PATH leak could be fixed also.

I guess we do not get reports, because after all compilation works, and those error messages are covered by miles of compilation output. Since the code is already here, it works, and it is not a chore to maintain I don't see a reason to drop a supported scenario.

@BridgeAR BridgeAR requested a review from a team January 31, 2018 02:18
@seishun
Copy link
Contributor Author

seishun commented Jan 31, 2018

it is not a chore to maintain

It does carry a maintenance burden as explained in OP. Whether it's greater than the usefulness of this scenario is subjective, so we probably won't convince each other. Unless TSC chimes in, I'll just land the PR that gains more approvals.

@bzoz
Copy link
Contributor

bzoz commented Jan 31, 2018

The other PR has -1 from @refack, so only this PR can land.

@seishun
Copy link
Contributor Author

seishun commented Jan 31, 2018

There was a lot of discussion after he requested changes. I asked him if it's still relevant in #17293 (comment) and didn't get a response.

@bzoz
Copy link
Contributor

bzoz commented Jan 31, 2018

I think it is still relevant and I'm also -1 for that PR. If you trace #13900 it is a fix for a real issue described in nodejs/CTC#147 (comment).

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

@seishun I do not really see the maintenance burden here and I feel like this one is not controversial so far. So as soon as you address @rvagg comments and there is a green CI, I would go ahead and land this.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

@seishun would you be so kind and address the comments? As soon as that is done I would go ahead and land this.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2018
@seishun seishun mentioned this pull request Feb 7, 2018
Closed
2 tasks
@BridgeAR
Copy link
Member

I am going to close this since a mentioned alternative has landed. @seishun if this is not correct, please reopen.

@BridgeAR BridgeAR closed this Feb 16, 2018
@seishun seishun deleted the find-python branch February 16, 2018 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvagg rvagg rvagg left review comments

@bzoz bzoz bzoz approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@seishun @benjamingr @rvagg @BridgeAR @bzoz @nodejs-github-bot