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: fix windows build if python is a bat file #2594

Closed
wants to merge 1 commit into from

Conversation

skomski
Copy link
Contributor

@skomski skomski commented Aug 28, 2015

Invoking a bat file without stopping requires call.

depot_tools uses a bat file which triggered this error.

Invoking a bat file without stopping requires call
@rvagg
Copy link
Member

rvagg commented Aug 28, 2015

Hah, interesting. /cc @nodejs/platform-windows

Are you saying that depot_tools is using Node's vcbuild.bat? What for?

@skomski
Copy link
Contributor Author

skomski commented Aug 28, 2015

Are you saying that depot_tools is using Node's vcbuild.bat? What for?

No I installed depot_tools in my PATH which uses a python.bat.

@targos targos added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Aug 28, 2015
@domenic
Copy link
Contributor

domenic commented Aug 28, 2015

I guess it's kind of harmless, but meh... a third party software installation has effed up, and now we have to work around it? :( Where does it stop---do we execute all executables with call?

I've gotten around this in the place by placing depot_tools/python_bin in my path directly.

@jbergstroem
Copy link
Member

The downside here seems to be that call python doesn't work in powershell. FWIW, I'm -0 to this.

@skomski
Copy link
Contributor Author

skomski commented Sep 3, 2015

@domenic I would say python is a special case because of the history of hacks to handle multiple python versions and this change doesn't do any harm it simply makes it work for more people out-of-the-box and the overlap between depot_tools and node build users is most likely not low.

@jbergstroem It works when invoked from powershell because it just executes vcbuild.bat via cmd and vcbuild.bat already uses call multiple times. It would make sense to maybe in the future switch to a powershell script.

@orangemocha
Copy link
Contributor

I guess it's kind of harmless, but meh... a third party software installation has effed up, and now we have to work around it? :( Where does it stop---do we execute all executables with call?

I kind of agree with that statement.

Would invoking python**.exe** explicitly in vcbuild.bat solve the issue?

@skomski
Copy link
Contributor Author

skomski commented Sep 9, 2015

Would invoking python.exe explicitly in vcbuild.bat solve the issue?

No.

I don't know why there is such a resistance against this simple change.

  • Doesn't make the code harder to read rather makes it clear that it is calling a blocking subprocess
  • Doesn't break any existing setup
  • One third-party software didn't eff up rather it is normal setup to use a bat file for python to call the correct python version
  • It is hyperbole to say where does it stop because python is a special case

It just works for more people out-of-the-box without any fiddling.

@orangemocha
Copy link
Contributor

One third-party software didn't eff up rather it is normal setup to use a bat file for python to call the correct python version

I disagree. Creating that batch file is ok, but the moment you add it to the system path, it's bound to interfere with other software.

The resistance, at least on my part, is because this is not a bug in Node. Even if we change those lines to use call, we have no way of enforcing that future additional invocations of python will be done that way. It sounds like you'd be better off fixing the problem on your end.

@skomski
Copy link
Contributor Author

skomski commented Sep 9, 2015

Well, I guess back to node.js policies. 🃏

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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants