Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 7 months ago by Ian Cullinan Modified:
5 years, 7 months ago Reviewers:
Dirk Pranke, msw, nednguyen, yfriedman, gunsch, cjhopman, Yaron CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, yfriedman+watch_chromium.org, klundberg+watch_chromium.org, jbudorick+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland: Actually use --extra-r-text-files in process_resources.py
Originally landed in crrev.com/1104703003
Reverted in crrev.com/1129103002 due to breaking GN APKs with no
resouces. This now handles the case where aapt doesn't generate an
R.txt (no resources) by creating an empty R.txt.
Generate extra packages' R.java files only containing the resources
listed in their R.txt files. This significantly reduces the number of
DEX field IDs taken up by resources.
BUG=480036
R=cjhopman,yfriedman
Committed: https://crrev.com/4b5d0300e3fa2a382ac3c1c157247a82583d7ed0
Cr-Commit-Position: refs/heads/master@{#329219}
Patch Set 1 : Original patch #Patch Set 2 : Handle APKs with no resources and add a --include-all-resources option #
Total comments: 3
Patch Set 3 : Fix nits #
Total comments: 1
Patch Set 4 : Clobber instead of touching empty R.txt output #Patch Set 5 : No bad_resources apk #
Created: 5 years, 7 months ago
Messages
Total messages: 38 (14 generated)
cullinan@amazon.com changed reviewers: + gunsch@chromium.org, msw@chromium.org
cjhopman: build/android yfriedman: chrome/android/sync_shell PTAL This version creates empty R.txt files for targets that have no resources, and introduces a --include-all-resources flag (exposed as include_all_resources in GYP and GN) that does things the old way. I've included a bad_resources_test_apk example that reproduces the build problems we've identified so far and demonstrates the use of the --include-all-resources flag. Happy to take that out or leave it in, whichever you prefer.
This doesn't seem to break mojo/runner:mojo_runner_apk as before; thanks!
On 2015/05/07 23:40:16, msw wrote: > This doesn't seem to break mojo/runner:mojo_runner_apk as before; thanks! chrome/android/sync_shell lgtm
Note: this doesn't fix the Cast internal case of the APK, but it might be due to what cjhopman suggested about our using additional_res_packages incorrectly (e.g. without additional_R_text_files), and getting by because of all resources being included all the time. We can fix this in our downstream issue. This does, however, seem to fix the issue where at runtime we didn't actually have resources from another package (the com/android/tv/settings resources mentioned in the previous CL discussion). So LGTM from chromecast/, thanks! https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... File build/android/tests/bad_resources/bad_resources.gyp (right): https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... build/android/tests/bad_resources/bad_resources.gyp:1: # Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... build/android/tests/bad_resources/bad_resources.gyp:4: { nit: blank line above https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... build/android/tests/bad_resources/bad_resources.gyp:44: # Include this APK's resources in the R.java files of it's dependencies grammar nit: it's --> its
On 2015/05/08 17:48:14, gunsch wrote: > Note: this doesn't fix the Cast internal case of the APK, but it might be due to > what cjhopman suggested about our using additional_res_packages incorrectly > (e.g. without additional_R_text_files), and getting by because of all resources > being included all the time. We can fix this in our downstream issue. Does setting include_all_resources on your APK target workaround the problem in the meantime? > This does, however, seem to fix the issue where at runtime we didn't actually > have resources from another package (the com/android/tv/settings resources > mentioned in the previous CL discussion). So LGTM from chromecast/, thanks! > > https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... > File build/android/tests/bad_resources/bad_resources.gyp (right): > > https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... > build/android/tests/bad_resources/bad_resources.gyp:1: # Copyright (c) 2015 The > Chromium Authors. All rights reserved. > nit: no (c) > > https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... > build/android/tests/bad_resources/bad_resources.gyp:4: { > nit: blank line above > > https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... > build/android/tests/bad_resources/bad_resources.gyp:44: # Include this APK's > resources in the R.java files of it's dependencies > grammar nit: it's --> its All fixed. Thanks for the review!
https://codereview.chromium.org/1136653002/diff/40001/build/android/gyp/proce... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/1136653002/diff/40001/build/android/gyp/proce... build/android/gyp/process_resources.py:410: build_utils.Touch(options.r_text_out) I think you want to make sure that options.r_text_out is empty here (a previous build may have left a non-empty one there).
On 2015/05/08 18:04:08, Ian Cullinan wrote: > On 2015/05/08 17:48:14, gunsch wrote: > > Note: this doesn't fix the Cast internal case of the APK, but it might be due > to > > what cjhopman suggested about our using additional_res_packages incorrectly > > (e.g. without additional_R_text_files), and getting by because of all > resources > > being included all the time. We can fix this in our downstream issue. > > Does setting include_all_resources on your APK target workaround the problem in > the meantime? I can confirm that also works, thanks for the suggestion. It looks from this CL like that flag basically just disables the new behavior? > > > This does, however, seem to fix the issue where at runtime we didn't actually > > have resources from another package (the com/android/tv/settings resources > > mentioned in the previous CL discussion). So LGTM from chromecast/, thanks! > > > > > https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... > > File build/android/tests/bad_resources/bad_resources.gyp (right): > > > > > https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... > > build/android/tests/bad_resources/bad_resources.gyp:1: # Copyright (c) 2015 > The > > Chromium Authors. All rights reserved. > > nit: no (c) > > > > > https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... > > build/android/tests/bad_resources/bad_resources.gyp:4: { > > nit: blank line above > > > > > https://codereview.chromium.org/1136653002/diff/20001/build/android/tests/bad... > > build/android/tests/bad_resources/bad_resources.gyp:44: # Include this APK's > > resources in the R.java files of it's dependencies > > grammar nit: it's --> its > > All fixed. Thanks for the review!
On 2015/05/08 18:08:49, cjhopman wrote: > https://codereview.chromium.org/1136653002/diff/40001/build/android/gyp/proce... > File build/android/gyp/process_resources.py (right): > > https://codereview.chromium.org/1136653002/diff/40001/build/android/gyp/proce... > build/android/gyp/process_resources.py:410: > build_utils.Touch(options.r_text_out) > I think you want to make sure that options.r_text_out is empty here (a previous > build may have left a non-empty one there). You're right. Fixed.
On 2015/05/08 18:41:27, gunsch wrote: > On 2015/05/08 18:04:08, Ian Cullinan wrote: > > On 2015/05/08 17:48:14, gunsch wrote: > > > Note: this doesn't fix the Cast internal case of the APK, but it might be > due > > to > > > what cjhopman suggested about our using additional_res_packages incorrectly > > > (e.g. without additional_R_text_files), and getting by because of all > > resources > > > being included all the time. We can fix this in our downstream issue. > > > > Does setting include_all_resources on your APK target workaround the problem > in > > the meantime? > > I can confirm that also works, thanks for the suggestion. It looks from this CL > like that flag basically just disables the new behavior? Correct.
lgtm
The CQ bit was checked by cullinan@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1136653002/#ps60001 (title: "Clobber instead of touching empty R.txt output")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136653002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
cullinan@amazon.com changed reviewers: + dpranke@chromium.org
+dpranke Looks like I need an OWNER review for //BUILD.gn PTAL
lgtm
The CQ bit was checked by cullinan@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136653002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cullinan@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136653002/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_...)
cullinan@amazon.com changed reviewers: + nednguyen@google.com
telemetry_dependencies_unittest thinks that this CL adds a new dependency (the bad_resources example) to Telemetry. Does anyone object to landing this without the bad_resources example?
The CQ bit was checked by cullinan@amazon.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, gunsch@chromium.org, cjhopman@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1136653002/#ps80001 (title: "No bad_resources apk")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136653002/80001
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 cullinan@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136653002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4b5d0300e3fa2a382ac3c1c157247a82583d7ed0 Cr-Commit-Position: refs/heads/master@{#329219} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
