GN: Create a :gles2_interface target to resolve some check errors in //cc.

GN: Create a :gles2_interface target to resolve some check errors in //cc.

gles2_interface.h defines a pure virtual interface; targets which use it should
depend on it, but not necessarily on an implementation target. Consequently,
a //gpu/command_buffer/client:gles2_interface target was created to reflect the
dependency on this interface without linking a particular implementation.

This resolves an issue with //cc trying to use this header without having a
dependency on a target which includes it.

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

[jbroman, 2014-08-13 14:58:33]

I believe this is the correct way to model this sort of
situation in GN, but I'm not sure. What do you think?

[jamesr, 2014-08-13 17:31:33]

This is backwards.  //cc will eventually depend on surfaces and not the other
way around

[jbroman, 2014-08-13 17:35:41]

On 2014/08/13 17:31:33, jamesr wrote:
> This is backwards.  //cc will eventually depend on surfaces and not the other
> way around

Hmm, okay. I was looking only at how the dependency graph is today.

It sounds like you want me to drop this and leave the check errors be until the
dependencies are inverted, so I'll just close this CL.

[jamesr, 2014-08-13 17:37:49]

For the //cc part, yes. The gles2_interface change looks good though, could you
separate that bit out?

[jbroman, 2014-08-13 17:53:01]

Changed to only do this for the :gles2_interface target.

[jamesr, 2014-08-13 17:55:01]

Can you update the patch description?

https://codereview.chromium.org/464153002/diff/20001/gpu/command_buffer/clien...
File gpu/command_buffer/client/BUILD.gn (right):

https://codereview.chromium.org/464153002/diff/20001/gpu/command_buffer/clien...
gpu/command_buffer/client/BUILD.gn:99: all_dependent_configs = [
"//third_party/khronos:khronos_headers" ]
can you make this direct_dependent_configs? not everything that lives downstream
of a target that uses GL should have the khronos include paths. if you have a
target that depends on gles2_interface and exposes #includes in its public
headers that need the khronos paths to work, use forward_dependent_configs_from
= [...] in that target

[jbroman, 2014-08-13 19:27:16]

I think I've done what you've asked. It's a little thorny, especially because we
don't have a clear distinction between public and private headers AFAICT.

https://codereview.chromium.org/464153002/diff/20001/gpu/command_buffer/clien...
File gpu/command_buffer/client/BUILD.gn (right):

https://codereview.chromium.org/464153002/diff/20001/gpu/command_buffer/clien...
gpu/command_buffer/client/BUILD.gn:99: all_dependent_configs = [
"//third_party/khronos:khronos_headers" ]
On 2014/08/13 17:55:01, jamesr wrote:
> can you make this direct_dependent_configs? not everything that lives
downstream
> of a target that uses GL should have the khronos include paths. if you have a
> target that depends on gles2_interface and exposes #includes in its public
> headers that need the khronos paths to work, use
forward_dependent_configs_from
> = [...] in that target

There are a lot of other places that put this in all_dependent_configs, and
converting them all into direct_dependent_configs (with forwarding) seems like a
larger task.

How does "not making the problem any worse" (by using direct_dependent_configs
on this target, and leaving the existing ones for now) sound?

https://codereview.chromium.org/464153002/diff/80001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/464153002/diff/80001/content/common/BUILD.gn#...
content/common/BUILD.gn:31: "//gpu/command_buffer/client:gles2_interface",
Used in gl_helper.h. This doesn't seem to be a public header (though we don't
seem to have an explicit notion of a public vs private header), so I haven't
forwarded its configs here.

https://codereview.chromium.org/464153002/diff/100001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/464153002/diff/100001/cc/BUILD.gn#newcode638
cc/BUILD.gn:638: "//gpu:test_support",
test_context_provider.h includes gles2_interface_stub.h from //gpu:test_support.

https://codereview.chromium.org/464153002/diff/100001/cc/BUILD.gn#newcode772
cc/BUILD.gn:772: "//gpu/command_buffer/client:gles2_interface",
//cc doesn't forward configs for this, and I believe it shouldn't (since it's
not used in any cc headers). Adding explicit dependency here, since
renderer_pixeltest.cc uses gles2_interface.h.

https://codereview.chromium.org/464153002/diff/100001/gpu/BUILD.gn
File gpu/BUILD.gn (right):

https://codereview.chromium.org/464153002/diff/100001/gpu/BUILD.gn#newcode60
gpu/BUILD.gn:60: "//gpu/command_buffer/client:gles2_interface",
used in gles2_interface_stub.h, which is public (used in
texture_uploader_unittest.cc, for instance)

https://codereview.chromium.org/464153002/diff/100001/gpu/BUILD.gn#newcode66
gpu/BUILD.gn:66: "//gpu/command_buffer/client:gles2_interface",
gles2_interface_stub.h includes gles2_interface.h

[jamesr, 2014-08-13 19:31:26]

Yeah, this seems better.  Cleaning up all the dependent configs is a big task,
for sure

[brettw, 2014-08-13 20:14:30]

lgtm

[jbroman, 2014-09-15 14:18:13]

this failed to get needed lgtms and probably doesn't apply anymore; I'm just
going to close this issue

[jamesr, 2014-09-15 15:32:44]

lgtm

[brettw, 2014-09-15 16:47:55]

lgtm

https://codereview.chromium.org/464153002/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/BUILD.gn (right):

https://codereview.chromium.org/464153002/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/BUILD.gn:97: source_set("gles2_interface") {
Can you add a comment here about what this target is for?

[jbroman, 2014-09-15 20:20:41]

added a few more places that newly depend on gles2_interface.h and were causing
'gn check' complaints; will seek additional reviewers to cover gpu/ and
ui/aura/.

https://codereview.chromium.org/464153002/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/BUILD.gn (right):

https://codereview.chromium.org/464153002/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/BUILD.gn:97: source_set("gles2_interface") {
On 2014/09/15 16:47:55, brettw wrote:
> Can you add a comment here about what this target is for?

Done.

[jbroman, 2014-09-15 20:24:37]

jbroman@chromium.org changed reviewers:
+ piman@chromium.org, sky@chromium.org

[jbroman, 2014-09-15 20:24:38]

+piman for gpu/
+ski for ui/aura/

[piman, 2014-09-15 21:20:35]

lgtm

[sky, 2014-09-15 22:25:27]

LGTM

[jbroman, 2014-09-15 23:27:06]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-09-15 23:32:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/464153002/140001

[commit-bot: I haz the power, 2014-09-16 01:05:25]

Committed patchset #8 (id:140001) as d8e3deeabfdc2678a2fbcc15ec31fce3543a45e5

[commit-bot: I haz the power, 2014-09-16 01:06:58]

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