GN: Check if targets with precompiled headers use unsupported toolchains.

GN: Check if targets with precompiled headers use unsupported toolchains.

A target with a precompiled header needs a toolchain which sets
precompiled_header_type in the tool()s used by the target.

In the past, having precompiled_header_type unset led to broken Ninja files
being generated, now an error is raised after the target is resolved.

BUG=649899

[timn, 2016-10-06 21:19:08]

Description was changed from

==========
GN: Check if targets with precompiled headers use unsupported toolchains.

A target with a precompiled header needs a toolchain which sets
precompiled_header_type in the tool()s used by the target.

In the past, having precompiled_header_type unset lead to broken Ninja files
being generated, now an error is raised after the target is resolved.

BUG=649899
==========

to

==========
GN: Check if targets with precompiled headers use unsupported toolchains.

A target with a precompiled header needs a toolchain which sets
precompiled_header_type in the tool()s used by the target.

In the past, having precompiled_header_type unset led to broken Ninja files
being generated, now an error is raised after the target is resolved.

BUG=649899
==========

[timn, 2016-10-06 21:19:08]

tim@rnc-ag.de changed reviewers:
+ brettw@chromium.org

[brettw, 2016-10-07 18:12:04]

Thanks for noticing this bug.

I originally wrote this expecting that if the tool didn't define the PCH type,
it would just be unused for that compile.

In the Ninja writing code, it generally checks for both PCH on the target as
well as the tool. I can see in NinjaBinaryTargetWriter::WriteOneFlag where it
doesn't write the flags if the tool doesn't define a type. What it we just add
an else case there and write the flags?

If everything else works with the above change, I would prefer that approach. Or
are there other subtle things missing?

[timn, 2016-10-07 22:35:39]

On 2016/10/07 18:12:04, brettw (ping on IM after 24h) wrote:
> Thanks for noticing this bug.
> 
> I originally wrote this expecting that if the tool didn't define the PCH type,
> it would just be unused for that compile.

Makes sense as well, though I would prefer an error/warning in this case.
At least that's what I'd expect when using PCHs when they're not available.

btw.: The gcc_toolchain() template doesn't set precompiled_header_type (while
the MSVC one does).
Is this an oversight or intended?

> In the Ninja writing code, it generally checks for both PCH on the target as
> well as the tool. I can see in NinjaBinaryTargetWriter::WriteOneFlag where it
> doesn't write the flags if the tool doesn't define a type. What it we just add
> an else case there and write the flags?

Yes, that'd fix the bug.

(That's what I tried originally, but in the end I preferred the error-out
variant.)

> If everything else works with the above change, I would prefer that approach.
Or
> are there other subtle things missing?

No, the missing flags is the only thing I noticed so far.

[brettw, 2016-10-10 19:21:40]

On 2016/10/07 22:35:39, timn wrote:
> On 2016/10/07 18:12:04, brettw (ping on IM after 24h) wrote:
> > Thanks for noticing this bug.
> > 
> > I originally wrote this expecting that if the tool didn't define the PCH
type,
> > it would just be unused for that compile.
> 
> Makes sense as well, though I would prefer an error/warning in this case.
> At least that's what I'd expect when using PCHs when they're not available.
> 
> btw.: The gcc_toolchain() template doesn't set precompiled_header_type (while
> the MSVC one does).
> Is this an oversight or intended?

I have a lot of sympathy for the pedantic error messages. But the second point
about gcc_toolchain kind of gets to the reason behind the silently-ignore
design. The Chrome build uses many toolchains (even gcc_toolchain is used on
Windows for NaCl), and I wasn't sure all of them would support PCH. If this
failed hard, we would have to do a bunch of tracking to ensure that enabling PCH
in the config matches the toolchain it's being compiled into. As of now this
isn't an issue because PCH is only supported on Mac and Windows native
toolchains, so the condition is trivial.

So I still lean toward fixing the flags writer instead.