[PATCH] D59003: [GN] Locate prebuilt binaries correctly.

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: llvm/utils/gn/gn.py:41
-    if subprocess.call([gn, '--version'], stdout=open(os.devnull, 'w'),
-                                          stderr=subprocess.STDOUT) != 0:
         # Not on path. See if get.py downloaded a prebuilt binary and run that
----------------
hctim wrote:
> thakis wrote:
> > pcc wrote:
> > > thakis wrote:
> > > > pcc wrote:
> > > > > Maybe you could pass `shell=True` here instead? On my machine at least, that causes the shell to fail if the command is not found instead of an exception being thrown.
> > > > I like this suggestion, but shell=True means that the args array is interpreted differently on Win and non-Win. On non-Win, the args are passed to the shell and you have to make the first item something like 'gn --version' as a single string, while on Windows iirc you have to have two list entries (or the other way round)?
> > > A quick test on Linux and Windows reveals that passing a string instead of an array for args appears to do the same thing on both platforms. This is also how the documentation suggests using subprocess.call with shell=True:
> > > https://docs.python.org/2/library/subprocess.html
> > hctim, does using `subprocess.call('gn --version', ..., shell=True)` work for you?
> Yep, adding shell=True works fine for me. Would you prefer that oneliner to be committed or this change?
I'd prefer the oneliner since this change as-is breaks the depot_tools-with-existing-but-broken-gn-binary-on-path use case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59003/new/

https://reviews.llvm.org/D59003