Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1475453002:
Make remoting_unittests work on GN & Android (Closed)
DescriptionMake remoting_unittests work on GN & Android
BUG=544298
Committed: https://crrev.com/86ba5e7f5e0c06350e0c5680625a2857540064cd
Cr-Commit-Position: refs/heads/master@{#361726}
Patch Set 1 #
Total comments: 9
Messages
Total messages: 13 (4 generated)
pkotwicz@chromium.org changed reviewers: + agrieve@chromium.org
agrieve@ can you please take a look?
On 2015/11/23 23:48:06, pkotwicz wrote: > agrieve@ can you please take a look? lgtm
pkotwicz@chromium.org changed reviewers: + lambroslambrou@chromium.org
lambroslambrou@ can you please take a look?
Sorry, I'm completely new to GN, and don't understand it well, despite reading through the docs. Especially, the stuff about GN "templates" is very mysterious to me. I guess "apk_deps" is some kind of template, and somehow pulls in "things" into other "things"? Also, should the remoting/android GN targets be defined in a remoting/android/BUILD.gn file? https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (left): https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#oldcode48 remoting/BUILD.gn:48: #"//remoting:remoting_unittests_apk", I guess the convention is that commented-out lines refer to GYP targets that aren't yet enabled for GN. Removing this line signifies that you're enabling remoting_unittests_apk for GN, but I don't see where that target is enabled or defined. https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#oldcode146 remoting/BUILD.gn:146: deps += [ "//testing/android/native_test:native_test_native_code" ] Is this removed, because the net_java "apk_deps" line somehow brings this in transitively? https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#newcode160 remoting/BUILD.gn:160: apk_deps = [ "//net/android:net_java" ] I don't understand this. There aren't any Android GN targets in this file, are there? So who uses apk_deps?
lambroslambrou@ can you please take another look? Thank you very much for taking the time to review this CL. I read the GN documentation in https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/ If you have specific things that you find confusing w.r.t to GN, please send me a list and I will attempt to add them to the documentation. I had no part in the making of GN, but I have a vested interest in making GN understandable. Unfortunately, the GN rules for Android are more complex than for the other platforms. I will try to document what each variable for each of the android templates does in the coming weeks. I describe what "apk_deps" does further later. It is a variable for the test() template. "apk_deps" is not a template. https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (left): https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#oldcode48 remoting/BUILD.gn:48: #"//remoting:remoting_unittests_apk", On Android, the test() template generates the _apk target. The test() template can only be used for gtests. See https://code.google.com/p/chromium/codesearch#chromium/src/testing/test.gni&l=18 https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#oldcode146 remoting/BUILD.gn:146: deps += [ "//testing/android/native_test:native_test_native_code" ] The test() template always adds in this dependency. See https://code.google.com/p/chromium/codesearch#chromium/src/testing/test.gni&l=59 (The remote_unittests target does not define the "use_default_launcher" variable) https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#newcode160 remoting/BUILD.gn:160: apk_deps = [ "//net/android:net_java" ] The test() template automatically generates the "_apk" target. "apk_deps" is very weird. I do not know why "apk_deps" is separate from "deps" "apk_deps" must be provided because the native code calls into Java on Android. All Java dependencies for test() targets must be included via "apk_deps" in the test() target. The dependencies specified via "apk_deps" are the only ones which are forwarded to the unittest_apk() target as per https://code.google.com/p/chromium/codesearch#chromium/src/testing/test.gni&l=77 Using deps for Java dependencies works for android_apk() targets. "apk_deps" is not a valid parameter for android_apk().
lgtm, thanks for explaining, it makes more sense, though I still don't understand GN (and never properly understood GYP either!) https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (left): https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#oldcode48 remoting/BUILD.gn:48: #"//remoting:remoting_unittests_apk", On 2015/11/24 20:35:29, pkotwicz wrote: > On Android, the test() template generates the _apk target. The test() template > can only be used for gtests. > > See > https://code.google.com/p/chromium/codesearch#chromium/src/testing/test.gni&l=18 Acknowledged. https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#oldcode146 remoting/BUILD.gn:146: deps += [ "//testing/android/native_test:native_test_native_code" ] On 2015/11/24 20:35:29, pkotwicz wrote: > The test() template always adds in this dependency. See > https://code.google.com/p/chromium/codesearch#chromium/src/testing/test.gni&l=59 > > (The remote_unittests target does not define the "use_default_launcher" > variable) Acknowledged. https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1475453002/diff/1/remoting/BUILD.gn#newcode160 remoting/BUILD.gn:160: apk_deps = [ "//net/android:net_java" ] On 2015/11/24 20:35:29, pkotwicz wrote: > The test() template automatically generates the "_apk" target. > > "apk_deps" is very weird. I do not know why "apk_deps" is separate from "deps" > > "apk_deps" must be provided because the native code calls into Java on Android. > All Java dependencies for test() targets must be included via "apk_deps" in the > test() target. The dependencies specified via "apk_deps" are the only ones which > are forwarded to the unittest_apk() target as per > https://code.google.com/p/chromium/codesearch#chromium/src/testing/test.gni&l=77 > > Using deps for Java dependencies works for android_apk() targets. "apk_deps" is > not a valid parameter for android_apk(). Acknowledged.
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475453002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475453002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Make remoting_unittests work on GN & Android BUG=544298 ========== to ========== Make remoting_unittests work on GN & Android BUG=544298 Committed: https://crrev.com/86ba5e7f5e0c06350e0c5680625a2857540064cd Cr-Commit-Position: refs/heads/master@{#361726} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/86ba5e7f5e0c06350e0c5680625a2857540064cd Cr-Commit-Position: refs/heads/master@{#361726} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
