Linux: use generated shim headers for system mesa.

Linux: use generated shim headers for system mesa.

BUG=161389, 165264

[greggman, 2012-12-28 05:35:19]

On 2012/12/28 01:25:50, Paweł Hajdan Jr. wrote:

Maybe I'm missing something. this seems like adding work for no good reason.
Adding a dependency we don't have control over, the system mesa which could be
at any random version, is just asking for trouble, conflicts and complications
down the road. Why do we want to burden chromium developers with these issues

Or am I not getting it?

[Paweł Hajdan Jr., 2012-12-28 16:51:17]

On 2012/12/28 05:35:19, greggman wrote:
> On 2012/12/28 01:25:50, Paweł Hajdan Jr. wrote:
> 
> Maybe I'm missing something. this seems like adding work for no good reason.
> Adding a dependency we don't have control over, the system mesa which could be
> at any random version, is just asking for trouble, conflicts and complications
> down the road. Why do we want to burden chromium developers with these issues
> 
> Or am I not getting it?

First, this doesn't change anything for Google Chrome (it would continue to use
bundled mesa). The switches are only for Linux distro packagers who create
Chromium packages. Then, the switches are not officially supported. Now the
issue is that Linux distro packagers really want to use system libraries:
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries .

Then the bug about doing this is open for over a month with many patches on the
way to that goal reviewed and committed, and I've been asking questions about
that on chromium-dev some time ago as well. I'm surprised you're surprised about
what's happening here. ;-)

Finally, if you want to be sure about the project policy, please refer to
http://www.chromium.org/developers/adding-3rd-party-libraries , especially "You
should provide a gyp option to use the system version of the library, if it is
packaged by Linux distributions."

[greggman, 2013-01-02 21:29:23]

On 2012/12/28 16:51:17, Paweł Hajdan Jr. wrote:
> On 2012/12/28 05:35:19, greggman wrote:
> > On 2012/12/28 01:25:50, Paweł Hajdan Jr. wrote:
> > 
> > Maybe I'm missing something. this seems like adding work for no good reason.
> > Adding a dependency we don't have control over, the system mesa which could
be
> > at any random version, is just asking for trouble, conflicts and
complications
> > down the road. Why do we want to burden chromium developers with these
issues
> > 
> > Or am I not getting it?
> 
> First, this doesn't change anything for Google Chrome (it would continue to
use
> bundled mesa). The switches are only for Linux distro packagers who create
> Chromium packages. Then, the switches are not officially supported. Now the
> issue is that Linux distro packagers really want to use system libraries:
> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries .
> 
> Then the bug about doing this is open for over a month with many patches on
the
> way to that goal reviewed and committed, and I've been asking questions about
> that on chromium-dev some time ago as well. I'm surprised you're surprised
about
> what's happening here. ;-)
> 
> Finally, if you want to be sure about the project policy, please refer to
> http://www.chromium.org/developers/adding-3rd-party-libraries , especially
"You
> should provide a gyp option to use the system version of the library, if it is
> packaged by Linux distributions."

This is more work for us. If people try to use the system libraries and
something breaks they will file bugs. Those bugs will need triage. The person
doing the triage will assign it to someone on the GPU team. Even if the GPU
team's response it "too bad for you, system libs unsupported" that's still time
spent both by the person doing triage and the gpu team.

[Paweł Hajdan Jr., 2013-01-07 17:15:38]

Alright, after some more time - what do you think?

Note that I'm open to technical feedback about the way it's implemented.
However, I'm pretty sure the use_system_mesa option will get to a working state.

[greggman, 2013-01-07 20:23:32]

https://codereview.chromium.org/11693007/diff/1/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/11693007/diff/1/ui/gl/generate_bindings.py#ne...
ui/gl/generate_bindings.py:1711: def ResolveHeader(header, use_system_mesa):
All the changes in this file seem like adding a burden to every other platform
just so Linux and use system headers.

Is there a way to make this work without editing this file?

[Paweł Hajdan Jr., 2013-01-07 21:20:04]

