Fix quoting shell args on Windows

Fix quoting shell args on Windows

Fixes test/defines-escaping and a couple others. (Windows shell quoting: Not
The Best Design Ever.)
Committed: https://code.google.com/p/gyp/source/detail?r=1215

[scottmg, 2012-02-20 06:02:38]

I'm sure both of you will be ever so interested in the intricacies of cmd and ".

[Nico, 2012-02-20 18:33:22]

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:58: windows_quoter_regex = re.compile(r'(\\*)"')
grit is very slow because it has all these module-scope 're.compile' calls which
get executed at module load time. grit imports all its modules but uses only a
few of these regexen on every run depending on various flags. So as a result
grit has slow startup because of its 'global' re.compile statements. Maybe we
can prevent this somehow?

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:67: if sys.platform == 'win32':
It's probably better to key this off 'flavor' (which you could pass in)? Then
msvc projects could be created on linux (GYP_GENERATORS=ninja-win
build/gyp_chromium)

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:68: # See http://goo.gl/tRtOd and
http://goo.gl/dhPnp including the comment
I get "The resource you are looking for has been removed, had its name changed,
or is temporarily unavailable." for the first link.

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:91: else:
Can you put this branch first? Then the common case ( :-) ) comes first, and you
can dedent the whole windows block.

More generally, do you think you'll do MSVC option emulation eventually? If so,
maybe you want to put the windows bits here into a new function into a new 'msvc
emulation' module (maybe pylib/gyp/msvc_emulation.py to parallel
xcode_emulation.py?), then this function could just look like

  ...
  if sys.platform == 'win32':
    return msvc_emulation.QuoteCmdExeArgument(arg)
  return "'" + arg.replace...

[scottmg, 2012-02-20 22:30:48]

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:58: windows_quoter_regex = re.compile(r'(\\*)"')
On 2012/02/20 18:33:22, Nico wrote:
> grit is very slow because it has all these module-scope 're.compile' calls
which
> get executed at module load time. grit imports all its modules but uses only a
> few of these regexen on every run depending on various flags. So as a result
> grit has slow startup because of its 'global' re.compile statements. Maybe we
> can prevent this somehow?

I guess it depends if we end up with a lot, for some definition of a lot.
Quoting shell arguments happens lots, otherwise we could just not bother
precompiling the regex at all I suppose. I guess I could make it a lazily
initialized global if you think adding global re's is a bad idea (so it'd just
be a test per shell arg, rather than a compile).

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:67: if sys.platform == 'win32':
On 2012/02/20 18:33:22, Nico wrote:
> It's probably better to key this off 'flavor' (which you could pass in)? Then
> msvc projects could be created on linux (GYP_GENERATORS=ninja-win
> build/gyp_chromium)

Done.

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:68: # See http://goo.gl/tRtOd and
http://goo.gl/dhPnp including the comment
On 2012/02/20 18:33:22, Nico wrote:
> I get "The resource you are looking for has been removed, had its name
changed,
> or is temporarily unavailable." for the first link.

Fixed.

https://chromiumcodereview.appspot.com/9427002/diff/3001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:91: else:
On 2012/02/20 18:33:22, Nico wrote:
> Can you put this branch first? Then the common case ( :-) ) comes first, and
you
> can dedent the whole windows block.
> 
> More generally, do you think you'll do MSVC option emulation eventually? If
so,
> maybe you want to put the windows bits here into a new function into a new
'msvc
> emulation' module (maybe pylib/gyp/msvc_emulation.py to parallel
> xcode_emulation.py?), then this function could just look like
> 
>   ...
>   if sys.platform == 'win32':
>     return msvc_emulation.QuoteCmdExeArgument(arg)
>   return "'" + arg.replace...

Yeah, options emulation is the next thing to do after the basic tests all pass.
I'm not sure cmd.exe has anything to do with msvs (vs. bash implementation
details belonging in the main module), but it's a convenient place to stuff it
for now.

