Make android_aar_prebuilt() aware of remaining features

Make android_aar_prebuilt() aware of remaining features

* Fail if it finds a non-trivial AndroidManifest.xml
* Fail if it finds any .so files
* Fail if it finds any assets
* Support proguard.txt

This also tweaks the naming of the sub-jar targets to give them better
target names (which show up in .jar names).

TBR=bshe
BUG=640836
Committed: https://crrev.com/8dbd4fb5770dc164dfa20845f839d49c154b0e9c
Cr-Commit-Position: refs/heads/master@{#417160}

[agrieve, 2016-09-04 03:14:25]

agrieve@chromium.org changed reviewers:
+ bshe@chromium.org, ianwen@chromium.org

[agrieve, 2016-09-04 03:14:26]

bshe: please review third_party/gvr-android-sdk/BUILD.gn

ianwen: everything else :)

[agrieve, 2016-09-04 03:14:36]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-04 03:14:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-04 04:12:37]

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

[commit-bot: I haz the power, 2016-09-04 04:12:38]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-09-05 00:17:05]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-05 00:17:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-05 00:19:47]

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

[commit-bot: I haz the power, 2016-09-05 00:19:48]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-09-06 18:52:37]

On 2016/09/04 03:14:26, agrieve wrote:
> bshe: please review third_party/gvr-android-sdk/BUILD.gn
> 
> ianwen: everything else :)

Note: tryjob succeeded on second run. was just an tree failure at time of run.

[Ian Wen, 2016-09-06 19:09:59]

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py
File build/android/gyp/aar.py (right):

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:91: data['subjars'].append(name)
One way to simplify the code a bit, is to still use jars as a unified  list for
both classes.jar and other jars. jar_labels will still be needed and its first
entry will be "classes.jar".

WDYT?

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:96: data['has_native_libraries'] = True
Instead of outputting a boolean, have you considered outputting a list of SO
files directly? Later you could check whether the list is empty to determine if
native libraries are there.

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:100: data['has_proguard_flags'] = True
How about AIDLs and annotations?

AIDLs are included in support v4, yet we don't seem to need it right now. For
annotations, will we ever need it? It seems annotations are there to ensure
android-style enums.

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

https://codereview.chromium.org/2309643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:2683: # Create android_java_prebuilt targets for
jar files.
Nit: either keep this line or remove #2663 as well?

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

https://codereview.chromium.org/2309643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:2655: # Create android_java_prebuilt targets for
jar files witin jars/.
s/jars\//libs\/

Also we should move it down to #2711?

https://codereview.chromium.org/2309643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:2692: if (!_ignore_resources &&
_scanned_files.resources != []) {
Q: which target do you expect will need to ignore resources? It used to be quite
often that people forgot about adding resources from support library.

https://codereview.chromium.org/2309643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:2712: if (!defined(_counter)) {
Q: Is this considered as a better practice to guard the local variable, rather
than declaring it before hand and use it like a C-style variable?

I don't see _counter declared in other places in this file...

[agrieve, 2016-09-06 21:00:51]

Great stuff! aidl... who knew!?

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py
File build/android/gyp/aar.py (right):

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:91: data['subjars'].append(name)
On 2016/09/06 19:09:59, Ian Wen wrote:
> One way to simplify the code a bit, is to still use jars as a unified  list
for
> both classes.jar and other jars. jar_labels will still be needed and its first
> entry will be "classes.jar".
> 
> WDYT?

I originally separated this from the main classes.jar because I thought I could
make these sub-jars not a part of the main java_group(). That turned out to be
false, so we definitely could return this to be grouped with the others.

However, I think I like having it split out anyways, since it then more closely
matches the structure of the .aar, and allows us to specify fewer arguments on
the sub-jars (e.g. input_jars_paths, proguard_configs).

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:96: data['has_native_libraries'] = True
On 2016/09/06 19:09:59, Ian Wen wrote:
> Instead of outputting a boolean, have you considered outputting a list of SO
> files directly? Later you could check whether the list is empty to determine
if
> native libraries are there.

I did consider it. The tricky bit is that the there's an architecture directory
(arm, arm64, etc) within jni, so having the full list of .so files isn't really
what you want. In the end I don't think we ever want to support (famous last
words) .so files in .aars, since we want to keep chrome a single .so for speed
reasons, so a boolean is best for now.

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:100: data['has_proguard_flags'] = True
On 2016/09/06 19:09:59, Ian Wen wrote:
> How about AIDLs and annotations?
> 
> AIDLs are included in support v4, yet we don't seem to need it right now. For
> annotations, will we ever need it? It seems annotations are there to ensure
> android-style enums.

Was going by the list here:
http://tools.android.com/tech-docs/new-build-system/aar-format
which doesn't mention .aidl!

I do indeed see some with v4 support though! Nice find!

Update to look for them as well.

As for annotations: just filed http://crbug.com/644442 to mention that they are
a thing.

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

https://codereview.chromium.org/2309643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:2683: # Create android_java_prebuilt targets for
jar files.
On 2016/09/06 19:09:59, Ian Wen wrote:
> Nit: either keep this line or remove #2663 as well?

Done.

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

https://codereview.chromium.org/2309643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:2655: # Create android_java_prebuilt targets for
jar files witin jars/.
On 2016/09/06 19:09:59, Ian Wen wrote:
> s/jars\//libs\/
> 
> Also we should move it down to #2711?

Done.

https://codereview.chromium.org/2309643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:2692: if (!_ignore_resources &&
_scanned_files.resources != []) {
On 2016/09/06 19:09:59, Ian Wen wrote:
> Q: which target do you expect will need to ignore resources? It used to be
quite
> often that people forgot about adding resources from support library.

Just added for completeness. I don't have any targets that will think will use
this.

https://codereview.chromium.org/2309643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:2712: if (!defined(_counter)) {
On 2016/09/06 19:09:59, Ian Wen wrote:
> Q: Is this considered as a better practice to guard the local variable, rather
> than declaring it before hand and use it like a C-style variable?
> 
> I don't see _counter declared in other places in this file...

No. The reason for this change is a bit confusing.

Before, classes.jar was included in _scanned_files.jars, so this loop was always
entered. That made the variable always used.

Now, the loop is not always entered, so it was complaining that the variable was
unused.

I could have also used the assert() # mark as used trick, but opted for this
instead.

[Ian Wen, 2016-09-06 21:38:35]

I really look forward to seeing this CL going through so that I don't have to
include proguard tweaks in https://codereview.chromium.org/2304943003. :)

Yet I do have some reservations with adding those parameters. If necessary shall
we sync via VC tomorrow morning?

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py
File build/android/gyp/aar.py (right):

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:91: data['subjars'].append(name)
On 2016/09/06 21:00:51, agrieve wrote:
> On 2016/09/06 19:09:59, Ian Wen wrote:
> > One way to simplify the code a bit, is to still use jars as a unified  list
> for
> > both classes.jar and other jars. jar_labels will still be needed and its
first
> > entry will be "classes.jar".
> > 
> > WDYT?
> 
> I originally separated this from the main classes.jar because I thought I
could
> make these sub-jars not a part of the main java_group(). That turned out to be
> false, so we definitely could return this to be grouped with the others.
> 
> However, I think I like having it split out anyways, since it then more
closely
> matches the structure of the .aar, and allows us to specify fewer arguments on
> the sub-jars (e.g. input_jars_paths, proguard_configs).
> 
> 

Okay sg. :)

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:96: data['has_native_libraries'] = True
On 2016/09/06 21:00:51, agrieve wrote:
> On 2016/09/06 19:09:59, Ian Wen wrote:
> > Instead of outputting a boolean, have you considered outputting a list of SO
> > files directly? Later you could check whether the list is empty to determine
> if
> > native libraries are there.
> 
> I did consider it. The tricky bit is that the there's an architecture
directory
> (arm, arm64, etc) within jni, so having the full list of .so files isn't
really
> what you want. In the end I don't think we ever want to support (famous last
> words) .so files in .aars, since we want to keep chrome a single .so for speed
> reasons, so a boolean is best for now.

