Reland: Actually use --extra-r-text-files in process_resources.py

Reland: 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}

[Ian Cullinan, 2015-05-07 23:00:50]

cullinan@amazon.com changed reviewers:
+ gunsch@chromium.org, msw@chromium.org

[Ian Cullinan, 2015-05-07 23:00:51]

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.

[msw, 2015-05-07 23:40:16]

This doesn't seem to break mojo/runner:mojo_runner_apk as before; thanks!

[Yaron, 2015-05-08 15:46:00]

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

[gunsch, 2015-05-08 17:48:14]

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

[Ian Cullinan, 2015-05-08 18:04:08]

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!

[cjhopman, 2015-05-08 18:08:49]

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).

[gunsch, 2015-05-08 18:41:27]

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!

[Ian Cullinan, 2015-05-08 18:44:15]

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.

[Ian Cullinan, 2015-05-08 18:44:55]

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.

[cjhopman, 2015-05-08 19:51:51]

lgtm

[Ian Cullinan, 2015-05-08 20:23:21]

The CQ bit was checked by cullinan@amazon.com

[Ian Cullinan, 2015-05-08 20:23:21]

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")

[commit-bot: I haz the power, 2015-05-08 20:24:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136653002/60001

[commit-bot: I haz the power, 2015-05-08 20:36:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-08 20:36:16]

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...)

[Ian Cullinan, 2015-05-08 20:38:24]

cullinan@amazon.com changed reviewers:
+ dpranke@chromium.org

[Ian Cullinan, 2015-05-08 20:38:25]

+dpranke

Looks like I need an OWNER review for //BUILD.gn

PTAL

[Dirk Pranke, 2015-05-08 23:05:23]

lgtm

[Ian Cullinan, 2015-05-08 23:12:02]

The CQ bit was checked by cullinan@amazon.com

[commit-bot: I haz the power, 2015-05-08 23:12:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136653002/60001

[commit-bot: I haz the power, 2015-05-09 02:22:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-09 02:22:46]

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_...)

[Ian Cullinan, 2015-05-11 14:22:21]

The CQ bit was checked by cullinan@amazon.com

[commit-bot: I haz the power, 2015-05-11 14:22:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136653002/60001

[commit-bot: I haz the power, 2015-05-11 15:39:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 15:39:36]

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_...)

[Ian Cullinan, 2015-05-11 16:56:45]

cullinan@amazon.com changed reviewers:
+ nednguyen@google.com

[Ian Cullinan, 2015-05-11 16:56:46]

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?

[Ian Cullinan, 2015-05-11 16:57:06]

The CQ bit was checked by cullinan@amazon.com to run a CQ dry run

[Ian Cullinan, 2015-05-11 16:57:06]

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")

[commit-bot: I haz the power, 2015-05-11 17:01:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136653002/80001

[commit-bot: I haz the power, 2015-05-11 18:37:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 18:37:41]

Dry run: This issue passed the CQ dry run.

[Ian Cullinan, 2015-05-11 19:02:11]

The CQ bit was checked by cullinan@amazon.com

[commit-bot: I haz the power, 2015-05-11 19:03:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136653002/80001

[commit-bot: I haz the power, 2015-05-11 19:11:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-11 19:12:31]

Patchset 5 (id:??) landed as
https://crrev.com/4b5d0300e3fa2a382ac3c1c157247a82583d7ed0
Cr-Commit-Position: refs/heads/master@{#329219}