ninja/win: Put common msvs_system_include_dirs into %INCLUDE%

ninja/win: Put common msvs_system_include_dirs into %INCLUDE%

Paths in INCLUDE are considered system headers by clang-cl and it will suppress
warnings from system headers.

BUG=chromium:395405
r1953

[Nico, 2014-07-19 01:05:31]

That's how the gyp-based thing looks. Worth cleaning up and writing tests for?

It seems kind of useful to me, and it seems to work in chromium (i think; only
ran gyp, haven't done a build with it yet)

[scottmg, 2014-07-19 18:49:36]

Sure, seems reasonable if it helps.

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

https://codereview.chromium.org/406523005/diff/1/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:1784: for qualified_target in target_list:
is this target_list everything? how does it relate to all_targets at 2123?

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

https://codereview.chromium.org/406523005/diff/1/pylib/gyp/msvs_emulation.py#...
pylib/gyp/msvs_emulation.py:935: system_includes = system_includes |
set(env.get('INCLUDE', '').split(';'))
i think order should be preserved (though i'm not sure what that means when
combining). mostly just that someone could prepend to INCLUDE to inject their
own blah.h i guess.

[Nico, 2014-07-19 23:05:35]

ptal. (I'll fix the CL description in a second.)

I started writing a test, but then realized that this isn't testable all that
well.

I'm not sure if the test I have is all that useful; I suppose it provides at
least coverage which is useful since there's no compiler to check types. (This
is the first msvs_system_include_dirs test :-( )

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

https://codereview.chromium.org/406523005/diff/1/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:1784: for qualified_target in target_list:
On 2014/07/19 18:49:36, scottmg wrote:
> is this target_list everything? how does it relate to all_targets at 2123?

as far as I understand, all_targets gets everything referenced from the input
gyp file (usually build/all.gyp), while this is everything. (Say all.gyp depends
on foo.gyp:bar, but foo.gyp has also a target baz that nothing depends on. Then
all_targets won't have baz, but this list will.)

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

https://codereview.chromium.org/406523005/diff/1/pylib/gyp/msvs_emulation.py#...
pylib/gyp/msvs_emulation.py:935: system_includes = system_includes |
set(env.get('INCLUDE', '').split(';'))
On 2014/07/19 18:49:36, scottmg wrote:
> i think order should be preserved (though i'm not sure what that means when
> combining). mostly just that someone could prepend to INCLUDE to inject their
> own blah.h i guess.

Changed everything to use OrderedDict

https://codereview.chromium.org/406523005/diff/100001/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/406523005/diff/100001/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:163: all_system_includes = all_system_includes &
OrderedSet(system_includes)
I had this for the previous lines:

  all_system_includes = OrderedSet.intersection(
      *(OrderedSet(config.get('msvs_system_include_dirs', []))
        for config in configs])

...but OrderedSet doesn't have set.intersection. Oh well.

[scottmg, 2014-07-20 05:18:10]

lgtm

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1784: if not
generator_flags.get('ninja_use_custom_environment_files', 0):
Does this mean this isn't going to work with goma anyway then?

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:159: all_system_includes = OrderedSet(
I'm vaguely uneasy about differing / \ here, but I guess it's OK because if they
mismatch things will just be left alone. (I think it best not to attempt
normalization)

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:161: for config in configs:
configs[1:] maybe would be clearer

[scottmg, 2014-07-20 05:20:07]

https://codereview.chromium.org/406523005/diff/150001/test/win/system-include...
File test/win/system-include/test.gyp (right):

https://codereview.chromium.org/406523005/diff/150001/test/win/system-include...
test/win/system-include/test.gyp:11: 'common',  # Same for all targets
This might need to be absolute? :/

[Nico, 2014-07-20 05:51:02]

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1784: if not
generator_flags.get('ninja_use_custom_environment_files', 0):
On 2014/07/20 05:18:10, scottmg wrote:
> Does this mean this isn't going to work with goma anyway then?

...uhrr? No idea!
http://build.chromium.org/p/chromium/builders/Win/builds/22677/steps/runhooks...
doesn't seem to set this at least. I would've thought that this setting was more
used to build the goma client binary, not for building chrome with goma. (?)

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/406523005/diff/150001/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:161: for config in configs:
On 2014/07/20 05:18:10, scottmg wrote:
> configs[1:] maybe would be clearer

Done.

https://codereview.chromium.org/406523005/diff/150001/test/win/system-include...
File test/win/system-include/test.gyp (right):

https://codereview.chromium.org/406523005/diff/150001/test/win/system-include...
test/win/system-include/test.gyp:11: 'common',  # Same for all targets
On 2014/07/20 05:20:07, scottmg wrote:
> This might need to be absolute? :/

Maybe, apparently. Passes for me locally as is :-/

[Nico, 2014-07-20 05:53:14]

ima try landing the source files first. Maybe the try server has trouble
applying the patch; the raw git diff looks a bit weird.

[Nico, 2014-07-20 16:10:43]

On 2014/07/20 05:53:14, Nico (away) wrote:
> ima try landing the source files first. Maybe the try server has trouble
> applying the patch; the raw git diff looks a bit weird.

that worked! :-)

:-(