Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031063002:
gn: Add all v8 targets to gn_all (Closed)
|
Created:
4 years, 6 months ago by Michael Achenbach Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongn: Add all v8 targets to gn_all
BUG=chromium:474921
Committed: https://crrev.com/640e53da9f25de7218ddc2cf5a0c7592f1c60810
Cr-Commit-Position: refs/heads/master@{#397995}
Patch Set 1 #Patch Set 2 : mark gn_visibility testonly #
Total comments: 3
Patch Set 3 : Review #Messages
Total messages: 21 (9 generated)
Description was changed from ========== gn: Add all v8 targets to gn_all BUG= ========== to ========== gn: Add all v8 targets to gn_all BUG=chromium:474921 ==========
machenbach@chromium.org changed reviewers: + brucedawson@chromium.org, dpranke@chromium.org, jochen@chromium.org
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031063002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2031063002/20001
PTAL. This is for catching v8 compilation issues early. Before, only some bots that use "all" were compiling all v8 targets. https://codereview.chromium.org/2031063002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2031063002/diff/20001/BUILD.gn#newcode787 BUILD.gn:787: testonly = true Not sure about marking this testonly. If I don't I need to put v8:gn_all somewhere else as it's testonly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2031063002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2031063002/diff/20001/BUILD.gn#newcode787 BUILD.gn:787: testonly = true On 2016/06/03 15:17:17, Michael Achenbach wrote: > Not sure about marking this testonly. If I don't I need to put v8:gn_all > somewhere else as it's testonly. Marking this testonly is probably fine. https://codereview.chromium.org/2031063002/diff/20001/BUILD.gn#newcode797 BUILD.gn:797: "//v8:gn_all", This almost certainly shouldn't be part of "gn_visibility", since the purpose of this target it to reference only targets that are not otherwise visibile, but are pulled in by "all". I think this should probably be a dependency of either ":gn_all", or ":both_gn_and_gyp", or ":gn_only". Ideally it should be one of the latter two, depending on whether it references targets that exist in both or targets that exist only in GN. If it references a mixture, then it's fine to use gn_all instead.
On 2016/06/03 18:14:55, Dirk Pranke wrote: > https://codereview.chromium.org/2031063002/diff/20001/BUILD.gn > File BUILD.gn (right): > > https://codereview.chromium.org/2031063002/diff/20001/BUILD.gn#newcode787 > BUILD.gn:787: testonly = true > On 2016/06/03 15:17:17, Michael Achenbach wrote: > > Not sure about marking this testonly. If I don't I need to put v8:gn_all > > somewhere else as it's testonly. > > Marking this testonly is probably fine. > > https://codereview.chromium.org/2031063002/diff/20001/BUILD.gn#newcode797 > BUILD.gn:797: "//v8:gn_all", > This almost certainly shouldn't be part of "gn_visibility", since the purpose of > this target it to reference only targets that are not otherwise visibile, but > are pulled in by "all". > > I think this should probably be a dependency of either ":gn_all", or > ":both_gn_and_gyp", or ":gn_only". Ideally it should be one of the latter two, > depending on whether it references targets that exist in both or targets that > exist only in GN. Most exist in gyp and gn but were never built with gyp. Or has that nothing to do with it? Some targets have slightly different names than in gyp and maybe some inner nodes in the dependency tree are different. Does that rather justify gn_all directly? > If it references a mixture, then it's fine to use gn_all instead.
PTAL at patch 3
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031063002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031063002/40001
Message was sent while issue was closed.
Description was changed from ========== gn: Add all v8 targets to gn_all BUG=chromium:474921 ========== to ========== gn: Add all v8 targets to gn_all BUG=chromium:474921 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== gn: Add all v8 targets to gn_all BUG=chromium:474921 ========== to ========== gn: Add all v8 targets to gn_all BUG=chromium:474921 Committed: https://crrev.com/640e53da9f25de7218ddc2cf5a0c7592f1c60810 Cr-Commit-Position: refs/heads/master@{#397995} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/640e53da9f25de7218ddc2cf5a0c7592f1c60810 Cr-Commit-Position: refs/heads/master@{#397995} |
