Make java.gypi export jar path to input_jars_paths variable

Make java.gypi export jar path to input_jars_paths variable

Both java.gypi and apk_test.gypi expect the jar path in
input_jars_paths. This means that we were specifying the dependence in
both 'dependencies' and 'input_jars_paths'. This change makes it so that
we don't need that redundancy.

Also, make java.gypi use input_jars_paths in its input so that we
actually rebuild targets when we should.

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

[Yaron, 2012-08-07 16:41:08]

http://codereview.chromium.org/10837143/diff/3001/build/java.gypi
File build/java.gypi (right):

http://codereview.chromium.org/10837143/diff/3001/build/java.gypi#newcode31
build/java.gypi:31: 'direct_dependent_settings': {
This seems to work as-is but I wonder if we'd ultimately want this to be
all_dependent settings. The at the chrome layer, the test says it depends on
chrome_java and picks up all the jars transitively. What do you think?

[cjhopman, 2012-08-07 19:12:11]

http://codereview.chromium.org/10837143/diff/3001/build/java.gypi
File build/java.gypi (right):

http://codereview.chromium.org/10837143/diff/3001/build/java.gypi#newcode31
build/java.gypi:31: 'direct_dependent_settings': {
On 2012/08/07 16:41:09, Yaron wrote:
> This seems to work as-is but I wonder if we'd ultimately want this to be
> all_dependent settings. The at the chrome layer, the test says it depends on
> chrome_java and picks up all the jars transitively. What do you think?

So, I've changed it to all_dependent_settings. 

Looking at how normal libraries are handled, it appears that they mostly use
direct_dependent_settings and then the dependent uses export_dependent_settings
so that the settings are passed on to its dependents. I could do it that way,
but the way that this is used, we would just always do export_dependent_settings
and then would essentially be specifying all the *_java dependencies twice.

[Yaron, 2012-08-07 23:17:27]

lgtm

Seems simpler to keep it this way.

[cjhopman, 2012-08-07 23:38:09]

brettw: base/ chrome/ content/
sky: ui/
rsleevi: net/
zea: sync/

[brettw, 2012-08-07 23:41:03]

Owners LGTM stamp but I don't actually understand what you're doing here. I'm
assuming one of your other reviewers does.

[Nicolas Zea, 2012-08-07 23:43:21]

lgtm

[sky, 2012-08-07 23:57:31]

LGTM

[nilesh, 2012-08-08 00:11:05]

http://codereview.chromium.org/10837143/diff/5001/sync/sync.gyp
File sync/sync.gyp (left):

http://codereview.chromium.org/10837143/diff/5001/sync/sync.gyp#oldcode837
sync/sync.gyp:837: '../base/base.gyp:base_java',
You should be able to remove the direct dependency on base_java now.

[Ryan Sleevi, 2012-08-08 05:47:47]

So I feel like I understand the motivation here, and I certainly agree that
having to specify the jars and the target dependency are undesirable.

What I'm not a fan of is the elimination of the (direct) dependencies and the
reliance on transitive settings. Such things have really been problematic in the
past.

For the short-/medium-term, using direct_dependent+export_dependent and making
sure any 'final' target (eg: the APK) fully specifies its dependencies seems
correct, both for understanding the dependency tree and for making sure that all
the other GYP behaviours are preserved. Long term, it may be worthwhile seeing
about getting the ANT steps treated as a 'link-like' step from GYP's POV, which
would let you use link_settings and thus ensure transitive dependencies behave
properly.

See below

http://codereview.chromium.org/10837143/diff/3001/build/java.gypi
File build/java.gypi (right):

http://codereview.chromium.org/10837143/diff/3001/build/java.gypi#newcode31
build/java.gypi:31: 'direct_dependent_settings': {
On 2012/08/07 19:12:11, cjhopman wrote:
> On 2012/08/07 16:41:09, Yaron wrote:
> > This seems to work as-is but I wonder if we'd ultimately want this to be
> > all_dependent settings. The at the chrome layer, the test says it depends on
> > chrome_java and picks up all the jars transitively. What do you think?
> 
> So, I've changed it to all_dependent_settings. 
> 
> Looking at how normal libraries are handled, it appears that they mostly use
> direct_dependent_settings and then the dependent uses
export_dependent_settings
> so that the settings are passed on to its dependents. I could do it that way,
> but the way that this is used, we would just always do
export_dependent_settings
> and then would essentially be specifying all the *_java dependencies twice.

This is indeed the correct and preferred way.

I'm going to have to disagree with Yaron - all_dependent_settings in GYP is a
giant screaming hack. While I realize that shoe-horning Java compilation into
GYP is itself a bit hacky, it does feel "wrong"

Whenever "all_dependent_settings" is present, it's typically a sign of "I depend
on something, but I haven't updated the GYP files to reflect that dependency
tree". When that happens, all sorts of random flakes can be introduced -
particularly when GYP starts pruning dependencies or re-ordering build steps,
and some dependent (file, target) is eliminated, thus it's missing during a
build phase.

I'd like to push back on this, in favor of
direct_dependent+export_dependent_settings.

Really, what I think you want is "link_settings", but since Java hasn't been
added to GYP proper, it doesn't understand that the ant_ step is effectively the
link step (which is when it would merge in link_settings and all would be right
with the world). Perhaps that's something you can investigate?

[cjhopman, 2012-08-08 21:40:26]

I've switched it back to direct_dependent_settings, and the *_java targets use
the relevant export_dependent_settings. I agree that this keeps the dependencies
and dependent settings inheritance much clearer.

I definitely think that, as a longer term goal, we should look into getting all
of these java steps (java.gypi, java_aidl.gypi, apk_test.gypi, etc.) better
integrated into gyp.

On 2012/08/08 05:47:47, Ryan Sleevi wrote:
> So I feel like I understand the motivation here, and I certainly agree that
> having to specify the jars and the target dependency are undesirable.
> 
> What I'm not a fan of is the elimination of the (direct) dependencies and the
> reliance on transitive settings. Such things have really been problematic in
the
> past.
> 
> For the short-/medium-term, using direct_dependent+export_dependent and making
> sure any 'final' target (eg: the APK) fully specifies its dependencies seems
> correct, both for understanding the dependency tree and for making sure that
all
> the other GYP behaviours are preserved. Long term, it may be worthwhile seeing
> about getting the ANT steps treated as a 'link-like' step from GYP's POV,
which
> would let you use link_settings and thus ensure transitive dependencies behave
> properly.
> 
> See below
> 
> http://codereview.chromium.org/10837143/diff/3001/build/java.gypi
> File build/java.gypi (right):
> 
> http://codereview.chromium.org/10837143/diff/3001/build/java.gypi#newcode31
> build/java.gypi:31: 'direct_dependent_settings': {
> On 2012/08/07 19:12:11, cjhopman wrote:
> > On 2012/08/07 16:41:09, Yaron wrote:
> > > This seems to work as-is but I wonder if we'd ultimately want this to be
> > > all_dependent settings. The at the chrome layer, the test says it depends
on
> > > chrome_java and picks up all the jars transitively. What do you think?
> > 
> > So, I've changed it to all_dependent_settings. 
> > 
> > Looking at how normal libraries are handled, it appears that they mostly use
> > direct_dependent_settings and then the dependent uses
> export_dependent_settings
> > so that the settings are passed on to its dependents. I could do it that
way,
> > but the way that this is used, we would just always do
> export_dependent_settings
> > and then would essentially be specifying all the *_java dependencies twice.
> 
> This is indeed the correct and preferred way.
> 
> I'm going to have to disagree with Yaron - all_dependent_settings in GYP is a
> giant screaming hack. While I realize that shoe-horning Java compilation into
> GYP is itself a bit hacky, it does feel "wrong"
> 
> Whenever "all_dependent_settings" is present, it's typically a sign of "I
depend
> on something, but I haven't updated the GYP files to reflect that dependency
> tree". When that happens, all sorts of random flakes can be introduced -
> particularly when GYP starts pruning dependencies or re-ordering build steps,
> and some dependent (file, target) is eliminated, thus it's missing during a
> build phase.
> 
> I'd like to push back on this, in favor of
> direct_dependent+export_dependent_settings.
> 
> Really, what I think you want is "link_settings", but since Java hasn't been
> added to GYP proper, it doesn't understand that the ant_ step is effectively
the
> link step (which is when it would merge in link_settings and all would be
right
> with the world). Perhaps that's something you can investigate?

[nilesh, 2012-08-08 22:24:20]

http://codereview.chromium.org/10837143/diff/3017/chrome/chrome.gyp
File chrome/chrome.gyp (left):

http://codereview.chromium.org/10837143/diff/3017/chrome/chrome.gyp#oldcode1083
chrome/chrome.gyp:1083: '../base/base.gyp:base_java',
I think chrome_java directly depends on base_java and the dependency should be
reflected here. And we should export this dependency in
export_dependency_settings.

http://codereview.chromium.org/10837143/diff/3017/chrome/chrome.gyp#oldcode1085
chrome/chrome.gyp:1085: '../net/net.gyp:net_java',
Same for net.

[cjhopman, 2012-08-08 22:38:57]

http://codereview.chromium.org/10837143/diff/3017/chrome/chrome.gyp
File chrome/chrome.gyp (left):

http://codereview.chromium.org/10837143/diff/3017/chrome/chrome.gyp#oldcode1083
chrome/chrome.gyp:1083: '../base/base.gyp:base_java',
On 2012/08/08 22:24:20, nileshagrawal1 wrote:
> I think chrome_java directly depends on base_java and the dependency should be
> reflected here. And we should export this dependency in
> export_dependency_settings.

Done. Also done for content_java target.

http://codereview.chromium.org/10837143/diff/3017/chrome/chrome.gyp#oldcode1085
chrome/chrome.gyp:1085: '../net/net.gyp:net_java',
On 2012/08/08 22:24:20, nileshagrawal1 wrote:
> Same for net.

Done.

[Yaron, 2012-08-09 00:46:15]

I'm not excited about the redundancy but I understand the motivation. lgtm

[Ryan Sleevi, 2012-08-09 01:45:22]

lgtm - one nit below, but no need for re-review

http://codereview.chromium.org/10837143/diff/10003/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

http://codereview.chromium.org/10837143/diff/10003/chrome/chrome_tests.gypi#o...
chrome/chrome_tests.gypi:4593: '../net/net.gyp:net_java',
You've removed both the dependencies and the input_jar_paths - I just want to
confirm this was intentional (eg: it's being picked up by chrome_java)

http://codereview.chromium.org/10837143/diff/10003/content/content_tests.gypi
File content/content_tests.gypi (left):

http://codereview.chromium.org/10837143/diff/10003/content/content_tests.gypi...
content/content_tests.gypi:761: '../net/net.gyp:net_java',
nit: sort

[commit-bot: I haz the power, 2012-08-09 16:27:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/10837143/13001

[commit-bot: I haz the power, 2012-08-09 18:08:24]

Change committed as 150850