Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1118863002:
Port Chromium's blink_tests target to GN. (Closed)
DescriptionPort Chromium's blink_tests target to GN.
Tested on Linux with the following command lines:
$ gn gen out-gn/Debug
$ ninja -C out-gn/Debug :blink_tests
Tested on Android with the following command lines:
$ gn gen out-gn-android/Debug --args='symbol_level=1 is_debug=true target_os="android" target_cpu="arm"'
$ ninja -C out-gn-android/Debug :blink_tests
BUG=483171
TEST=see above
R=dpranke@chromium.org
Committed: https://crrev.com/cb2638bc4343b8b4298e4878f3451ca410b02e47
Cr-Commit-Position: refs/heads/master@{#329340}
Patch Set 1 #
Total comments: 2
Patch Set 2 : host_toolchain #
Total comments: 4
Patch Set 3 : TODOs #Patch Set 4 : blink test binaries #
Total comments: 2
Patch Set 5 : do not depend directly in third_party/WebKit/public:blink_tests #
Total comments: 2
Patch Set 6 : add comment #
Total comments: 2
Patch Set 7 : Dirk's wording #
Total comments: 2
Patch Set 8 : depend on all_blink #
Total comments: 2
Patch Set 9 : add comment #Patch Set 10 : improve wording #
Total comments: 5
Patch Set 11 : Dirk's comment #Messages
Total messages: 47 (3 generated)
Not sure why I couldn't use 'blink_tests'. It required me ':blink_tests'.
dpranke@chromium.org changed reviewers: + brettw@chromium.org
> Not sure why I couldn't use 'blink_tests'. It required me ':blink_tests'. You're getting this because GN will not generate a top-level phony target for a group when there are multiple groups with the same base name (in this case //:blink_tests and //third_party/WebKit/public:blink_tests). The theory was that the phony target would be ambiguous and so it was better to not try to guess which one we wanted. However, looking at how we handle this in GYP, it looks like we generate a phony target that depends on *both* blink_tests, suggesting that it'll generate a target that depends on all targets w/ the same name. We might not want this behavior in GN, though, as we have cases where 'unit_tests' refers to lots of little source sets as well as the binary. Brett, thoughts? The easiest thing to do is to rename Blink's blink_tests target. I'd doubt that anyone uses it directly. https://codereview.chromium.org/1118863002/diff/1/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/1/BUILD.gn#newcode725 BUILD.gn:725: #'../breakpad/breakpad.gyp:dump_syms#host', why are all these deps commented out? most of them should exist ...
https://codereview.chromium.org/1118863002/diff/1/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/1/BUILD.gn#newcode725 BUILD.gn:725: #'../breakpad/breakpad.gyp:dump_syms#host', On 2015/04/30 22:42:27, Dirk Pranke wrote: > why are all these deps commented out? most of them should exist ... Does GN handles the #host part transparently? If not, how I translate that to GN? If yes, then I can just depend on //breakpad:dump_syms?
On 2015/04/30 22:54:25, tfarina wrote: > Does GN handles the #host part transparently? If not, how I translate that to > GN? No, not transparently. This is where 'toolchains' come into play. In particular, you want to use the 'host_toolchain' variable we define in //build/config/BUILDCONFIG.gn, so: deps = [ ... "//breakpad:dump_syms($host_toolchain)" ... ]
On 2015/04/30 22:59:30, Dirk Pranke wrote: > On 2015/04/30 22:54:25, tfarina wrote: > > Does GN handles the #host part transparently? If not, how I translate that to > > GN? > > No, not transparently. This is where 'toolchains' come into play. In particular, > you > want to use the 'host_toolchain' variable we define in > //build/config/BUILDCONFIG.gn, so: > > deps = [ > ... > "//breakpad:dump_syms($host_toolchain)" > ... > ] Done. PTAL.
mostly looks good, but I don't want to land this without fixing the underling 'blink_tests' collision one way or another. https://codereview.chromium.org/1118863002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/20001/BUILD.gn#newcode743 BUILD.gn:743: #'../content/content_shell_and_tests.gyp:layout_test_helper', maybe add TODO(GYP)'s for these? https://codereview.chromium.org/1118863002/diff/20001/BUILD.gn#newcode761 BUILD.gn:761: #'../breakpad/breakpad.gyp:dump_syms#host' I think you can delete line 761, right?
Dirk, do you want me to rename Blink's blink_tests target? If yes, to what name? If not, what is your suggestion for handling the name collision? PTAL. https://codereview.chromium.org/1118863002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/20001/BUILD.gn#newcode743 BUILD.gn:743: #'../content/content_shell_and_tests.gyp:layout_test_helper', On 2015/05/01 19:55:03, Dirk Pranke wrote: > maybe add TODO(GYP)'s for these? Done. https://codereview.chromium.org/1118863002/diff/20001/BUILD.gn#newcode761 BUILD.gn:761: #'../breakpad/breakpad.gyp:dump_syms#host' On 2015/05/01 19:55:03, Dirk Pranke wrote: > I think you can delete line 761, right? Done.
Maybe rename 'blink_tests' to 'blink_test_binaries'?
Dirk, could you take a look at patch set 4? It depends on https://codereview.chromium.org/1131763003/.
lgtm
https://codereview.chromium.org/1118863002/diff/60001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/60001/BUILD.gn#newcode135 BUILD.gn:135: "//third_party/WebKit/public:blink_tests", Dirk, is it fine to keep this here?
https://codereview.chromium.org/1118863002/diff/60001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/60001/BUILD.gn#newcode135 BUILD.gn:135: "//third_party/WebKit/public:blink_tests", On 2015/05/07 01:56:56, tfarina wrote: > Dirk, is it fine to keep this here? No, we should either list the underlying binaries or refer to :blink_tests instead.
On 2015/05/07 01:58:38, Dirk Pranke wrote: > https://codereview.chromium.org/1118863002/diff/60001/BUILD.gn > File BUILD.gn (right): > > https://codereview.chromium.org/1118863002/diff/60001/BUILD.gn#newcode135 > BUILD.gn:135: "//third_party/WebKit/public:blink_tests", > On 2015/05/07 01:56:56, tfarina wrote: > > Dirk, is it fine to keep this here? > > No, we should either list the underlying binaries or refer to :blink_tests > instead. To be clear: the end goal of this is to delete the //third_party/WebKit/public:blink_tests target, so we don't want to depend on it here.
Please, take another look at patch set 5.
lgtm
https://codereview.chromium.org/1118863002/diff/80001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/80001/BUILD.gn#newcode743 BUILD.gn:743: "//breakpad:dump_syms($host_toolchain)", I wouldn't expect any of these targets to be in the list of "tests" which it seems like this target is going for. I assume they're needed at runtime? If that's the case, they should be listed as data_deps on the targets that need them at runtime rather than here. Or is this something that the test infrastructure assumes exists when we build these tests?
lgtm https://codereview.chromium.org/1118863002/diff/80001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/80001/BUILD.gn#newcode743 BUILD.gn:743: "//breakpad:dump_syms($host_toolchain)", On 2015/05/07 16:56:23, brettw wrote: > I wouldn't expect any of these targets to be in the list of "tests" which it > seems like this target is going for. I assume they're needed at runtime? If > that's the case, they should be listed as data_deps on the targets that need > them at runtime rather than here. Or is this something that the test > infrastructure assumes exists when we build these tests? The latter (the test infrastructure wants them to exist).
On 2015/05/07 17:07:11, Dirk Pranke wrote: > lgtm > > https://codereview.chromium.org/1118863002/diff/80001/BUILD.gn > File BUILD.gn (right): > > https://codereview.chromium.org/1118863002/diff/80001/BUILD.gn#newcode743 > BUILD.gn:743: "//breakpad:dump_syms($host_toolchain)", > On 2015/05/07 16:56:23, brettw wrote: > > I wouldn't expect any of these targets to be in the list of "tests" which it > > seems like this target is going for. I assume they're needed at runtime? If > > that's the case, they should be listed as data_deps on the targets that need > > them at runtime rather than here. Or is this something that the test > > infrastructure assumes exists when we build these tests? > > The latter (the test infrastructure wants them to exist). Can we add a comment about this then?
Dirk, could you take another look and see if the comment I added is OK?
https://codereview.chromium.org/1118863002/diff/100001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/100001/BUILD.gn#newcode742 BUILD.gn:742: # Hence they are built here. The test stuff only cares about dump_syms and minidump_stackwalk. content_shell and content_shell_apk are needed regardless. Though, you could just replace the comment with "the following deps are needed to run the layout tests".
https://codereview.chromium.org/1118863002/diff/100001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/100001/BUILD.gn#newcode742 BUILD.gn:742: # Hence they are built here. On 2015/05/07 23:38:10, Dirk Pranke wrote: > The test stuff only cares about dump_syms and minidump_stackwalk. > > content_shell and content_shell_apk are needed regardless. > > Though, you could just replace the comment with "the following deps are needed > to run the layout tests". Done.
lgtm.
https://codereview.chromium.org/1118863002/diff/120001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/120001/BUILD.gn#newcode741 BUILD.gn:741: # NOTE: The following deps are needed to run the layout tests. If this comment is literally true, then the layout test targets themselves should have data_deps on these targets. And actually, I don't see layout tests in this target, it looks only like unit tests. My understanding was that the way the bots run these tests it expects these targets to exist. But if I do "ninja webkit_unit_tests" and then run the tests, they're not required. If this understanding is correct, then the comment should say "The bots expect the following targets to exist to run the test environment." or something. If particular tests have particular requirements, those requirements should be on those tests.
https://codereview.chromium.org/1118863002/diff/120001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/120001/BUILD.gn#newcode741 BUILD.gn:741: # NOTE: The following deps are needed to run the layout tests. My understanding is that this target (blink_tests) is built for when you run third_party/WebKit/Tools/Scripts/run-webkit-tests
On 2015/05/08 17:03:51, tfarina wrote: > https://codereview.chromium.org/1118863002/diff/120001/BUILD.gn > File BUILD.gn (right): > > https://codereview.chromium.org/1118863002/diff/120001/BUILD.gn#newcode741 > BUILD.gn:741: # NOTE: The following deps are needed to run the layout tests. > My understanding is that this target (blink_tests) is built for when you run > third_party/WebKit/Tools/Scripts/run-webkit-tests Yeah, but that doesn't really answer the question. Are these helper targets required by the test binaries if you run them manually? Or is it just something needed by other infrastructure that runs them in a certain way?
Brett, I think there is some confusion/misunderstanding going on. blink_tests target is not a target you build because of the individual test binary targets like heap_unittests, platform_unittests, wtf_unittests. You don't build blink_tests target because you want to run one of them. You build blink_tests because you want to run layout_tests (run-webkit_tests). I think the confusion arose because I listed these test targets individually, when I should have depended on all_blink target instead (which is what build/all.gyp:blink_tests does). For which I'm fixing in patch set 8. Dirk, Brett, ptal.
Actually, the point of "blink_tests" is to build all of the binaries needed to run all of the tests in Blink (i.e., it is a blink dev's main target). We do not currently have a build target for the layout tests, because the layout test harness is all python. We could create a dummy target for them, and we would eventually want one if/when we're able to run them through swarming. Brett, does that make more sense? If not, we can discuss it more when I'm back in the office on Monday. (The 'blink_tests' target has existed in gyp with this set of dependencies for a long time, for what it's worth).
In that case, we should have a comment above the target saying "This target is the group of things needed by the Python Blink test runner." or something.
Comment added. ptal
lgtm
On 2015/05/08 22:24:33, brettw wrote: > lgtm No lgtm. We shouldn't add that comment, because it's not accurate. There is no "Python Blink test runner". There is run-webkit-tests, which is what the comment I had him add earlier is referring to, and which every blink dev would've understood, but we can certainly add the exact script name if you want. The other test binaries are normal gtest tests. There is also test-webkitpy and a couple other test scripts (For things like the idl compiler), which don't need to be compiled but could be confused by "Python Blink test runner". If you want to add a comment where Thiago added his, it should say something like "these are all of the binaries needed to run all of the tests for Blink", but frankly, that's what "blink_tests" says to me, so it feels redundant. If you still disagree, let's discuss this on Monday so that we can stop wasting Thiago's time on the back-and-forth.
On 2015/05/08 23:11:56, Dirk Pranke wrote: > On 2015/05/08 22:24:33, brettw wrote: > > lgtm > > No lgtm. > > We shouldn't add that comment, because it's not accurate. > > There is no "Python Blink test runner". There is run-webkit-tests, which is what > the comment I had him add earlier is referring to, and which every blink dev > would've understood, but we can certainly add the exact script name if you want. > > The other test binaries are normal gtest tests. There is also test-webkitpy and > a couple other test scripts (For things like the idl compiler), which don't need > to be compiled but could be confused by "Python Blink test runner". > > If you want to add a comment where Thiago added his, it should say something > like "these are all of the binaries needed to run all of the tests for Blink", > but frankly, that's what "blink_tests" says to me, so it feels redundant. > > If you still disagree, let's discuss this on Monday so that we can stop wasting > Thiago's time on the back-and-forth. I don't care what it says precisely, but it needs to make clear the thing expecting these dependencies is external to the GN build (i.e. in Python). Otherwise, this target shouldn't exist.
On 2015/05/08 23:24:49, brettw wrote: > I don't care what it says precisely, but it needs to make clear the thing > expecting these dependencies is external to the GN build (i.e. in Python). > Otherwise, this target shouldn't exist. I don't know why you say that; there's certainly precedent for other roll-up groups in our build that collect all of the things that sub-teams care about?
Dirk, Brett, did you get a time to reach a consensus on what should go in the comment? I don't care much what goes in there. I just want to unblock this and move forward.
https://codereview.chromium.org/1118863002/diff/140001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/140001/BUILD.gn#newcode738 BUILD.gn:738: # NOTE: The following deps are needed to run the layout tests. Okay, Brett and I got a chance to confer, and his concern is that the comment in line 738 is not necessarily clearly to a non-Blink person, who may not know what "the layout tests" are or why they aren't just a GN target that could specify these dependencies directly. So, let's change this to: # NOTE: The following deps are needed to run the layout tests (run-webkit-tests) but there is no GN target for the layout tests, so we need to specify the dependencies here instead. or something like that.
https://codereview.chromium.org/1118863002/diff/140001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/140001/BUILD.gn#newcode738 BUILD.gn:738: # NOTE: The following deps are needed to run the layout tests. On 2015/05/11 21:15:11, Dirk Pranke wrote: > Okay, Brett and I got a chance to confer, and his concern is that the comment in > line 738 is not necessarily clearly to a non-Blink person, who may not know what > "the layout tests" are or why they aren't just a GN target that could specify > these dependencies directly. > > So, let's change this to: > > # NOTE: The following deps are needed to run the layout tests (run-webkit-tests) > but there is no GN target for the layout tests, so we need to specify the > dependencies here instead. > > or something like that. Done. https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn#newcode731 BUILD.gn:731: # This target is the group of things needed by the Python Blink test runner. What about this one? I think Brett also had concerns about it.
https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn#newcode731 BUILD.gn:731: # This target is the group of things needed by the Python Blink test runner. On 2015/05/11 22:37:04, tfarina wrote: > What about this one? I think Brett also had concerns about it. That is Brett's comment. I would delete it completely. If Brett still wants some sort of comment here, I would suggest: "This group includes all of the targets needed to build and test Blink, including running the layout tests (see below)" or something like that. But "Python Blink test runner" needs to go :) Brett?
LGTM with indicated change. https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn#newcode731 BUILD.gn:731: # This target is the group of things needed by the Python Blink test runner. The comment below is OK, so dirks comment with "see below" is OK with me.
https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn#newcode731 BUILD.gn:731: # This target is the group of things needed by the Python Blink test runner. On 2015/05/11 22:42:31, Dirk Pranke wrote: > On 2015/05/11 22:37:04, tfarina wrote: > > What about this one? I think Brett also had concerns about it. > > That is Brett's comment. I would delete it completely. If Brett still wants some > sort of comment here, I would suggest: > > "This group includes all of the targets needed to build and test Blink, > including running the layout tests (see below)" > > or something like that. > > But "Python Blink test runner" needs to go :) > > Brett? Done. https://codereview.chromium.org/1118863002/diff/180001/BUILD.gn#newcode731 BUILD.gn:731: # This target is the group of things needed by the Python Blink test runner. On 2015/05/12 00:17:50, brettw wrote: > The comment below is OK, so dirks comment with "see below" is OK with me. Done.
lgtm
The CQ bit was checked by tfarina@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1118863002/#ps200001 (title: "Dirk's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118863002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/cb2638bc4343b8b4298e4878f3451ca410b02e47 Cr-Commit-Position: refs/heads/master@{#329340} |