[Nico, 2012-02-20 23:17:49]

lgtm with comments. Everything except the docstrings and test question are
optional.

http://codereview.chromium.org/9427002/diff/6001/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/9427002/diff/6001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:181: def QuoteShellArgument(self, arg):
nit: I would've made flavor a parameter instead

http://codereview.chromium.org/9427002/diff/6001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

http://codereview.chromium.org/9427002/diff/6001/pylib/gyp/msvs_emulation.py#...
pylib/gyp/msvs_emulation.py:4: 
Needs module docstring

http://codereview.chromium.org/9427002/diff/6001/pylib/gyp/msvs_emulation.py#...
pylib/gyp/msvs_emulation.py:9: def QuoteCmdExeArgument(arg):
need function docstring

http://codereview.chromium.org/9427002/diff/6001/pylib/gyp/msvs_emulation.py#...
pylib/gyp/msvs_emulation.py:10: # See http://goo.gl/cuFbX and
http://goo.gl/dhPnp including the comment
nit: I wouldn't use a URL shortener

http://codereview.chromium.org/9427002/diff/6001/pylib/gyp/msvs_emulation.py#...
pylib/gyp/msvs_emulation.py:21: # Now, we need to escape some things that are
actually for the shell. %'s
The '%' handling happens further down, so I wouldn't mention it in this comment.

http://codereview.chromium.org/9427002/diff/6001/pylib/gyp/msvs_emulation.py#...
pylib/gyp/msvs_emulation.py:33: return '"' + tmp + '"'
I assume all this is covered by existing tests.

[scottmg, 2012-02-21 00:14:44]

https://chromiumcodereview.appspot.com/9427002/diff/6001/pylib/gyp/generator/...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/9427002/diff/6001/pylib/gyp/generator/...
pylib/gyp/generator/ninja.py:181: def QuoteShellArgument(self, arg):
On 2012/02/20 23:17:49, Nico wrote:
> nit: I would've made flavor a parameter instead

Done.

https://chromiumcodereview.appspot.com/9427002/diff/6001/pylib/gyp/msvs_emula...
File pylib/gyp/msvs_emulation.py (right):

https://chromiumcodereview.appspot.com/9427002/diff/6001/pylib/gyp/msvs_emula...
pylib/gyp/msvs_emulation.py:4: 
On 2012/02/20 23:17:49, Nico wrote:
> Needs module docstring

Done.

https://chromiumcodereview.appspot.com/9427002/diff/6001/pylib/gyp/msvs_emula...
pylib/gyp/msvs_emulation.py:9: def QuoteCmdExeArgument(arg):
On 2012/02/20 23:17:49, Nico wrote:
> need function docstring

Done.

https://chromiumcodereview.appspot.com/9427002/diff/6001/pylib/gyp/msvs_emula...
pylib/gyp/msvs_emulation.py:10: # See http://goo.gl/cuFbX and
http://goo.gl/dhPnp including the comment
On 2012/02/20 23:17:49, Nico wrote:
> nit: I wouldn't use a URL shortener

The urls were longer than 80 so it looked yucky.

https://chromiumcodereview.appspot.com/9427002/diff/6001/pylib/gyp/msvs_emula...
pylib/gyp/msvs_emulation.py:21: # Now, we need to escape some things that are
actually for the shell. %'s
On 2012/02/20 23:17:49, Nico wrote:
> The '%' handling happens further down, so I wouldn't mention it in this
comment.

Done.

https://chromiumcodereview.appspot.com/9427002/diff/6001/pylib/gyp/msvs_emula...
pylib/gyp/msvs_emulation.py:33: return '"' + tmp + '"'
On 2012/02/20 23:17:49, Nico wrote:
> I assume all this is covered by existing tests.

Yes, this change is just trying to get existing tests working
("defines-escaping" is pretty thorough). I'll add new tests if I find other
things that aren't tested, as I get farther.