GN: Fix up various gpu targets

GN: Fix up various gpu targets

This makes the gl_tests and gpu_unittests targets link and run and fixes
up a few other misc issues:

*) outdated nss filelists and defines
*) outdated cc file lists

R=brettw@chromium.org, piman@chromium.org
TBR=ben@chromium.org for ui/events/ change

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277958

[jamesr, 2014-06-16 06:43:45]

R=brettw for GN stuff, piman for logic and owners

https://codereview.chromium.org/338633004/diff/1/gpu/BUILD.gn
File gpu/BUILD.gn (right):

https://codereview.chromium.org/338633004/diff/1/gpu/BUILD.gn#newcode66
gpu/BUILD.gn:66: # TODO: This is copied in command_buffer/client/BUILD.gn
this isn't ideal - i suppose i could have a
command_buffer/client/gles2_c_lib.gni - but it's just 5 files so that seems a
bit overkill

https://codereview.chromium.org/338633004/diff/1/gpu/BUILD.gn#newcode102
gpu/BUILD.gn:102: sources += gles2_c_lib_source_files
brett - is there a better syntax for this?

[brettw, 2014-06-16 16:35:01]

https://codereview.chromium.org/338633004/diff/1/gpu/BUILD.gn
File gpu/BUILD.gn (right):

https://codereview.chromium.org/338633004/diff/1/gpu/BUILD.gn#newcode66
gpu/BUILD.gn:66: # TODO: This is copied in command_buffer/client/BUILD.gn
On 2014/06/16 06:43:45, jamesr wrote:
> this isn't ideal - i suppose i could have a
> command_buffer/client/gles2_c_lib.gni - but it's just 5 files so that seems a
> bit overkill

It's not clear to me why it has to be this way. Maybe piman can answer. Can we
just depend on gles2_c_lib? The only difference I can see is that this target
defines GL_GLEXT_PROTOTYPES, the the only use of that in these files is to
unconditionally define it in gles2_c_lib.cc.

[piman, 2014-06-16 19:17:44]

https://codereview.chromium.org/338633004/diff/1/gpu/BUILD.gn
File gpu/BUILD.gn (right):

https://codereview.chromium.org/338633004/diff/1/gpu/BUILD.gn#newcode66
gpu/BUILD.gn:66: # TODO: This is copied in command_buffer/client/BUILD.gn
On 2014/06/16 16:35:01, brettw wrote:
> On 2014/06/16 06:43:45, jamesr wrote:
> > this isn't ideal - i suppose i could have a
> > command_buffer/client/gles2_c_lib.gni - but it's just 5 files so that seems
a
> > bit overkill
> 
> It's not clear to me why it has to be this way. Maybe piman can answer. Can we
> just depend on gles2_c_lib? The only difference I can see is that this target
> defines GL_GLEXT_PROTOTYPES, the the only use of that in these files is to
> unconditionally define it in gles2_c_lib.cc.

Not sure why gman did it that way. I'm fine if we depend on gles2_c_lib.

[piman, 2014-06-16 19:18:32]

lgtm

https://codereview.chromium.org/338633004/diff/1/gpu/command_buffer/client/BU...
File gpu/command_buffer/client/BUILD.gn (right):

https://codereview.chromium.org/338633004/diff/1/gpu/command_buffer/client/BU...
gpu/command_buffer/client/BUILD.gn:162: if (is_win) {
nit: indent?

[brettw, 2014-06-16 19:32:18]

On 2014/06/16 19:17:44, piman wrote:
> Not sure why gman did it that way. I'm fine if we depend on gles2_c_lib.

You mentioning gman reminded me of why all these files were weird (I spent a
while digging when converting some previous targets). It was trying to prevent a
circular dependency in .gyp files (even though the targets were not circularly
dependent). Some things ended up being split in weird ways, and GN doesn't have
this problem.

So we should just add that dep and remove the duplicated files.

[jamesr, 2014-06-16 19:39:17]

Two complications:

1.) //gpu/command_buffer/client:gles2_c_lib depends on :gles2_implementation.
//gpu:gl_tests depends on
//gpu/command_buffer/client:gles2_implementation_client_side_arrays.  The
different flavors of gles2_implementation define a lot of the same symbols so I
can't depend on both in the same binary.  I could make another target that is
the gles2_c_lib without the dep on gles2_implementation, or possibly remove that
dep and make everyone that depends on gles2_c_lib pick the gles2_implementation
that they want to use

2.) To compile the gles2_c_lib source files I need include paths that come from
other targets gl_tests depend on (like third_party/mesa/src/include from the
//ui/gl dependency).  Maybe gles2_c_lib should just depend on this?  I'm not
sure if it always wants to use the mesa headers or if it should be using the
khronos ones sometime

Assuming I fix one or both of these, should I try to make the same changes in
the gyp files?

[brettw, 2014-06-16 20:19:11]

On 2014/06/16 19:39:17, jamesr wrote:
> Two complications:
> 
> 1.) //gpu/command_buffer/client:gles2_c_lib depends on :gles2_implementation.
> //gpu:gl_tests depends on
> //gpu/command_buffer/client:gles2_implementation_client_side_arrays.  The
> different flavors of gles2_implementation define a lot of the same symbols so
I
> can't depend on both in the same binary.  I could make another target that is
> the gles2_c_lib without the dep on gles2_implementation, or possibly remove
that
> dep and make everyone that depends on gles2_c_lib pick the
gles2_implementation
> that they want to use

