Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
147533004:
Remove unneeded JNI registrations. (Closed)
|
Created:
6 years, 10 months ago by ostap Modified:
6 years, 4 months ago Reviewers:
jochen (gone - plz use gerrit), Torne, mkosiba (inactive), bulach, DaveMoore, viettrungluu, Mark Mentovai, Yaron CC:
chromium-reviews, davidben+watch_chromium.org, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, tfarina, jam, joi+watch-content_chromium.org, gavinp+prer_chromium.org, dominich+watch_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, viettrungluu Base URL:
https://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRemove unneeded JNI registrations.
Rather than registering all jni bindings at startup, only get references
to the class object for those files which require bindings. All others
are satisfied by exporting symbols which can be found automatically by
dalvik.
This patch replaces excldue-libs=ALL with ld version script to strip unwanted
symbols: https://sourceware.org/binutils/docs-2.24/ld/VERSION.html#VERSION
BUG=
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Rebased. Linker "version script" used for exports. #Patch Set 4 : Fix android webview build issues. #
Total comments: 2
Patch Set 5 : Rebaes, added native_exports switch, don't remove empty RegisterNatives. #
Total comments: 24
Patch Set 6 : Rebased and updated by review comments. #Patch Set 7 : Fix mojo exports. #Patch Set 8 : Fix windows build. #Patch Set 9 : Don't use linker script for mojo_native_viewport_service library. #
Total comments: 12
Patch Set 10 : Updated by review comments. #
Total comments: 7
Patch Set 11 : Rebased and updated by review comments. #Patch Set 12 : Moved dependency for android_exports.lst copy and removed unneeded inner class mapping from GetJNIR… #Patch Set 13 : Rebased and fixed by Torne comments. #Patch Set 14 : Fix mojo build issues #Patch Set 15 : Rebased. #Patch Set 16 : Remove whitespace change. #Patch Set 17 : Rebase #Patch Set 18 : Rebase. #
Total comments: 1
Patch Set 19 : Fixed dependencies for linker script copy. #Patch Set 20 : Use link_settings in JNI generator .gypis to provide linker exports list. #
Total comments: 2
Patch Set 21 : Fix exports in non-JNI shared libs. #
Messages
Total messages: 74 (0 generated)
Finalizing patch https://codereview.chromium.org/18161004/ by Yaron's suggestion.
+bulach, -others for now (as they appear to be for OWNERS stamps) I'd also link to https://sourceware.org/bugzilla/show_bug.cgi?id=16693 and/or https://sourceware.org/binutils/docs-2.24/ld/VERSION.html#VERSION CL description should also mention that the fundamental change is to switch from using excldue-libs=ALL to using version scripts to strip unwanted symbols. I take it you've confirmed that this behaves much in the same way as before, and the size change is negligible? Would be very interested to see impact if any on perf bots. Something which is lost from https://codereview.chromium.org/18161004/ is that it's now not clear whether you need to call RegisterNativesImpl or not. I kind of like it how it was a no-op before. That is unless we can fully go the way of generating the registrars as we've perviously discussed off thread... It looks like the component build would work with this but wanted to double check that you've tested it. https://codereview.chromium.org/147533004/diff/130001/build/android/android_e... File build/android/android_exports.lst (right): https://codereview.chromium.org/147533004/diff/130001/build/android/android_e... build/android/android_exports.lst:1: { This needs a copyright and comments about what it's for and how it's used. https://codereview.chromium.org/147533004/diff/130001/chrome/chrome_android.gypi File chrome/chrome_android.gypi (right): https://codereview.chromium.org/147533004/diff/130001/chrome/chrome_android.g... chrome/chrome_android.gypi:57: '-Wl,--version-script="../../build/android/android_exports.lst"', Why not do this everywhere (build/common.gypi)? This should affect how all shared libraries are packaged or it'll break some (e.g.a unit tests which calls into java and back)
On 2014/04/02 21:02:52, Yaron wrote: > +bulach, -others for now (as they appear to be for OWNERS stamps) > > I'd also link to https://sourceware.org/bugzilla/show_bug.cgi?id=16693 and/or > https://sourceware.org/binutils/docs-2.24/ld/VERSION.html#VERSION > > CL description should also mention that the fundamental change is to switch from > using excldue-libs=ALL to using version scripts to strip unwanted symbols. > > I take it you've confirmed that this behaves much in the same way as before, Yes, it works as before. Of course I didn't do full testing, but chrome/content shell works fine, including location support in google search. > and > the size change is negligible? Would be very interested to see impact if any on > perf bots. The resulting libraries are a bit smaller with patch: -rwxr-xr-x 1 sl sl 2592592704 Apr 2 22:24 libchromeshell.so -rwxr-xr-x 1 sl sl 2158208268 Apr 2 22:20 libcontent_shell_content_view.so without patch: -rwxr-xr-x 1 sl sl 2593096048 Apr 2 23:24 libchromeshell.so -rwxr-xr-x 1 sl sl 2158375892 Apr 2 23:23 libcontent_shell_content_view.so > Something which is lost from https://codereview.chromium.org/18161004/ is that > it's now not clear whether you need to call RegisterNativesImpl or not. I kind > of like it how it was a no-op before. Sorry, I've missed that. It would also make things a bit simpler for me. > That is unless we can fully go the way of > generating the registrars as we've perviously discussed off thread... Yes, I think this would be the next step. Also, I'd like to eliminate function tables and generate register calls directly into function body. This way optimizer might do some work and in general direct calls are faster than pointer calls. > It looks like the component build would work with this but wanted to double > check that you've tested it. Yes, I've tested. > https://codereview.chromium.org/147533004/diff/130001/build/android/android_e... > File build/android/android_exports.lst (right): > > https://codereview.chromium.org/147533004/diff/130001/build/android/android_e... > build/android/android_exports.lst:1: { > This needs a copyright and comments about what it's for and how it's used. Done > https://codereview.chromium.org/147533004/diff/130001/chrome/chrome_android.gypi > File chrome/chrome_android.gypi (right): > > https://codereview.chromium.org/147533004/diff/130001/chrome/chrome_android.g... > chrome/chrome_android.gypi:57: > '-Wl,--version-script="../../build/android/android_exports.lst"', > Why not do this everywhere (build/common.gypi)? This should affect how all > shared libraries are packaged or it'll break some (e.g.a unit tests which calls > into java and back) At 1st I thought that there could be libraries that have their own, non JNI exports. I didn't realize the amount of test libs full android build creates (~45GB in lib folder). I'll do reverse - use version script in common.gypi and undef it for targets libraries that don't need it.
On 2014/04/03 03:56:23, ostap wrote: > On 2014/04/02 21:02:52, Yaron wrote: > > +bulach, -others for now (as they appear to be for OWNERS stamps) > > > > I'd also link to https://sourceware.org/bugzilla/show_bug.cgi?id=16693 and/or > > https://sourceware.org/binutils/docs-2.24/ld/VERSION.html#VERSION > > > > CL description should also mention that the fundamental change is to switch > from > > using excldue-libs=ALL to using version scripts to strip unwanted symbols. > > > > I take it you've confirmed that this behaves much in the same way as before, > > Yes, it works as before. Of course I didn't do full testing, but chrome/content > shell works fine, including location support in google search. > > > and > > the size change is negligible? Would be very interested to see impact if any > on > > perf bots. > > The resulting libraries are a bit smaller with patch: > -rwxr-xr-x 1 sl sl 2592592704 Apr 2 22:24 libchromeshell.so > -rwxr-xr-x 1 sl sl 2158208268 Apr 2 22:20 libcontent_shell_content_view.so > > without patch: > -rwxr-xr-x 1 sl sl 2593096048 Apr 2 23:24 libchromeshell.so > -rwxr-xr-x 1 sl sl 2158375892 Apr 2 23:23 libcontent_shell_content_view.so > > > Something which is lost from https://codereview.chromium.org/18161004/ is that > > it's now not clear whether you need to call RegisterNativesImpl or not. I kind > > of like it how it was a no-op before. > > Sorry, I've missed that. It would also make things a bit simpler for me. > > > That is unless we can fully go the way of > > generating the registrars as we've perviously discussed off thread... > > Yes, I think this would be the next step. > Also, I'd like to eliminate function tables and generate register calls directly > into function body. This way optimizer might do some work and in general direct > calls are faster than pointer calls. > > > It looks like the component build would work with this but wanted to double > > check that you've tested it. > > Yes, I've tested. > > > > https://codereview.chromium.org/147533004/diff/130001/build/android/android_e... > > File build/android/android_exports.lst (right): > > > > > https://codereview.chromium.org/147533004/diff/130001/build/android/android_e... > > build/android/android_exports.lst:1: { > > This needs a copyright and comments about what it's for and how it's used. > > Done > > > > https://codereview.chromium.org/147533004/diff/130001/chrome/chrome_android.gypi > > File chrome/chrome_android.gypi (right): > > > > > https://codereview.chromium.org/147533004/diff/130001/chrome/chrome_android.g... > > chrome/chrome_android.gypi:57: > > '-Wl,--version-script="../../build/android/android_exports.lst"', > > Why not do this everywhere (build/common.gypi)? This should affect how all > > shared libraries are packaged or it'll break some (e.g.a unit tests which > calls > > into java and back) > > At 1st I thought that there could be libraries that have their own, non JNI > exports. I didn't realize the amount of test libs full android build creates > (~45GB in lib folder). I'll do reverse - use version script in common.gypi and > undef it for targets libraries that don't need it. I quite like the idea of removing the registration, thanks for driving this! one small request: this jni_generator is used by other projects, and every now and then we merge all these changes from various other repositories. whilst I'm sure they'll be interested in this approach :) could you make it so that it'll be an extra option only used by chromium at this stage? this will make their transitions smoother, and then later on we can remove the option and clean up the generator... would that be ok?
On 2014/04/07 15:37:26, bulach_ooo_14apr wrote: > > At 1st I thought that there could be libraries that have their own, non JNI > > exports. I didn't realize the amount of test libs full android build creates > > (~45GB in lib folder). I'll do reverse - use version script in common.gypi and > > undef it for targets libraries that don't need it. > > I quite like the idea of removing the registration, thanks for driving this! > one small request: this jni_generator is used by other projects, and every now > and then we merge all these changes from various other repositories. > whilst I'm sure they'll be interested in this approach :) could you make it so > that it'll be an extra option only used by chromium at this stage? > this will make their transitions smoother, and then later on we can remove the > option and clean up the generator... > would that be ok? Yes, of course. Thanks for looking at this.
On 2014/04/07 16:04:07, ostap wrote: > On 2014/04/07 15:37:26, bulach_ooo_14apr wrote: > > > At 1st I thought that there could be libraries that have their own, non JNI > > > exports. I didn't realize the amount of test libs full android build creates > > > (~45GB in lib folder). I'll do reverse - use version script in common.gypi > and > > > undef it for targets libraries that don't need it. > > > > I quite like the idea of removing the registration, thanks for driving this! > > one small request: this jni_generator is used by other projects, and every now > > and then we merge all these changes from various other repositories. > > whilst I'm sure they'll be interested in this approach :) could you make it so > > that it'll be an extra option only used by chromium at this stage? > > this will make their transitions smoother, and then later on we can remove the > > option and clean up the generator... > > would that be ok? > > Yes, of course. > Thanks for looking at this. also, another idea :) this patch as is will require tons of OWNERs at once, and rebasing it multiple times, etc.. in the past, for such large changes, we made it an option not only at the python level but also exposed it at the gypi level. this way, you can change incrementally, and have N patches each with a smaller set of OWNERS.. life will be simpler with fewer / smaller rebases :)
On 2014/04/08 15:33:03, bulach_ooo_14apr wrote: > On 2014/04/07 16:04:07, ostap wrote: > > On 2014/04/07 15:37:26, bulach_ooo_14apr wrote: > > > > At 1st I thought that there could be libraries that have their own, non > JNI > > > > exports. I didn't realize the amount of test libs full android build > creates > > > > (~45GB in lib folder). I'll do reverse - use version script in common.gypi > > and > > > > undef it for targets libraries that don't need it. > > > > > > I quite like the idea of removing the registration, thanks for driving this! > > > one small request: this jni_generator is used by other projects, and every > now > > > and then we merge all these changes from various other repositories. > > > whilst I'm sure they'll be interested in this approach :) could you make it > so > > > that it'll be an extra option only used by chromium at this stage? > > > this will make their transitions smoother, and then later on we can remove > the > > > option and clean up the generator... > > > would that be ok? > > > > Yes, of course. > > Thanks for looking at this. > > also, another idea :) > this patch as is will require tons of OWNERs at once, and rebasing it multiple > times, etc.. > in the past, for such large changes, we made it an option not only at the python > level but also exposed it at the gypi level. > this way, you can change incrementally, and have N patches each with a smaller > set of OWNERS.. > life will be simpler with fewer / smaller rebases :) Actually, Yaron suggested to keep RegisterNatives methods even if they are empty. This way patch will be much smaller.
good stuff, thanks! I think we're on the right path, adding torne@ for validating the webview.. some suggestions and nits below: https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:734: return '\nextern "C" {\n' + "\n".join(ret) + "\n};" nit: use '\n' instead of "\n" in the second string. also, at the end, it'd probably be nicer to annotate the closing brace: '\n}; // extern "C"' ditto for 757 below. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:949: __attribute__((alias("${NAME}"), visibility("default"))) ${RETURN} nit: move the ${RETURN} to the next line and put it together with the Java_${JAVA_NAME} ... so that the generated code wouldn't be indented and would look like: __attribute__((alias("${NAME}"), visibility("default"))) void Java_Foo_nativeBar(JNIEnv* env, ....) https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:954: java_name = JniParams.RemapClassName(self.fully_qualified_class).replace('_', '_1').replace('/', '_') nit: >80cols, suggest: java_name = java_name = JniParams.RemapClassName(self.fully_qualified_class) java_name = java_name.replace('_', '_1').replace('/', '_') https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:956: java_name += "_00024" + native.java_class_name nit: s/"/'/ https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:976: ${RETURN} Java_${JAVA_NAME}_native${NAME}(JNIEnv* env, nit: unindent this line https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:994: java_name = JniParams.RemapClassName(self.fully_qualified_class).replace('_', '_1').replace('/', '_') nit: as above, >80 cols https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:996: java_name += "_00024" + native.java_class_name nit: s/"/'/ and below in 998 https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:1370: action='store_true', dest='native_exports', nit: no need for dest https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... File base/android/jni_generator/jni_generator_tests.py (right): https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator_tests.py:1006: private static native int nativeMethod(long nativeTest, int arg1); nit: how about adding a non-static method too? https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... File base/android/jni_generator/testNativeExportsOption.golden (right): https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/testNativeExportsOption.golden:28: static jint Method(JNIEnv* env, jobject jcaller, shouldn't this method name start with Java_org_chromium_example_.... ? https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/testNativeExportsOption.golden:37: jint Java_org_chromium_example_jni_1generator_Test_nativeMethod(JNIEnv* env, what's up with "1"generator? also: if I'm understanding this correctly, the classes will now have to expose this long method name, right? instead of just having a declaration, can we make it a stub and call the short method name as it was defined before? wdyt? https://codereview.chromium.org/147533004/diff/150001/build/android/android_e... File build/android/android_exports.lst (right): https://codereview.chromium.org/147533004/diff/150001/build/android/android_e... build/android/android_exports.lst:6: # Check ld version sript manual: nit: s/sript/script/
https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:734: return '\nextern "C" {\n' + "\n".join(ret) + "\n};" On 2014/04/14 13:16:02, bulach wrote: > nit: use '\n' instead of "\n" in the second string. > also, at the end, it'd probably be nicer to annotate the closing brace: > > '\n}; // extern "C"' > > ditto for 757 below. Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:949: __attribute__((alias("${NAME}"), visibility("default"))) ${RETURN} On 2014/04/14 13:16:02, bulach wrote: > nit: move the ${RETURN} to the next line and put it together with the > Java_${JAVA_NAME} ... so that the generated code wouldn't be indented and would > look like: > > __attribute__((alias("${NAME}"), visibility("default"))) > void Java_Foo_nativeBar(JNIEnv* env, ....) Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:954: java_name = JniParams.RemapClassName(self.fully_qualified_class).replace('_', '_1').replace('/', '_') On 2014/04/14 13:16:02, bulach wrote: > nit: >80cols, suggest: > java_name = java_name = JniParams.RemapClassName(self.fully_qualified_class) > java_name = java_name.replace('_', '_1').replace('/', '_') Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:956: java_name += "_00024" + native.java_class_name On 2014/04/14 13:16:02, bulach wrote: > nit: s/"/'/ Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:976: ${RETURN} Java_${JAVA_NAME}_native${NAME}(JNIEnv* env, On 2014/04/14 13:16:02, bulach wrote: > nit: unindent this line Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:994: java_name = JniParams.RemapClassName(self.fully_qualified_class).replace('_', '_1').replace('/', '_') On 2014/04/14 13:16:02, bulach wrote: > nit: as above, >80 cols Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:996: java_name += "_00024" + native.java_class_name On 2014/04/14 13:16:02, bulach wrote: > nit: s/"/'/ and below in 998 Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:1370: action='store_true', dest='native_exports', On 2014/04/14 13:16:02, bulach wrote: > nit: no need for dest Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... File base/android/jni_generator/jni_generator_tests.py (right): https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/jni_generator_tests.py:1006: private static native int nativeMethod(long nativeTest, int arg1); On 2014/04/14 13:16:02, bulach wrote: > nit: how about adding a non-static method too? Done. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... File base/android/jni_generator/testNativeExportsOption.golden (right): https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/testNativeExportsOption.golden:28: static jint Method(JNIEnv* env, jobject jcaller, On 2014/04/14 13:16:02, bulach wrote: > shouldn't this method name start with Java_org_chromium_example_.... ? No, here it is as it defined in native code. https://codereview.chromium.org/147533004/diff/150001/base/android/jni_genera... base/android/jni_generator/testNativeExportsOption.golden:37: jint Java_org_chromium_example_jni_1generator_Test_nativeMethod(JNIEnv* env, On 2014/04/14 13:16:02, bulach wrote: > what's up with "1"generator? The class is org/chromium/example/jni_generator/Test The underscore in "jni_generator" is replaced with "_1". > also: if I'm understanding this correctly, the classes will now have to expose > this long method name, right? > instead of just having a declaration, can we make it a stub and call the short > method name as it was defined before? wdyt? Actually, using simple wrapper solves problem with clang incompatibilities. For example, in clang internal representation static methods are always mangled even if they are inside 'extern "C"' block, so I would have to create alias for mangled name. Also, compiler drop statics even if they are aliased to the export symbol, but not referenced any other way. Here I can drop aliased export and use generated export instead of static, but add wrapper function for static methods. Since it will be single use of native statics, compiler will inline the function and there will be no additional call. https://codereview.chromium.org/147533004/diff/150001/build/android/android_e... File build/android/android_exports.lst (right): https://codereview.chromium.org/147533004/diff/150001/build/android/android_e... build/android/android_exports.lst:6: # Check ld version sript manual: On 2014/04/14 13:16:02, bulach wrote: > nit: s/sript/script/ Done.
thanks! just a few more nits.. it looks like one function stub wasn't generated, wdyt? https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:739: if self.options.native_exports and ret != []: nit: no need for "!= []", this should be equivalent: if self.options.native_exports and ret: https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:763: if self.options.native_exports and ret != []: nit: as above, remove "!= []" https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:961: params = [] nit: params_in_call ? since there's a PARAMS in the template that is unrelated, and this |params| is only used to build |params_in_call| https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:969: java_name += "_00024" + native.java_class_name nit: s/"/'/ https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... File base/android/jni_generator/jni_generator_tests.py (right): https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator_tests.py:1005: private static native boolean nativeInitNativeClass(); as above, the stub for this class is not being generated.. https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... File base/android/jni_generator/testNativeExportsOption.golden (right): https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/testNativeExportsOption.golden:43: Java_org_chromium_example_jni_1generator_SampleForTests_nativeMethod(JNIEnv* hmm... looks like "nativeInitClass" is missing? it should delegate straight to the function rather than doing the cast...
https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:739: if self.options.native_exports and ret != []: On 2014/04/16 11:22:35, bulach wrote: > nit: no need for "!= []", this should be equivalent: > > if self.options.native_exports and ret: Done. https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:763: if self.options.native_exports and ret != []: On 2014/04/16 11:22:35, bulach wrote: > nit: as above, remove "!= []" Done. https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:961: params = [] On 2014/04/16 11:22:35, bulach wrote: > nit: params_in_call ? > since there's a PARAMS in the template that is unrelated, and this |params| is > only used to build |params_in_call| Done. https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:969: java_name += "_00024" + native.java_class_name On 2014/04/16 11:22:35, bulach wrote: > nit: s/"/'/ Done. https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... File base/android/jni_generator/jni_generator_tests.py (right): https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/jni_generator_tests.py:1005: private static native boolean nativeInitNativeClass(); On 2014/04/16 11:22:35, bulach wrote: > as above, the stub for this class is not being generated.. Done. https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... File base/android/jni_generator/testNativeExportsOption.golden (right): https://codereview.chromium.org/147533004/diff/230001/base/android/jni_genera... base/android/jni_generator/testNativeExportsOption.golden:43: Java_org_chromium_example_jni_1generator_SampleForTests_nativeMethod(JNIEnv* On 2014/04/16 11:22:35, bulach wrote: > hmm... looks like "nativeInitClass" is missing? > it should delegate straight to the function rather than doing the cast... That's was a special case for native methods registration. For some reason I decided, that it is not necessary if we have native exports. IMHO, if it is specified, it should be exported. BTW, init method name generation was wrong. It doesn't take into account underscores in path (jni_generator to jni_1generator) and doesn't do class remapping from jars. I added fix for --native_exports case, but left it old way because of compatibility with other projects.
lgtm, thanks a ton, truly appreciate! Yaron will be back next week, would you mind waiting for his final look? I don't expect any extra complications with our internal repositories, but would like him to double check.. thanks again!
On 2014/04/17 12:17:46, bulach wrote: > lgtm, thanks a ton, truly appreciate! > > Yaron will be back next week, would you mind waiting for his final look? > I don't expect any extra complications with our internal repositories, but would > like him to double check.. thanks again! Yes, sure. I still cannot figure out what's wrong with android_aosp build. Seems I'll have to checkout whole android to find fix.
torne may be able to help.. looks like it's trying to find the .lst at: /mnt/scratch0/b_used/build/slave/android_aosp/build/src/build/android/android_exports.lst when the other files look like in: external/chromium_org/... it seems that the GYP step is using the fully qualified name. perhaps it would work with just: <(DEPTH)/build/android/android_exports.lst ? torne?
On 2014/04/17 14:09:08, bulach wrote: > torne may be able to help.. > looks like it's trying to find the .lst at: > /mnt/scratch0/b_used/build/slave/android_aosp/build/src/build/android/android_exports.lst > > when the other files look like in: > external/chromium_org/... > > it seems that the GYP step is using the fully qualified name. > perhaps it would work with just: > <(DEPTH)/build/android/android_exports.lst > ? torne? No, <(DEPTH)/build/android/android_exports.lst doesn't work, because it generates relative path to src during the gyp processing stage. Relative path to src can be different for build dir. There are a lot of places that use the same approach (full path), like binutils, sysroot, ndk/sdk and other tools/libraries. It seems DEPTH points to some other place, not to the chromium src during the whole android build.
Added viettrungluu for mojo gyp changes.
-> davemoore, instead of me, since he's been looking at android linking stuff (-Wl,--exclude-libs=ALL is being removed; I wonder if we should move to a linker script for services)
https://codereview.chromium.org/147533004/diff/170002/mojo/mojo_public.gypi File mojo/mojo_public.gypi (right): https://codereview.chromium.org/147533004/diff/170002/mojo/mojo_public.gypi#n... mojo/mojo_public.gypi:24: ], The only special case related to exports that I know of is in mojo_system. I believe if you rebase you'll see a recent change that explictly removes the --exclude-libs=ALL from targets dependent on this, because part of the api to users of this library is an exported function. Can this new script be used to somehow automate this in a better way? Also, I'm not sure why the 3 shared libraries you special case here are different from the other mojo applications. Is it now a constraint that if a library exports any api that it needs to have special code for android?
https://codereview.chromium.org/147533004/diff/170002/mojo/mojo_public.gypi File mojo/mojo_public.gypi (right): https://codereview.chromium.org/147533004/diff/170002/mojo/mojo_public.gypi#n... mojo/mojo_public.gypi:24: ], On 2014/04/21 23:34:51, DaveMoore wrote: > The only special case related to exports that I know of is in mojo_system. I > believe if you rebase you'll see a recent change that explictly removes the > --exclude-libs=ALL from targets dependent on this, because part of the api to > users of this library is an exported function. Can this new script be used to > somehow automate this in a better way? > > Also, I'm not sure why the 3 shared libraries you special case here are > different from the other mojo applications. >Is it now a constraint that if a > library exports any api that it needs to have special code for android? This sort of already was the case because of exclude-libs=ALL since typically everything's a static library. mojo is the first place I'm seeing in chromium where we use a separate shared library in a non-component build. Previously we'd only need need to export JNI_OnLoad and the JNI generator would generate all needed associations for jni calls.
https://codereview.chromium.org/147533004/diff/170002/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/147533004/diff/170002/base/android/jni_genera... base/android/jni_generator/jni_generator.py:975: java_name += '_00024' + native.java_class_name Why is this logic not needed in GetJNIRegisterNativesString? Looks like I had it in https://codereview.chromium.org/18161004/.
On 2014/04/17 17:01:51, ostap wrote: > On 2014/04/17 14:09:08, bulach wrote: > > torne may be able to help.. > > looks like it's trying to find the .lst at: > > > /mnt/scratch0/b_used/build/slave/android_aosp/build/src/build/android/android_exports.lst > > > > when the other files look like in: > > external/chromium_org/... > > > > it seems that the GYP step is using the fully qualified name. > > perhaps it would work with just: > > <(DEPTH)/build/android/android_exports.lst > > ? torne? > > No, <(DEPTH)/build/android/android_exports.lst doesn't work, because it > generates relative path to src during the gyp processing stage. Relative path to > src can be different for build dir. There are a lot of places that use the same > approach (full path), like binutils, sysroot, ndk/sdk and other tools/libraries. > It seems DEPTH points to some other place, not to the chromium src during the > whole android build. I don't think we have any safe way to pass a path to a file in the source tree to the linker; gyp and the android build system disagree about what relative paths mean. The only workaround I can think of right now is to have a build step which copies this linker script to SHARED_INTERMEDIATE_DIR so you can refer to it there.
https://codereview.chromium.org/147533004/diff/170002/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/147533004/diff/170002/build/common.gypi#newco... build/common.gypi:1633: 'android_linker_script%': '<!(cd <(DEPTH) && pwd -P)/build/android/android_exports.lst', You mustn't use absolute paths here; for the webview we pregenerate makefiles and check them into the android tree, so absolute paths break between different machines.
https://codereview.chromium.org/147533004/diff/170002/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/147533004/diff/170002/base/android/jni_genera... base/android/jni_generator/jni_generator.py:975: java_name += '_00024' + native.java_class_name SoOn 2014/04/22 02:43:08, Yaron wrote: > Why is this logic not needed in GetJNIRegisterNativesString? Looks like I had it > in https://codereview.chromium.org/18161004/. Sorry, missed it. Fixed. https://codereview.chromium.org/147533004/diff/170002/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/147533004/diff/170002/build/common.gypi#newco... build/common.gypi:1633: 'android_linker_script%': '<!(cd <(DEPTH) && pwd -P)/build/android/android_exports.lst', On 2014/04/22 10:53:21, Torne wrote: > You mustn't use absolute paths here; for the webview we pregenerate makefiles > and check them into the android tree, so absolute paths break between different > machines. Done.
https://codereview.chromium.org/147533004/diff/170002/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/147533004/diff/170002/base/android/jni_genera... base/android/jni_generator/jni_generator.py:975: java_name += '_00024' + native.java_class_name On 2014/04/22 02:43:08, Yaron wrote: > Why is this logic not needed in GetJNIRegisterNativesString? Looks like I had it > in https://codereview.chromium.org/18161004/. Actually, RegisterNatives will never be in inner class, so it doesn't need _00024 remaping.
On 2014/04/22 10:52:58, Torne wrote: > On 2014/04/17 17:01:51, ostap wrote: > > On 2014/04/17 14:09:08, bulach wrote: > > > torne may be able to help.. > > > looks like it's trying to find the .lst at: > > > > > > /mnt/scratch0/b_used/build/slave/android_aosp/build/src/build/android/android_exports.lst > > > > > > when the other files look like in: > > > external/chromium_org/... > > > > > > it seems that the GYP step is using the fully qualified name. > > > perhaps it would work with just: > > > <(DEPTH)/build/android/android_exports.lst > > > ? torne? > > > > No, <(DEPTH)/build/android/android_exports.lst doesn't work, because it > > generates relative path to src during the gyp processing stage. Relative path > to > > src can be different for build dir. There are a lot of places that use the > same > > approach (full path), like binutils, sysroot, ndk/sdk and other > tools/libraries. > > It seems DEPTH points to some other place, not to the chromium src during the > > whole android build. > > I don't think we have any safe way to pass a path to a file in the source tree > to the linker; gyp and the android build system disagree about what relative > paths mean. The only workaround I can think of right now is to have a build step > which copies this linker script to SHARED_INTERMEDIATE_DIR so you can refer to > it there. Still linker doesn't find android_exports . From log I see that it was copied to out/target/product/generic/obj/GYP/shared_intermediates/ , but linker doesn't find it at the same place. Any ideas? Thanks.
On 2014/04/24 15:47:52, ostap wrote: > On 2014/04/22 10:52:58, Torne wrote: > > I don't think we have any safe way to pass a path to a file in the source tree > > to the linker; gyp and the android build system disagree about what relative > > paths mean. The only workaround I can think of right now is to have a build > step > > which copies this linker script to SHARED_INTERMEDIATE_DIR so you can refer to > > it there. > > Still linker doesn't find android_exports . From log I see that it was copied to > out/target/product/generic/obj/GYP/shared_intermediates/ , but linker doesn't > find it at the same place. > Any ideas? Hm. I'll apply this patch locally and see if I can figure out what's going wrong.
On 2014/04/25 13:22:45, Torne wrote: > On 2014/04/24 15:47:52, ostap wrote: > > On 2014/04/22 10:52:58, Torne wrote: > > > I don't think we have any safe way to pass a path to a file in the source > tree > > > to the linker; gyp and the android build system disagree about what relative > > > paths mean. The only workaround I can think of right now is to have a build > > step > > > which copies this linker script to SHARED_INTERMEDIATE_DIR so you can refer > to > > > it there. > > > > Still linker doesn't find android_exports . From log I see that it was copied > to > > out/target/product/generic/obj/GYP/shared_intermediates/ , but linker doesn't > > find it at the same place. > > Any ideas? > > Hm. I'll apply this patch locally and see if I can figure out what's going > wrong. Aha, found the issue. It's actually looking for a file literally called "out/target/product/generic/obj/GYP/shared_intermediates/android_exports.lst" - i.e. a directory whose name is actually the *four* characters "out with the double quote included. This is a fun one as it makes the error message very confusing ;) Remove the double quotes from -Wl,--version-script="<(android_linker_script)" and it should work fine; it does when I tested it locally. There shouldn't be any need to quote it.
On 2014/04/25 14:07:50, Torne wrote: > On 2014/04/25 13:22:45, Torne wrote: > > On 2014/04/24 15:47:52, ostap wrote: > > > On 2014/04/22 10:52:58, Torne wrote: > > > > I don't think we have any safe way to pass a path to a file in the source > > tree > > > > to the linker; gyp and the android build system disagree about what > relative > > > > paths mean. The only workaround I can think of right now is to have a > build > > > step > > > > which copies this linker script to SHARED_INTERMEDIATE_DIR so you can > refer > > to > > > > it there. > > > > > > Still linker doesn't find android_exports . From log I see that it was > copied > > to > > > out/target/product/generic/obj/GYP/shared_intermediates/ , but linker > doesn't > > > find it at the same place. > > > Any ideas? > > > > Hm. I'll apply this patch locally and see if I can figure out what's going > > wrong. > > Aha, found the issue. It's actually looking for a file literally called > "out/target/product/generic/obj/GYP/shared_intermediates/android_exports.lst" - > i.e. a directory whose name is actually the *four* characters "out with the > double quote included. This is a fun one as it makes the error message very > confusing ;) > > Remove the double quotes from -Wl,--version-script="<(android_linker_script)" > and it should work fine; it does when I tested it locally. There shouldn't be > any need to quote it. Thanks a lot!
On 2014/04/28 07:01:15, ostap wrote: > Thanks a lot! You're welcome; sorry our build is so weird and inconvenient ;) LGTM.
lgtm thanks
On 2014/04/28 16:02:44, Yaron wrote: > lgtm thanks Adding Mark to reviewers for changes in base.gyp . DaveMoore, please, could you take a look at the changes in mojo/*.gypi ? Thanks.
On 2014/04/28 17:27:25, ostap wrote: > On 2014/04/28 16:02:44, Yaron wrote: > > lgtm thanks > > Adding Mark to reviewers for changes in base.gyp . > > DaveMoore, please, could you take a look at the changes in mojo/*.gypi ? Thanks. ping DaveMoore, Mark Mentovai, could you take a look at the .gyp changes for this CL? DaveMoore, do you think there could be some better solution for the libraries that have non-JNI exports? Thanks!
Seems fine to me as long as Marcus and Yaron are happy. LGTM.
DaveMoore/viettrungluu: can one of you please stamp for mojo/OWNERS?
Sorry for not seeing the ping earlier. I have no idea if this is entirely correct, but since we don't run tests on Android we're obviously okay with being broken, so LGTM.
The CQ bit was checked by sl.ostapenko@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/147533004/290001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
dimich@chromium.org: Please review changes in components/gcm_driver/gcm_driver_android.cc
jochen@chromium.org: Please review changes in components/gcm_driver/gcm_driver_android.cc Thanks
lgtm in general, it's preferable to pick an OWNER as close as possible to the file
The CQ bit was checked by sl.ostapenko@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/147533004/330001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by sl.ostapenko@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/147533004/350001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13743) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/16846)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by sl.ostapenko@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/147533004/370001
Message was sent while issue was closed.
Change committed as 275652
Message was sent while issue was closed.
https://codereview.chromium.org/147533004/diff/370001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/147533004/diff/370001/build/common.gypi#newco... build/common.gypi:1736: 'android_linker_script%': '<!(cd <(DEPTH) && pwd -P)/build/android/android_exports.lst', Unfortunately this has broken the WebView build downstream in Android. Android rely on checking the generated makefiles into the tree once they are generated, and so constructing absolute paths at gyp time results in the makefiles being unusable on any other machine. Can this be done without an absolute path? If not then this might need to be special cased for the WebView build somehow.. :(
Message was sent while issue was closed.
On 2014/06/08 14:18:36, Torne wrote: > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi#newco... > build/common.gypi:1736: 'android_linker_script%': '<!(cd <(DEPTH) && pwd > -P)/build/android/android_exports.lst', > Unfortunately this has broken the WebView build downstream in Android. Android > rely on checking the generated makefiles into the tree once they are generated, > and so constructing absolute paths at gyp time results in the makefiles being > unusable on any other machine. > > Can this be done without an absolute path? If not then this might need to be > special cased for the WebView build somehow.. :( In fact, we previously discussed this in this very review now I look. What happened to the previous version of the patch which copied the file to <(SHARED_INTERMEDIATE_DIR)? That appeared to be working fine for WebView.
Message was sent while issue was closed.
On 2014/06/08 14:21:05, Torne wrote: > On 2014/06/08 14:18:36, Torne wrote: > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi > > File build/common.gypi (right): > > > > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi#newco... > > build/common.gypi:1736: 'android_linker_script%': '<!(cd <(DEPTH) && pwd > > -P)/build/android/android_exports.lst', > > Unfortunately this has broken the WebView build downstream in Android. Android > > rely on checking the generated makefiles into the tree once they are > generated, > > and so constructing absolute paths at gyp time results in the makefiles being > > unusable on any other machine. Sorry, didn't know that. > > Can this be done without an absolute path? If not then this might need to be > > special cased for the WebView build somehow.. :( Already experimenting with it. > In fact, we previously discussed this in this very review now I look. What > happened to the previous version of the patch which copied the file to > <(SHARED_INTERMEDIATE_DIR)? That appeared to be working fine for WebView. There was a dependency to copy exports file in JNI generator rules. Unfortunately, later I got broken targets and flaky builds because some targets don't use JNI generator (memconsumer, for example) and this way build fails if those targets are built before any JNI rule and exports file get copied to out. The right way to do this is to add dependency for every .so library, but I'm getting weird gyp errors when I and 'dependency' inside 'target_conditions'. Workaround that I'm trying to find is to add dependency somewhere that is executed early enough for all targets and copy exports file to the target folder.
Message was sent while issue was closed.
On 2014/06/08 18:28:05, ostap wrote: > On 2014/06/08 14:21:05, Torne wrote: > > On 2014/06/08 14:18:36, Torne wrote: > > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi > > > File build/common.gypi (right): > > > > > > > > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi#newco... > > > build/common.gypi:1736: 'android_linker_script%': '<!(cd <(DEPTH) && pwd > > > -P)/build/android/android_exports.lst', > > > Unfortunately this has broken the WebView build downstream in Android. > Android > > > rely on checking the generated makefiles into the tree once they are > > generated, > > > and so constructing absolute paths at gyp time results in the makefiles > being > > > unusable on any other machine. > > Sorry, didn't know that. > > > > Can this be done without an absolute path? If not then this might need to be > > > special cased for the WebView build somehow.. :( > > Already experimenting with it. Can you revert this change until you find a solution? (I can do it if you want). > > In fact, we previously discussed this in this very review now I look. What > > happened to the previous version of the patch which copied the file to > > <(SHARED_INTERMEDIATE_DIR)? That appeared to be working fine for WebView. > > There was a dependency to copy exports file in JNI generator rules. > Unfortunately, later I got broken targets and flaky builds because some targets > don't use JNI generator (memconsumer, for example) and this way build fails if > those targets are built before any JNI rule and exports file get copied to out. Why do those targets need to use the export file at all then? Perhaps you've already tried this and there's a problem I'm not thinking of, but couldn't you specify the exports file as a link setting for the JNI generator targets, instead of globally? > The right way to do this is to add dependency for every .so library, but I'm > getting weird gyp errors when I and 'dependency' inside 'target_conditions'. The phases of gyp execution are really complicated, but if I remember correctly, target_conditions is expanded after dependencies are already resolved, so I don't think this will work. > Workaround that I'm trying to find is to add dependency somewhere that is > executed early enough for all targets and copy exports file to the target > folder. This is inherently unreliable: even if some such place exists now it's not guaranteed to work for targets added in the future.
Message was sent while issue was closed.
On 2014/06/09 09:53:06, Torne wrote: > On 2014/06/08 18:28:05, ostap wrote: > > On 2014/06/08 14:21:05, Torne wrote: > > > On 2014/06/08 14:18:36, Torne wrote: > > > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi > > > > File build/common.gypi (right): > > > > > > > > > > > > > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi#newco... > > > > build/common.gypi:1736: 'android_linker_script%': '<!(cd <(DEPTH) && pwd > > > > -P)/build/android/android_exports.lst', > > > > Unfortunately this has broken the WebView build downstream in Android. > > Android > > > > rely on checking the generated makefiles into the tree once they are > > > generated, > > > > and so constructing absolute paths at gyp time results in the makefiles > > being > > > > unusable on any other machine. > > > > Sorry, didn't know that. > > > > > > Can this be done without an absolute path? If not then this might need to > be > > > > special cased for the WebView build somehow.. :( > > > > Already experimenting with it. > > Can you revert this change until you find a solution? (I can do it if you want). > > > > In fact, we previously discussed this in this very review now I look. What > > > happened to the previous version of the patch which copied the file to > > > <(SHARED_INTERMEDIATE_DIR)? That appeared to be working fine for WebView. > > > > There was a dependency to copy exports file in JNI generator rules. > > Unfortunately, later I got broken targets and flaky builds because some > targets > > don't use JNI generator (memconsumer, for example) and this way build fails if > > those targets are built before any JNI rule and exports file get copied to > out. > > Why do those targets need to use the export file at all then? They don't use JNI generator, but still have JNI. They need JNI exports. The only exceptions are some mojo libraries which are already handled. > Perhaps you've > already tried this and there's a problem I'm not thinking of, but couldn't you > specify the exports file as a link setting for the JNI generator targets, > instead of globally? There were no non-JNI .so libraries in android build before mojo . Linker exports setting is added only for .so libraries in android build. I think detection, that .so library contains JNI exports would be really complex, because it should also detect that all static libs that are linked also have JNI exports. > > The right way to do this is to add dependency for every .so library, but I'm > > getting weird gyp errors when I and 'dependency' inside 'target_conditions'. > > The phases of gyp execution are really complicated, but if I remember correctly, > target_conditions is expanded after dependencies are already resolved, so I > don't think this will work. I see. That's why gyp cannot find dependency in dependencies list. > > Workaround that I'm trying to find is to add dependency somewhere that is > > executed early enough for all targets and copy exports file to the target > > folder. > This is inherently unreliable: even if some such place exists now it's not > guaranteed to work for targets added in the future. I've tried adding copy directly to target conditions where linker script option added. Like this: 'inputs': [ '<(DEPTH)/build/android/android_exports.lst', ], 'outputs': [ '<(android_linker_script)', ], 'copies': [ { 'destination': '<(SHARED_INTERMEDIATE_DIR)', 'files': [ '<@(_inputs)', ], }, ], It works, but I'm getting a lot of warnings from ninja: ninja: warning: multiple rules generate gen/android_exports.lst. builds involving this target will not be correct; continuing anyway There is already similar warning for icudtl.dat, but it is single one and for android_exports.lst I'm getting about 100 of them.
Message was sent while issue was closed.
On 2014/06/09 14:46:40, ostap wrote: > On 2014/06/09 09:53:06, Torne wrote: > > On 2014/06/08 18:28:05, ostap wrote: > > > On 2014/06/08 14:21:05, Torne wrote: > > > > On 2014/06/08 14:18:36, Torne wrote: > > > > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi > > > > > File build/common.gypi (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/147533004/diff/370001/build/common.gypi#newco... > > > > > build/common.gypi:1736: 'android_linker_script%': '<!(cd <(DEPTH) && pwd > > > > > -P)/build/android/android_exports.lst', > > > > > Unfortunately this has broken the WebView build downstream in Android. > > > Android > > > > > rely on checking the generated makefiles into the tree once they are > > > > generated, > > > > > and so constructing absolute paths at gyp time results in the makefiles > > > being > > > > > unusable on any other machine. > > > > > > Sorry, didn't know that. > > > > > > > > Can this be done without an absolute path? If not then this might need > to > > be > > > > > special cased for the WebView build somehow.. :( > > > > > > Already experimenting with it. > > > > Can you revert this change until you find a solution? (I can do it if you > want). > > > > > > In fact, we previously discussed this in this very review now I look. What > > > > happened to the previous version of the patch which copied the file to > > > > <(SHARED_INTERMEDIATE_DIR)? That appeared to be working fine for WebView. > > > > > > There was a dependency to copy exports file in JNI generator rules. > > > Unfortunately, later I got broken targets and flaky builds because some > > targets > > > don't use JNI generator (memconsumer, for example) and this way build fails > if > > > those targets are built before any JNI rule and exports file get copied to > > out. > > > > Why do those targets need to use the export file at all then? > > They don't use JNI generator, but still have JNI. They need JNI exports. > The only exceptions are some mojo libraries which are already handled. They must use the JNI generator somewhere in their transitive dependencies, though, otherwise they wouldn't need this at all? > > Perhaps you've > > already tried this and there's a problem I'm not thinking of, but couldn't you > > specify the exports file as a link setting for the JNI generator targets, > > instead of globally? > > There were no non-JNI .so libraries in android build before mojo . > Linker exports setting is added only for .so libraries in android build. > I think detection, that .so library contains JNI exports would be really > complex, > because it should also detect that all static libs that are linked also have > JNI exports. gyp can do this for you: link_settings of a target are exported to any linkable target which depends on it, even indirectly. So if every target which generates JNI calls specifies that it depends on the target which copies the export list, and also declares in its link_settings that the export list should be used, then any linkable target which consumes it should get the linker setting, and won't get built until the copying is done. > > > The right way to do this is to add dependency for every .so library, but I'm > > > getting weird gyp errors when I and 'dependency' inside 'target_conditions'. > > > > The phases of gyp execution are really complicated, but if I remember > correctly, > > target_conditions is expanded after dependencies are already resolved, so I > > don't think this will work. > > I see. That's why gyp cannot find dependency in dependencies list. > > > > Workaround that I'm trying to find is to add dependency somewhere that is > > > executed early enough for all targets and copy exports file to the target > > > folder. > > This is inherently unreliable: even if some such place exists now it's not > > guaranteed to work for targets added in the future. > > I've tried adding copy directly to target conditions where linker script option > added. > Like this: > > 'inputs': [ > '<(DEPTH)/build/android/android_exports.lst', > ], > 'outputs': [ > '<(android_linker_script)', > ], > 'copies': [ > { > 'destination': '<(SHARED_INTERMEDIATE_DIR)', > 'files': [ > '<@(_inputs)', > ], > }, > ], > > > It works, but I'm getting a lot of warnings from ninja: > > ninja: warning: multiple rules generate gen/android_exports.lst. builds > involving this target will not be correct; continuing anyway > > There is already similar warning for icudtl.dat, but it is single one and for > android_exports.lst I'm getting about 100 of them. You can't do this; you must not have multiple rules that generate the same file. The warning for icudtl.dat is a bug that should be fixed as well.
On 2014/06/09 15:03:30, Torne wrote:
> On 2014/06/09 14:46:40, ostap wrote:
> > > Why do those targets need to use the export file at all then?
> >
> > They don't use JNI generator, but still have JNI. They need JNI exports.
> > The only exceptions are some mojo libraries which are already handled.
>
> They must use the JNI generator somewhere in their transitive dependencies,
> though, otherwise they wouldn't need this at all?
Instead of using JNI generator they do exports manually, like in
memconsumer_hook.cc :
extern "C" {
JNIEXPORT void JNICALL
Java_org_chromium_memconsumer_ResidentService_nativeUseMemory(JNIEnv* env,
jobject clazz,
jlong memory);
}
But it still exports JNI symbols, so I'd prefer to keep linker script to be
consistent with android libraries.
In fact, it appeared, that memconsumer is the only project that has JNI exports,
but doesn't have any dependency on other chromium components. I added dependency
on linker script copy and it solves the problem.
> > > Perhaps you've
> > > already tried this and there's a problem I'm not thinking of, but couldn't
> you
> > > specify the exports file as a link setting for the JNI generator targets,
> > > instead of globally?
> >
> > There were no non-JNI .so libraries in android build before mojo .
> > Linker exports setting is added only for .so libraries in android build.
> > I think detection, that .so library contains JNI exports would be really
> > complex,
> > because it should also detect that all static libs that are linked also have
> > JNI exports.
>
> gyp can do this for you: link_settings of a target are exported to any
linkable
> target which depends on it, even indirectly. So if every target which
generates
> JNI calls specifies that it depends on the target which copies the export
list,
> and also declares in its link_settings that the export list should be used,
then
> any linkable target which consumes it should get the linker setting, and won't
> get built until the copying is done.
I've tried playing with 'ldflags' in jni_generator.gypi and
jar_file_jni_generator.gypi . Couldn't make it add linker option from those
files.
I've uploaded working solution that doesn't produce any warnings. It is similar
to my previous patch, but handles dependencies a bit better.
Unfortunately, it cannot be landed until
https://code.google.com/p/chromium/issues/detail?id=382481 is fixed.
On 2014/06/09 21:02:41, ostap wrote:
> On 2014/06/09 15:03:30, Torne wrote:
> > On 2014/06/09 14:46:40, ostap wrote:
> > > > Why do those targets need to use the export file at all then?
> > >
> > > They don't use JNI generator, but still have JNI. They need JNI exports.
> > > The only exceptions are some mojo libraries which are already handled.
> >
> > They must use the JNI generator somewhere in their transitive dependencies,
> > though, otherwise they wouldn't need this at all?
>
> Instead of using JNI generator they do exports manually, like in
> memconsumer_hook.cc :
>
> extern "C" {
> JNIEXPORT void JNICALL
> Java_org_chromium_memconsumer_ResidentService_nativeUseMemory(JNIEnv* env,
> jobject
clazz,
> jlong
memory);
> }
>
> But it still exports JNI symbols, so I'd prefer to keep linker script to be
> consistent with android libraries.
> In fact, it appeared, that memconsumer is the only project that has JNI
exports,
> but doesn't have any dependency on other chromium components. I added
dependency
> on linker script copy and it solves the problem.
>
> > > > Perhaps you've
> > > > already tried this and there's a problem I'm not thinking of, but
couldn't
> > you
> > > > specify the exports file as a link setting for the JNI generator
targets,
> > > > instead of globally?
> > >
> > > There were no non-JNI .so libraries in android build before mojo .
> > > Linker exports setting is added only for .so libraries in android build.
> > > I think detection, that .so library contains JNI exports would be really
> > > complex,
> > > because it should also detect that all static libs that are linked also
have
> > > JNI exports.
> >
> > gyp can do this for you: link_settings of a target are exported to any
> linkable
> > target which depends on it, even indirectly. So if every target which
> generates
> > JNI calls specifies that it depends on the target which copies the export
> list,
> > and also declares in its link_settings that the export list should be used,
> then
> > any linkable target which consumes it should get the linker setting, and
won't
> > get built until the copying is done.
>
> I've tried playing with 'ldflags' in jni_generator.gypi and
> jar_file_jni_generator.gypi . Couldn't make it add linker option from those
> files.
Were you putting it inside a link_settings section? See
https://code.google.com/p/gyp/wiki/GypLanguageSpecification#targets
> I've uploaded working solution that doesn't produce any warnings. It is
similar
> to my previous patch, but handles dependencies a bit better.
> Unfortunately, it cannot be landed until
> https://code.google.com/p/chromium/issues/detail?id=382481 is fixed.
On 2014/06/10 09:23:57, Torne wrote: > On 2014/06/09 21:02:41, ostap wrote: > > I've tried playing with 'ldflags' in jni_generator.gypi and > > jar_file_jni_generator.gypi . Couldn't make it add linker option from those > > files. > > Were you putting it inside a link_settings section? See > https://code.google.com/p/gyp/wiki/GypLanguageSpecification#targets Hm! No, I was using ldflags without link_settings. A lot of other places also use it outside of link_settings. But inside of link_setting it worked. Very interesting difference. I've uploaded updated patch.
All gyp settings normally only apply to the current target. If the current target is a target that requires running the linker (i.e. an executable or shared library) then it's perfectly fine to specify ldflags directly. link_settings just makes the setting propagate across dependencies to anything that links this target. https://codereview.chromium.org/147533004/diff/410001/build/common.gypi File build/common.gypi (left): https://codereview.chromium.org/147533004/diff/410001/build/common.gypi#oldco... build/common.gypi:4220: '-Wl,--exclude-libs=ALL', If we remove this, doesn't it mean that any target which *doesn't* depend on JNI will end up with all its symbols exported? https://codereview.chromium.org/147533004/diff/410001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/147533004/diff/410001/build/common.gypi#newco... build/common.gypi:4159: 'inputs': [ Does this actually have any effect? I wouldn't expect "inputs" here to do anything, since it's not inside of an action or rule.
On 2014/06/11 10:29:47, Torne wrote: > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi > File build/common.gypi (left): > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi#oldco... > build/common.gypi:4220: '-Wl,--exclude-libs=ALL', > If we remove this, doesn't it mean that any target which *doesn't* depend on JNI > will end up with all its symbols exported? There is very few libs like this. 2 in mojo which already handled. I'll check how I can do better. > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi#newco... > build/common.gypi:4159: 'inputs': [ > Does this actually have any effect? I wouldn't expect "inputs" here to do > anything, since it's not inside of an action or rule. Hm! I though that it will add direct dependency to .so linking. Unfortunately, it doesn't. I think it would be useful if target could specify inputs and outputs and gyp could do automatic dependency mapping.
On 2014/06/11 16:01:23, ostap wrote: > On 2014/06/11 10:29:47, Torne wrote: > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi > > File build/common.gypi (left): > > > > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi#oldco... > > build/common.gypi:4220: '-Wl,--exclude-libs=ALL', > > If we remove this, doesn't it mean that any target which *doesn't* depend on > JNI > > will end up with all its symbols exported? > > There is very few libs like this. 2 in mojo which already handled. > I'll check how I can do better. > > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi > > File build/common.gypi (right): > > > > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi#newco... > > build/common.gypi:4159: 'inputs': [ > > Does this actually have any effect? I wouldn't expect "inputs" here to do > > anything, since it's not inside of an action or rule. > > Hm! > I though that it will add direct dependency to .so linking. Unfortunately, it > doesn't. Indeed. gyp unfortunately doesn't tell you when something is meaningless or has no effect; unknown things are just silently ignored. > I think it would be useful if target could specify inputs and outputs and gyp > could do automatic dependency mapping. It might be, but it's also not how it works. The build rules generated don't build *targets*, they build individual files, which are the things that have inputs and outputs.
On 2014/06/11 16:01:23, ostap wrote: > On 2014/06/11 10:29:47, Torne wrote: > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi > > File build/common.gypi (left): > > > > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi#oldco... > > build/common.gypi:4220: '-Wl,--exclude-libs=ALL', > > If we remove this, doesn't it mean that any target which *doesn't* depend on > JNI > > will end up with all its symbols exported? > > There is very few libs like this. 2 in mojo which already handled. > I'll check how I can do better. > > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi > > File build/common.gypi (right): > > > > > https://codereview.chromium.org/147533004/diff/410001/build/common.gypi#newco... > > build/common.gypi:4159: 'inputs': [ > > Does this actually have any effect? I wouldn't expect "inputs" here to do > > anything, since it's not inside of an action or rule. > > Hm! > I though that it will add direct dependency to .so linking. Unfortunately, it > doesn't. > I think it would be useful if target could specify inputs and outputs and gyp > could do automatic dependency mapping. just FYI, I'll try to land https://codereview.chromium.org/329223002/, i.e., enabling the crazy_linker for content shell and chrome shell... this will make it more realistic with the full chrome, but unfortunate, it'll prevent this from relanding. :(
On 2014/06/11 16:03:07, Torne wrote: I uploaded patch that fixes non-JNI shared libs. It also solves mojo "exceptions". Thanks a lot for you reviews!
On 2014/06/11 16:03:46, bulach wrote: > just FYI, I'll try to land https://codereview.chromium.org/329223002/, i.e., > enabling the crazy_linker for content shell and chrome shell... > this will make it more realistic with the full chrome, but unfortunate, it'll > prevent this from relanding. :( Yes, I know that it was coming. Thanks. I'm looking how to intercept dlopen and make it work inside crazy linker.
Thanks a lot for iterating on this. The new arrangement of settings in gyp LGTM. I don't know what we can do about the crazy linker interaction though.
+mkosiba who has some thoughts on how to improve this.
Message was sent while issue was closed.
On 2014/08/07 17:56:37, Torne wrote: > +mkosiba who has some thoughts on how to improve this. Closing, the code landed as https://codereview.chromium.org/454923002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
