ninja windows: support precompiled headers

ninja windows: support precompiled headers

Make existing msvs-only test for PCH be msvs+ninja. Adds various flags to cflags and build rule in similar manner to existing Mac GCH support. Minor modifications to xcode_emulation to share top-level code between Windows and Mac.

R=thakis@chromium.org
BUG=126314
Committed: https://code.google.com/p/gyp/source/detail?r=1397

[Nico, 2012-05-23 20:53:28]

I just skimmed, I stopped after the last comment.

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

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/generator...
pylib/gyp/generator/ninja.py:1309: '$cflags_pch_c /c $in $outflag$out $pchobj
/Fd$pdbname '
Out of interest: Have you compared empty build times of this with having a
separate gch rule? (slightly fewer variable substitutions for compile rules.
probably not measurable, but if you happen to have measured it, I'd like to see
the numbers). If you haven't, don't bother.

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/msvs_emul...
File pylib/gyp/msvs_emulation.py (right):

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/msvs_emul...
pylib/gyp/msvs_emulation.py:293: return ['/Yu' + pch, '/FI' + pch,
'/Fp${pchprefix}.' + pch + '.pch']
http://msdn.microsoft.com/en-us/library/z0atkd6c(v=vs.71) sounds like /Yu
implies /FI, are you sure you need them both?

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/msvs_emul...
pylib/gyp/msvs_emulation.py:500: pch_ext =
os.path.splitext(self._PchSource())[1][1:]
Dealing with extensions in python is so painful :-/ (not your fault, obviously)

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/xcode_emu...
File pylib/gyp/xcode_emulation.py (right):

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/xcode_emu...
pylib/gyp/xcode_emulation.py:736: returns a list of gch files, where |gch| is
the build-directory relative
My guess is that this interface change will break preprocessed headers in the
mac/make build.

[scottmg, 2012-05-24 20:34:39]

thanks

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

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/generator...
pylib/gyp/generator/ninja.py:1309: '$cflags_pch_c /c $in $outflag$out $pchobj
/Fd$pdbname '
On 2012/05/23 20:53:29, Nico wrote:
> Out of interest: Have you compared empty build times of this with having a
> separate gch rule? (slightly fewer variable substitutions for compile rules.
> probably not measurable, but if you happen to have measured it, I'd like to
see
> the numbers). If you haven't, don't bother.

I didn't time it. I thought about doing it like that, but then the chance of
inconsistency seemed worse than a possible small difference in .ninja parse
time.

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/msvs_emul...
File pylib/gyp/msvs_emulation.py (right):

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/msvs_emul...
pylib/gyp/msvs_emulation.py:293: return ['/Yu' + pch, '/FI' + pch,
'/Fp${pchprefix}.' + pch + '.pch']
On 2012/05/23 20:53:29, Nico wrote:
> http://msdn.microsoft.com/en-us/library/z0atkd6c%28v=vs.71) sounds like /Yu
> implies /FI, are you sure you need them both?

I don't think it implies that. Normally projects have the literal text '#include
"precompile.h"' in all .cc files. Because that's not practical for us, we use
/FI to effectively inject it at the top of all .cc files that use pch.

(Also, the msvs generator uses ForceInclude so it's probably best to do the same
in both places so that builds work/break in the same way.)

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/msvs_emul...
pylib/gyp/msvs_emulation.py:500: pch_ext =
os.path.splitext(self._PchSource())[1][1:]
On 2012/05/23 20:53:29, Nico wrote:
> Dealing with extensions in python is so painful :-/ (not your fault,
obviously)

Yeah :/. I switched to literals '.x' to remove the [1:]s... a bit less ugly?

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/xcode_emu...
File pylib/gyp/xcode_emulation.py (right):

https://chromiumcodereview.appspot.com/10407108/diff/2001/pylib/gyp/xcode_emu...
pylib/gyp/xcode_emulation.py:736: returns a list of gch files, where |gch| is
the build-directory relative
On 2012/05/23 20:53:29, Nico wrote:
> My guess is that this interface change will break preprocessed headers in the
> mac/make build.

Thanks, those "unused" returns make a lot more sense now! Ahem.

[Nico, 2012-05-25 22:35:38]

lgtm!

http://codereview.chromium.org/10407108/diff/13001/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/10407108/diff/13001/pylib/gyp/generator/make.p...
pylib/gyp/generator/make.py:1245: for gch, lang_flag, lang, input, extra_vars in
pch_commands:
s/extra_vars/_/ else pylint might complain (i think bradn is trying to get gyp
pylint clean)

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

