Remove {base,net}_java dependencies from GYP client targets on Android.

Remove {base,net}_java dependencies from GYP client targets on Android.

On Android, some GYP client targets were explicitly depending on both the
native and Java sides of a library.
This removes the dependencies to {base,net}_java and make the native side
('base' and 'net) of these libraries depend on their Java counterpart.
On Android it rarely makes sense to depend on a single side of a Java/C++
library.
The {base,net}_java can now be considered as "private" targets although GYP
does not support this concept unfortunately (AFAICT).
Note that I made sure that the resulting APKs' size is unchanged.

Additionally, this CL removes 'base_java' (i.e. does not replace it with
'base') from the targets including 'build/apk_test.gypi'. This dependency
should not have been there (in the wrong layer) in the first place. It's needed
by ChromeNativeTestActivity.java which clients should not know about.

BUG=146323
TBR=lipalani,sky,willchan,brettw

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155737

[Philippe, 2012-09-05 14:08:54]

I'm adding you guys first for a sanity check before I send that to other
reviewers :)

[Philippe, 2012-09-06 09:02:03]

Ping :)

[Peter Beverloo, 2012-09-06 10:09:09]

LGTM, thank you Philippe! I'd like Marcus to have a look as well, though.

There'd be a large amount of OWNERS needed to get this CL landed. Since these
are minor gyp updates, I think it'd be fine to TBR them.

https://chromiumcodereview.appspot.com/10913083/diff/2001/net/net.gyp
File net/net.gyp (right):

https://chromiumcodereview.appspot.com/10913083/diff/2001/net/net.gyp#newcode823
net/net.gyp:823: ['OS=="android" and _toolset=="target"', {
The OS==* conditions in this block start around line 1020, it may be better to
keep them grouped together. How about placing this right in front of the
OS==android block around line 1100?

[bulach, 2012-09-06 10:22:14]

lgtm, with peter's comments..

I think your solution is better, but just some background.. :)
there are in fact three things in play here..
for a given component "foo", there are three sides of it:

foo ==> the native C++ component

foo_jni_implementation ==> this would be the "private" target you mentioned,
which implements the java-side that the C++ "foo" component requires..

foo_java ==> this would be the "public" java component, without any native
dependencies...

this way, "bar_java" could depend on "foo_java", that is, a java library
depending on another java library..

however, due to the gyp+jar+ant+apk and some other acronyms, these dependencies
are impossible to keep this fine grained, so I agree that having just "foo" and
"bar" simplifies everything (in the end, all three targets above, "foo",
"foo_jni" and "foo_java" would end up being a single set of .a + .jar that would
be packaged in a single .apk anyways).

I guess your solution simplifies this by having a single "foo" that anybody can
depend on..

thanks! :)

[Philippe, 2012-09-06 15:49:46]

Thanks for the quick review guys and for the detailed explanation Marcus! I
agree that TBR seems reasonable given that this change only affects us.
I'm adding Yaron just in case. If he's ok with it too I will submit it.

https://chromiumcodereview.appspot.com/10913083/diff/2001/net/net.gyp
File net/net.gyp (right):

