Add use_apk_split parameter to java_apk.gypi / android_apk (gn)

Add use_apk_split parameter to java_apk.gypi / android_apk (gn)

Testing:
- For GN, splits install and crash on start-up just like non-split version
- For GYP, tested with:
  - component=shared_library, component=static_library
  - gyp_managed_install=1, 0
  - load_library_from_zip_file=1, 0

BUG=484797
Committed: https://crrev.com/63349e4f235ae666faff75c1eee2da001fb7341f
Cr-Commit-Position: refs/heads/master@{#330523}

[agrieve, 2015-05-13 15:06:03]

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

[agrieve, 2015-05-13 20:25:36]

agrieve@chromium.org changed reviewers:
+ yfriedman@chromium.org

[agrieve, 2015-05-13 20:25:36]

yfriedman@chromium.org: Please review changes in 

LibraryLoader.java

[Yaron, 2015-05-13 21:38:23]

https://codereview.chromium.org/1137313003/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1137313003/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:258:
apkFilePath = getLibraryApkPath(context);
I'm confused. Isn't this for the case where load_library_from_zip_file==1 ?
Perhaps include with your upcoming linker change? Or do you need this to get far
enough and actually load the library when load_library_from_zip_file is 0?

https://codereview.chromium.org/1137313003/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:374:
private static String getLibraryApkPath(Context context) {
I think you need to add:
 @TargetApi(Build.VERSION_CODES.LOLLIPOP) 

to avoid a lint error

https://codereview.chromium.org/1137313003/diff/20001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1137313003/diff/20001/build/java_apk.gypi#new...
build/java_apk.gypi:129: 'split_android_manifest_path':
'<(intermediate_dir)/split-manifests/<(android_app_abi)/AndroidManifest.xml',
Shouldn't this include <(target_name) somewhere so that you could have multiple
apks with splits? Otherwise it seems like they'd clobber

https://codereview.chromium.org/1137313003/diff/20001/build/java_apk.gypi#new...
build/java_apk.gypi:551: 'action_name': 'finalize_base',
nit: move to top

[agrieve, 2015-05-14 14:53:34]

https://codereview.chromium.org/1137313003/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1137313003/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:258:
apkFilePath = getLibraryApkPath(context);
On 2015/05/13 21:38:22, Yaron wrote:
> I'm confused. Isn't this for the case where load_library_from_zip_file==1 ?
> Perhaps include with your upcoming linker change? Or do you need this to get
far
> enough and actually load the library when load_library_from_zip_file is 0?

This is the linker patch. The fix was easy enough that I figured it might as
well go into the same patch.

https://codereview.chromium.org/1137313003/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:374:
private static String getLibraryApkPath(Context context) {
On 2015/05/13 21:38:22, Yaron wrote:
> I think you need to add:
>  @TargetApi(Build.VERSION_CODES.LOLLIPOP) 
> 
> to avoid a lint error

Done.

https://codereview.chromium.org/1137313003/diff/20001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1137313003/diff/20001/build/java_apk.gypi#new...
build/java_apk.gypi:129: 'split_android_manifest_path':
'<(intermediate_dir)/split-manifests/<(android_app_abi)/AndroidManifest.xml',
On 2015/05/13 21:38:22, Yaron wrote:
> Shouldn't this include <(target_name) somewhere so that you could have
multiple
> apks with splits? Otherwise it seems like they'd clobber

<(intermediate_dir) includes the target_name

https://codereview.chromium.org/1137313003/diff/20001/build/java_apk.gypi#new...
build/java_apk.gypi:551: 'action_name': 'finalize_base',
On 2015/05/13 21:38:22, Yaron wrote:
> nit: move to top

Done.

[Yaron, 2015-05-14 15:32:44]

https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi#new...
build/java_apk.gypi:192: 'load_library_from_zip_file%' : 0,
I think it makes sense to rename this throughout. To me it's easier to reason
about having a default value and then override it in certain places rather than
having to copy an almost-identically-named value always when using
finalize_apk_action.gypi. Granted you'll have to update the internal tree too :S

https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi#new...
build/java_apk.gypi:619: 'load_library_from_zip': 0,
Can you add the comment I had or you think it's obvious in context? I think it's
easy enough to see why as we build these up incrementally but much harder to
grok after the fact. Is gyp the new perl? :)

[agrieve1, 2015-05-14 15:44:18]

agrieve@google.com changed reviewers:
+ agrieve@google.com

[agrieve1, 2015-05-14 15:44:18]

https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi#new...
build/java_apk.gypi:192: 'load_library_from_zip_file%' : 0,
On 2015/05/14 15:32:44, Yaron wrote:
> I think it makes sense to rename this throughout. To me it's easier to reason
> about having a default value and then override it in certain places rather
than
> having to copy an almost-identically-named value always when using
> finalize_apk_action.gypi. Granted you'll have to update the internal tree too
:S

I agree we should rename, but didn't want to hold up this up by it. OK to do the
rename after?

https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi#new...
build/java_apk.gypi:619: 'load_library_from_zip': 0,
On 2015/05/14 15:32:44, Yaron wrote:
> Can you add the comment I had or you think it's obvious in context? I think
it's
> easy enough to see why as we build these up incrementally but much harder to
> grok after the fact. Is gyp the new perl? :)

I think it's pretty obvious - assuming you understand gyp_managed_install.
What's wrong with perl? :P

[Yaron, 2015-05-14 17:02:54]

lgtm

https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi#new...
build/java_apk.gypi:192: 'load_library_from_zip_file%' : 0,
On 2015/05/14 15:44:18, agrieve1 wrote:
> On 2015/05/14 15:32:44, Yaron wrote:
> > I think it makes sense to rename this throughout. To me it's easier to
reason
> > about having a default value and then override it in certain places rather
> than
> > having to copy an almost-identically-named value always when using
> > finalize_apk_action.gypi. Granted you'll have to update the internal tree
too
> :S
> 
> I agree we should rename, but didn't want to hold up this up by it. OK to do
the
> rename after?

Acknowledged.

[agrieve, 2015-05-15 23:25:16]

On 2015/05/14 17:02:54, Yaron wrote:
> lgtm
> 
> https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi
> File build/java_apk.gypi (right):
> 
>
https://codereview.chromium.org/1137313003/diff/60001/build/java_apk.gypi#new...
> build/java_apk.gypi:192: 'load_library_from_zip_file%' : 0,
> On 2015/05/14 15:44:18, agrieve1 wrote:
> > On 2015/05/14 15:32:44, Yaron wrote:
> > > I think it makes sense to rename this throughout. To me it's easier to
> reason
> > > about having a default value and then override it in certain places rather
> > than
> > > having to copy an almost-identically-named value always when using
> > > finalize_apk_action.gypi. Granted you'll have to update the internal tree
> too
> > :S
> > 
> > I agree we should rename, but didn't want to hold up this up by it. OK to do
> the
> > rename after?
> 
> Acknowledged.

ping @cjhopman

[cjhopman, 2015-05-16 01:31:10]

https://codereview.chromium.org/1137313003/diff/100001/base/android/java/src/...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1137313003/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:390:
if (packageInfo.splitNames[i].startsWith("abi_")) {
Is the abi_ prefix something enforced by android? If not, I don't think we
should hide this assumption all the way down here.

https://codereview.chromium.org/1137313003/diff/100001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1137313003/diff/100001/build/config/android/r...
build/config/android/rules.gni:1304: #     went out of scope.
I'd just do:

# Mark _create_abi_split as used
assert(_create_abi_split || true)

[agrieve, 2015-05-16 02:21:40]

https://codereview.chromium.org/1137313003/diff/100001/base/android/java/src/...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1137313003/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:390:
if (packageInfo.splitNames[i].startsWith("abi_")) {
On 2015/05/16 01:31:10, cjhopman wrote:
> Is the abi_ prefix something enforced by android? If not, I don't think we
> should hide this assumption all the way down here.

We name it in our build rules. What would you suggest as an alternative?

https://codereview.chromium.org/1137313003/diff/100001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1137313003/diff/100001/build/config/android/r...
build/config/android/rules.gni:1304: #     went out of scope.
On 2015/05/16 01:31:10, cjhopman wrote:
> I'd just do:
> 
> # Mark _create_abi_split as used
> assert(_create_abi_split || true)

It's not obvious to my why that would be better.

[cjhopman, 2015-05-16 02:29:08]

https://codereview.chromium.org/1137313003/diff/100001/base/android/java/src/...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1137313003/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:390:
if (packageInfo.splitNames[i].startsWith("abi_")) {
On 2015/05/16 02:21:40, agrieve wrote:
> On 2015/05/16 01:31:10, cjhopman wrote:
> > Is the abi_ prefix something enforced by android? If not, I don't think we
> > should hide this assumption all the way down here.
> 
> We name it in our build rules. What would you suggest as an alternative?

Oh, I just don't like it when our code makes hard to find assumptions about what
the build rules are doing. The very best would be if these assumptions were only
done in like one or two classes or something within a package where they'd be
easy to find.

I don't expect you to go that far, but just making it more obvious that this is
making such an assumption would be nice: maybe move the "abi_" string to a
static field with a comment calling out the assumption, or breaking out the
check to a function with such a comment would make me happy.

https://codereview.chromium.org/1137313003/diff/100001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1137313003/diff/100001/build/config/android/r...
build/config/android/rules.gni:1304: #     went out of scope.
On 2015/05/16 02:21:40, agrieve wrote:
> On 2015/05/16 01:31:10, cjhopman wrote:
> > I'd just do:
> > 
> > # Mark _create_abi_split as used
> > assert(_create_abi_split || true)
> 
> It's not obvious to my why that would be better.

I think such a short comment is more typical when marking a variable used in
this way, and I guess I sort of assume that people reading it understand that an
unused variable is an error in gn (without needing the full text of that error).
I don't feel strongly about it though.

[agrieve, 2015-05-17 02:06:10]

https://codereview.chromium.org/1137313003/diff/100001/base/android/java/src/...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1137313003/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:390:
if (packageInfo.splitNames[i].startsWith("abi_")) {
On 2015/05/16 02:29:07, cjhopman wrote:
> On 2015/05/16 02:21:40, agrieve wrote:
> > On 2015/05/16 01:31:10, cjhopman wrote:
> > > Is the abi_ prefix something enforced by android? If not, I don't think we
> > > should hide this assumption all the way down here.
> > 
> > We name it in our build rules. What would you suggest as an alternative?
> 
> Oh, I just don't like it when our code makes hard to find assumptions about
what
> the build rules are doing. The very best would be if these assumptions were
only
> done in like one or two classes or something within a package where they'd be
> easy to find.
> 
> I don't expect you to go that far, but just making it more obvious that this
is
> making such an assumption would be nice: maybe move the "abi_" string to a
> static field with a comment calling out the assumption, or breaking out the
> check to a function with such a comment would make me happy.

Thanks for spelling it out. Brand new to the codebase, so the hand-holding is
appreciated! :)

Discovered there's a PackageUtil class in base, so moved a piece into there, and
moved the name check into it's own function to be even more clear about intent.

https://codereview.chromium.org/1137313003/diff/100001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1137313003/diff/100001/build/config/android/r...
build/config/android/rules.gni:1304: #     went out of scope.
On 2015/05/16 02:29:07, cjhopman wrote:
> On 2015/05/16 02:21:40, agrieve wrote:
> > On 2015/05/16 01:31:10, cjhopman wrote:
> > > I'd just do:
> > > 
> > > # Mark _create_abi_split as used
> > > assert(_create_abi_split || true)
> > 
> > It's not obvious to my why that would be better.
> 
> I think such a short comment is more typical when marking a variable used in
> this way, and I guess I sort of assume that people reading it understand that
an
> unused variable is an error in gn (without needing the full text of that
error).
> I don't feel strongly about it though.

Fair enough! :)

[cjhopman, 2015-05-18 18:51:33]

lgtm

[agrieve, 2015-05-19 14:23:04]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-05-19 14:23:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1137313003/#ps120001
(title: "Address style nits.")

[commit-bot: I haz the power, 2015-05-19 14:23:28]

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

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

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-19 16:59:46]

Patchset 7 (id:??) landed as
https://crrev.com/63349e4f235ae666faff75c1eee2da001fb7341f
Cr-Commit-Position: refs/heads/master@{#330523}