Hmm if we are not going to support native libraries in AAR, maybe we should
simply make it not supported by default, and don't add the
"ignore_native_libraries" as argument? I feel it's strange to have a parameter
that does nothing in the end.

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

https://codereview.chromium.org/2309643002/diff/20001/build/config/android/ru...
build/config/android/rules.gni:2609: #   ignore_aidl: Whether to ignore .aidl
files found with the .aar.
I feel like once we include an AAR file, we should respect all the possible
features of the file and we shouldn't give us the option to exclude some
features. If these flags are here temporarily, I think it's fine. Otherwise I
would vote for not including targets that we don't use.

Recently I read an article about Yagni rule
(https://drive.google.com/a/google.com/file/d/0B-yWrS8S9kyqQU1VbDF5N3hndW8/view)
and I think it applies here.

"ignore_native_libraries" might be the only exception though, since they
generally are not compiled using the same flow as java.

https://codereview.chromium.org/2309643002/diff/20001/third_party/gvr-android...
File third_party/gvr-android-sdk/BUILD.gn (right):

https://codereview.chromium.org/2309643002/diff/20001/third_party/gvr-android...
third_party/gvr-android-sdk/BUILD.gn:20: ignore_manifest = true
Q: Will we keep ignoring merging the manifest, even after it is supported? Or
this is temporary?

[agrieve, 2016-09-07 02:19:31]

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py
File build/android/gyp/aar.py (right):

https://codereview.chromium.org/2309643002/diff/1/build/android/gyp/aar.py#ne...
build/android/gyp/aar.py:96: data['has_native_libraries'] = True
On 2016/09/06 21:38:34, Ian Wen wrote:
> On 2016/09/06 21:00:51, agrieve wrote:
> > On 2016/09/06 19:09:59, Ian Wen wrote:
> > > Instead of outputting a boolean, have you considered outputting a list of
SO
> > > files directly? Later you could check whether the list is empty to
determine
> > if
> > > native libraries are there.
> > 
> > I did consider it. The tricky bit is that the there's an architecture
> directory
> > (arm, arm64, etc) within jni, so having the full list of .so files isn't
> really
> > what you want. In the end I don't think we ever want to support (famous last
> > words) .so files in .aars, since we want to keep chrome a single .so for
speed
> > reasons, so a boolean is best for now.
> 
> Hmm if we are not going to support native libraries in AAR, maybe we should
> simply make it not supported by default, and don't add the
> "ignore_native_libraries" as argument? I feel it's strange to have a parameter
> that does nothing in the end.

Much of the motivation behind these flags is that I don't want people to think
that their .aar files are working properly when infact things are being omitted.
By forcing .aars that have .so files to hit an assert and add a flag, it informs
the developer that the .so even exists, and also forces the GN code to be
documents as "there is an .so in here that is not being used".

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