https://codereview.chromium.org/11693007/diff/1/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/11693007/diff/1/ui/gl/generate_bindings.py#ne...
ui/gl/generate_bindings.py:1711: def ResolveHeader(header, use_system_mesa):
On 2013/01/07 20:23:33, greggman wrote:
> All the changes in this file seem like adding a burden to every other platform
> just so Linux and use system headers.
> 
> Is there a way to make this work without editing this file?

No. This file is executed during the build process, and would otherwise refer to
the bundled headers instead of system headers (and the bundled headers may not
even exist at that time since they can be removed to ensure they are not used).

There might be some other approach to be used in modifying this file though. I'm
open to suggestions.

Note that such burden is often overestimated by people, seriously. You do not
need to test or support the use_system code paths. I do understand the concerns
about that though.

[greggman, 2013-01-07 21:32:22]

https://codereview.chromium.org/11693007/diff/1/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/11693007/diff/1/ui/gl/generate_bindings.py#ne...
ui/gl/generate_bindings.py:1711: def ResolveHeader(header, use_system_mesa):
On 2013/01/07 21:20:05, Paweł Hajdan Jr. wrote:
> On 2013/01/07 20:23:33, greggman wrote:
> > All the changes in this file seem like adding a burden to every other
platform
> > just so Linux and use system headers.
> > 
> > Is there a way to make this work without editing this file?
> 
> No. This file is executed during the build process, and would otherwise refer
to
> the bundled headers instead of system headers (and the bundled headers may not
> even exist at that time since they can be removed to ensure they are not
used).
> 
> There might be some other approach to be used in modifying this file though.
I'm
> open to suggestions.
> 
> Note that such burden is often overestimated by people, seriously. You do not
> need to test or support the use_system code paths. I do understand the
concerns
> about that though.

It's not just a burden. The code you added is the only code in that entire file
that's platform specific. In order to edit that file I have to understand what
it's doing. I shouldn't need to understand anything about use_system_mesa. the
word shouldn't even appear in this file.

One suggestion would be to find a way to remove the "../../third_party/khronos"
and "../third_party/mesa" from this file and update gl.gyp so it still compiles
and uses stuff from third_party/khronos and third_party/mesa by having the
correct include paths.

Then. In, in a separate CL, update gl.gyp so it can instead go through your shim
for the system mesa

How's that sound?

[Paweł Hajdan Jr., 2013-01-07 22:24:02]

On 2013/01/07 21:32:22, greggman wrote:
> One suggestion would be to find a way to remove the
"../../third_party/khronos"
> and "../third_party/mesa" from this file and update gl.gyp so it still
compiles
> and uses stuff from third_party/khronos and third_party/mesa by having the
> correct include paths.
> 
> Then. In, in a separate CL, update gl.gyp so it can instead go through your
shim
> for the system mesa
> 
> How's that sound?

I can push the logic out of the .py file to the .gyp file, totally fine with
that. Note that this doesn't seem to change much, only pushes the issue to a
different place (i.e. the gyp file). Now, by that logic, you'd need to
understand that to modify the gyp file...

Oh, and the ResolveHeader logic would still need to be in the .py file, or I
would need to push even more things to the .gyp. I'm not sure if that's optimal
either, but I can do.

WDYT?

[greggman, 2013-01-08 20:26:07]

On 2013/01/07 22:24:02, Paweł Hajdan Jr. wrote:
> On 2013/01/07 21:32:22, greggman wrote:
> > One suggestion would be to find a way to remove the
> "../../third_party/khronos"
> > and "../third_party/mesa" from this file and update gl.gyp so it still
> compiles
> > and uses stuff from third_party/khronos and third_party/mesa by having the
> > correct include paths.
> > 
> > Then. In, in a separate CL, update gl.gyp so it can instead go through your
> shim
> > for the system mesa
> > 
> > How's that sound?
> 
> I can push the logic out of the .py file to the .gyp file, totally fine with
> that. Note that this doesn't seem to change much, only pushes the issue to a
> different place (i.e. the gyp file). Now, by that logic, you'd need to
> understand that to modify the gyp file...
> 
> Oh, and the ResolveHeader logic would still need to be in the .py file, or I
> would need to push even more things to the .gyp. I'm not sure if that's
optimal
> either, but I can do.
> 
> WDYT?

