ninja windows: Make pch work on VS2012, and simplify implementation

ninja windows: Make pch work on VS2012, and simplify implementation

Previously, the chromium .gypi files injected a .cc file for PCH, which compiled "normally", and then the generator compiled it again (to a different .obj). Now, the build rule for the injected .cc has its ccflags modified to cause generation of the pch, which matches the msvs generator. Also get to remove the ugly separate 'cc_pch'/'cxx_pch' rules.

I believe this will also fix some very unreproducible build errors we were having (the linked bug) because the dependencies for the pch pdb is much simpler.

BUG=142362, 143646
Committed: https://code.google.com/p/gyp/source/detail?r=1565

[scottmg, 2013-01-23 18:52:26]

Tests: test/win/precompiled currently fails on VS2012, and passes with this
change. But, we don't have a 2012 bot on go/gypbot yet unfortunately.

[Nico, 2013-01-23 19:08:54]

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:833: order_only=predepends, variables=variables)
This adds a variable to every single source file in a target. How much does this
slow down empty build time? (ninja needs to parse all these variables)

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/msvs_emulation.p...
pylib/gyp/msvs_emulation.py:657: return ([('cflags_cc', map(expand_special,
cflags_cc + pch_output))],
Do you need to call expand_special for all flags, or is calling it just for
pch_output enough?

[scottmg, 2013-01-23 19:20:53]

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:780: self.WriteVariableList('cflags_c',
map(self.ExpandSpecial, cflags_c))
here

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:833: order_only=predepends, variables=variables)
On 2013/01/23 19:08:54, Nico wrote:
> This adds a variable to every single source file in a target. How much does
this
> slow down empty build time? (ninja needs to parse all these variables)

The default is [], so ninja_syntax.py doesn't emit anything except when
self.flavor == 'win' and the flags are set to non-empty.

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/msvs_emulation.p...
pylib/gyp/msvs_emulation.py:657: return ([('cflags_cc', map(expand_special,
cflags_cc + pch_output))],
On 2013/01/23 19:08:54, Nico wrote:
> Do you need to call expand_special for all flags, or is calling it just for
> pch_output enough?

It was mirroring the marked 'here' location in ninja.py. I could pass in the
pre-expanded ones, and then only call it on pch_output? Was just more locals in
ninja.py.

[Nico, 2013-01-23 19:22:08]

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:833: order_only=predepends, variables=variables)
On 2013/01/23 19:20:53, scottmg wrote:
> On 2013/01/23 19:08:54, Nico wrote:
> > This adds a variable to every single source file in a target. How much does
> this
> > slow down empty build time? (ninja needs to parse all these variables)
> 
> The default is [], so ninja_syntax.py doesn't emit anything except when
> self.flavor == 'win' and the flags are set to non-empty.

Understood :-) Still curious.

[scottmg, 2013-01-23 20:50:53]

On 2013/01/23 19:22:08, Nico wrote:
>
https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja.py
> File pylib/gyp/generator/ninja.py (right):
> 
>
https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
> pylib/gyp/generator/ninja.py:833: order_only=predepends, variables=variables)
> On 2013/01/23 19:20:53, scottmg wrote:
> > On 2013/01/23 19:08:54, Nico wrote:
> > > This adds a variable to every single source file in a target. How much
does
> > this
> > > slow down empty build time? (ninja needs to parse all these variables)
> > 
> > The default is [], so ninja_syntax.py doesn't emit anything except when
> > self.flavor == 'win' and the flags are set to non-empty.
> 
> Understood :-) Still curious.

You mean the one source file per target that has a cflags_cc variables?

I just tried and can't measure any difference (the noise on Windows is > .1s).

There is one less source file per target too (i.e the flags are modified on one,
and another that was pointless is removed). So, I guess there's roughly the same
amount of "stuff" for ninja to parse.

[Nico, 2013-01-23 21:25:26]

> You mean the one source file per target that has a cflags_cc variables?

Ah, I misread the change and thought this was added to every source file in the
target. Then this is not a concern, thanks for humoring me and measuring the
impact though :-)


After a good start establishing my cluelessness with the above question, I shall
solidify this position with the following:

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:846: for gch, lang_flag, lang, input in
pch_commands:
This is now an empty loop on windows, right?

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1466: '$cflags_pch_c /c $in /Fo$out /Fd$pdbname ')
So what ends up setting cflags_pch_c?

