Fix ui/gl and gpu target building with GN on Windows.

Fix ui/gl and gpu target building with GN on Windows.

Committed: https://crrev.com/e43326b6726b227c739383bae5f51368833a12d3
Cr-Commit-Position: refs/heads/master@{#309754}

[Slava Chigrin, 2014-12-16 17:45:24]

vchigrin@yandex-team.ru changed reviewers:
+ cpu@chromium.org, piman@chromium.org

[Slava Chigrin, 2014-12-16 17:51:57]

vchigrin@yandex-team.ru changed reviewers:
+ sievers@chromium.org
- piman@chromium.org

[Slava Chigrin, 2014-12-16 17:53:49]

This patch in fact depends on https://codereview.chromium.org/806183002/, so
must be landed only after it (because of some targets, enabled in this CL, pull
other targets, that former CL fixes).

https://codereview.chromium.org/809783002/diff/1/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/809783002/diff/1/ui/gl/BUILD.gn#newcode28
ui/gl/BUILD.gn:28: # and compilation will fail.
Unfortunatelly, I can not find any more elegant solution. Windows SDK includes
added to most targets in src/build/config/BUILDCONFIG.gn, and they appear in
include list before include dirs for any other configs.

If there is some GN feature, that allows include dir re-order by configs, I'll
be happy to rewrite this patch.

[no sievers, 2014-12-16 19:38:46]

https://codereview.chromium.org/809783002/diff/1/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/809783002/diff/1/ui/gl/BUILD.gn#newcode28
ui/gl/BUILD.gn:28: # and compilation will fail.
On 2014/12/16 17:53:49, Slava Chigrin wrote:
> Unfortunatelly, I can not find any more elegant solution. Windows SDK includes
> added to most targets in src/build/config/BUILDCONFIG.gn, and they appear in
> include list before include dirs for any other configs.
> 
> If there is some GN feature, that allows include dir re-order by configs, I'll
> be happy to rewrite this patch.

hmm yea gyp has something to inject dependencies for dependents. i'd think GN
would have something too.

https://codereview.chromium.org/809783002/diff/1/ui/gl/BUILD.gn#newcode328
ui/gl/BUILD.gn:328: include_dirs = [ "//third_party/mesa/src/include" ]
why do we need this? we usually prefer khronos headers I think. and why was the
problem windows-specific?

[cpu_(ooo_6.6-7.5), 2014-12-16 20:35:52]

cpu@chromium.org changed reviewers:
+ brettw@chromium.org

[cpu_(ooo_6.6-7.5), 2014-12-16 20:35:53]

reviewer change per question.

[Slava Chigrin, 2014-12-16 22:32:38]

Many thanks for review! 

@cpu - sorry, I did not understand your comment. May be because of my English is
not perfect. Could you explain in a bit more detail, what should I do? Or maybe
I did something wrong? Thank you!

https://codereview.chromium.org/809783002/diff/1/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/809783002/diff/1/ui/gl/BUILD.gn#newcode28
ui/gl/BUILD.gn:28: # and compilation will fail.
On 2014/12/16 19:38:46, sievers wrote:
> On 2014/12/16 17:53:49, Slava Chigrin wrote:
> > Unfortunatelly, I can not find any more elegant solution. Windows SDK
includes
> > added to most targets in src/build/config/BUILDCONFIG.gn, and they appear in
> > include list before include dirs for any other configs.
> > 
> > If there is some GN feature, that allows include dir re-order by configs,
I'll
> > be happy to rewrite this patch.
> 
> hmm yea gyp has something to inject dependencies for dependents. i'd think GN
> would have something too.

Sorry, let me explain problem, arising here, in more details. Root problem is
that these files pick gl.h header from system SDK, rather then from mesa, and
fails to compile with it. If we pick gl.h from mesa, compilation will succeed.
On Windows We add path to Windows SDK to (almost?) all targets. It is always
gets added in the beginning of compiler command line. If we would want just add
mesa/src/include to the command line, then dependencies (or dependencies for
dependencies) perfectly solve problem. But unfortunately, we need not only add
new path, but add it BEFORE windows SDK path. Seems, in GN this task can be done
only by manually adding it to include_dirs of appropriate targets, since dirs
from dependent configs gets added only AFTER SDK path.

It is certainly possible that I took wrong way somewhere, just tell  me, is it
supposed that these files be able to be build using system gl.h? If no, then is
it possible to re-order config paths using GN configs?

https://codereview.chromium.org/809783002/diff/1/ui/gl/BUILD.gn#newcode328
ui/gl/BUILD.gn:328: include_dirs = [ "//third_party/mesa/src/include" ]
On 2014/12/16 19:38:46, sievers wrote:
> why do we need this? we usually prefer khronos headers I think. and why was
the
> problem windows-specific?

OK, will change include path, if khronos have gl.h, that will help. But in any
case, include search order problems remains. I guess, problem is not met on
Linux, since there we just add --sysroot command line option, if required,
instead of adding path to /usr/include to beginning of compiler command line.
Thus, all provided include dirs (mesa, or khronos, as you said) get searched
before compiler goes to examine system SDK dirs. On windows at present it checks
SDK dirs first.

[Slava Chigrin, 2014-12-17 15:28:13]

I managed to resolve include serch order problem in better way, without any
modifications in /gpu or /ui/gl targets.
Thanks for review!

[brettw, 2014-12-17 19:21:35]

https://codereview.chromium.org/809783002/diff/20001/build/config/win/visual_...
File build/config/win/visual_studio_version.gni (right):

https://codereview.chromium.org/809783002/diff/20001/build/config/win/visual_...
build/config/win/visual_studio_version.gni:27: system_include_dirs = []
Can we delete this now? Actually it looks like it was abandoned even before this
patch.

https://codereview.chromium.org/809783002/diff/20001/build/config/win/visual_...
build/config/win/visual_studio_version.gni:45: # System include dirs must be set
by setup_toolchain.py script. Since in
Seems like this comment can be deleted. It doesn't make sense in isolation (i.e.
without looking at this diff).