I'm not sure I understand why the ResolveHeader logic would be needed. If the
generate geneartes #include "gles2/gl2.h" instead of "#include
"../../third_party/khronos/gl2.h" then those files becomes exactly the same as
the files in other parts of the code that don't need any special handling.

As for .py vs .gyp there's already use_system_xxx stuff in the gyp files so
keeping that stuff in one place seems better than spreading it around

[Paweł Hajdan Jr., 2013-01-08 21:45:52]

On 2013/01/08 20:26:07, greggman wrote:
> I'm not sure I understand why the ResolveHeader logic would be needed. If the
> generate geneartes #include "gles2/gl2.h" instead of "#include
> "../../third_party/khronos/gl2.h" then those files becomes exactly the same as
> the files in other parts of the code that don't need any special handling.

If it was only used to write #includes, that would be fine.

However, the script also needs to print its inputs/outputs for gyp, and for that
it needs to know the exact paths. Furthermore, it actually reads the headers to
determine the generated output.

> As for .py vs .gyp there's already use_system_xxx stuff in the gyp files so
> keeping that stuff in one place seems better than spreading it around

[Paweł Hajdan Jr., 2013-01-10 00:26:23]

PTAL

[greggman, 2013-01-10 21:21:02]

https://codereview.chromium.org/11693007/diff/9002/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/11693007/diff/9002/gpu/GLES2/gl2extchromium.h...
gpu/GLES2/gl2extchromium.h:279: /* GL_ANGLE_instanced_arrays */
I don't understand this change. Why are we editing this file to use the system
mesa headers?

[Paweł Hajdan Jr., 2013-01-10 21:52:03]

https://codereview.chromium.org/11693007/diff/9002/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/11693007/diff/9002/gpu/GLES2/gl2extchromium.h...
gpu/GLES2/gl2extchromium.h:279: /* GL_ANGLE_instanced_arrays */
On 2013/01/10 21:21:02, greggman wrote:
> I don't understand this change. Why are we editing this file to use the system
> mesa headers?

System headers are buggy and do not contain "#define GL_ANGLE_instanced_arrays
1" line, confusing generate_bindings.py (in fact it just bails out with an
error).

[greggman, 2013-01-10 22:02:31]

On 2013/01/10 21:52:03, Paweł Hajdan Jr. wrote:
> https://codereview.chromium.org/11693007/diff/9002/gpu/GLES2/gl2extchromium.h
> File gpu/GLES2/gl2extchromium.h (right):
> 
>
https://codereview.chromium.org/11693007/diff/9002/gpu/GLES2/gl2extchromium.h...
> gpu/GLES2/gl2extchromium.h:279: /* GL_ANGLE_instanced_arrays */
> On 2013/01/10 21:21:02, greggman wrote:
> > I don't understand this change. Why are we editing this file to use the
system
> > mesa headers?
> 
> System headers are buggy and do not contain "#define GL_ANGLE_instanced_arrays
> 1" line, confusing generate_bindings.py (in fact it just bails out with an
> error).

Then don't use the system headers. The fact that the system headers are buggy is
precisely why we have headers checked into third party.

[Paweł Hajdan Jr., 2013-01-10 22:09:17]

On 2013/01/10 22:02:31, greggman wrote:
> Then don't use the system headers. The fact that the system headers are buggy
is
> precisely why we have headers checked into third party.

They just lag behind here for a Google (I think) extension. I wonder who sent
them a version with the bug...

We had the discussion about this change before. Technical feedback is fine. I
have responded to that. Let's not keep going in circles about whether to use the
system headers at all.

If in doubt, feel free to use IM or chat to reduce latency.

[greggman, 2013-01-10 22:12:47]