https://chromiumcodereview.appspot.com/10913083/diff/2001/net/net.gyp#newcode823
net/net.gyp:823: ['OS=="android" and _toolset=="target"', {
On 2012/09/06 10:09:09, Peter Beverloo wrote:
> The OS==* conditions in this block start around line 1020, it may be better to
> keep them grouped together. How about placing this right in front of the
> OS==android block around line 1100?

Done.

[Yaron, 2012-09-06 19:01:48]

There's a minor downside in that there's less parallelism to the build, right?
This makes it such that chrome_java can't build until all the lower layers are
fully compiled whereas previously if base_java built, we could proceed to
net_java, content_java, and chrome_java. The java build is pretty darn fast so
maybe it's moot. Actually, how does this affect incremental builds? If I touch a
file in say base_java, does this now cause rebuilds in c++ code for
net/media/content/chrome or does it still only affect the java-side? 

Also, Chris is doing lots with our java build so it's probably best for him to
do a once-over.

[cjhopman, 2012-09-06 20:29:24]

I think that having input_jars_paths be a direct_dependent_setting and then
everything having to export_dependent_setting that target will be changed to be
all_dependent_settings to act more like native library's link_settings. With
this change, that would mean that a java target could not depend on the native
library without also getting the jar path in its input_jars_paths. I think
that's probably fine, though.

Overall, I like this change.

https://chromiumcodereview.appspot.com/10913083/diff/1015/build/apk_test.gypi
File build/apk_test.gypi (right):

https://chromiumcodereview.appspot.com/10913083/diff/1015/build/apk_test.gypi...
build/apk_test.gypi:27: },
Can this be specified as a dependency on base here rather than adding the path
directly to input_jars_paths?

https://chromiumcodereview.appspot.com/10913083/diff/1015/content/content.gyp
File content/content.gyp (left):

https://chromiumcodereview.appspot.com/10913083/diff/1015/content/content.gyp...
content/content.gyp:246: '../base/base.gyp:base_java',
I believe that the dependency on base_java here was due to the fact that
content_java depends directly on base_java. By removing it, you still inherit
the dependent settings through net, but lose the documentation aspect that this
depends directly on base. I would specify the dependency here (unless
content_java doesn't actually depend directly on base_java).

https://chromiumcodereview.appspot.com/10913083/diff/1015/content/content.gyp...
content/content.gyp:253: '../net/net.gyp:net_java',
Removing these exports would make an apk that specifies a dependency on
content_java (or content, now) but not the others break as it would no longer
inherit the input_jars_paths from base and net. There probably aren't any such
targets currently.

[cjhopman, 2012-09-06 20:41:38]

On 2012/09/06 19:01:48, Yaron wrote:
> There's a minor downside in that there's less parallelism to the build, right?
> This makes it such that chrome_java can't build until all the lower layers are
> fully compiled whereas previously if base_java built, we could proceed to
> net_java, content_java, and chrome_java. The java build is pretty darn fast so
> maybe it's moot. 
Yeah, the jar targets are really fast (~0-1 second each). The apk targets are
slower, but they have to wait for the native parts anyway.

> Actually, how does this affect incremental builds? If I touch a
> file in say base_java, does this now cause rebuilds in c++ code for
> net/media/content/chrome or does it still only affect the java-side?
A target is only rebuilt when one of its inputs changes, not necessarily when
one of its dependents changes. So, I believe this won't affect what gets
rebuilt.

[Philippe, 2012-09-07 09:45:57]

I tried to make changes in base/android/java and re-build. I confirm that it
only re-triggers the Java build (which is pretty fast indeed).

https://chromiumcodereview.appspot.com/10913083/diff/1015/build/apk_test.gypi
File build/apk_test.gypi (right):

https://chromiumcodereview.appspot.com/10913083/diff/1015/build/apk_test.gypi...
build/apk_test.gypi:27: },
On 2012/09/06 20:29:25, cjhopman wrote:
> Can this be specified as a dependency on base here rather than adding the path
> directly to input_jars_paths? 

This does not work unfortunately :/ The Python script invoked below copies the
jars in |input_jars_paths| to a target directory.
It doesn't know how to deal with GYP dependencies unfortunately.

https://chromiumcodereview.appspot.com/10913083/diff/1015/content/content.gyp
File content/content.gyp (left):

https://chromiumcodereview.appspot.com/10913083/diff/1015/content/content.gyp...
content/content.gyp:246: '../base/base.gyp:base_java',
On 2012/09/06 20:29:25, cjhopman wrote:
> I believe that the dependency on base_java here was due to the fact that
> content_java depends directly on base_java. By removing it, you still inherit
> the dependent settings through net, but lose the documentation aspect that
this
> depends directly on base. I would specify the dependency here (unless
> content_java doesn't actually depend directly on base_java).

These changes were pretty random indeed. The code under
content/public/android/java does depend explicitly on base so this dependency
should be kept.

https://chromiumcodereview.appspot.com/10913083/diff/1015/content/content.gyp...
content/content.gyp:253: '../net/net.gyp:net_java',
On 2012/09/06 20:29:25, cjhopman wrote:
> Removing these exports would make an apk that specifies a dependency on
> content_java (or content, now) but not the others break as it would no longer
> inherit the input_jars_paths from base and net. There probably aren't any such
> targets currently.

I don't think I had strong reasons to do this. I finally kept it (after I did
s/_java//).

[cjhopman, 2012-09-07 16:37:01]

lgtm

https://chromiumcodereview.appspot.com/10913083/diff/1015/build/apk_test.gypi
File build/apk_test.gypi (right):

https://chromiumcodereview.appspot.com/10913083/diff/1015/build/apk_test.gypi...
build/apk_test.gypi:27: },
On 2012/09/07 09:45:57, Philippe wrote:
> On 2012/09/06 20:29:25, cjhopman wrote:
> > Can this be specified as a dependency on base here rather than adding the
path
> > directly to input_jars_paths? 
> 
> This does not work unfortunately :/ The Python script invoked below copies the
> jars in |input_jars_paths| to a target directory.
> It doesn't know how to deal with GYP dependencies unfortunately.

Depending on base should add the path to chromium_base.jar to input_jars_paths
automatically. In fact, the targets that included this .gypi were using that
technique themselves (i.e. they were depending on base_java and so getting the
path added to input_jars_paths).

If it doesn't work, though, leaving it like this is fine.

[Yaron, 2012-09-07 20:41:09]

lgtm

[commit-bot: I haz the power, 2012-09-10 09:45:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10913083/14002

[commit-bot: I haz the power, 2012-09-10 09:59:11]

Try job failure for 10913083-14002 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-09-10 13:50:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10913083/14002

[commit-bot: I haz the power, 2012-09-10 14:00:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10913083/14002

[commit-bot: I haz the power, 2012-09-10 15:52:48]

Change committed as 155737