Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646443002:
Creating libwebrtc bundle jar (Closed)
DescriptionCreating libwebrtc bundle jar
Creates a JAR which includes:
- //webrtc/base:base_java
- //webrtc/modules/audio_device:audio_device_java
- //webrtc/sdk/android:libjingle_peerconnection_java
- //webrtc/sdk/android:libjingle_peerconnection_metrics_default_java
The libwebrtc.jar file will be generated at '<output_dir>/lib.java/webrtc/sdk/android/libwebrtc.jar'.
BUG=webrtc:6356
Review-Url: https://codereview.webrtc.org/2646443002
Cr-Commit-Position: refs/heads/master@{#16189}
Committed: https://chromium.googlesource.com/external/webrtc/+/a62a82b7e7da5a1bbbf8b5614ef19334cc1603ce
Patch Set 1 #
Total comments: 11
Patch Set 2 : Renaming dist_jar target to libwebrtc #
Total comments: 4
Patch Set 3 : Removing unused template and fixing an error #Patch Set 4 : Skipping base_java in chromium builds #
Messages
Total messages: 23 (7 generated)
mbonadei@webrtc.org changed reviewers: + kjellander@webrtc.org, sakal@webrtc.org
This should generate a libjingle_peerconnection_java_with_deps.jar file with all the .class files from both 'webrtc/base:base_java' and 'webrtc/modules/audio_device:audio_device_java'. I have a doubt: this jar is not generated because the dist_jar target is never used as a dependency, which target is more suited to trigger the generation of this jar? We cannot just replace 'libjingle_peerconnection_java' with 'libjingle_peerconnection_java_with_deps' because it may cause problems during the dexing phase (maybe we can workaround this but it seems not too clean). kjellander@: I think that we can get rid of this too: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/webrt...
I think you are correct this will not be automatically generated because it is not used a dependency. However, it can be manually generated by specifying the target. I think this is fine for now because this will probably be used as a dependency to .aar-target in the future. https://codereview.webrtc.org/2646443002/diff/1/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2646443002/diff/1/webrtc/examples/BUILD.gn#newc... webrtc/examples/BUILD.gn:96: "//webrtc/modules/audio_device:audio_device_java", Is this really needed? //webrtc/sdk/android:libjingle_peerconnection_java already depends on //webrtc/modules/audio_device:audio_device_java. If you need to add this dependency to projects, this patch will break downstream dependencies that have to be fixed. https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn#n... webrtc/sdk/android/BUILD.gn:113: dist_jar("libjingle_peerconnection_java_with_deps") { Maybe a simpler name? I think this could even be called libwebrtc. https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn#n... webrtc/sdk/android/BUILD.gn:119: ":libjingle_peerconnection_java", I think libjingle_peerconnection_metrics_default_java should be included.
https://codereview.webrtc.org/2646443002/diff/1/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2646443002/diff/1/webrtc/examples/BUILD.gn#newc... webrtc/examples/BUILD.gn:96: "//webrtc/modules/audio_device:audio_device_java", On 2017/01/18 13:21:44, sakal wrote: > Is this really needed? //webrtc/sdk/android:libjingle_peerconnection_java > already depends on //webrtc/modules/audio_device:audio_device_java. > > If you need to add this dependency to projects, this patch will break downstream > dependencies that have to be fixed. It seems to be. I tried to comment the dependency but it results in this error: ../../webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:54: error: package org.webrtc.voiceengine does not exist import org.webrtc.voiceengine.WebRtcAudioManager; [...] It seems that deps are not transitive. Were you expecting them to be transitive? https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn#n... webrtc/sdk/android/BUILD.gn:113: dist_jar("libjingle_peerconnection_java_with_deps") { On 2017/01/18 13:21:44, sakal wrote: > Maybe a simpler name? I think this could even be called libwebrtc. Yep, I agree. I renamed the target to libwebrtc even if it is not aligned with the naming of the surrounding targets. https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn#n... webrtc/sdk/android/BUILD.gn:119: ":libjingle_peerconnection_java", On 2017/01/18 13:21:44, sakal wrote: > I think libjingle_peerconnection_metrics_default_java should be included. I see what you mean. I can include it but I have a proposal, let me know what do you think about that. libjingle_peerconnection_metrics_default_java seems to required only by libjingle_peerconnection_android_unittest (it is also a dependency of AppRTCMobile_javalib but it is not really needed), why are we keeping a separate target only for one class? Can't we just put the file "api/org/webrtc/Metrics.java" into the java_files of libjingle_peerconnection_java and remove the target libjingle_peerconnection_metrics_default_java?
https://codereview.webrtc.org/2646443002/diff/1/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2646443002/diff/1/webrtc/examples/BUILD.gn#newc... webrtc/examples/BUILD.gn:96: "//webrtc/modules/audio_device:audio_device_java", On 2017/01/19 08:30:45, mbonadei wrote: > On 2017/01/18 13:21:44, sakal wrote: > > Is this really needed? //webrtc/sdk/android:libjingle_peerconnection_java > > already depends on //webrtc/modules/audio_device:audio_device_java. > > > > If you need to add this dependency to projects, this patch will break > downstream > > dependencies that have to be fixed. > > It seems to be. I tried to comment the dependency but it results in this error: > > ../../webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:54: > error: package org.webrtc.voiceengine does not exist > import org.webrtc.voiceengine.WebRtcAudioManager; > [...] > > It seems that deps are not transitive. Were you expecting them to be transitive? It is a shame. It would be nice to be able to just include a single target to use WebRTC java code. I guess it is fine though. We are trying to bring all Java code under sdk/android eventually. https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn#n... webrtc/sdk/android/BUILD.gn:113: dist_jar("libjingle_peerconnection_java_with_deps") { On 2017/01/19 08:30:45, mbonadei wrote: > On 2017/01/18 13:21:44, sakal wrote: > > Maybe a simpler name? I think this could even be called libwebrtc. > > Yep, I agree. > I renamed the target to libwebrtc even if it is not aligned with the naming of > the surrounding targets. I think the surrounding target names have just some legacy reasons behind them. https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn#n... webrtc/sdk/android/BUILD.gn:119: ":libjingle_peerconnection_java", On 2017/01/19 08:30:45, mbonadei wrote: > On 2017/01/18 13:21:44, sakal wrote: > > I think libjingle_peerconnection_metrics_default_java should be included. > > I see what you mean. I can include it but I have a proposal, let me know what do > you think about that. > > libjingle_peerconnection_metrics_default_java seems to required only by > libjingle_peerconnection_android_unittest (it is also a dependency of > AppRTCMobile_javalib but it is not really needed), why are we keeping a separate > target only for one class? Can't we just put the file > "api/org/webrtc/Metrics.java" into the java_files of > libjingle_peerconnection_java and remove the target > libjingle_peerconnection_metrics_default_java? > The reason this target exists is to allow clients to provide custom metrics implementations. Metrics.java relies on using the default metrics_default.cc and is not compatible with custom implementations. I would expect the kind of applications that include the libwebrtc.jar to just use the default implementation though.
https://codereview.webrtc.org/2646443002/diff/1/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2646443002/diff/1/webrtc/examples/BUILD.gn#newc... webrtc/examples/BUILD.gn:96: "//webrtc/modules/audio_device:audio_device_java", On 2017/01/19 09:06:21, sakal wrote: > On 2017/01/19 08:30:45, mbonadei wrote: > > On 2017/01/18 13:21:44, sakal wrote: > > > Is this really needed? //webrtc/sdk/android:libjingle_peerconnection_java > > > already depends on //webrtc/modules/audio_device:audio_device_java. > > > > > > If you need to add this dependency to projects, this patch will break > > downstream > > > dependencies that have to be fixed. > > > > It seems to be. I tried to comment the dependency but it results in this > error: > > > > > ../../webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:54: > > error: package org.webrtc.voiceengine does not exist > > import org.webrtc.voiceengine.WebRtcAudioManager; > > [...] > > > > It seems that deps are not transitive. Were you expecting them to be > transitive? > > It is a shame. It would be nice to be able to just include a single target to > use WebRTC java code. I guess it is fine though. We are trying to bring all Java > code under sdk/android eventually. Yes it is, I think there will be some good reasons for this (such as those that led us to the dist_jar target instead of adding public_deps to android_library). https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2646443002/diff/1/webrtc/sdk/android/BUILD.gn#n... webrtc/sdk/android/BUILD.gn:119: ":libjingle_peerconnection_java", On 2017/01/19 09:06:21, sakal wrote: > On 2017/01/19 08:30:45, mbonadei wrote: > > On 2017/01/18 13:21:44, sakal wrote: > > > I think libjingle_peerconnection_metrics_default_java should be included. > > > > I see what you mean. I can include it but I have a proposal, let me know what > do > > you think about that. > > > > libjingle_peerconnection_metrics_default_java seems to required only by > > libjingle_peerconnection_android_unittest (it is also a dependency of > > AppRTCMobile_javalib but it is not really needed), why are we keeping a > separate > > target only for one class? Can't we just put the file > > "api/org/webrtc/Metrics.java" into the java_files of > > libjingle_peerconnection_java and remove the target > > libjingle_peerconnection_metrics_default_java? > > > > The reason this target exists is to allow clients to provide custom metrics > implementations. Metrics.java relies on using the default metrics_default.cc and > is not compatible with custom implementations. I would expect the kind of > applications that include the libwebrtc.jar to just use the default > implementation though. Ok, thanks for the explanation.
I am getting this error from a couple of trybots:
ERROR at //build/config/android/internal_rules.gni:121:23: Can't load input
file.
deps += [ "${_target_label}__build_config" ]
^-------------------------------
Unable to load:
/b/c/b/android/src/webrtc/base/BUILD.gn
I also checked in the secondary tree for:
/b/c/b/android/src/build/secondary/webrtc/base/BUILD.gn
GN gen failed: 1
This is really strange and at first sight it seems that we are missing a ":"
before "${_target_label}__build_config".
I tried to read some .gni files and there are things like:
[...]
build_config_target_name = "${target_name}__build_config"
[...]
deps += [ ":$build_config_target_name" ]
While in our case it is: deps += [ "${_target_label}__build_config" ]. No ":".
I think the trybot problem you're seeing can be solved by adding !build_with_chromium into the condition at https://cs.chromium.org/chromium/src/third_party/webrtc/base/BUILD.gn?rcl=0&l... https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/BUILD.gn (left): https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/BUILD.gn:349: if (!build_with_chromium && is_android) { Why remove !build_with_chromium ? We don't want to build this target from a Chromium build (since it's not used). webrtc/examples is not processed by Chromium either, so if it's because you added the dependency there, you don't need to change this. https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/BUILD.gn:350: android_shared_srcjar("audio_device_java") { Please remove the template from webrtc/build/webrtc.gni in this same CL, as it's no longer used.
also update the CL description, as we're creating a libwebrtc bundle jar, right? It would be useful to let the reader know where the created JAR file will be created as well.
https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/BUILD.gn (left): https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/BUILD.gn:349: if (!build_with_chromium && is_android) { On 2017/01/20 07:58:50, kjellander_webrtc wrote: > Why remove !build_with_chromium ? We don't want to build this target from a > Chromium build (since it's not used). > > webrtc/examples is not processed by Chromium either, so if it's because you > added the dependency there, you don't need to change this. Done, I don't exactly remember why I removed that. But it makes sense to skip this in chromium. https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/BUILD.gn:350: android_shared_srcjar("audio_device_java") { On 2017/01/20 07:58:50, kjellander_webrtc wrote: > Please remove the template from webrtc/build/webrtc.gni in this same CL, as it's > no longer used. Done.
Description was changed from ========== Creating libjingle_peerconnection_java bundle jar The goal of this CL is to create a libjingle_peerconnection_java JAR which also include its dependencies: - //webrtc/base:base_java - //webrtc/modules/audio_device:audio_device_java BUG=webrtc:6356 ========== to ========== Creating libwebrtc bundle jar The goal of this CL is to create a JAR which includes: - //webrtc/base:base_java - //webrtc/modules/audio_device:audio_device_java - //webrtc/sdk/android:libjingle_peerconnection_java - //webrtc/sdk/android:libjingle_peerconnection_metrics_default_java The libwebrtc.jar file will be generated at '<output_dir>/lib.java/webrtc/sdk/android/libwebrtc.jar'. BUG=webrtc:6356 ==========
On 2017/01/20 07:59:46, kjellander_webrtc wrote: > also update the CL description, as we're creating a libwebrtc bundle jar, right? > > It would be useful to let the reader know where the created JAR file will be > created as well. Done.
lgtm
On 2017/01/20 07:58:50, kjellander_webrtc wrote: > I think the trybot problem you're seeing can be solved by adding > !build_with_chromium into the condition at > https://cs.chromium.org/chromium/src/third_party/webrtc/base/BUILD.gn?rcl=0&l... I still think it's an improvement if we remove base_java from Chromium, since it'll speed up the Chromium build. I guess the error earlier was caused by the absolute dependency on //webrtc/base:base_java (which isn't found in Chromium). lgtm but please consider fixing the above. > https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/BUILD.gn (left): > > https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/BUILD.gn:349: if (!build_with_chromium && > is_android) { > Why remove !build_with_chromium ? We don't want to build this target from a > Chromium build (since it's not used). > > webrtc/examples is not processed by Chromium either, so if it's because you > added the dependency there, you don't need to change this. > > https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/BUILD.gn:350: > android_shared_srcjar("audio_device_java") { > Please remove the template from webrtc/build/webrtc.gni in this same CL, as it's > no longer used.
Description was changed from ========== Creating libwebrtc bundle jar The goal of this CL is to create a JAR which includes: - //webrtc/base:base_java - //webrtc/modules/audio_device:audio_device_java - //webrtc/sdk/android:libjingle_peerconnection_java - //webrtc/sdk/android:libjingle_peerconnection_metrics_default_java The libwebrtc.jar file will be generated at '<output_dir>/lib.java/webrtc/sdk/android/libwebrtc.jar'. BUG=webrtc:6356 ========== to ========== Creating libwebrtc bundle jar Creates a JAR which includes: - //webrtc/base:base_java - //webrtc/modules/audio_device:audio_device_java - //webrtc/sdk/android:libjingle_peerconnection_java - //webrtc/sdk/android:libjingle_peerconnection_metrics_default_java The libwebrtc.jar file will be generated at '<output_dir>/lib.java/webrtc/sdk/android/libwebrtc.jar'. BUG=webrtc:6356 ==========
On 2017/01/20 10:26:25, kjellander_webrtc wrote: > On 2017/01/20 07:58:50, kjellander_webrtc wrote: > > I think the trybot problem you're seeing can be solved by adding > > !build_with_chromium into the condition at > > > https://cs.chromium.org/chromium/src/third_party/webrtc/base/BUILD.gn?rcl=0&l... > > I still think it's an improvement if we remove base_java from Chromium, since > it'll speed up the Chromium build. > I guess the error earlier was caused by the absolute dependency on > //webrtc/base:base_java (which isn't found in Chromium). > > lgtm but please consider fixing the above. > > > > > https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... > > File webrtc/modules/audio_device/BUILD.gn (left): > > > > > https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... > > webrtc/modules/audio_device/BUILD.gn:349: if (!build_with_chromium && > > is_android) { > > Why remove !build_with_chromium ? We don't want to build this target from a > > Chromium build (since it's not used). > > > > webrtc/examples is not processed by Chromium either, so if it's because you > > added the dependency there, you don't need to change this. > > > > > https://codereview.webrtc.org/2646443002/diff/20001/webrtc/modules/audio_devi... > > webrtc/modules/audio_device/BUILD.gn:350: > > android_shared_srcjar("audio_device_java") { > > Please remove the template from webrtc/build/webrtc.gni in this same CL, as > it's > > no longer used. I slightly rephrased the CL description (since it's going to become the commit message, it might be confusing to some to read about "this CL" in there).
The CQ bit was checked by mbonadei@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2646443002/#ps60001 (title: "Skipping base_java in chromium builds")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484913222098490,
"parent_rev": "fefe076789f9294aaeb0ecc4221ad5e5970492e9", "commit_rev":
"a62a82b7e7da5a1bbbf8b5614ef19334cc1603ce"}
Message was sent while issue was closed.
Description was changed from ========== Creating libwebrtc bundle jar Creates a JAR which includes: - //webrtc/base:base_java - //webrtc/modules/audio_device:audio_device_java - //webrtc/sdk/android:libjingle_peerconnection_java - //webrtc/sdk/android:libjingle_peerconnection_metrics_default_java The libwebrtc.jar file will be generated at '<output_dir>/lib.java/webrtc/sdk/android/libwebrtc.jar'. BUG=webrtc:6356 ========== to ========== Creating libwebrtc bundle jar Creates a JAR which includes: - //webrtc/base:base_java - //webrtc/modules/audio_device:audio_device_java - //webrtc/sdk/android:libjingle_peerconnection_java - //webrtc/sdk/android:libjingle_peerconnection_metrics_default_java The libwebrtc.jar file will be generated at '<output_dir>/lib.java/webrtc/sdk/android/libwebrtc.jar'. BUG=webrtc:6356 Review-Url: https://codereview.webrtc.org/2646443002 Cr-Commit-Position: refs/heads/master@{#16189} Committed: https://chromium.googlesource.com/external/webrtc/+/a62a82b7e7da5a1bbbf8b5614... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/a62a82b7e7da5a1bbbf8b5614...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/2640023010/ by mbonadei@webrtc.org. The reason for reverting is: This breaks some chromium.webrtc.fyi buildbots with the following error: ERROR Unresolved dependencies. //third_party/webrtc/base:base(//build/toolchain/android:android_arm) needs //third_party/webrtc/base:base_java(//build/toolchain/android:android_arm) . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
