Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1473273002:
GN(Android): Add libosmesa.so to ContentShell.apk (Closed)
|
Created:
5 years ago by agrieve Modified:
5 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, Peter Beverloo, jam, jbudorick+watch_chromium.org, darin-cc_chromium.org, mikecase+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, klundberg+watch_chromium.org, jochen+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN(Android): Add libosmesa.so to ContentShell.apk
BUG=559289
Committed: https://crrev.com/da37b9ba5421dc03b28d507c2de7a422790a190a
Cr-Commit-Position: refs/heads/master@{#362866}
Patch Set 1 #
Total comments: 10
Patch Set 2 : add comment #
Total comments: 2
Patch Set 3 : rebase #
Total comments: 4
Patch Set 4 : comments #
Messages
Total messages: 31 (11 generated)
agrieve@chromium.org changed reviewers: + piman@chromium.org, pkotwicz@chromium.org
pkotwicz - Please look at build/ piman - Please look at content/ and third_party/mesa/
lgtm
Andrew, did you manage to get ContentShell.apk not to crash when --use-gl=osmesa is passed on the command line? ContentShell.apk crashed for me both when using a GYP build and when using a GN build with your changes. https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1199: # * not side-loaded for _incremental targets. Can you please: - Explain why someone would want to use this argument (loadable library) - Mention that people should probably use native_libs instead - Remove comments about how |loadable_modules| is different from |native_libs|. From reading the comments I am unable to make a decision which I should use. https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1389: write_build_config(build_config_target) { In GYP, osmesa is written to NativeLibraries.java I guess that makes the GN version more correct https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1623: rebased_gdbserver, Can we add the loadable_modules into the APK the same way that we add the gdbserver into the APK? This would allow us to keep on using |native_libs| and having to change apkbuilder.py https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1708: native_libs = _loadable_modules You are right. Having both |native_libs| and |native_lib_dir| is super confusing. I trust that you will make me happy again soon by removing |native_libs_dir|
https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1199: # * not side-loaded for _incremental targets. On 2015/11/25 01:18:34, pkotwicz wrote: > Can you please: > - Explain why someone would want to use this argument (loadable library) > - Mention that people should probably use native_libs instead > - Remove comments about how |loadable_modules| is different from |native_libs|. > From reading the comments I am unable to make a decision which I should use. Added to the end: # Use this instead of native_libs when you are going to load the library # conditionally, and only when native_libs doesn't work for you. I'd like to leave the existing points though, as I do think they help explain when you'd use it. https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1389: write_build_config(build_config_target) { On 2015/11/25 01:18:34, pkotwicz wrote: > In GYP, osmesa is written to NativeLibraries.java > I guess that makes the GN version more correct Acknowledged. https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1623: rebased_gdbserver, On 2015/11/25 01:18:34, pkotwicz wrote: > Can we add the loadable_modules into the APK the same way that we add the > gdbserver into the APK? > This would allow us to keep on using |native_libs| and having to change > apkbuilder.py Right now incremental install pushes all libs that end up in the output directory of "prepare_native", so I want to keep the file out of that intermediate directory. I'll look into if it's necessary to have gdbserver in there as well though. https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1708: native_libs = _loadable_modules On 2015/11/25 01:18:34, pkotwicz wrote: > You are right. Having both |native_libs| and |native_lib_dir| is super > confusing. I trust that you will make me happy again soon by removing > |native_libs_dir| Acknowledged.
Just to confirm, did you manage to get ContentShell.apk not to crash when --use-gl=osmesa is passed on the command line? ContentShell.apk crashed for me both when using a GYP build and when using a GN build with your changes. https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1623: rebased_gdbserver, For the sake of curiosity, why do we want to exclude libosmesa.so from the list of pushed libraries during incremental install? The package_apk() invocation for incremental installs does not forward native_libs_dir to the package_apk() template. Is see that this CL forwards native_libs
On 2015/11/25 22:41:25, pkotwicz wrote: > Just to confirm, did you manage to get ContentShell.apk not to crash when > --use-gl=osmesa > is passed on the command line? > ContentShell.apk crashed for me both when using a GYP build and when using a GN > build with your changes. Ah, right sorry. For me it crashed on Android L, but ran on Jellybean. > > https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.gni > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... > build/config/android/rules.gni:1623: rebased_gdbserver, > For the sake of curiosity, why do we want to exclude libosmesa.so from the list > of pushed libraries during incremental install? Since it would be unused. > > The package_apk() invocation for incremental installs does not forward > native_libs_dir to the package_apk() template. Is see that this CL forwards > native_libs The way libosmesa.so is loaded by chrome is not compatible with how libs for incremental install are side-loaded. So I pass native_libs=loadable_modules for _incremental so that the library is still packaged in the normal way.
https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1473273002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1623: rebased_gdbserver, On 2015/11/25 21:26:57, agrieve wrote: > On 2015/11/25 01:18:34, pkotwicz wrote: > > Can we add the loadable_modules into the APK the same way that we add the > > gdbserver into the APK? > > This would allow us to keep on using |native_libs| and having to change > > apkbuilder.py > > Right now incremental install pushes all libs that end up in the output > directory of "prepare_native", so I want to keep the file out of that > intermediate directory. > > I'll look into if it's necessary to have gdbserver in there as well though. okay, learned about gdbserver and it looks like I should be changing it to do the same as what loadable_modules is doing here (always add it to the apk). when debugging via adb_gdb, it's not used, but when debugging via Android Studio / Eclipse, it assumes that it's there. Will address in a follow-up
L - G - T - M once you remove native-libs-dir arg from apkbuilder.py https://codereview.chromium.org/1473273002/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1473273002/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:672: # write_asset_list: Adds an extra file to the assets, which contains a list of Let's get rid of this in this CL. Usually, I am against big CLs but having both native_libs & native_libs_dir is just too confusing https://codereview.chromium.org/1473273002/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:1043: "native_libs", Please add a comment as to why you are including native_libs Please also rename native_libs variable. optionally_loadable_native_libs perhaps?
On 2015/11/26 21:17:45, pkotwicz wrote: > L - G - T - M once you remove native-libs-dir arg from apkbuilder.py > > https://codereview.chromium.org/1473273002/diff/20001/build/config/android/in... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/1473273002/diff/20001/build/config/android/in... > build/config/android/internal_rules.gni:672: # write_asset_list: Adds an extra > file to the assets, which contains a list of > Let's get rid of this in this CL. > > Usually, I am against big CLs but having both native_libs & native_libs_dir is > just too confusing > > https://codereview.chromium.org/1473273002/diff/20001/build/config/android/in... > build/config/android/internal_rules.gni:1043: "native_libs", > Please add a comment as to why you are including native_libs > Please also rename native_libs variable. optionally_loadable_native_libs > perhaps? rebased onto split-out other CLs. ptal
build/config/android/rules.gni LGTM Thank you for being patient with me! I am not comfortable reviewing build/toolchain https://codereview.chromium.org/1473273002/diff/40001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1473273002/diff/40001/build/config/android/ru... build/config/android/rules.gni:1196: # * dependencies of this .so are not automatically included, Nit: Remove trailing comma https://codereview.chromium.org/1473273002/diff/40001/build/config/android/ru... build/config/android/rules.gni:1198: # * load_library_from_apk and enable_relocation_packing do not apply use_chromium_linker also does not apply
jbudorick@ can you please take a look at the changes in build/toolchain ?
Description was changed from ========== GN(Android): Add libosmesa.so to ContentShell.apk BUG=559289 ========== to ========== GN(Android): Add libosmesa.so to ContentShell.apk BUG=559289 ==========
agrieve@chromium.org changed reviewers: + dpranke@chromium.org
On 2015/12/02 01:33:24, pkotwicz wrote: > jbudorick@ can you please take a look at the changes in build/toolchain ? dpranke might be a better person to do that. Switched to R=dpranke.
https://codereview.chromium.org/1473273002/diff/40001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1473273002/diff/40001/build/config/android/ru... build/config/android/rules.gni:1196: # * dependencies of this .so are not automatically included, On 2015/12/02 01:33:05, pkotwicz wrote: > Nit: Remove trailing comma Done. https://codereview.chromium.org/1473273002/diff/40001/build/config/android/ru... build/config/android/rules.gni:1198: # * load_library_from_apk and enable_relocation_packing do not apply On 2015/12/02 01:33:05, pkotwicz wrote: > use_chromium_linker also does not apply Done.
On 2015/12/02 14:40:28, agrieve wrote: > On 2015/12/02 01:33:24, pkotwicz wrote: > > jbudorick@ can you please take a look at the changes in build/toolchain ? > > dpranke might be a better person to do that. Switched to R=dpranke. yeah, I haven't done anything with build/toolchain/.
lgtm
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1473273002/#ps60001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473273002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473273002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by agrieve@chromium.org
The CQ bit was unchecked by agrieve@chromium.org
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473273002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473273002/60001
Message was sent while issue was closed.
Description was changed from ========== GN(Android): Add libosmesa.so to ContentShell.apk BUG=559289 ========== to ========== GN(Android): Add libosmesa.so to ContentShell.apk BUG=559289 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== GN(Android): Add libosmesa.so to ContentShell.apk BUG=559289 ========== to ========== GN(Android): Add libosmesa.so to ContentShell.apk BUG=559289 Committed: https://crrev.com/da37b9ba5421dc03b28d507c2de7a422790a190a Cr-Commit-Position: refs/heads/master@{#362866} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/da37b9ba5421dc03b28d507c2de7a422790a190a Cr-Commit-Position: refs/heads/master@{#362866} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
