Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
334413006:
Add support for uncompress library in APK to the build system (Closed)
|
Created:
6 years, 6 months ago by Anton Modified:
6 years, 5 months ago Reviewers:
rmcilroy CC:
chromium-reviews, klundberg+watch_chromium.org, erikwright+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd support for uncompress library in APK to the build system
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280346
Patch Set 1 #
Total comments: 18
Patch Set 2 : Update for Ross' review comments #Patch Set 3 : Update for Ross' review #
Total comments: 13
Patch Set 4 : Update for Ross' review #
Total comments: 2
Patch Set 5 : Update for Ross' review #
Total comments: 2
Patch Set 6 : Update for Ross' review #
Messages
Total messages: 14 (0 generated)
This change depends on https://codereview.chromium.org/333433002, please rubber stamp that first (it must land first).
https://codereview.chromium.org/334413006/diff/1/base/android/java/templates/... File base/android/java/templates/NativeLibraries.template (right): https://codereview.chromium.org/334413006/diff/1/base/android/java/templates/... base/android/java/templates/NativeLibraries.template:47: #define ENABLE_CHROMIUM_LINKER 1 I really think this should be an error rather than silently enabling the chromium linker (see comment below in java_apk.gypi. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... File build/android/gyp/finalize_apk.py (right): https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:90: parser.add_option('--uncompress-lib', type='int', As discussed below, I think you should also add a '--rename-lib' option, both for clarity on what the options do, and to enable independently testing of apks which contain an uncompressed library which is not loaded in-place by the crazy linker (i.e., is still copied and loaded by system linker). https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:91: help='If non-zero, build an APK with the library uncompressed') Please make this an "action='store_true'" type of option rather than an int type. Also add a full stop in the help text here and below to be consistent with the other options. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:92: parser.add_option('--rezip-path', help='Path to the rezip executable') nit - move this up to be below --zipalign-path https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:106: apk_to_sign = intermediate_file.name + ".librenamed" Use the same "with tempfile.NamedTemporaryFile()" approach to create the temporary intermediate filesnames as is used for signed_apk_path to create the intermediates you need. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:128: else: I think this is pretty hacky to have two complete branches for the different flows (which leads to maintenance issues if someone changes one branch without remembering to change the other). With the --rename_library flag added, how about merging it into one branch, e.g.: if (options.rename_library): RenameLibInApk(...) else: apk_to_sign = unsigned_apk_path SignApk(...) DropDataDescriptorsInApk(...) # presumably this is safe to do even if we aren't uncompressing the library ? if (options.uncompress_lib): UncompressLibAndPageAlignInApk(...) else: apk_to_align = apk_without_descriptors AlignApk(...) WDYT? https://codereview.chromium.org/334413006/diff/1/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/334413006/diff/1/build/java_apk.gypi#newcode49 build/java_apk.gypi:49: # use_library_in_zip_file - When using the dynamic linker, load the library nit - load_library_from_zip_file https://codereview.chromium.org/334413006/diff/1/build/java_apk.gypi#newcode275 build/java_apk.gypi:275: '--defines', 'ENABLE_CHROMIUM_LINKER_LIBRARY_IN_ZIP_FILE', Does this work? I would have though the linker_gcc_preprocess_defines would have been redefined in line 286 below? In any case, it seems simpler to have a one-to-one mapping between these defines and the gyp variables - e.g., only check for load_library_from_zip_file == 1, and set a new variable (e.g., load_from_zip_gcc_preprocessor_defines) which adds a ENABLE_LOAD_LIBRARY_FROM_ZIP_FILE define, and append it to gcc_preprocessor_defines in line 308 below. (and as mentioned in NativeLibraries.template - make it an error to define ENABLE_LOAD_LIBRARY_FROM_ZIP_FILE but not ENABLE_CHROMIUM_LINKER).
https://codereview.chromium.org/334413006/diff/1/base/android/java/templates/... File base/android/java/templates/NativeLibraries.template (right): https://codereview.chromium.org/334413006/diff/1/base/android/java/templates/... base/android/java/templates/NativeLibraries.template:47: #define ENABLE_CHROMIUM_LINKER 1 On 2014/06/19 09:46:12, rmcilroy wrote: > I really think this should be an error rather than silently enabling the > chromium linker (see comment below in java_apk.gypi. Made it an error to have both. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... File build/android/gyp/finalize_apk.py (right): https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:90: parser.add_option('--uncompress-lib', type='int', On 2014/06/19 09:46:12, rmcilroy wrote: > As discussed below, I think you should also add a '--rename-lib' option, both > for clarity on what the options do, and to enable independently testing of apks > which contain an uncompressed library which is not loaded in-place by the crazy > linker (i.e., is still copied and loaded by system linker). It is over engineering to implement functionality we don't want or need and it will make this tool more complicated and harder to understand. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:91: help='If non-zero, build an APK with the library uncompressed') On 2014/06/19 09:46:12, rmcilroy wrote: > Please make this an "action='store_true'" type of option rather than an int > type. Also add a full stop in the help text here and below to be consistent > with the other options. This just makes things more complicated with no upside. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:92: parser.add_option('--rezip-path', help='Path to the rezip executable') On 2014/06/19 09:46:12, rmcilroy wrote: > nit - move this up to be below --zipalign-path Done. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:106: apk_to_sign = intermediate_file.name + ".librenamed" On 2014/06/19 09:46:12, rmcilroy wrote: > Use the same "with tempfile.NamedTemporaryFile()" approach to create the > temporary intermediate filesnames as is used for signed_apk_path to create the > intermediates you need. Done. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:128: else: On 2014/06/19 09:46:12, rmcilroy wrote: > I think this is pretty hacky to have two complete branches for the different > flows (which leads to maintenance issues if someone changes one branch without > remembering to change the other). With the --rename_library flag added, how > about merging it into one branch, e.g.: > > if (options.rename_library): > RenameLibInApk(...) > else: > apk_to_sign = unsigned_apk_path > SignApk(...) > DropDataDescriptorsInApk(...) # presumably this is safe to do even if we > aren't uncompressing the library ? > if (options.uncompress_lib): > UncompressLibAndPageAlignInApk(...) > else: > apk_to_align = apk_without_descriptors > AlignApk(...) > > WDYT? Actually I separated it into two branches which made it look much cleaner and more readable. https://codereview.chromium.org/334413006/diff/1/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/334413006/diff/1/build/java_apk.gypi#newcode49 build/java_apk.gypi:49: # use_library_in_zip_file - When using the dynamic linker, load the library On 2014/06/19 09:46:12, rmcilroy wrote: > nit - load_library_from_zip_file Done. https://codereview.chromium.org/334413006/diff/1/build/java_apk.gypi#newcode275 build/java_apk.gypi:275: '--defines', 'ENABLE_CHROMIUM_LINKER_LIBRARY_IN_ZIP_FILE', On 2014/06/19 09:46:12, rmcilroy wrote: > Does this work? I would have though the linker_gcc_preprocess_defines would > have been redefined in line 286 below? In any case, it seems simpler to have a > one-to-one mapping between these defines and the gyp variables - e.g., only > check for load_library_from_zip_file == 1, and set a new variable (e.g., > load_from_zip_gcc_preprocessor_defines) which adds a > ENABLE_LOAD_LIBRARY_FROM_ZIP_FILE define, and append it to > gcc_preprocessor_defines in line 308 below. (and as mentioned in > NativeLibraries.template - make it an error to define > ENABLE_LOAD_LIBRARY_FROM_ZIP_FILE but not > ENABLE_CHROMIUM_LINKER). What you are suggesting does not work. The tool does not in fact accept a list as you might expect from the GYP file.
https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... File build/android/gyp/finalize_apk.py (right): https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:90: parser.add_option('--uncompress-lib', type='int', On 2014/06/19 13:39:45, Anton wrote: > On 2014/06/19 09:46:12, rmcilroy wrote: > > As discussed below, I think you should also add a '--rename-lib' option, both > > for clarity on what the options do, and to enable independently testing of > apks > > which contain an uncompressed library which is not loaded in-place by the > crazy > > linker (i.e., is still copied and loaded by system linker). > > It is over engineering to implement functionality we don't want or need and it > will make this tool more complicated and harder to understand. I disagree, for testing and debugging as stated above. https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... build/android/gyp/finalize_apk.py:128: else: On 2014/06/19 13:39:45, Anton wrote: > On 2014/06/19 09:46:12, rmcilroy wrote: > > I think this is pretty hacky to have two complete branches for the different > > flows (which leads to maintenance issues if someone changes one branch without > > remembering to change the other). With the --rename_library flag added, how > > about merging it into one branch, e.g.: > > > > if (options.rename_library): > > RenameLibInApk(...) > > else: > > apk_to_sign = unsigned_apk_path > > SignApk(...) > > DropDataDescriptorsInApk(...) # presumably this is safe to do even if we > > aren't uncompressing the library ? > > if (options.uncompress_lib): > > UncompressLibAndPageAlignInApk(...) > > else: > > apk_to_align = apk_without_descriptors > > AlignApk(...) > > > > WDYT? > > Actually I separated it into two branches which made it look much cleaner and > more readable. I disagree entirely - it makes it more difficult to follow the differences and will end up leading to steps being added to one branch but not the other. https://codereview.chromium.org/334413006/diff/40001/build/android/finalize_a... File build/android/finalize_apk_action.gypi (right): https://codereview.chromium.org/334413006/diff/40001/build/android/finalize_a... build/android/finalize_apk_action.gypi:26: # Webview doesn't use zipalign or rezip nit - fullstop. https://codereview.chromium.org/334413006/diff/40001/build/android/gyp/finali... File build/android/gyp/finalize_apk.py (right): https://codereview.chromium.org/334413006/diff/40001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:98: with tempfile.NamedTemporaryFile() as apk_to_sign_tmp: If we are using python 2.7, you can just do: with tempfile.NamedTemporaryFile() as intermediate_file, tempfile.NamedTemporaryFile() as apk_without_descriptors_tmp, .... to avoid the nested with's
On 2014/06/20 14:42:59, rmcilroy wrote: > https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... > File build/android/gyp/finalize_apk.py (right): > > https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... > build/android/gyp/finalize_apk.py:90: parser.add_option('--uncompress-lib', > type='int', > On 2014/06/19 13:39:45, Anton wrote: > > On 2014/06/19 09:46:12, rmcilroy wrote: > > > As discussed below, I think you should also add a '--rename-lib' option, > both > > > for clarity on what the options do, and to enable independently testing of > > apks > > > which contain an uncompressed library which is not loaded in-place by the > > crazy > > > linker (i.e., is still copied and loaded by system linker). > > > > It is over engineering to implement functionality we don't want or need and it > > will make this tool more complicated and harder to understand. > > I disagree, for testing and debugging as stated above. > > https://codereview.chromium.org/334413006/diff/1/build/android/gyp/finalize_a... > build/android/gyp/finalize_apk.py:128: else: > On 2014/06/19 13:39:45, Anton wrote: > > On 2014/06/19 09:46:12, rmcilroy wrote: > > > I think this is pretty hacky to have two complete branches for the different > > > flows (which leads to maintenance issues if someone changes one branch > without > > > remembering to change the other). With the --rename_library flag added, how > > > about merging it into one branch, e.g.: > > > > > > if (options.rename_library): > > > RenameLibInApk(...) > > > else: > > > apk_to_sign = unsigned_apk_path > > > SignApk(...) > > > DropDataDescriptorsInApk(...) # presumably this is safe to do even if we > > > aren't uncompressing the library ? > > > if (options.uncompress_lib): > > > UncompressLibAndPageAlignInApk(...) > > > else: > > > apk_to_align = apk_without_descriptors > > > AlignApk(...) > > > > > > WDYT? > > > > Actually I separated it into two branches which made it look much cleaner and > > more readable. > > I disagree entirely - it makes it more difficult to follow the differences and > will end up leading to steps being added to one branch but not the other. > > https://codereview.chromium.org/334413006/diff/40001/build/android/finalize_a... > File build/android/finalize_apk_action.gypi (right): > > https://codereview.chromium.org/334413006/diff/40001/build/android/finalize_a... > build/android/finalize_apk_action.gypi:26: # Webview doesn't use zipalign or > rezip > nit - fullstop. > > https://codereview.chromium.org/334413006/diff/40001/build/android/gyp/finali... > File build/android/gyp/finalize_apk.py (right): > > https://codereview.chromium.org/334413006/diff/40001/build/android/gyp/finali... > build/android/gyp/finalize_apk.py:98: with tempfile.NamedTemporaryFile() as > apk_to_sign_tmp: > If we are using python 2.7, you can just do: > with tempfile.NamedTemporaryFile() as intermediate_file, > tempfile.NamedTemporaryFile() as apk_without_descriptors_tmp, .... > > to avoid the nested with's PTAL
Thanks - getting there. Couple more comments. https://codereview.chromium.org/334413006/diff/60001/base/android/java/templa... File base/android/java/templates/NativeLibraries.template (right): https://codereview.chromium.org/334413006/diff/60001/base/android/java/templa... base/android/java/templates/NativeLibraries.template:45: #if defined(ENABLE_CHROMIUM_LINKER_LIBRARY_IN_ZIP_FILE) nit - #if defined(ENABLE_CHROMIUM_LINKER_LIBRARY_IN_ZIP_FILE) && !defined(ENABLE_CHROMIUM_LINKER) https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... File build/android/gyp/finalize_apk.py (right): https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:84: help='If non-zero, build an APK with the library uncompressed.') update help comment to mention libraries are renamed and that they can only be loaded by the crazy linker if this flag is set. I also think you need a better name flag name than "--uncompress-lib" since the tool does much more than this if you set it. https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:88: in_apk = options.unsigned_apk_path current_apk_file_path https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:90: with tempfile.NamedTemporaryFile() as intermediate_file, \ rename intermediate_file appropriately (signed_apk_tmp)? https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:117: aligned_apk = options.final_apk_path Do the temp name tracking one way throughout the function - either the updating of the current_apk_file_path, as above, or the if / else you are doing here. https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:120: AlignApk(options.zipalign_path, in_apk, aligned_apk) you need to move the aligned_apk to options.final_apk_path if !options.uncompress_lib. https://codereview.chromium.org/334413006/diff/60001/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/334413006/diff/60001/build/java_apk.gypi#newc... build/java_apk.gypi:290: 'linker_load_from_zip_file_preprocess_defines': [], nit- linker_load_from_zip_file_gcc_preprocess_defines
https://codereview.chromium.org/334413006/diff/60001/base/android/java/templa... File base/android/java/templates/NativeLibraries.template (right): https://codereview.chromium.org/334413006/diff/60001/base/android/java/templa... base/android/java/templates/NativeLibraries.template:45: #if defined(ENABLE_CHROMIUM_LINKER_LIBRARY_IN_ZIP_FILE) On 2014/06/25 17:25:19, rmcilroy wrote: > nit - #if defined(ENABLE_CHROMIUM_LINKER_LIBRARY_IN_ZIP_FILE) && > !defined(ENABLE_CHROMIUM_LINKER) Done. https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... File build/android/gyp/finalize_apk.py (right): https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:84: help='If non-zero, build an APK with the library uncompressed.') On 2014/06/25 17:25:19, rmcilroy wrote: > update help comment to mention libraries are renamed and that they can only be > loaded by the crazy linker if this flag is set. I also think you need a better > name flag name than "--uncompress-lib" since the tool does much more than this > if you set it. Done. https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:88: in_apk = options.unsigned_apk_path On 2014/06/25 17:25:19, rmcilroy wrote: > current_apk_file_path Done. https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:90: with tempfile.NamedTemporaryFile() as intermediate_file, \ On 2014/06/25 17:25:19, rmcilroy wrote: > rename intermediate_file appropriately (signed_apk_tmp)? Done. https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:117: aligned_apk = options.final_apk_path On 2014/06/25 17:25:19, rmcilroy wrote: > Do the temp name tracking one way throughout the function - either the updating > of the current_apk_file_path, as above, or the if / else you are doing here. This is not the current_apk_file_path https://codereview.chromium.org/334413006/diff/60001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:120: AlignApk(options.zipalign_path, in_apk, aligned_apk) On 2014/06/25 17:25:19, rmcilroy wrote: > you need to move the aligned_apk to options.final_apk_path if > !options.uncompress_lib. This is what L117 is doing.
https://codereview.chromium.org/334413006/diff/80001/build/android/gyp/finali... File build/android/gyp/finalize_apk.py (right): https://codereview.chromium.org/334413006/diff/80001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:97: if options.load_library_from_zip_file: I still think the flow of this function is a bit unclear. How about this: if options.load_library_from_zip_file: apk_to_sign = apk_to_sign_tmp.name RenameLibInApk(options.rezip_path, options.unsigned_apk_path, apk_to_sign) else: apk_to_sign = options.unsigned_apk_path SignApk(options.key_path, options.key_name, options.key_passwd, apk_to_sign, signed_apk_path) if options.load_library_from_zip_file: apk_to_align = apk_without_descriptors_tmp.name aligned_apk = aligned_apk_tmp.name DropDataDescriptorsInApk( options.rezip_path, signed_apk_path, apk_to_align) else: apk_to_align = signed_apk_path aligned_apk = options.final_apk_path AlignApk(options.zipalign_path, apk_to_align, aligned_apk) if options.load_library_from_zip_file: UncompressLibAndPageAlignInApk( options.rezip_path, aligned_apk, options.final_apk_path)
https://codereview.chromium.org/334413006/diff/80001/build/android/gyp/finali... File build/android/gyp/finalize_apk.py (right): https://codereview.chromium.org/334413006/diff/80001/build/android/gyp/finali... build/android/gyp/finalize_apk.py:97: if options.load_library_from_zip_file: On 2014/06/27 10:02:15, rmcilroy wrote: > I still think the flow of this function is a bit unclear. How about this: > > if options.load_library_from_zip_file: > apk_to_sign = apk_to_sign_tmp.name > RenameLibInApk(options.rezip_path, options.unsigned_apk_path, apk_to_sign) > else: > apk_to_sign = options.unsigned_apk_path > > SignApk(options.key_path, options.key_name, options.key_passwd, > apk_to_sign, signed_apk_path) > > if options.load_library_from_zip_file: > apk_to_align = apk_without_descriptors_tmp.name > aligned_apk = aligned_apk_tmp.name > DropDataDescriptorsInApk( > options.rezip_path, signed_apk_path, apk_to_align) > else: > apk_to_align = signed_apk_path > aligned_apk = options.final_apk_path > > AlignApk(options.zipalign_path, apk_to_align, aligned_apk) > > if options.load_library_from_zip_file: > UncompressLibAndPageAlignInApk( > options.rezip_path, aligned_apk, options.final_apk_path) Done.
looks much better to me - thanks! lgtm https://codereview.chromium.org/334413006/diff/100001/base/android/java/templ... File base/android/java/templates/NativeLibraries.template (right): https://codereview.chromium.org/334413006/diff/100001/base/android/java/templ... base/android/java/templates/NativeLibraries.template:46: !defined(ENABLE_CHROMIUM_LINKER) nit - indent +2
https://codereview.chromium.org/334413006/diff/100001/base/android/java/templ... File base/android/java/templates/NativeLibraries.template (right): https://codereview.chromium.org/334413006/diff/100001/base/android/java/templ... base/android/java/templates/NativeLibraries.template:46: !defined(ENABLE_CHROMIUM_LINKER) On 2014/06/27 10:48:03, rmcilroy wrote: > nit - indent +2 Done.
The CQ bit was checked by anton@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anton@chromium.org/334413006/120001
Message was sent while issue was closed.
Change committed as 280346 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