There are a fair number of users of gles2_c_lib so it's nice to keep this
including the default implementation. Maybe make a source set with this called
gles2_c_stub that has these files, and make *c_lib = *stub + *implementation.



> 2.) To compile the gles2_c_lib source files I need include paths that come
from
> other targets gl_tests depend on (like third_party/mesa/src/include from the
> //ui/gl dependency).  Maybe gles2_c_lib should just depend on this?  I'm not
> sure if it always wants to use the mesa headers or if it should be using the
> khronos ones sometime

Piman might have to answer this.


> Assuming I fix one or both of these, should I try to make the same changes in
> the gyp files?

Personally, I say don't bother.

[brettw, 2014-06-16 20:19:12]

On 2014/06/16 19:39:17, jamesr wrote:
> Two complications:
> 
> 1.) //gpu/command_buffer/client:gles2_c_lib depends on :gles2_implementation.
> //gpu:gl_tests depends on
> //gpu/command_buffer/client:gles2_implementation_client_side_arrays.  The
> different flavors of gles2_implementation define a lot of the same symbols so
I
> can't depend on both in the same binary.  I could make another target that is
> the gles2_c_lib without the dep on gles2_implementation, or possibly remove
that
> dep and make everyone that depends on gles2_c_lib pick the
gles2_implementation
> that they want to use

There are a fair number of users of gles2_c_lib so it's nice to keep this
including the default implementation. Maybe make a source set with this called
gles2_c_stub that has these files, and make *c_lib = *stub + *implementation.



> 2.) To compile the gles2_c_lib source files I need include paths that come
from
> other targets gl_tests depend on (like third_party/mesa/src/include from the
> //ui/gl dependency).  Maybe gles2_c_lib should just depend on this?  I'm not
> sure if it always wants to use the mesa headers or if it should be using the
> khronos ones sometime

Piman might have to answer this.


> Assuming I fix one or both of these, should I try to make the same changes in
> the gyp files?

Personally, I say don't bother.

[jamesr, 2014-06-16 22:03:35]

Antoine suggests these probably shouldn't use the mesa headers at all.  I'll
experiment a bit with simplifying this in the gyp build and, if that's
plausible, update this to match.

[jamesr, 2014-06-17 05:08:07]

We appear to only have the desktop GL/glext.h from mesa in our repo (even though
khronos.org appears to have a fine glext.h) and we need it to see the definition
of glBeginQueryARB.  It seems pretty reasonable for the service-side targets to
use the mesa headers if they are the only desktop GL headers we have.  Maybe we
should use the khronos desktop GL headers and only use the mesa ones when
actually talking to osmesa.  ¯\_(ツ)_/¯

Regardless, the basic issue with gles2_c_lib is that the source files are used
to bind to gles2_implementation (aka the client side of our command buffer) and
to bind to other gles2 implementations (like the
gles2_implementation_client_side_arrays variant).  However, since everything
that depends on gles2_c_lib already declares a dep on something else, this just
drops the gles2_c_lib -> gles2_implementation dependency.  In theory gles2_c_lib
could be composed with any target that provided an appropriate GLES2Interface
implementation.

PTAL

[piman, 2014-06-17 05:20:33]

On 2014/06/17 05:08:07, jamesr wrote:
> We appear to only have the desktop GL/glext.h from mesa in our repo (even
though
> http://khronos.org appears to have a fine glext.h) and we need it to see the
definition
> of glBeginQueryARB.

Ah, yeah, it looks like we only have the GLES2 headers in third_party/khronos. I
think it's perfectly reasonable to also get the GL heders from there too though.

>  It seems pretty reasonable for the service-side targets to
> use the mesa headers if they are the only desktop GL headers we have.  Maybe
we
> should use the khronos desktop GL headers and only use the mesa ones when
> actually talking to osmesa.  ¯\_(ツ)_/¯

Agreed.

> 
> Regardless, the basic issue with gles2_c_lib is that the source files are used
> to bind to gles2_implementation (aka the client side of our command buffer)
and
> to bind to other gles2 implementations (like the
> gles2_implementation_client_side_arrays variant).  However, since everything
> that depends on gles2_c_lib already declares a dep on something else, this
just
> drops the gles2_c_lib -> gles2_implementation dependency.  In theory
gles2_c_lib
> could be composed with any target that provided an appropriate GLES2Interface
> implementation.
> 
> PTAL


LGTM

[jamesr, 2014-06-17 05:26:20]

I can't figure out how to upload a patch relative to the libjpeg_turbo one, oh
well.

filed https://code.google.com/p/chromium/issues/detail?id=385476 about the
khronos/mesa header thing.  There's almost certainly some history here I'm
missing but we can figure it out there.

[jamesr, 2014-06-17 18:21:53]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-06-17 18:23:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/338633004/80001

[commit-bot: I haz the power, 2014-06-17 21:46:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 21:46:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[jamesr, 2014-06-18 00:55:21]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-06-18 00:58:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/338633004/80001

[Ben Goodger (Google), 2014-06-18 02:54:42]

lgtm

[commit-bot: I haz the power, 2014-06-18 07:01:12]

Change committed as 277958