Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319273002:
Make secondary abi work for component build (Closed)
|
Created:
4 years, 3 months ago by michaelbai Modified:
4 years, 3 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org, Dirk Pranke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake secondary abi work for component build
Add secondary_abi_shared_libraries to pack all the secondary abi
shared libraries into APK.
BUG=622855
Committed: https://crrev.com/3ac692c2e67e8030a30e4ebcd12240352944f898
Cr-Commit-Position: refs/heads/master@{#417441}
Patch Set 1 #
Total comments: 13
Patch Set 2 : address the comments #Patch Set 3 : change to root_out_dir #Patch Set 4 : sync #Patch Set 5 : add missing dep #
Total comments: 1
Messages
Total messages: 28 (15 generated)
Description was changed from ========== Make secondary abi work for component build Add secondary_abi_shared_libraries to pack all the secondary abi shared libraries into APK. BUG=622855 ========== to ========== Make secondary abi work for component build Add secondary_abi_shared_libraries to pack all the secondary abi shared libraries into APK. BUG=622855 ==========
michaelbai@chromium.org changed reviewers: + agrieve@chromium.org
Just some nits. lgtm https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn#newc... build/android/BUILD.gn:66: "${root_shlib_dir}/lib.unstripped/${_soname}", nit: these should stay as root_out_dir. If root_shlib_dir ever becomes != root_out_dir, this will break (maybe what we'd want to add is a "root_shlib_stripped_dir", but I doubt it's worth it at this point unless we're going to update all the mentions. https://codereview.chromium.org/2319273002/diff/1/build/android/gyp/apkbuilde... File build/android/gyp/apkbuilder.py (right): https://codereview.chromium.org/2319273002/diff/1/build/android/gyp/apkbuilde... build/android/gyp/apkbuilder.py:193: depfile_deps += list(secondary_native_libs) Does this do anything? https://codereview.chromium.org/2319273002/diff/1/build/android/gyp/write_bui... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2319273002/diff/1/build/android/gyp/write_bui... build/android/gyp/write_build_config.py:647: secondary_abi_java_libraries_list = ('{%s}' % ','.join( nit: this line is non-trivial. Would be better as a helper function I think. https://codereview.chromium.org/2319273002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2319273002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:2687: "file_list_json", nit: don't forward file_list_json and libraries_filearg since they aren't actually being "forwarded" to action(). Instead, just use invoker. to reference them below. https://codereview.chromium.org/2319273002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:2694: packed_libraries_dir = "$target_gen_dir/$target_name/packed-libs" nit: prefix with _
PTAL https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn#newc... build/android/BUILD.gn:66: "${root_shlib_dir}/lib.unstripped/${_soname}", On 2016/09/08 01:02:15, agrieve wrote: > nit: these should stay as root_out_dir. If root_shlib_dir ever becomes != > root_out_dir, this will break (maybe what we'd want to add is a > "root_shlib_stripped_dir", but I doubt it's worth it at this point unless we're > going to update all the mentions. the root_shlib_dir doesn't equal to root_out_dir for secondary toolchain, e.g. root_shlib_dir is out/debug/arm, but root_out_dir is still out/debug. https://codereview.chromium.org/2319273002/diff/1/build/android/gyp/apkbuilde... File build/android/gyp/apkbuilder.py (right): https://codereview.chromium.org/2319273002/diff/1/build/android/gyp/apkbuilde... build/android/gyp/apkbuilder.py:193: depfile_deps += list(secondary_native_libs) On 2016/09/08 01:02:15, agrieve wrote: > Does this do anything? No difference, I wanted to match the native_libs, remove it. https://codereview.chromium.org/2319273002/diff/1/build/android/gyp/write_bui... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2319273002/diff/1/build/android/gyp/write_bui... build/android/gyp/write_build_config.py:647: secondary_abi_java_libraries_list = ('{%s}' % ','.join( On 2016/09/08 01:02:15, agrieve wrote: > nit: this line is non-trivial. Would be better as a helper function I think. Done. https://codereview.chromium.org/2319273002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2319273002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:2687: "file_list_json", On 2016/09/08 01:02:15, agrieve wrote: > nit: don't forward file_list_json and libraries_filearg since they aren't > actually being "forwarded" to action(). Instead, just use invoker. to reference > them below. Thanks, sometime, I am confused about what should be forward. https://codereview.chromium.org/2319273002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:2694: packed_libraries_dir = "$target_gen_dir/$target_name/packed-libs" On 2016/09/08 01:02:15, agrieve wrote: > nit: prefix with _ Done.
https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn#newc... build/android/BUILD.gn:66: "${root_shlib_dir}/lib.unstripped/${_soname}", On 2016/09/08 18:52:50, michaelbai wrote: > On 2016/09/08 01:02:15, agrieve wrote: > > nit: these should stay as root_out_dir. If root_shlib_dir ever becomes != > > root_out_dir, this will break (maybe what we'd want to add is a > > "root_shlib_stripped_dir", but I doubt it's worth it at this point unless > we're > > going to update all the mentions. > > the root_shlib_dir doesn't equal to root_out_dir for secondary toolchain, e.g. > root_shlib_dir is out/debug/arm, but root_out_dir is still out/debug. Are you sure? root_build_dir should be out/debug, but root_out_dir should be out/debug/arm If that's not the case, there is something subtle going on.
https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn#newc... build/android/BUILD.gn:66: "${root_shlib_dir}/lib.unstripped/${_soname}", On 2016/09/08 18:57:18, agrieve wrote: > On 2016/09/08 18:52:50, michaelbai wrote: > > On 2016/09/08 01:02:15, agrieve wrote: > > > nit: these should stay as root_out_dir. If root_shlib_dir ever becomes != > > > root_out_dir, this will break (maybe what we'd want to add is a > > > "root_shlib_stripped_dir", but I doubt it's worth it at this point unless > > we're > > > going to update all the mentions. > > > > the root_shlib_dir doesn't equal to root_out_dir for secondary toolchain, e.g. > > root_shlib_dir is out/debug/arm, but root_out_dir is still out/debug. > > Are you sure? root_build_dir should be out/debug, but root_out_dir should be > out/debug/arm > > If that's not the case, there is something subtle going on. Yes, they are same for both toolchains, what could be wrong in your mind? or we can only find out when we run into it.
https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2319273002/diff/1/build/android/BUILD.gn#newc... build/android/BUILD.gn:66: "${root_shlib_dir}/lib.unstripped/${_soname}", On 2016/09/08 19:03:41, michaelbai wrote: > On 2016/09/08 18:57:18, agrieve wrote: > > On 2016/09/08 18:52:50, michaelbai wrote: > > > On 2016/09/08 01:02:15, agrieve wrote: > > > > nit: these should stay as root_out_dir. If root_shlib_dir ever becomes != > > > > root_out_dir, this will break (maybe what we'd want to add is a > > > > "root_shlib_stripped_dir", but I doubt it's worth it at this point unless > > > we're > > > > going to update all the mentions. > > > > > > the root_shlib_dir doesn't equal to root_out_dir for secondary toolchain, > e.g. > > > root_shlib_dir is out/debug/arm, but root_out_dir is still out/debug. > > > > Are you sure? root_build_dir should be out/debug, but root_out_dir should be > > out/debug/arm > > > > If that's not the case, there is something subtle going on. > > Yes, they are same for both toolchains, what could be wrong in your mind? or we > can only find out when we run into it. Sorry, misread your comment, You are right, the root_build_dir is always out/debug, but root_out_dir is out/debug/arm
The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by michaelbai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2319273002/#ps80001 (title: "add missing dep")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make secondary abi work for component build Add secondary_abi_shared_libraries to pack all the secondary abi shared libraries into APK. BUG=622855 ========== to ========== Make secondary abi work for component build Add secondary_abi_shared_libraries to pack all the secondary abi shared libraries into APK. BUG=622855 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make secondary abi work for component build Add secondary_abi_shared_libraries to pack all the secondary abi shared libraries into APK. BUG=622855 ========== to ========== Make secondary abi work for component build Add secondary_abi_shared_libraries to pack all the secondary abi shared libraries into APK. BUG=622855 Committed: https://crrev.com/3ac692c2e67e8030a30e4ebcd12240352944f898 Cr-Commit-Position: refs/heads/master@{#417441} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3ac692c2e67e8030a30e4ebcd12240352944f898 Cr-Commit-Position: refs/heads/master@{#417441}
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
FYI dpranke: Yet another thing that broke V8 downstream: https://codereview.chromium.org/2325873003/ ERROR at //build/config/android/internal_rules.gni:8:1: Can't load input file. import("//third_party/android_platform/config.gni") ^------------------------------------------------- Unable to load: /usr/local/google/home/machenbach/v8/v8/third_party/android_platform/config.gni I also checked in the secondary tree for: /usr/local/google/home/machenbach/v8/v8/build/secondary/third_party/android_platform/config.gni See //build/config/android/rules.gni:6:1: whence it was imported. import("//build/config/android/internal_rules.gni") ^------------------------------------------------- See //BUILD.gn:12:3: whence it was imported. import("//build/config/android/rules.gni") ^---------------------------------------- GN gen failed: 1
Message was sent while issue was closed.
Could you try to find a solution asap (e.g. revert this) that unblocks the V8 auto-roller? https://codereview.chromium.org/2319273002/diff/80001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2319273002/diff/80001/build/config/android/in... build/config/android/internal_rules.gni:8: import("//third_party/android_platform/config.gni") Unconditionally importing things outside of //build breaks every other project relying on build and not having these dependencies.
Message was sent while issue was closed.
On 2016/09/09 09:00:44, machenbach (slow) wrote: > Could you try to find a solution asap (e.g. revert this) that unblocks the V8 > auto-roller? > > https://codereview.chromium.org/2319273002/diff/80001/build/config/android/in... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/2319273002/diff/80001/build/config/android/in... > build/config/android/internal_rules.gni:8: > import("//third_party/android_platform/config.gni") > Unconditionally importing things outside of //build breaks every other project > relying on build and not having these dependencies. Sorry for missing this. We'll try and reland with this fixed.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2327063002/ by agrieve@chromium.org. The reason for reverting is: Broke v8 roller. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
