Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2610823002:
Fixing package-boundary violation with srjar_deps (Closed)
|
Created:
3 years, 11 months ago by mbonadei Modified:
3 years, 11 months ago Reviewers:
kjellander_webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixing package-boundary violation with srjar_deps
Without the usage of the srcjar_deps attribute we were not able to
include .java files from other packages without violating the package
boundary contraint.
As an example, in this CL the target "libjingle_peerconnection_java" was
directly including .java files from another packages in its "java_files"
attribute.
Using srcjar_deps we are able to declare the dependency of the target
avoiding to create hidden dependencies in the codebase.
This is not fixing the webrtc:6356 bug directly but it is a first step to
include ThreadUtils classes in libjingle_peerconnection_client_java.jar
again.
It seems also to be related to the chromium:648244 bug. This can be solved
if we can find a way to perform srcjar generation in the android_library
target without changing the semantic of the target.
BUG=webrtc:6356
Review-Url: https://codereview.webrtc.org/2610823002
Cr-Commit-Position: refs/heads/master@{#15914}
Committed: https://chromium.googlesource.com/external/webrtc/+/10a76592a78762187bfaf3547b7e9986454062ab
Patch Set 1 #
Total comments: 26
Patch Set 2 : Addressing comments #
Total comments: 2
Patch Set 3 : Addressing some formatting comments #Patch Set 4 : Moving generate_srcjar script to webrtc/build #Patch Set 5 : Including .gni android configs only if is_android #
Messages
Total messages: 19 (7 generated)
mbonadei@webrtc.org changed reviewers: + kjellander@webrtc.org
This is the proof of concept. It seems to work, we are now able to include voiceengine into libjingle_peerconnection_java without violating the cross boundary check. The name of the template and the template itself are under scrutiny, it would be great to enable the generation of the srcjar in the android_library itself, so we can get that feature for free. But I don't know it is a feature that violates other properties of the android_library template.
Can you provide some small examples on how you verify this is working? e.g. running jar tvf libjingle_peerconnection_java.jar | wc -l before and after this change, verifying with this change, the number is higher (maybe also grep on the new ones to show they're included). It could be useful to verify the paths are also correct. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py File webrtc/generate_srcjar.py (right): https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:3: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. New files should have the current year. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:11: #TODO(mbonadei): move this script into chromium (build/android/gyp) Comments usually don't start right after the #. I'm not sure the style guide mandates it but I think it's more readable, so please add a space. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:21: import ntpath sort alphabetically https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:23: #TODO(mbonadei): fix this import and meke it relative to the chromium meke -> make https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:25: sys.path.insert(0, os.path.abspath('../../build/android/gyp/util')) Please build the path using os.path.join and os.pardir, like this: os.path.join(os.pardir, os.pardir, 'build', 'android', 'gyp', 'util) otherwise this will fail on Windows. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:29: def FindZipPathFromSource(src_file): Can you add a docstring what this method does? I assume it's looking up a directory path to be used for the file inside the zip file, based on the package name of the .java file? That kind of text could go into the docstring. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:30: with open(src_file, "r") as f: You can leave out 'r', as it's the default if mode is omitted. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:32: package = re.search("package (.*);", file_src).group(1) Please use single-quotes for all strings. The style guide [1] says "Be consistent with your choice of string quote character within a file. Pick ' or " and stick with it. ". We usually have single-quotes in the WebRTC codebase, and we align with Chromium's style recommendations. [1] http://dev.chromium.org/chromium-os/python-style-guidelines and more specifically http://google.github.io/styleguide/pyguide.html?showone=Strings#Strings https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:34: file_name = ntpath.basename(src_file) I don't ntpath is useful here. Just regular os.path.basename seems better and more portable. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:41: parser.add_option('--srcjar', For our use case this is a required flag, right? Then the comment should be updated and also throw a parser.error if it's omitted. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:53: zip_path = FindZipPathFromSource(src_path) This manipulation just to figure out the path inside the zip file is a bit annoying, but I guess it's OK. https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... webrtc/modules/audio_device/BUILD.gn:9: import("//webrtc/build/webrtc.gni") Please restore this one. Otherwise it'll break in Chromium. https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... webrtc/modules/audio_device/BUILD.gn:303: template("android_shared_srcjar") { Let's put this template into webrtc/build/webrtc.gni right away.
This is the manual test done after a 'ninja -C out/android': jar tvf out/android/lib.java/webrtc/sdk/android/libjingle_peerconnection_java.jar | wc -l 204 jar tvf out/android/lib.java/webrtc/sdk/android/libjingle_peerconnection_java.jar | grep voiceengine 7617 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioRecord.class 7866 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioEffects.class 5433 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioUtils.class 1141 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/BuildInfo.class 2686 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioRecord$AudioRecordThread.class 7208 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioTrack.class 8061 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager.class 3666 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioTrack$AudioTrackThread.class 1702 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager$VolumeLogger.class 1683 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager$VolumeLogger$LogVolumeTask.class https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py File webrtc/generate_srcjar.py (right): https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:3: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/04 14:25:21, kjellander_webrtc wrote: > New files should have the current year. Good catch, I am still writing 2016 everywhere. :-) https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:11: #TODO(mbonadei): move this script into chromium (build/android/gyp) On 2017/01/04 14:25:21, kjellander_webrtc wrote: > Comments usually don't start right after the #. > I'm not sure the style guide mandates it but I think it's more readable, so > please add a space. Acknowledged. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:21: import ntpath On 2017/01/04 14:25:21, kjellander_webrtc wrote: > sort alphabetically Acknowledged. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:23: #TODO(mbonadei): fix this import and meke it relative to the chromium On 2017/01/04 14:25:21, kjellander_webrtc wrote: > meke -> make Acknowledged. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:25: sys.path.insert(0, os.path.abspath('../../build/android/gyp/util')) On 2017/01/04 14:25:21, kjellander_webrtc wrote: > Please build the path using os.path.join and os.pardir, like this: > os.path.join(os.pardir, os.pardir, 'build', 'android', 'gyp', 'util) > > otherwise this will fail on Windows. Acknowledged. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:29: def FindZipPathFromSource(src_file): On 2017/01/04 14:25:21, kjellander_webrtc wrote: > Can you add a docstring what this method does? > > I assume it's looking up a directory path to be used for the file inside the zip > file, based on the package name of the .java file? > That kind of text could go into the docstring. Acknowledged. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:30: with open(src_file, "r") as f: On 2017/01/04 14:25:21, kjellander_webrtc wrote: > You can leave out 'r', as it's the default if mode is omitted. Acknowledged. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:32: package = re.search("package (.*);", file_src).group(1) On 2017/01/04 14:25:21, kjellander_webrtc wrote: > Please use single-quotes for all strings. The style guide [1] says "Be > consistent with your choice of string quote character within a file. Pick ' or " > and stick with it. ". > We usually have single-quotes in the WebRTC codebase, and we align with > Chromium's style recommendations. > > [1] http://dev.chromium.org/chromium-os/python-style-guidelines and more > specifically > http://google.github.io/styleguide/pyguide.html?showone=Strings#Strings Acknowledged. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:34: file_name = ntpath.basename(src_file) On 2017/01/04 14:25:21, kjellander_webrtc wrote: > I don't ntpath is useful here. Just regular os.path.basename seems better and > more portable. Acknowledged. https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:41: parser.add_option('--srcjar', On 2017/01/04 14:25:21, kjellander_webrtc wrote: > For our use case this is a required flag, right? Then the comment should be > updated and also throw a parser.error if it's omitted. Acknowledged. I am not throwing parser.error in the next patch set because it prints a useful message if not present (because there is no default). https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... webrtc/generate_srcjar.py:53: zip_path = FindZipPathFromSource(src_path) On 2017/01/04 14:25:21, kjellander_webrtc wrote: > This manipulation just to figure out the path inside the zip file is a bit > annoying, but I guess it's OK. Yeah, I agree but I think that probably it is the most reliable source for that kind of information. https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... webrtc/modules/audio_device/BUILD.gn:9: import("//webrtc/build/webrtc.gni") On 2017/01/04 14:25:21, kjellander_webrtc wrote: > Please restore this one. Otherwise it'll break in Chromium. Acknowledged. Ok, I was wondering why there was the relative path. :) https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... webrtc/modules/audio_device/BUILD.gn:303: template("android_shared_srcjar") { On 2017/01/04 14:25:21, kjellander_webrtc wrote: > Let's put this template into webrtc/build/webrtc.gni right away. Acknowledged.
On 2017/01/04 15:19:06, mbonadei wrote: > This is the manual test done after a 'ninja -C out/android': > > jar tvf > out/android/lib.java/webrtc/sdk/android/libjingle_peerconnection_java.jar | wc > -l > 204 > > jar tvf > out/android/lib.java/webrtc/sdk/android/libjingle_peerconnection_java.jar | grep > voiceengine > 7617 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioRecord.class > 7866 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioEffects.class > 5433 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioUtils.class > 1141 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/BuildInfo.class > 2686 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioRecord$AudioRecordThread.class > 7208 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioTrack.class > 8061 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioManager.class > 3666 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioTrack$AudioTrackThread.class > 1702 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioManager$VolumeLogger.class > 1683 Wed Jan 04 16:14:18 CET 2017 > org/webrtc/voiceengine/WebRtcAudioManager$VolumeLogger$LogVolumeTask.class > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py > File webrtc/generate_srcjar.py (right): > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:3: # Copyright (c) 2016 The WebRTC project authors. > All Rights Reserved. > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > New files should have the current year. > > Good catch, I am still writing 2016 everywhere. :-) > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:11: #TODO(mbonadei): move this script into chromium > (build/android/gyp) > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > Comments usually don't start right after the #. > > I'm not sure the style guide mandates it but I think it's more readable, so > > please add a space. > > Acknowledged. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:21: import ntpath > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > sort alphabetically > > Acknowledged. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:23: #TODO(mbonadei): fix this import and meke it > relative to the chromium > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > meke -> make > > Acknowledged. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:25: sys.path.insert(0, > os.path.abspath('../../build/android/gyp/util')) > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > Please build the path using os.path.join and os.pardir, like this: > > os.path.join(os.pardir, os.pardir, 'build', 'android', 'gyp', 'util) > > > > otherwise this will fail on Windows. > > Acknowledged. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:29: def FindZipPathFromSource(src_file): > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > Can you add a docstring what this method does? > > > > I assume it's looking up a directory path to be used for the file inside the > zip > > file, based on the package name of the .java file? > > That kind of text could go into the docstring. > > Acknowledged. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:30: with open(src_file, "r") as f: > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > You can leave out 'r', as it's the default if mode is omitted. > > Acknowledged. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:32: package = re.search("package (.*);", > file_src).group(1) > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > Please use single-quotes for all strings. The style guide [1] says "Be > > consistent with your choice of string quote character within a file. Pick ' or > " > > and stick with it. ". > > We usually have single-quotes in the WebRTC codebase, and we align with > > Chromium's style recommendations. > > > > [1] http://dev.chromium.org/chromium-os/python-style-guidelines and more > > specifically > > http://google.github.io/styleguide/pyguide.html?showone=Strings#Strings > > Acknowledged. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:34: file_name = ntpath.basename(src_file) > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > I don't ntpath is useful here. Just regular os.path.basename seems better and > > more portable. > > Acknowledged. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:41: parser.add_option('--srcjar', > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > For our use case this is a required flag, right? Then the comment should be > > updated and also throw a parser.error if it's omitted. > > Acknowledged. > > I am not throwing parser.error in the next patch set because it prints a useful > message if not present (because there is no default). > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/generate_srcjar.py#new... > webrtc/generate_srcjar.py:53: zip_path = FindZipPathFromSource(src_path) > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > This manipulation just to figure out the path inside the zip file is a bit > > annoying, but I guess it's OK. > > Yeah, I agree but I think that probably it is the most reliable source for that > kind of information. > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... > File webrtc/modules/audio_device/BUILD.gn (right): > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... > webrtc/modules/audio_device/BUILD.gn:9: import("//webrtc/build/webrtc.gni") > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > Please restore this one. Otherwise it'll break in Chromium. > > Acknowledged. > > Ok, I was wondering why there was the relative path. :) > > https://codereview.webrtc.org/2610823002/diff/1/webrtc/modules/audio_device/B... > webrtc/modules/audio_device/BUILD.gn:303: template("android_shared_srcjar") { > On 2017/01/04 14:25:21, kjellander_webrtc wrote: > > Let's put this template into webrtc/build/webrtc.gni right away. > > Acknowledged. Just give me 30 minutes (I have a meeting) and I will paste the same manual test on master.
Here is the full comparison. Before (with the old include which is violating the package boundaries): jar tvf out/android/lib.java/webrtc/sdk/android/libjingle_peerconnection_java.jar | wc -l 204 jar tvf out/android/lib.java/webrtc/sdk/android/libjingle_peerconnection_java.jar | grep voiceengine 7617 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioRecord.class 7866 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioEffects.class 5433 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioUtils.class 1141 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/BuildInfo.class 2686 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioRecord$AudioRecordThread.class 7208 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioTrack.class 8061 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager.class 3666 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioTrack$AudioTrackThread.class 1702 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager$VolumeLogger.class 1683 Wed Jan 04 16:56:48 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager$VolumeLogger$LogVolumeTask.class After (with this CL): jar tvf out/android/lib.java/webrtc/sdk/android/libjingle_peerconnection_java.jar | wc -l 204 jar tvf out/android/lib.java/webrtc/sdk/android/libjingle_peerconnection_java.jar | grep voiceengine 7617 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioRecord.class 7866 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioEffects.class 5433 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioUtils.class 1141 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/BuildInfo.class 2686 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioRecord$AudioRecordThread.class 7208 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioTrack.class 8061 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager.class 3666 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioTrack$AudioTrackThread.class 1702 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager$VolumeLogger.class 1683 Wed Jan 04 16:14:18 CET 2017 org/webrtc/voiceengine/WebRtcAudioManager$VolumeLogger$LogVolumeTask.class
Nice work! lgtm with nit addressed. Please also try to format the CL title and description according Git message standard: http://stackoverflow.com/questions/2290016/git-commit-messages-50-72-formatting https://codereview.webrtc.org/2610823002/diff/20001/webrtc/generate_srcjar.py File webrtc/generate_srcjar.py (right): https://codereview.webrtc.org/2610823002/diff/20001/webrtc/generate_srcjar.py... webrtc/generate_srcjar.py:30: """Given a path to a .java source file it returns the path of the file The first sentence in the docstring (the short description) must fit on one line. So move out some into another sentence, which is separated by a blank line.
On 2017/01/04 20:03:45, kjellander_webrtc wrote: > Nice work! > lgtm with nit addressed. > > Please also try to format the CL title and description according Git message > standard: > http://stackoverflow.com/questions/2290016/git-commit-messages-50-72-formatting > > https://codereview.webrtc.org/2610823002/diff/20001/webrtc/generate_srcjar.py > File webrtc/generate_srcjar.py (right): > > https://codereview.webrtc.org/2610823002/diff/20001/webrtc/generate_srcjar.py... > webrtc/generate_srcjar.py:30: """Given a path to a .java source file it returns > the path of the file > The first sentence in the docstring (the short description) must fit on one > line. > > So move out some into another sentence, which is separated by a blank line. Also rephrase it to something more descriptive than "Trying to include java files..."
Description was changed from ========== Trying to include java files from another library with srcjar_deps. This is not fixing the bug directly but it is a first step to include ThreadUtils classes in libjingle_peerconnection_java.jar again. It can be also related to chromium:648244 which we can solve if we find a way to perform srcjar generation in the android_library target without changing the current semantic of the target. BUG=webrtc:6356 ========== to ========== Fixing package-boundary violation with srjar_deps Without the usage of the srcjar_deps attribute we were not able to include .java files from other packages without violating the package boundary contraint. As an example, in this CL the target "libjingle_peerconnection_java" was directly including .java files from another packages in its "java_files" attribute. Using srcjar_deps we are able to declare the dependency of the target avoiding to create hidden dependencies in the codebase. This is not fixing the webrtc:6356 bug directly but it is a first step to include ThreadUtils classes in libjingle_peerconnection_client_java.jar again. It seems also to be related to the chromium:648244 bug. This can be solved if we can find a way to perform srcjar generation in the android_library target without changing the semantic of the target. BUG=webrtc:6356 ==========
https://codereview.webrtc.org/2610823002/diff/20001/webrtc/generate_srcjar.py File webrtc/generate_srcjar.py (right): https://codereview.webrtc.org/2610823002/diff/20001/webrtc/generate_srcjar.py... webrtc/generate_srcjar.py:30: """Given a path to a .java source file it returns the path of the file On 2017/01/04 20:03:45, kjellander_webrtc wrote: > The first sentence in the docstring (the short description) must fit on one > line. > > So move out some into another sentence, which is separated by a blank line. Acknowledged.
On 2017/01/05 08:17:52, mbonadei wrote: > https://codereview.webrtc.org/2610823002/diff/20001/webrtc/generate_srcjar.py > File webrtc/generate_srcjar.py (right): > > https://codereview.webrtc.org/2610823002/diff/20001/webrtc/generate_srcjar.py... > webrtc/generate_srcjar.py:30: """Given a path to a .java source file it returns > the path of the file > On 2017/01/04 20:03:45, kjellander_webrtc wrote: > > The first sentence in the docstring (the short description) must fit on one > > line. > > > > So move out some into another sentence, which is separated by a blank line. > > Acknowledged. Actually, let's move the generate_srcjar.py script into webrtc/build.
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 Link to the patchset: https://codereview.webrtc.org/2610823002/#ps80001 (title: "Including .gni android configs only if is_android")
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": 80001, "attempt_start_ts": 1483613457475320,
"parent_rev": "2964ae3c974e55cf872aec5819463238617194bf", "commit_rev":
"10a76592a78762187bfaf3547b7e9986454062ab"}
Message was sent while issue was closed.
Description was changed from ========== Fixing package-boundary violation with srjar_deps Without the usage of the srcjar_deps attribute we were not able to include .java files from other packages without violating the package boundary contraint. As an example, in this CL the target "libjingle_peerconnection_java" was directly including .java files from another packages in its "java_files" attribute. Using srcjar_deps we are able to declare the dependency of the target avoiding to create hidden dependencies in the codebase. This is not fixing the webrtc:6356 bug directly but it is a first step to include ThreadUtils classes in libjingle_peerconnection_client_java.jar again. It seems also to be related to the chromium:648244 bug. This can be solved if we can find a way to perform srcjar generation in the android_library target without changing the semantic of the target. BUG=webrtc:6356 ========== to ========== Fixing package-boundary violation with srjar_deps Without the usage of the srcjar_deps attribute we were not able to include .java files from other packages without violating the package boundary contraint. As an example, in this CL the target "libjingle_peerconnection_java" was directly including .java files from another packages in its "java_files" attribute. Using srcjar_deps we are able to declare the dependency of the target avoiding to create hidden dependencies in the codebase. This is not fixing the webrtc:6356 bug directly but it is a first step to include ThreadUtils classes in libjingle_peerconnection_client_java.jar again. It seems also to be related to the chromium:648244 bug. This can be solved if we can find a way to perform srcjar generation in the android_library target without changing the semantic of the target. BUG=webrtc:6356 Review-Url: https://codereview.webrtc.org/2610823002 Cr-Commit-Position: refs/heads/master@{#15914} Committed: https://chromium.googlesource.com/external/webrtc/+/10a76592a78762187bfaf3547... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/10a76592a78762187bfaf3547...
Message was sent while issue was closed.
Description was changed from ========== Fixing package-boundary violation with srjar_deps Without the usage of the srcjar_deps attribute we were not able to include .java files from other packages without violating the package boundary contraint. As an example, in this CL the target "libjingle_peerconnection_java" was directly including .java files from another packages in its "java_files" attribute. Using srcjar_deps we are able to declare the dependency of the target avoiding to create hidden dependencies in the codebase. This is not fixing the webrtc:6356 bug directly but it is a first step to include ThreadUtils classes in libjingle_peerconnection_client_java.jar again. It seems also to be related to the chromium:648244 bug. This can be solved if we can find a way to perform srcjar generation in the android_library target without changing the semantic of the target. BUG=webrtc:6356 Review-Url: https://codereview.webrtc.org/2610823002 Cr-Commit-Position: refs/heads/master@{#15914} Committed: https://chromium.googlesource.com/external/webrtc/+/10a76592a78762187bfaf3547... ========== to ========== Fixing package-boundary violation with srjar_deps Without the usage of the srcjar_deps attribute we were not able to include .java files from other packages without violating the package boundary contraint. As an example, in this CL the target "libjingle_peerconnection_java" was directly including .java files from another packages in its "java_files" attribute. Using srcjar_deps we are able to declare the dependency of the target avoiding to create hidden dependencies in the codebase. This is not fixing the webrtc:6356 bug directly but it is a first step to include ThreadUtils classes in libjingle_peerconnection_client_java.jar again. It seems also to be related to the chromium:648244 bug. This can be solved if we can find a way to perform srcjar generation in the android_library target without changing the semantic of the target. BUG=webrtc:6356 Review-Url: https://codereview.webrtc.org/2610823002 Cr-Commit-Position: refs/heads/master@{#15914} Committed: https://chromium.googlesource.com/external/webrtc/+/10a76592a78762187bfaf3547... ==========
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.webrtc.org/2617533005/ by mbonadei@webrtc.org. The reason for reverting is: This CL is breaking a chromium.webrtc.fyi: https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder/b... I am trying to reproduce the issue on my local machine and I will try to re-land the CL later.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
