Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184433003:
Prefer Khronos EGL headers instead Mesa for all OS (Closed)
|
Created:
6 years, 9 months ago by vignatti (out of this project) Modified:
6 years, 9 months ago CC:
chromium-reviews, ozone-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPrefer Khronos EGL headers instead Mesa for all OS
This patch change the order that gl target reads Mesa headers, giving
precedence to third_party/khronos over third_party/mesa/src/include.
BUG=266310
TEST=locally on Linux only
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255915
Patch Set 1 #
Total comments: 2
Messages
Total messages: 19 (0 generated)
PTAL.
so this looks reasonable but I'm not comfortable reviewing. And please run a full set of try jobs...
On 2014/02/28 16:34:59, rjkroege wrote: > so this looks reasonable but I'm not comfortable reviewing. And please run a > full set of try jobs... This is the right thing to do IMO. I actually have the same change in my tree. If we don't do it, we can pick up system headers in some files and this can lead to type errors because we redefine the native EGL types to intptr_t.
The only way to know whether this change will work is to build it on all platforms. I sent it to the win, mac and linux GPU try bots. Have you built and tested this change on Android?
https://codereview.chromium.org/184433003/diff/1/ui/gl/gl.gyp File ui/gl/gl.gyp (right): https://codereview.chromium.org/184433003/diff/1/ui/gl/gl.gyp#newcode33 ui/gl/gl.gyp:33: '<(DEPTH)/third_party/mesa/src/include', Can we remove mesa here? We already specify the explicit path where we need additional headers, such as in gl_surface_x11.cc: #include "third_party/mesa/src/include/GL/osmesa.h"
https://codereview.chromium.org/184433003/diff/1/ui/gl/gl.gyp File ui/gl/gl.gyp (right): https://codereview.chromium.org/184433003/diff/1/ui/gl/gl.gyp#newcode33 ui/gl/gl.gyp:33: '<(DEPTH)/third_party/mesa/src/include', On 2014/02/28 20:53:26, sievers wrote: > Can we remove mesa here? > > We already specify the explicit path where we need additional headers, such as > in gl_surface_x11.cc: > #include "third_party/mesa/src/include/GL/osmesa.h" I don't know -- a try job would be needed. In general I think the answer is no, though. The OpenGL and OpenGL ES headers themselves refer to paths like <GL/gl.h> and <KHR/khrplatform.h>.
On 2014/02/28 16:34:59, rjkroege wrote: > so this looks reasonable but I'm not comfortable reviewing. And please run a > full set of try jobs... I've tried to run a full set of tests on try bots but now all of them are resulting as red for me. And these red icons don't link anywhere so I'm not sure what's going on. rjkroege@, I *think* I don't have access on trybots. Do you mind do nominate me?
On 2014/03/03 09:29:59, vignatti wrote: > On 2014/02/28 16:34:59, rjkroege wrote: > > so this looks reasonable but I'm not comfortable reviewing. And please run a > > full set of try jobs... Look pretty green to me... The red ones are either flake or permissions issues (due to no trybot access). > > I've tried to run a full set of tests on try bots but now all of them are > resulting as red for me. And these red icons don't link anywhere so I'm not sure > what's going on. > > rjkroege@, I *think* I don't have access on trybots. Do you mind do nominate me?
On 2014/03/05 14:39:34, spang wrote: > On 2014/03/03 09:29:59, vignatti wrote: > > On 2014/02/28 16:34:59, rjkroege wrote: > > > so this looks reasonable but I'm not comfortable reviewing. And please run a > > > full set of try jobs... > > Look pretty green to me... The red ones are either flake or permissions issues > (due to no trybot access). yeah, access for me on trybots was the main problem before... The bots now look pretty green for me as well, with a few reds only that don't seem to be related to this CL. kbr@ wdyt?
On 2014/03/05 16:01:25, vignatti wrote: > On 2014/03/05 14:39:34, spang wrote: > > On 2014/03/03 09:29:59, vignatti wrote: > > > On 2014/02/28 16:34:59, rjkroege wrote: > > > > so this looks reasonable but I'm not comfortable reviewing. And please run > a > > > > full set of try jobs... > > > > Look pretty green to me... The red ones are either flake or permissions issues > > (due to no trybot access). > > yeah, access for me on trybots was the main problem before... > > The bots now look pretty green for me as well, with a few reds only that don't > seem to be related to this CL. kbr@ wdyt? If you have try server access now then please re-send it to the try servers. I just spent ~5 minutes trying to click all the boxes to simulate a try job only to have codereview.chromium.org report an error trying to update the page. Without seeing green try jobs I can't review this with confidence.
On 2014/03/05 23:34:06, Ken Russell wrote: > On 2014/03/05 16:01:25, vignatti wrote: > > On 2014/03/05 14:39:34, spang wrote: > > > On 2014/03/03 09:29:59, vignatti wrote: > > > > On 2014/02/28 16:34:59, rjkroege wrote: > > > > > so this looks reasonable but I'm not comfortable reviewing. And please > run > > a > > > > > full set of try jobs... > > > > > > Look pretty green to me... The red ones are either flake or permissions > issues > > > (due to no trybot access). > > > > yeah, access for me on trybots was the main problem before... > > > > The bots now look pretty green for me as well, with a few reds only that don't > > seem to be related to this CL. kbr@ wdyt? > > If you have try server access now then please re-send it to the try servers. I > just spent ~5 minutes trying to click all the boxes to simulate a try job only > to have http://codereview.chromium.org report an error trying to update the page. > Without seeing green try jobs I can't review this with confidence. We're still waiting on someone to create an account. I've run new jobs (again) by patching in the CL and doing "git cl try".
On 2014/03/06 16:07:28, spang wrote: > On 2014/03/05 23:34:06, Ken Russell wrote: > > On 2014/03/05 16:01:25, vignatti wrote: > > > On 2014/03/05 14:39:34, spang wrote: > > > > On 2014/03/03 09:29:59, vignatti wrote: > > > > > On 2014/02/28 16:34:59, rjkroege wrote: > > > > > > so this looks reasonable but I'm not comfortable reviewing. And please > > run > > > a > > > > > > full set of try jobs... > > > > > > > > Look pretty green to me... The red ones are either flake or permissions > > issues > > > > (due to no trybot access). > > > > > > yeah, access for me on trybots was the main problem before... > > > > > > The bots now look pretty green for me as well, with a few reds only that > don't > > > seem to be related to this CL. kbr@ wdyt? > > > > If you have try server access now then please re-send it to the try servers. I > > just spent ~5 minutes trying to click all the boxes to simulate a try job only > > to have http://codereview.chromium.org report an error trying to update the > page. > > Without seeing green try jobs I can't review this with confidence. > > We're still waiting on someone to create an account. > > I've run new jobs (again) by patching in the CL and doing "git cl try". Great, thanks. The try jobs look good. LGTM as is. If you want to try to remove the Mesa headers, that would require a new set of try jobs.
The CQ bit was checked by spang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiago.vignatti@intel.com/184433003/1
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiago.vignatti@intel.com/184433003/1
The CQ bit was unchecked by spang@chromium.org
The CQ bit was checked by tiago.vignatti@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiago.vignatti@intel.com/184433003/1
Message was sent while issue was closed.
Change committed as 255915 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
