Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1322223003:
Fix support for internal_gles2_conform_test = true (Closed)
|
Created:
5 years, 3 months ago by Peter Mayo Modified:
5 years, 3 months ago CC:
chromium-reviews, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix support for internal_gles2_conform_test = true
The build file seems fairly approximate for this use case.
This flag is set by ChromeOS ebuild contexts in GYP.
BUG=527968
TEST=set flag in gn args, gn gen
Committed: https://crrev.com/81a6d587813badcf21de8bf771a9621e307d5ebb
Cr-Commit-Position: refs/heads/master@{#347511}
Patch Set 1 : Typo in name of output field #Patch Set 2 : Fix invocation of gypi #Patch Set 3 : fix typo in scope[d] #Patch Set 4 : Correct list nesting #Patch Set 5 : Fix path typo #Patch Set 6 : Fix local reference #Patch Set 7 : Reformat with gn #Patch Set 8 : Fix file references #Patch Set 9 : Fix reference to GTK #Patch Set 10 : Fix dangling reference to libnocheck #
Total comments: 6
Patch Set 11 : fetch/rebase #Patch Set 12 : Fix embedded data argument paths #Patch Set 13 : Fix typo #
Total comments: 1
Patch Set 14 : Enverbiage comment #Patch Set 15 : sync&rebase #Messages
Total messages: 34 (4 generated)
Fix invocation of gypi
fix typo in scope[d]
Correct list nesting
Fix path typo
Fix local reference
Reformat with gn
Fix file references
Fix reference to GTK
Fix dangling reference to libnocheck
petermayo@chromium.org changed reviewers: + dpranke@chromium.org, piman@chromium.org, sievers@chromium.org
PTAL dpranke for content/correctness piman|sievers for OWNERS
https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/BUILD.gn (right): https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... gpu/gles2_conform_support/BUILD.gn:82: #"//gpu/command_buffer/client:gles2_c_libnocheck", Should this be removed? https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... gpu/gles2_conform_support/BUILD.gn:190: #data_deps = [ "//third_party/gles2_conform/GTF_ES/" ] Should this be removed?
dpranke@chromium.org changed reviewers: + hendrikw@chromium.org
+hendrikw, who has been struggling w/ this target in the past couple days as well. https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/BUILD.gn (right): https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... gpu/gles2_conform_support/BUILD.gn:87: deps += [ "//build/config/linux/gtk" ] the old thing didn't exist, but I don't think //build/config/linux/gtk exists, either. I think you need either gtk2 or gtk3 ?
On 2015/09/03 20:28:47, Dirk Pranke wrote: > +hendrikw, who has been struggling w/ this target in the past couple days as > well. > > https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... > File gpu/gles2_conform_support/BUILD.gn (right): > > https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... > gpu/gles2_conform_support/BUILD.gn:87: deps += [ "//build/config/linux/gtk" ] > the old thing didn't exist, but I don't think //build/config/linux/gtk exists, > either. I think you need either gtk2 or gtk3 ? Hmm, trial and error says it is on my client, I'll re-evaluate when I sync next, but I want to dump the consistent stack of fixes first.
fetch/rebase
https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/BUILD.gn (right): https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... gpu/gles2_conform_support/BUILD.gn:82: #"//gpu/command_buffer/client:gles2_c_libnocheck", On 2015/09/03 20:28:02, piman (OOO back 2015-08-31) wrote: > Should this be removed? Was already done ahead of my last sync - rebase caught it. https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... gpu/gles2_conform_support/BUILD.gn:87: deps += [ "//build/config/linux/gtk" ] On 2015/09/03 20:28:47, Dirk Pranke wrote: > the old thing didn't exist, but I don't think //build/config/linux/gtk exists, > either. I think you need either gtk2 or gtk3 ? head moved to gtk2, rebased accordingly. https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... gpu/gles2_conform_support/BUILD.gn:190: #data_deps = [ "//third_party/gles2_conform/GTF_ES/" ] On 2015/09/03 20:28:02, piman (OOO back 2015-08-31) wrote: > Should this be removed? I don't know why it was there in the first place. Is there set of test data still to migrate here, or is it really no longer required? Dirk?
On 2015/09/03 21:13:35, Peter Mayo wrote: > On 2015/09/03 20:28:02, piman (OOO back 2015-08-31) wrote: > > Should this be removed? > > I don't know why it was there in the first place. Is there set of test data > still to migrate here, or is it really no longer required? Dirk? I believe there is an internal set of test data that still needs to be ported over, but I'm not positive. I would leave it in for now.
Fix embedded data argument paths
Fix typo
lgtm
On 2015/09/04 18:35:22, Dirk Pranke wrote: > lgtm lgtm with nit however, I'm not an owner @piman or @sievers, Please take another look, thanks!
https://codereview.chromium.org/1322223003/diff/230001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/BUILD.gn (right): https://codereview.chromium.org/1322223003/diff/230001/gpu/gles2_conform_supp... gpu/gles2_conform_support/BUILD.gn:191: #data_deps = [ "//third_party/gles2_conform/GTF_ES/" ] nit: should this be removed?
On 2015/09/04 18:43:56, hendrikw wrote: > https://codereview.chromium.org/1322223003/diff/230001/gpu/gles2_conform_supp... > File gpu/gles2_conform_support/BUILD.gn (right): > > https://codereview.chromium.org/1322223003/diff/230001/gpu/gles2_conform_supp... > gpu/gles2_conform_support/BUILD.gn:191: #data_deps = [ > "//third_party/gles2_conform/GTF_ES/" ] > nit: should this be removed? Addressed in review at patch set 10: https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... Dirk believes there is an open set of internal data reference still to be migrated into there, hence the dangling comment has marginal placeholder value.
On Fri, Sep 4, 2015 at 12:11 PM, <petermayo@chromium.org> wrote: > On 2015/09/04 18:43:56, hendrikw wrote: > > > https://codereview.chromium.org/1322223003/diff/230001/gpu/gles2_conform_supp... > >> File gpu/gles2_conform_support/BUILD.gn (right): >> > > > > https://codereview.chromium.org/1322223003/diff/230001/gpu/gles2_conform_supp... > >> gpu/gles2_conform_support/BUILD.gn:191: #data_deps = [ >> "//third_party/gles2_conform/GTF_ES/" ] >> nit: should this be removed? >> > > Addressed in review at patch set 10: > > https://codereview.chromium.org/1322223003/diff/170001/gpu/gles2_conform_supp... > > Dirk believes there is an open set of internal data reference still to be > migrated into there, hence the dangling comment has marginal placeholder > value. > Please avoid commented rules for no reason. The next person looking at / editing this file will have no idea why this is there. Either remove the line, or add a comment/TODO about why this is here and what it's for. > https://codereview.chromium.org/1322223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Enverbiage comment
On 2015/09/04 19:16:37, piman (slow to review) wrote: > > Please avoid commented rules for no reason. The next person looking at / > editing this file will have no idea why this is there. > Either remove the line, or add a comment/TODO about why this is here and > what it's for. Done.
sync&rebase
lgtm
The CQ bit was checked by petermayo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, hendrikw@chromium.org Link to the patchset: https://codereview.chromium.org/1322223003/#ps270001 (title: "sync&rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1322223003/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322223003/270001
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/81a6d587813badcf21de8bf771a9621e307d5ebb Cr-Commit-Position: refs/heads/master@{#347511} |