On 2013/01/10 22:09:17, Paweł Hajdan Jr. wrote:
> On 2013/01/10 22:02:31, greggman wrote:
> > Then don't use the system headers. The fact that the system headers are
buggy
> is
> > precisely why we have headers checked into third party.
> 
> They just lag behind here for a Google (I think) extension. I wonder who sent
> them a version with the bug...
> 
> We had the discussion about this change before. Technical feedback is fine. I
> have responded to that. Let's not keep going in circles about whether to use
the
> system headers at all.
> 
> If in doubt, feel free to use IM or chat to reduce latency.

We're not going around in circles. The guideline I was given is if there is
impact on chromium then it is unacceptable.

The changes in generate_bindings.py, gl2extchromium.h impact chromium IMO. If
you can remove them I'll approve. If you can't I won't approve.

[greggman, 2013-01-10 22:30:46]

On 2013/01/10 22:12:47, greggman wrote:
> On 2013/01/10 22:09:17, Paweł Hajdan Jr. wrote:
> > On 2013/01/10 22:02:31, greggman wrote:
> > > Then don't use the system headers. The fact that the system headers are
> buggy
> > is
> > > precisely why we have headers checked into third party.
> > 
> > They just lag behind here for a Google (I think) extension. I wonder who
sent
> > them a version with the bug...
> > 
> > We had the discussion about this change before. Technical feedback is fine.
I
> > have responded to that. Let's not keep going in circles about whether to use
> the
> > system headers at all.
> > 
> > If in doubt, feel free to use IM or chat to reduce latency.
> 
> We're not going around in circles. The guideline I was given is if there is
> impact on chromium then it is unacceptable.
> 
> The changes in generate_bindings.py, gl2extchromium.h impact chromium IMO. If
> you can remove them I'll approve. If you can't I won't approve.

Sorry, I missed the changes to generate_headers.py that made them non-linux
specific. I'm fine with the latest version. Thanks for making that more generic.

That just leaves gl2extchromium.h

I'm not sure why the changes to gl2extchromium.h are needed. The same functions
are included in third_party/khronos/GLES2/gl2ext.h. Is it just a matter of
making sure that file is always scanned?

[Paweł Hajdan Jr., 2013-01-11 00:04:51]

On 2013/01/10 22:30:46, greggman wrote:
> Sorry, I missed the changes to generate_headers.py that made them non-linux
> specific. I'm fine with the latest version. Thanks for making that more
generic.

OK
 
> That just leaves gl2extchromium.h

I removed it in latest patch set.

> I'm not sure why the changes to gl2extchromium.h are needed. The same
functions
> are included in third_party/khronos/GLES2/gl2ext.h. Is it just a matter of
> making sure that file is always scanned?

For reference, the third_party header would not be used with system mesa. I
think I can get it to work in some other way though.

[greggman, 2013-01-11 02:09:18]

https://codereview.chromium.org/11693007/diff/13010/tools/generate_shim_heade...
File tools/generate_shim_headers/generate_shim_headers.py (right):

https://codereview.chromium.org/11693007/diff/13010/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:30:
parser.add_option('--additional-headers-root', action='append', default=[])
why not just make --headers-root be action='append' and then specify it twice in
the gyp?

[Paweł Hajdan Jr., 2013-01-11 21:25:14]

PTAL

https://codereview.chromium.org/11693007/diff/13010/tools/generate_shim_heade...
File tools/generate_shim_headers/generate_shim_headers.py (right):

https://codereview.chromium.org/11693007/diff/13010/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:30:
parser.add_option('--additional-headers-root', action='append', default=[])
On 2013/01/11 02:09:18, greggman wrote:
> why not just make --headers-root be action='append' and then specify it twice
in
> the gyp?

Done.

[greggman, 2013-01-12 00:59:35]

lgtm

[greggman, 2013-01-12 01:00:05]

thank you for putting up with me

[Paweł Hajdan Jr., 2013-01-15 00:02:11]

Could you just take another quick look at the changes needed to get Windows to
work? They are in generate_bindings.py and gl.gyp (please look at changes
directly, not the changes from last patch set, since I had to rebase and the
latter contain unrelated changes from tree).

[greggman, 2013-01-15 01:02:11]

lgtm