GN: Enable loading libraries from apks

GN: Enable loading libraries from apks

This enables all the bits for an apk that uses uncompressed native
libraries that are loaded directly from the apk without being extracted
on the device.

BUG=359249
TBR=brettw
NOTRY=true

Committed: https://crrev.com/3151133068b2d1cfd44af0e8a546b640989aeb3d
Cr-Commit-Position: refs/heads/master@{#300814}

[cjhopman, 2014-10-16 18:00:07]

cjhopman@chromium.org changed reviewers:
+ anton@chromium.org

[cjhopman, 2014-10-16 18:00:07]

anton: ptal

[cjhopman, 2014-10-21 02:03:44]

cjhopman@chromium.org changed reviewers:
+ rmcilroy@chromium.org
- anton@chromium.org

[cjhopman, 2014-10-21 02:03:44]

rmcilroy: ptal as anton@ is OOO

[rmcilroy, 2014-10-21 10:55:45]

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.g...
build/config/android/rules.gni:838: assert(_use_chromium_linker || true)
I'm not sure what this is testing?

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.g...
build/config/android/rules.gni:864: if (is_component_build || cpu_arch ==
"arm64" || cpu_arch == "x64") {
I realize you only moved this, but we still want _use_chromium_linker for arm64
and x64 (they are enabled in gyp now).

https://codereview.chromium.org/654383002/diff/1/content/shell/android/BUILD.gn
File content/shell/android/BUILD.gn (right):

https://codereview.chromium.org/654383002/diff/1/content/shell/android/BUILD....
content/shell/android/BUILD.gn:101: load_library_from_apk = true
We don't do this for the content shell APK with gyp - is there a reason your
doing this for content shell with GN or was it just testing?  I think it would
be best to keep gyp and GN the same - if so we can remove the changes to
ChromiumLinkerTestActivity.java / ContentShellActivity.java as well.

[cjhopman, 2014-10-21 17:28:53]

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.g...
build/config/android/rules.gni:838: assert(_use_chromium_linker || true)
On 2014/10/21 10:55:45, rmcilroy wrote:
> I'm not sure what this is testing?
Eh, this just marks _use_chromium_linker as "used" so that gn doesn't complain
if nothing below uses it. I've moved the variable into something guarded so that
it's always used.

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.g...
build/config/android/rules.gni:864: if (is_component_build || cpu_arch ==
"arm64" || cpu_arch == "x64") {
On 2014/10/21 10:55:45, rmcilroy wrote:
> I realize you only moved this, but we still want _use_chromium_linker for
arm64
> and x64 (they are enabled in gyp now).
Done. It is not enabled in upstream targets for arm64/x64 in gyp, afaict though
(see chrome/chrome_android.gypi).

I've moved the concept of whether the linker is supported into it's own
config.gni file so that we don't end up with similar inconsistencies in GN.

https://codereview.chromium.org/654383002/diff/1/content/shell/android/BUILD.gn
File content/shell/android/BUILD.gn (right):

https://codereview.chromium.org/654383002/diff/1/content/shell/android/BUILD....
content/shell/android/BUILD.gn:101: load_library_from_apk = true
On 2014/10/21 10:55:45, rmcilroy wrote:
> We don't do this for the content shell APK with gyp - is there a reason your
> doing this for content shell with GN or was it just testing?  I think it would
> be best to keep gyp and GN the same - if so we can remove the changes to
> ChromiumLinkerTestActivity.java / ContentShellActivity.java as well.

We definitely should be doing this for some target upstream so that we can
actually catch regressions before they are committed to chromium. I'll leave it
out of this change, but we should update gyp and gn at some point to use it.

[rmcilroy, 2014-10-21 21:21:03]

rmcilroy@chromium.org changed reviewers:
+ simonb@chromium.org

[rmcilroy, 2014-10-21 21:21:04]

+simonb: Could you verify that removing the target_arch conditions upstream is
OK?

lgtm with some comments.

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.g...
build/config/android/rules.gni:864: if (is_component_build || cpu_arch ==
"arm64" || cpu_arch == "x64") {
On 2014/10/21 17:28:52, cjhopman wrote:
> On 2014/10/21 10:55:45, rmcilroy wrote:
> > I realize you only moved this, but we still want _use_chromium_linker for
> arm64
> > and x64 (they are enabled in gyp now).
> Done. It is not enabled in upstream targets for arm64/x64 in gyp, afaict
though
> (see chrome/chrome_android.gypi).
> 
> I've moved the concept of whether the linker is supported into it's own
> config.gni file so that we don't end up with similar inconsistencies in GN.
> 

You are right, I was thinking of downstream.  Thanks for changing this.  I'm not
super familiar with the gyp/gn transition, but do you need to include
chromium_linker_supported in chrome_android.gypi in order to keep gyp/gn
consistant, or does the use of it in rules.gni do this?  If not, could you
update chrome_android.gypi to remove the cpu_arch conditions?

I think it should be consistent upstream and downstream, but adding simonb just
in case there was a reason for this.

https://codereview.chromium.org/654383002/diff/1/content/shell/android/BUILD.gn
File content/shell/android/BUILD.gn (right):

https://codereview.chromium.org/654383002/diff/1/content/shell/android/BUILD....
content/shell/android/BUILD.gn:101: load_library_from_apk = true
On 2014/10/21 17:28:53, cjhopman wrote:
> On 2014/10/21 10:55:45, rmcilroy wrote:
> > We don't do this for the content shell APK with gyp - is there a reason your
> > doing this for content shell with GN or was it just testing?  I think it
would
> > be best to keep gyp and GN the same - if so we can remove the changes to
> > ChromiumLinkerTestActivity.java / ContentShellActivity.java as well.
> 
> We definitely should be doing this for some target upstream so that we can
> actually catch regressions before they are committed to chromium. I'll leave
it
> out of this change, but we should update gyp and gn at some point to use it.

Agreed, I'll take a look at enabling the linker tests and chrome_test_shell once
this CL lands.

https://codereview.chromium.org/654383002/diff/20001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/654383002/diff/20001/build/config/android/rul...
build/config/android/rules.gni:630: template("java_library") {
I'm not sure how gn works, but this looks pretty similar to the
android_java_library template - is there any way the common rules can be split
out to a common shared template?

[cjhopman, 2014-10-22 21:44:07]

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/654383002/diff/1/build/config/android/rules.g...
build/config/android/rules.gni:864: if (is_component_build || cpu_arch ==
"arm64" || cpu_arch == "x64") {
On 2014/10/21 21:21:03, rmcilroy wrote:
> On 2014/10/21 17:28:52, cjhopman wrote:
> > On 2014/10/21 10:55:45, rmcilroy wrote:
> > > I realize you only moved this, but we still want _use_chromium_linker for
> > arm64
> > > and x64 (they are enabled in gyp now).
> > Done. It is not enabled in upstream targets for arm64/x64 in gyp, afaict
> though
> > (see chrome/chrome_android.gypi).
> > 
> > I've moved the concept of whether the linker is supported into it's own
> > config.gni file so that we don't end up with similar inconsistencies in GN.
> > 
> 
> You are right, I was thinking of downstream.  Thanks for changing this.  I'm
not
> super familiar with the gyp/gn transition, but do you need to include
> chromium_linker_supported in chrome_android.gypi in order to keep gyp/gn
> consistant, or does the use of it in rules.gni do this?  If not, could you
> update chrome_android.gypi to remove the cpu_arch conditions?
> 
> I think it should be consistent upstream and downstream, but adding simonb
just
> in case there was a reason for this.

In gyp we could do something similar... it would look like:

calculate chromium_linker_supported in some linker_config.gypi (or, as is
typical in gyp, build/common.gypi)
use that in java_apk.gypi and turn off the linker when it's not supported
targets that want to use the linker then just enable without calculating on
their own whether the linker is supported (though they could feel free to use
the flag).

I think that would be a little better than what we are doing now, but didn't
feel strongly enough to make that change in gyp myself. I want the GN build
rules, though, to sort of start off with good practices as examples.

https://codereview.chromium.org/654383002/diff/20001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/654383002/diff/20001/build/config/android/rul...
build/config/android/rules.gni:630: template("java_library") {
On 2014/10/21 21:21:03, rmcilroy wrote:
> I'm not sure how gn works, but this looks pretty similar to the
> android_java_library template - is there any way the common rules can be split
> out to a common shared template?
Yeah, it's similar. Almost all of the work is already shared in the compile_java
template. Most of this template is just forwarding the arguments to compile_java
(which is kind of a verbose operation in GN).

I expect in the future android and non-android java libraries will essentially
be using exactly the same templates with an "android_java" or some such flag to
control the parts that are different. I don't want to block bringing up the
targets on that change, though.

[cjhopman, 2014-10-22 22:24:53]

cjhopman@chromium.org changed reviewers:
+ brettw@chromium.org

[cjhopman, 2014-10-22 22:25:32]

TBR=brettw for OWNERS of //BUILD.gn and //base/base.gyp

[cjhopman, 2014-10-22 23:39:25]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-10-22 23:40:56]

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

[cjhopman, 2014-10-23 01:02:31]

The CQ bit was unchecked by cjhopman@chromium.org

[cjhopman, 2014-10-23 01:02:32]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-10-23 01:03:36]

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

[commit-bot: I haz the power, 2014-10-23 01:05:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-23 01:06:13]

Patchset 3 (id:??) landed as
https://crrev.com/3151133068b2d1cfd44af0e8a546b640989aeb3d
Cr-Commit-Position: refs/heads/master@{#300814}