https://codereview.chromium.org/2309643002/diff/20001/build/config/android/ru...
build/config/android/rules.gni:2609: #   ignore_aidl: Whether to ignore .aidl
files found with the .aar.
On 2016/09/06 21:38:34, Ian Wen wrote:
> I feel like once we include an AAR file, we should respect all the possible
> features of the file and we shouldn't give us the option to exclude some
> features. If these flags are here temporarily, I think it's fine. Otherwise I
> would vote for not including targets that we don't use.
> 
> Recently I read an article about Yagni rule
>
(https://drive.google.com/a/google.com/file/d/0B-yWrS8S9kyqQU1VbDF5N3hndW8/view)
> and I think it applies here.
> 
> "ignore_native_libraries" might be the only exception though, since they
> generally are not compiled using the same flow as java.

That's a fair point. I've removed "ignore_resources", and we'll consider the
remaining ones to be temporarily necessary to document the fact that an .aar has
features that are being ignored.

https://codereview.chromium.org/2309643002/diff/20001/third_party/gvr-android...
File third_party/gvr-android-sdk/BUILD.gn (right):

https://codereview.chromium.org/2309643002/diff/20001/third_party/gvr-android...
third_party/gvr-android-sdk/BUILD.gn:20: ignore_manifest = true
On 2016/09/06 21:38:34, Ian Wen wrote:
> Q: Will we keep ignoring merging the manifest, even after it is supported? Or
> this is temporary?

Just temporary. Added a comment explaining motivation.

[Ian Wen, 2016-09-07 17:13:48]

lgtm

This is an awesome change and I'm excited that we are closer to the goal! :)

After we make the final implementation about aidls, manifests and assets, we
might remove some of the unused arguments.

[agrieve, 2016-09-08 00:38:27]

Description was changed from

==========
Make android_aar_prebuilt() aware of remaining features

* Fail if it finds a non-trivial AndroidManifest.xml
* Fail if it finds any .so files
* Fail if it finds any assets
* Support proguard.txt

This also tweaks the naming of the sub-jar targets to give them better
target names (which show up in .jar names).

BUG=640836
==========

to

==========
Make android_aar_prebuilt() aware of remaining features

* Fail if it finds a non-trivial AndroidManifest.xml
* Fail if it finds any .so files
* Fail if it finds any assets
* Support proguard.txt

This also tweaks the naming of the sub-jar targets to give them better
target names (which show up in .jar names).

TBR=bshe
BUG=640836
==========

[agrieve, 2016-09-08 00:38:41]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-09-08 00:38:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[agrieve, 2016-09-08 00:39:08]

On 2016/09/07 17:13:48, Ian Wen wrote:
> lgtm
> 
> This is an awesome change and I'm excited that we are closer to the goal! :)
> 
> After we make the final implementation about aidls, manifests and assets, we
> might remove some of the unused arguments.

tbr for trivial gvr-android-sdk/BUILD.gn part.

[commit-bot: I haz the power, 2016-09-08 02:03:06]

Description was changed from

==========
Make android_aar_prebuilt() aware of remaining features

* Fail if it finds a non-trivial AndroidManifest.xml
* Fail if it finds any .so files
* Fail if it finds any assets
* Support proguard.txt

This also tweaks the naming of the sub-jar targets to give them better
target names (which show up in .jar names).

TBR=bshe
BUG=640836
==========

to

==========
Make android_aar_prebuilt() aware of remaining features

* Fail if it finds a non-trivial AndroidManifest.xml
* Fail if it finds any .so files
* Fail if it finds any assets
* Support proguard.txt

This also tweaks the naming of the sub-jar targets to give them better
target names (which show up in .jar names).

TBR=bshe
BUG=640836
==========

[commit-bot: I haz the power, 2016-09-08 02:03:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 02:04:54]

Description was changed from

==========
Make android_aar_prebuilt() aware of remaining features

* Fail if it finds a non-trivial AndroidManifest.xml
* Fail if it finds any .so files
* Fail if it finds any assets
* Support proguard.txt

This also tweaks the naming of the sub-jar targets to give them better
target names (which show up in .jar names).

TBR=bshe
BUG=640836
==========

to

==========
Make android_aar_prebuilt() aware of remaining features

* Fail if it finds a non-trivial AndroidManifest.xml
* Fail if it finds any .so files
* Fail if it finds any assets
* Support proguard.txt

This also tweaks the naming of the sub-jar targets to give them better
target names (which show up in .jar names).

TBR=bshe
BUG=640836

Committed: https://crrev.com/8dbd4fb5770dc164dfa20845f839d49c154b0e9c
Cr-Commit-Position: refs/heads/master@{#417160}
==========

[commit-bot: I haz the power, 2016-09-08 02:04:56]

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