Any update on this, its blocking us to migrate to GN.

Apparently this was not required for the GN transition.

Andrew: do you know what this is about? I don't understand the jar build process. Is this something we still might want?

The android templates have no notion of public_deps vs deps, and only the "deps" variable is respected.

What's the use-case for wanting public_deps rather than deps?

In WebRTC, "libjingle_peerconnection_java" depends on "//webrtc/base:base_java". Both targets are android libraries containing Java files. We would like to create a jar that contains Java files from both libraries. Currently, libjingle_peerconnection_java.jar only contains Java files defined by target "libjingle_peerconnection_java".

See bug this bug is blocking: https://bugs.chromium.org/p/webrtc/issues/detail?id=6356

Gotcha. I think the best way to accomplish this would be to use a python script that just merges the .jar files.

And I think there's one that already exists! :)

https://cs.chromium.org/chromium/src/build/config/android/rules.gni?q=create_dist_jar&sq=package:chromium&l=1781 (create_dist_jar.py).

Maybe try pulling that action() out into a standalone template so that webrtc can call it as well?

Ack, I don't know when I have the time to implement this though. Marking bug as Available for now if someone wants to take it.

Is it possible to use the "srcjar_deps" attribute of the "android_library" template to solve this issue? [1]

To use that we should add a target which is able to generate a .srcjar file to the "srcjar_deps" list. 

We have done that in webrtc [2] but probably it would be useful that the "android_library" template should be able to generate an .srcjar file as an implicit output so it would be easier to add them in the srcjar_deps list.

What do you think?


[1] - https://cs.chromium.org/chromium/src/build/config/android/rules.gni?q=build/config/android/ru&sq=package:chromium&l=1209
[2] - https://codereview.webrtc.org/2612953004/

I think that sounds more complicated than what I suggested in #6, and would result in the .java files being compiled a second time rather than just .class files copied.

AFICT, the request here basically equates to unzipping two jar files to the same directory, and then zipping them up again as one .jar. Should be pretty easy.

It might be possible to use the somewhat new additional_jar_files field to accomplish this now as well:
https://cs.chromium.org/chromium/src/build/config/android/rules.gni?q=android_library+rules&sq=package:chromium&l=1131

E.g. pass the output jar of base_java as an additional_jar_files to libjingle_peerconnection_java

Yep, I was looking at "additional_jar_files" but it seems that it adds a list of files to the output jar, so passing the output jar of base_java will not result in the inclusion of all the .class files but in the inclusion of the jar file. (not 100% sure of this but it is what I understood).

I was also looking into the comment #6 approach. I like it but I am not sure on how to implement it.
Let me try... At the end of the android_library target we can use a "foreach" to iterate on the public_deps list. Then foreach public_dep we have to invoke "create_dist_jar.py" with the public_dep jar file (it should be located at <target_out_dir> + <name>.jar) and the current target jar file as args.

What do you think?

#10 sounds good to me. I would prefer a script that gets passed all the jar files to combine in a single run but the end result should be the same.

Sounding like it might be easiest if I just go ahead and do what's in #6. I should have time tomorrow.

#12: Yes, this is what I am trying to do now.

The script is already there and it works fine. I am just struggling in finding the paths to the jar files of all the targets in "public_deps" and the path to the jar file of the current target. Have you got any hints (I am new to gn)?

If I am able to find the paths I can open a CL with you as a reviewer and we can discuss there.

Thanks.

Here's a patch that adds a "dist_jar" template:
https://codereview.chromium.org/2623243002

Could one of you patch it in and see if this does what you want?

Thank you!

Tomorrow I will try and I will let you know.

I tried the patch you provided: https://codereview.webrtc.org/2628933004

It seems it didn't include any of the files defined by libjingle_peerconnection_java or libjingle_peerconnection_metrics_default_java.

However, it did include some javax classes and Android support libraries. I don't think this is desirable.

Maybe the script should only include public dependencies instead of all dependencies?

#16: I tried to add 'public_deps' support to 'android_library' (https://codereview.chromium.org/2627223003) and it seems to do what we want.
The problem you have experienced is probably due to a comment I left on https://codereview.chromium.org/2623243002/diff/1/build/android/gyp/write_build_config.py#oldcode635.

To solve the overall issue I would like to add support to 'public_deps' in android_library using the 'dist_jar' template. It seems more "self contained". Or do you want to use the 'dist_jar' template on its own to link N targets?

Hrm, I wondered it maybe dist_jar would pull in too much...

I don't think adding public_deps is a good idea, because of:
java_libarary("A") {...}
java_libarary("B") {
  public_deps = [":A"]
}
java_libarary("C") {
  deps = [":A", ":B"]
}

Now C will have two copies of A in its classpath, and when dexing, I believe it fails when the same class is added twice.



I don't think we could change dist_jar to only work on direct deps by default (because currently android_apk relies on it adding everything), but we could certainly add that as an option, or use filters. E.g.

dist_jar("libpeerconnection_dist") {
  deps = [":A", ":B"]
  include_transitive_deps = false  # Only include class files from :A and :B
}

dist_jar("libpeerconnection_dist") {
  deps = ...
  include_filters = ["*cronet*"] # We expect users to manually add other dependencies
}

wdyt?

Yeah, I see what you mean about adding public_deps.

On the other topic it looks good to me to use the option (include_transitive_deps), it is more straightforward than include_filters (unless you are planning to add both of them).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77ad9ee9d1c3ca1ef984900a31cb206bf885b936

commit 77ad9ee9d1c3ca1ef984900a31cb206bf885b936
Author: agrieve <agrieve@chromium.org>
Date: Tue Jan 17 15:33:23 2017

android: Create a GN template for create_dist_jar.py

BUG= 648244 

Review-Url: https://codereview.chromium.org/2623243002
Cr-Commit-Position: refs/heads/master@{#444054}

[modify] https://crrev.com/77ad9ee9d1c3ca1ef984900a31cb206bf885b936/build/android/gyp/write_build_config.py
[modify] https://crrev.com/77ad9ee9d1c3ca1ef984900a31cb206bf885b936/build/config/android/internal_rules.gni
[modify] https://crrev.com/77ad9ee9d1c3ca1ef984900a31cb206bf885b936/build/config/android/rules.gni