build,win: replace find-python subroutine with PYTHON variable#17804
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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 added build 
seishun commented Dec 21, 2017
CI: https://ci.nodejs.org/job/node-test-pull-request/12247/ cc @nodejs/build @nodejs/platform-windows |
benjamingr commented Dec 21, 2017
| @rem Generate the VS project. | ||
| call :run-python configure %configure_flags% | ||
| echo configure %configure_flags% |
| "%config%\cctest"%cctest_args% | ||
| :run-test-py | ||
| call :run-python tools\test.py %test_args% | ||
| echo running 'python tools\test.py %test_args%' |
rvagg commented Dec 28, 2017
This approach sgtm, I don't really see the problem here though @seishun:
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. |
BridgeAR commented Jan 19, 2018
Ping @seishun |
seishun commented Jan 19, 2018
bzoz commented Jan 30, 2018
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 Since no one has reported these issues, I suspect very few people, if any, rely on this scenario. |
bzoz commented Jan 30, 2018
#17015 will fix the error messages, 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. |
seishun commented Jan 31, 2018
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 commented Jan 31, 2018
The other PR has -1 from @refack, so only this PR can land. |
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 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 commented Feb 1, 2018
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 added the author ready BridgeAR commented Feb 16, 2018
I am going to close this since a mentioned alternative has landed. @seishun if this is not correct, please reopen. |
6 participants
A subroutine does not work as a replacement for the
pythoncommandsince one cannot use a subroutine call in a
for /Floop.Similarly to #17298 and #17015, this introduces a maintenance burden: namely, whenever significant changes are introduced in
vcbuild.bat, one has to make surefind_python.cmdis called before the first Python usage. Consequently, I would prefer #17293 to be landed instead.Checklist
Affected core subsystem(s)
build