Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12093055:
Linux sandbox: Make the test run on Android as Native executable (Closed)
|
Created:
7 years, 10 months ago by jln (very slow on Chromium) Modified:
7 years, 10 months ago CC:
chromium-reviews, bulach+watch_chromium.org, yfriedman+watch_chromium.org, klundberg+watch_chromium.org, agl, ilevy+watch_chromium.org, jln+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLinux sandbox: Make the test run on Android as Native executable
Make it possible to run the tests via run_tests.py --exe.
BUG=169416
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180236
Patch Set 1 #
Total comments: 8
Patch Set 2 : Follow Nilesh' instructions. #Patch Set 3 : Additional nit. #Patch Set 4 : Rebase #
Total comments: 5
Patch Set 5 : Rename unit tests. #
Messages
Total messages: 18 (0 generated)
Nilesh: could you take a look for build/android/pylib/forwarder.py ? Markus: for the rest. Also if you have any idea what magic I should do to let both the APK and the native executable build, please let me know. I tried everything I could think of, but it would always fail in various ways. I think it's possibly because the tests "automatically" register and the linker just "omits" them because they are not required, if they are in an external static library. https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.gypi File sandbox/linux/sandbox_linux.gypi (right): https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... sandbox/linux/sandbox_linux.gypi:9: 'build_android_jni_test': 1, This is a poor solution to keep both APK and non APK options available. I tried everything I could think of: - make a separate "files" target of type none and then a shared_library and an executable target. - dynamically detect via sandbox_linux_unittests_apk_target what we were trying to currently build. Everything seems to fail in one way or another. I think there is a lot of magic going on with unit tests that I don't know about. For instance if not directly including unit test related files (but instead adding a dependency that will include these files), the tests don't run.
lgtm I don't even pretend to fully understand the GYP changes. But whatever superficial inspection I can do, does in fact pass the review. It looks to me, as if you are not actually changing current behavior, and that appears to be intentional. As for disabling the test on Android. That part of the change list looks fine -- and I do understand it :-)
https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.gypi File sandbox/linux/sandbox_linux.gypi (right): https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... sandbox/linux/sandbox_linux.gypi:9: 'build_android_jni_test': 1, On 2013/01/30 00:04:25, Julien Tinnes wrote: > This is a poor solution to keep both APK and non APK options available. > > I tried everything I could think of: > - make a separate "files" target of type none and then a shared_library and an > executable target. > - dynamically detect via sandbox_linux_unittests_apk_target what we were trying > to currently build. > > Everything seems to fail in one way or another. I think there is a lot of magic > going on with unit tests that I don't know about. > > For instance if not directly including unit test related files (but instead > adding a dependency that will include these files), the tests don't run. I am not sure why you are doing this. If you want to build executable by default, you can just change the type to executable, and when you need to build sharedlibrary you change it back to gtest_target_type locally. If you want to build shared_library by default, you can keep gtest_target_type and modify the gyp variable during the gyp step when you need to build an executable. You wont even need to modify any file in this case. I am sorry I fail to understand the reasoning here. https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... sandbox/linux/sandbox_linux.gypi:233: 'sandbox_linux_unittests_apk_target': 1, What does this accomplish?
https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.gypi File sandbox/linux/sandbox_linux.gypi (right): https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... sandbox/linux/sandbox_linux.gypi:9: 'build_android_jni_test': 1, > I am not sure why you are doing this. If you want to build executable by > default, you can just change the type to executable, and when you need to build > sharedlibrary you change it back to gtest_target_type locally. > If you want to build shared_library by default, you can keep gtest_target_type > and modify the gyp variable during the gyp step when you need to build an > executable. You wont even need to modify any file in this case. > > I am sorry I fail to understand the reasoning here. There must be something I don't know. Are you saying that there is a command line option that I can use to set gtest_target_type ? What I was trying to achieve (and failed) was to have one target sandbox_linux_unittests_apk that is the APK with JNI etc and one target sandbox_linux_unittests that is a normal ELF file. So that one could easily build one on the other, depending on the chosen target. It's actually very hard, surprisingly to have two different targets that include the same tests. https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... sandbox/linux/sandbox_linux.gypi:233: 'sandbox_linux_unittests_apk_target': 1, On 2013/01/30 01:43:08, nileshagrawal wrote: > What does this accomplish? It's not used at the moment. I was fiddling with it when trying to accomplish what I described above. I kept it around, because it's a fairly standard thing that many targets define anyways.
On 2013/01/30 01:56:29, Julien Tinnes wrote: > https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.gypi > File sandbox/linux/sandbox_linux.gypi (right): > > https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... > sandbox/linux/sandbox_linux.gypi:9: 'build_android_jni_test': 1, > > I am not sure why you are doing this. If you want to build executable by > > default, you can just change the type to executable, and when you need to > build > > sharedlibrary you change it back to gtest_target_type locally. > > If you want to build shared_library by default, you can keep gtest_target_type > > and modify the gyp variable during the gyp step when you need to build an > > executable. You wont even need to modify any file in this case. > > > > I am sorry I fail to understand the reasoning here. I read more slowly and I understand what you were saying. But I would like people to be able to build either easily, with a different target. You have a good point that I could keep gtest_target_type as the type and just change it as a GYP variable, but it's even harder to figure out for people than opening this file, reading this comment and changing this parameter.
https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.gypi File sandbox/linux/sandbox_linux.gypi (right): https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... sandbox/linux/sandbox_linux.gypi:9: 'build_android_jni_test': 1, On 2013/01/30 01:56:29, Julien Tinnes wrote: > > I am not sure why you are doing this. If you want to build executable by > > default, you can just change the type to executable, and when you need to > build > > sharedlibrary you change it back to gtest_target_type locally. > > If you want to build shared_library by default, you can keep gtest_target_type > > and modify the gyp variable during the gyp step when you need to build an > > executable. You wont even need to modify any file in this case. > > > > I am sorry I fail to understand the reasoning here. > > There must be something I don't know. Are you saying that there is a command > line option that I can use to set gtest_target_type ? IIRC, you can modify GYP_DEFINES in your environment to change gtest_target_type. Actually strike that, gtest_target_type no longer comes from a gyp define after this CL. http://src.chromium.org/viewvc/chrome/trunk/src/build/common.gypi?view=diff&r... > > What I was trying to achieve (and failed) was to have one target > sandbox_linux_unittests_apk that is the APK with JNI etc and one target > sandbox_linux_unittests that is a normal ELF file. > > So that one could easily build one on the other, depending on the chosen target. > > It's actually very hard, surprisingly to have two different targets that include > the same tests. It should be doable by having two targets: sandbox_linux_unittests: type executable sandbox_linux_unittests_shlib: type sh_lib, will depend on ../testing/android/native_test.gyp:native_test_native_code the _apk target will depend on the shlib target. both can share the same source files by having a test_sources.gypi (which will list all the source files). https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... sandbox/linux/sandbox_linux.gypi:233: 'sandbox_linux_unittests_apk_target': 1, On 2013/01/30 01:56:29, Julien Tinnes wrote: > On 2013/01/30 01:43:08, nileshagrawal wrote: > > What does this accomplish? > > It's not used at the moment. I was fiddling with it when trying to accomplish > what I described above. I kept it around, because it's a fairly standard thing > that many targets define anyways. I still do not understand this. these variables are read by the included gyp template apk_test.gypi Can you point to another such usage of this?
On 2013/01/30 02:05:05, Julien Tinnes wrote: > On 2013/01/30 01:56:29, Julien Tinnes wrote: > > > https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.gypi > > File sandbox/linux/sandbox_linux.gypi (right): > > > > > https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... > > sandbox/linux/sandbox_linux.gypi:9: 'build_android_jni_test': 1, > > > I am not sure why you are doing this. If you want to build executable by > > > default, you can just change the type to executable, and when you need to > > build > > > sharedlibrary you change it back to gtest_target_type locally. > > > If you want to build shared_library by default, you can keep > gtest_target_type > > > and modify the gyp variable during the gyp step when you need to build an > > > executable. You wont even need to modify any file in this case. > > > > > > I am sorry I fail to understand the reasoning here. > > I read more slowly and I understand what you were saying. But I would like > people to be able to build either easily, with a different target. > Having two different targets would best serve the purpose. Have you tried just listing all the source files again (in the new target). That should definitely work. > You have a good point that I could keep gtest_target_type as the type and just > change it as a GYP variable, but it's even harder to figure out for people than > opening this file, reading this comment and changing this parameter.
https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.gypi File sandbox/linux/sandbox_linux.gypi (right): https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... sandbox/linux/sandbox_linux.gypi:9: 'build_android_jni_test': 1, On 2013/01/30 02:17:05, nileshagrawal wrote: > On 2013/01/30 01:56:29, Julien Tinnes wrote: > > > I am not sure why you are doing this. If you want to build executable by > > > default, you can just change the type to executable, and when you need to > > build > > > sharedlibrary you change it back to gtest_target_type locally. > > > If you want to build shared_library by default, you can keep > gtest_target_type > > > and modify the gyp variable during the gyp step when you need to build an > > > executable. You wont even need to modify any file in this case. > > > > > > I am sorry I fail to understand the reasoning here. > > > > There must be something I don't know. Are you saying that there is a command > > line option that I can use to set gtest_target_type ? > > IIRC, you can modify GYP_DEFINES in your environment to change > gtest_target_type. > > Actually strike that, gtest_target_type no longer comes from a gyp define after > this CL. > http://src.chromium.org/viewvc/chrome/trunk/src/build/common.gypi?view=diff&r... > > > > What I was trying to achieve (and failed) was to have one target > > sandbox_linux_unittests_apk that is the APK with JNI etc and one target > > sandbox_linux_unittests that is a normal ELF file. > > > > So that one could easily build one on the other, depending on the chosen > target. > > > > It's actually very hard, surprisingly to have two different targets that > include > > the same tests. > > It should be doable by having two targets: > sandbox_linux_unittests: type executable > sandbox_linux_unittests_shlib: type sh_lib, will depend on > ../testing/android/native_test.gyp:native_test_native_code > > the _apk target will depend on the shlib target. I tried that (without an external gypi file though). I tried both with type "none", or by making a small static library. It doesn't work, the executable doesn't see any tests after that. I *think* it's because there is some magic that finds out that it's not necessary to link certain files to the executable (since strictly speaking there are no unmatched symbols at link time in the case of tests). But I'm really not sure what happens. In the meantime, the most important change for me would be a way to run the tests with --exe (which is independent from this change), as I commented on the bug.
On 2013/01/30 02:27:51, Julien Tinnes wrote: > https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.gypi > File sandbox/linux/sandbox_linux.gypi (right): > > https://codereview.chromium.org/12093055/diff/1/sandbox/linux/sandbox_linux.g... > sandbox/linux/sandbox_linux.gypi:9: 'build_android_jni_test': 1, > On 2013/01/30 02:17:05, nileshagrawal wrote: > > On 2013/01/30 01:56:29, Julien Tinnes wrote: > > > > I am not sure why you are doing this. If you want to build executable by > > > > default, you can just change the type to executable, and when you need to > > > build > > > > sharedlibrary you change it back to gtest_target_type locally. > > > > If you want to build shared_library by default, you can keep > > gtest_target_type > > > > and modify the gyp variable during the gyp step when you need to build an > > > > executable. You wont even need to modify any file in this case. > > > > > > > > I am sorry I fail to understand the reasoning here. > > > > > > There must be something I don't know. Are you saying that there is a command > > > line option that I can use to set gtest_target_type ? > > > > IIRC, you can modify GYP_DEFINES in your environment to change > > gtest_target_type. > > > > Actually strike that, gtest_target_type no longer comes from a gyp define > after > > this CL. > > > http://src.chromium.org/viewvc/chrome/trunk/src/build/common.gypi?view=diff&r... > > > > > > What I was trying to achieve (and failed) was to have one target > > > sandbox_linux_unittests_apk that is the APK with JNI etc and one target > > > sandbox_linux_unittests that is a normal ELF file. > > > > > > So that one could easily build one on the other, depending on the chosen > > target. > > > > > > It's actually very hard, surprisingly to have two different targets that > > include > > > the same tests. > > > > It should be doable by having two targets: > > sandbox_linux_unittests: type executable > > sandbox_linux_unittests_shlib: type sh_lib, will depend on > > ../testing/android/native_test.gyp:native_test_native_code > > > > the _apk target will depend on the shlib target. > > I tried that (without an external gypi file though). I tried both with type > "none", or by making a small static library. > It doesn't work, the executable doesn't see any tests after that. I *think* it's > because there is some magic that finds out that it's not necessary to link > certain files to the executable (since strictly speaking there are no unmatched > symbols at link time in the case of tests). But I'm really not sure what > happens. > I guess you tried creating a new "common" target which both exe and shlib depended on. From http://code.google.com/p/googletest/wiki/Primer "Note that RUN_ALL_TESTS() runs all tests in your link unit" By creating a separate gypi you just create a common template which is included by both the targets (just a fancy way of not listing source files twice). In this case, the test .o files are linked twice (once for each type of target). > In the meantime, the most important change for me would be a way to run the > tests with --exe (which is independent from this change), as I commented on the > bug.
Nilesh, thanks a lot for your help on this. Sorry for the delay, I was dragged to some urgent matters. Could you please take another look ?
https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... File sandbox/linux/sandbox_linux.gypi (right): https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... sandbox/linux/sandbox_linux.gypi:213: 'test_suite_name': 'sandbox_linux_unittests_jni', I think we should not change the test_suite_name as the bots already have a directory out/Debug/sandbox_linux_unittests It will also keep consistent with other tests. The test runner will know which one to run based on the --exe option
https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... File sandbox/linux/sandbox_linux.gypi (right): https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... sandbox/linux/sandbox_linux.gypi:213: 'test_suite_name': 'sandbox_linux_unittests_jni', On 2013/02/02 01:06:19, nileshagrawal1 wrote: > I think we should not change the test_suite_name as the bots already have a > directory out/Debug/sandbox_linux_unittests > > It will also keep consistent with other tests. The test runner will know which > one to run based on the --exe option How big of a deal is this? I think there is a future where the tests won't be the same (the current tests included in the jni version don't make sense, since they can't possibly work (because of the fork vs. thread issue)). If this is a big deal I can rename to sandbox_linux_unittests and change to a more different name if one day we have two pretty different test suites.
> I think there is a future where the tests won't be the same (the current tests > included in the jni version don't make sense, since they can't possibly work > (because of the fork vs. thread issue)). In addition, whatever we do, we need to do run_tests.py -s sandbox_linux_unittests_jni, because that's the name of the shared library. I thought it would be much less confusing to have the shared library and the test suite be named similarly (I actually don't know what the test suite name influences).
https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... File sandbox/linux/sandbox_linux.gypi (right): https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... sandbox/linux/sandbox_linux.gypi:78: 'target_name': 'sandbox_linux_unittests_jni', sandbox_linux_jni_unittests https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... sandbox/linux/sandbox_linux.gypi:213: 'test_suite_name': 'sandbox_linux_unittests_jni', On 2013/02/02 01:10:37, Julien Tinnes wrote: > On 2013/02/02 01:06:19, nileshagrawal1 wrote: > > I think we should not change the test_suite_name as the bots already have a > > directory out/Debug/sandbox_linux_unittests > > > > It will also keep consistent with other tests. The test runner will know which > > one to run based on the --exe option > > How big of a deal is this? > > I think there is a future where the tests won't be the same (the current tests > included in the jni version don't make sense, since they can't possibly work > (because of the fork vs. thread issue)). > > If this is a big deal I can rename to sandbox_linux_unittests and change to a > more different name if one day we have two pretty different test suites. I that case please rename this to sandbox_linux_jni_unittests . Basically we would like to have unittests and unittests_apk as the suffix. This is consistent with all the other targets.
Thanks, PTAL! https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... File sandbox/linux/sandbox_linux.gypi (right): https://chromiumcodereview.appspot.com/12093055/diff/13001/sandbox/linux/sand... sandbox/linux/sandbox_linux.gypi:213: 'test_suite_name': 'sandbox_linux_unittests_jni', On 2013/02/02 01:55:42, nileshagrawal1 wrote: > On 2013/02/02 01:10:37, Julien Tinnes wrote: > > On 2013/02/02 01:06:19, nileshagrawal1 wrote: > > > I think we should not change the test_suite_name as the bots already have a > > > directory out/Debug/sandbox_linux_unittests > > > > > > It will also keep consistent with other tests. The test runner will know > which > > > one to run based on the --exe option > > > > How big of a deal is this? > > > > I think there is a future where the tests won't be the same (the current tests > > included in the jni version don't make sense, since they can't possibly work > > (because of the fork vs. thread issue)). > > > > If this is a big deal I can rename to sandbox_linux_unittests and change to a > > more different name if one day we have two pretty different test suites. > > I that case please rename this to sandbox_linux_jni_unittests . Basically we > would like to have unittests and unittests_apk as the suffix. This is consistent > with all the other targets. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/12093055/20002
Message was sent while issue was closed.
Change committed as 180236 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