http://codereview.chromium.org/10407108/diff/13001/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:781: variables += extra_variables
maybe:
variables = [(var_name, lang_flag)] + extra_variables

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

http://codereview.chromium.org/10407108/diff/13001/pylib/gyp/msvs_emulation.p...
pylib/gyp/msvs_emulation.py:518: ('pchobj', '/Fo' + self._PchObj())]
nit: it'd be more consistent with the xcode code if there was a separate cc
command for pch files instead of extra_vars, this feels a bit like a gratuitous
difference (if I understood it right).

But I'm aware what they say about consistency and hobgoblins :-)

http://codereview.chromium.org/10407108/diff/13001/test/win/precompiled/gypte...
File test/win/precompiled/gyptest-all.py (right):

http://codereview.chromium.org/10407108/diff/13001/test/win/precompiled/gypte...
test/win/precompiled/gyptest-all.py:1: #!/usr/bin/env python
nit: Why is this file A not A+ like the others?

[scottmg, 2012-05-26 02:27:10]

http://codereview.chromium.org/10407108/diff/13001/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/10407108/diff/13001/pylib/gyp/generator/make.p...
pylib/gyp/generator/make.py:1245: for gch, lang_flag, lang, input, extra_vars in
pch_commands:
On 2012/05/25 22:35:39, Nico wrote:
> s/extra_vars/_/ else pylint might complain (i think bradn is trying to get gyp
> pylint clean)

Done.

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

http://codereview.chromium.org/10407108/diff/13001/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:781: variables += extra_variables
On 2012/05/25 22:35:39, Nico wrote:
> maybe:
> variables = [(var_name, lang_flag)] + extra_variables

Done.

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

http://codereview.chromium.org/10407108/diff/13001/pylib/gyp/msvs_emulation.p...
pylib/gyp/msvs_emulation.py:518: ('pchobj', '/Fo' + self._PchObj())]
On 2012/05/25 22:35:39, Nico wrote:
> nit: it'd be more consistent with the xcode code if there was a separate cc
> command for pch files instead of extra_vars, this feels a bit like a
gratuitous
> difference (if I understood it right).
> 
> But I'm aware what they say about consistency and hobgoblins :-)

OK, guess it's more localized that way anyway.

http://codereview.chromium.org/10407108/diff/13001/test/win/precompiled/gypte...
File test/win/precompiled/gyptest-all.py (right):

http://codereview.chromium.org/10407108/diff/13001/test/win/precompiled/gypte...
test/win/precompiled/gyptest-all.py:1: #!/usr/bin/env python
On 2012/05/25 22:35:39, Nico wrote:
> nit: Why is this file A not A+ like the others?

What does the + mean? Unchanged?

If that's it, then it's because I added ", 'ninja'" to the formats.

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

http://codereview.chromium.org/10407108/diff/23003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:781: map.update({'c': 'cc_pch', 'cc': 'cxx_pch'})
I'm guessing this isn't what you were thinking. I'm not sure how to shove it
into $cflags_pch_c because it needs to change the flag name that goes in front
of $out.

[Nico, 2012-05-26 18:16:15]

LGTM

I think this is somewhat simpler. But if you like the other approach better, I'm
happy with that too.

http://codereview.chromium.org/10407108/diff/13001/test/win/precompiled/gypte...
File test/win/precompiled/gyptest-all.py (right):

http://codereview.chromium.org/10407108/diff/13001/test/win/precompiled/gypte...
test/win/precompiled/gyptest-all.py:1: #!/usr/bin/env python
On 2012/05/26 02:27:12, scottmg wrote:
> On 2012/05/25 22:35:39, Nico wrote:
> > nit: Why is this file A not A+ like the others?
> 
> What does the + mean? Unchanged?

No, "moved". When you `svn mv` a file and then change it, svn shows the new file
as A+ and only shows what was changed from the old location (see the other
files). If you just `mv` and then `svn rm` the old and `svn add` the new, you
lose that.

> 
> If that's it, then it's because I added ", 'ninja'" to the formats.

[Nico, 2012-05-26 18:16:15]

LGTM

I think this is somewhat simpler. But if you like the other approach better, I'm
happy with that too.

[scottmg, 2012-05-28 18:26:39]

try from svn