[cpu_(ooo_6.6-7.5), 2014-12-17 22:12:17]

cpu@chromium.org changed reviewers:
- cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2014-12-17 22:12:18]

just saying that brettw is the appropriate reviewer for this change.

[Slava Chigrin, 2014-12-18 00:24:02]

If anybody considering approving these changes, please, trigger
chromium_win8_gn_* try bots. Seems, CQ does not check windows GN configs and I
am afraid breaking the tree. Unfortunately, I don't have rights to trigger try
jobs myself. 
Thank you!

[Slava Chigrin, 2014-12-21 18:04:59]

Patchset #4 (id:60001) has been deleted

[Slava Chigrin, 2014-12-23 00:56:02]

On 2014/12/18 00:24:02, Slava Chigrin wrote:
> If anybody considering approving these changes, please, trigger
> chromium_win8_gn_* try bots. Seems, CQ does not check windows GN configs and I
> am afraid breaking the tree. Unfortunately, I don't have rights to trigger try
> jobs myself. 
> Thank you!

Seems like now all try jobs (including Windows GN ones) completes OK.

[Slava Chigrin, 2014-12-24 00:09:09]

Please, take a look to the latest Patch set.
Thank you in advance!

[brettw, 2014-12-30 02:56:59]

lgtm

[Slava Chigrin, 2014-12-30 08:58:46]

The CQ bit was checked by vchigrin@yandex-team.ru

[commit-bot: I haz the power, 2014-12-30 08:59:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/809783002/80001

[commit-bot: I haz the power, 2014-12-30 09:42:02]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-12-30 09:42:45]

Patchset 4 (id:??) landed as
https://crrev.com/e43326b6726b227c739383bae5f51368833a12d3
Cr-Commit-Position: refs/heads/master@{#309754}