[Nico, 2013-01-23 21:27:07]

And another question: Does anything have a dependency on the pch file, or do
target dependencies have to make sure that it's built early enough?

[scottmg, 2013-01-23 21:32:21]

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:846: for gch, lang_flag, lang, input in
pch_commands:
On 2013/01/23 21:25:26, Nico wrote:
> This is now an empty loop on windows, right?

Yes, and linux too.

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1466: '$cflags_pch_c /c $in /Fo$out /Fd$pdbname ')
On 2013/01/23 21:25:26, Nico wrote:
> So what ends up setting cflags_pch_c?

Ah, good point. Removed both of those as they're not necessary (not sure if I
like 'undefined=empty' in ninja).

[scottmg, 2013-01-23 21:36:21]

On 2013/01/23 21:27:07, Nico wrote:
> And another question: Does anything have a dependency on the pch file, or do
> target dependencies have to make sure that it's built early enough?

No, the pch isn't actually in the graph any more. The object file that is
generated as part of the pch compile step is an implicit dependency for all
objects that use the pch. They used to all depend on the pch rather than the
obj, but that was messier because of $out in the compile rules, and I don't
think it makes any functional difference.

[Nico, 2013-01-23 21:43:35]

I think I get it now. lgtm, thanks for explaining.

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:846: for gch, lang_flag, lang, input in
pch_commands:
On 2013/01/23 21:32:21, scottmg wrote:
> On 2013/01/23 21:25:26, Nico wrote:
> > This is now an empty loop on windows, right?
> 
> Yes, and linux too.
> 

Well yes, linux doesn't support precompiled headers.

https://codereview.chromium.org/12052029/diff/10001/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/12052029/diff/10001/pylib/gyp/generator/ninja...
pylib/gyp/generator/ninja.py:825: implicit =
precompiled_header.GetObjDependencies([input], [output])
Ah, here. Thanks.

https://codereview.chromium.org/12052029/diff/10001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/12052029/diff/10001/pylib/gyp/msvs_emulation....
pylib/gyp/msvs_emulation.py:650: def GetFlagsModifications(self, input, output,
implicit, command,
nit: input, output order in the parameters is the opposite of their order in the
return tuple

[scottmg, 2013-01-23 21:47:48]

On 2013/01/23 21:43:35, Nico wrote:
> I think I get it now. lgtm, thanks for explaining.
> 
>
https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja.py
> File pylib/gyp/generator/ninja.py (right):
> 
>
https://codereview.chromium.org/12052029/diff/1003/pylib/gyp/generator/ninja....
> pylib/gyp/generator/ninja.py:846: for gch, lang_flag, lang, input in
> pch_commands:
> On 2013/01/23 21:32:21, scottmg wrote:
> > On 2013/01/23 21:25:26, Nico wrote:
> > > This is now an empty loop on windows, right?
> > 
> > Yes, and linux too.
> > 
> 
> Well yes, linux doesn't support precompiled headers.

Could call those ...MacPch... maybe? or Gch?

> 
>
https://codereview.chromium.org/12052029/diff/10001/pylib/gyp/generator/ninja.py
> File pylib/gyp/generator/ninja.py (right):
> 
>
https://codereview.chromium.org/12052029/diff/10001/pylib/gyp/generator/ninja...
> pylib/gyp/generator/ninja.py:825: implicit =
> precompiled_header.GetObjDependencies([input], [output])
> Ah, here. Thanks.
> 
>
https://codereview.chromium.org/12052029/diff/10001/pylib/gyp/msvs_emulation.py
> File pylib/gyp/msvs_emulation.py (right):
> 
>
https://codereview.chromium.org/12052029/diff/10001/pylib/gyp/msvs_emulation....
> pylib/gyp/msvs_emulation.py:650: def GetFlagsModifications(self, input,
output,
> implicit, command,
> nit: input, output order in the parameters is the opposite of their order in
the
> return tuple

Done.

[scottmg, 2013-01-23 21:51:59]

(Fix required in WebKit gypi https://bugs.webkit.org/show_bug.cgi?id=107700
before this lands)