Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2653533004:
Android: Script for building libwebrtc.aar. (Closed)
|
Created:
3 years, 11 months ago by sakal Modified:
3 years, 11 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid: Script for building libwebrtc.aar.
BUG=webrtc:7023
Review-Url: https://codereview.webrtc.org/2653533004
Cr-Commit-Position: refs/heads/master@{#16232}
Committed: https://chromium.googlesource.com/external/webrtc/+/a4a753857e6df10abcadaf8276eeff9bce56d156
Patch Set 1 : Update AdroidManifest. #
Total comments: 36
Patch Set 2 : Do a static release build instead of dynamic debug build. #Patch Set 3 : Changes according to kjellander's comments. #
Total comments: 2
Patch Set 4 : Changes according to kjellander's comments. #2 #Patch Set 5 : Fix typo. #
Total comments: 2
Messages
Total messages: 25 (13 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Android: Script for building .aar-files. BUG=webrtc:7023 ========== to ========== Android: Script for building .aar-files. BUG=webrtc:7023 ==========
sakal@webrtc.org changed reviewers: + kjellander@webrtc.org
PTAL
A lot of presubmit warnings are generated but I don't think they are from my
code:
************* Module debug_pb2
C: 5, 0: Exactly one space required around assignment
_b=sys.version_info[0]<3 and (lambda x:x) or (lambda x:x.encode('latin1'))
^ (bad-whitespace)
C: 5, 0: Exactly one space required around comparison
_b=sys.version_info[0]<3 and (lambda x:x) or (lambda x:x.encode('latin1'))
^ (bad-whitespace)
C: 5, 0: Exactly one space required after :
_b=sys.version_info[0]<3 and (lambda x:x) or (lambda x:x.encode('latin1'))
^ (bad-whitespace)
https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... File webrtc/build/android/build_aar.py (right): https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:1: #!/usr/bin/python3 Prefer #!/usr/bin/env python Especially Python 3 is cannot assumed be installed. https://google.github.io/styleguide/pyguide.html?showone=Shebang_Line#Shebang... https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:11: # Run from root src folder: ./webrtc/build/android/build_aar.py Add a docstring that briefly explains what this script does. I think it would be useful to list an example of how the resulting directory hierarchy of the .aar file looks like. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:11: # Run from root src folder: ./webrtc/build/android/build_aar.py Is it a requirement to run this from the src/ folder? That's fine, but make it clear and make the script fail if not done so, if you want to limit it to that. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:22: 'libc++_shared.so', Is there a way around hardcoding these into the script? https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:34: TMP_OUTPUT = '/tmp/webrtc_android_aar_build/' Please use Python's tempfile module to create a unique temporary directory instead. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:36: parser = argparse.ArgumentParser(description='libwebrtc.aar generator.') Refactor line 36-44 into a _ParseArgs function that you call from main() https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:39: parser.add_argument('--arch', action='append', default=[], Can't you set default=DEFAULT_ARCHS here? https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:43: parser.add_argument('--debug', action='store_true', default=False, I prefer --verbose, but I won't force you. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:46: def RunGN(args): Use two blank lines before top-level statements/functions: https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:93: logging.info('Building: ' + arch) Use string formatters: logging.info('Building: %s', arch) https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:113: def CollectCommon(aar, arch): aar -> aar_file to make it more clear what this object is. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:125: jni_dir = 'jni' Remove this variable, it doesn't add anything useful. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:130: os.path.join(abi_dir, so_file)) + indentation to alight with the above parenthesis. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:134: if not args.arch: Add a parser.error message if len(args.arch) < 1. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:135: args.arch = DEFAULT_ARCHS Can't you set this at line 39 instead? https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:137: logging.basicConfig(level=logging.INFO if not args.debug else logging.DEBUG) level=logging.DEBUG if args.debug else logging.INFO is easier to read IMO. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:143: CollectCommon(aar, args.arch[0]) This makes me wonder what different it makes which is the first architecture in the list? If it can matter, it'd be better to pass some kind of primary arch and "additional archs" so it's obvious what happens.
On 2017/01/23 13:56:06, sakal wrote:
> PTAL
>
> A lot of presubmit warnings are generated but I don't think they are from my
> code:
> ************* Module debug_pb2
> C: 5, 0: Exactly one space required around assignment
> _b=sys.version_info[0]<3 and (lambda x:x) or (lambda x:x.encode('latin1'))
> ^ (bad-whitespace)
> C: 5, 0: Exactly one space required around comparison
> _b=sys.version_info[0]<3 and (lambda x:x) or (lambda x:x.encode('latin1'))
> ^ (bad-whitespace)
> C: 5, 0: Exactly one space required after :
> _b=sys.version_info[0]<3 and (lambda x:x) or (lambda x:x.encode('latin1'))
> ^ (bad-whitespace)
I'm also unable to find where these come from. Possibly the blacklist for
processing needs to be expanded:
https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#437
If you run 'git cl presubmit -vvv' locally, maybe you can get more information?
I also changed some methods to the internal naming convention. It is unclear to me how we should differentiate between internal and public methods for self-contained scripts like this. It seems "git cl presubmit" was going into one of my output folders. I removed the folder and there aren't any warnings now. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... File webrtc/build/android/build_aar.py (right): https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:1: #!/usr/bin/python3 On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Prefer #!/usr/bin/env python > Especially Python 3 is cannot assumed be installed. > https://google.github.io/styleguide/pyguide.html?showone=Shebang_Line#Shebang... Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:11: # Run from root src folder: ./webrtc/build/android/build_aar.py On 2017/01/23 14:39:30, kjellander_webrtc wrote: > Add a docstring that briefly explains what this script does. I think it would be > useful to list an example of how the resulting directory hierarchy of the .aar > file looks like. Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:11: # Run from root src folder: ./webrtc/build/android/build_aar.py On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Is it a requirement to run this from the src/ folder? That's fine, but make it > clear and make the script fail if not done so, if you want to limit it to that. Do you have an eloquent method to check the folder? The best I can come up with is checking the relative path to the script itself. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:22: 'libc++_shared.so', On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Is there a way around hardcoding these into the script? Actually, it seems we only need libjingle_peerconnection_so.so when not doing a debug build. Yes, these could be read from the build files but I think we should keep it simple here. This is not something that changes often and should be easy to manually update when needed. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:34: TMP_OUTPUT = '/tmp/webrtc_android_aar_build/' On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Please use Python's tempfile module to create a unique temporary directory > instead. Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:36: parser = argparse.ArgumentParser(description='libwebrtc.aar generator.') On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Refactor line 36-44 into a _ParseArgs function that you call from main() Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:39: parser.add_argument('--arch', action='append', default=[], On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Can't you set default=DEFAULT_ARCHS here? No, additional arch parameters would be just be appended to that list. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:43: parser.add_argument('--debug', action='store_true', default=False, On 2017/01/23 14:39:31, kjellander_webrtc wrote: > I prefer --verbose, but I won't force you. Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:46: def RunGN(args): On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Use two blank lines before top-level statements/functions: > https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:93: logging.info('Building: ' + arch) On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Use string formatters: > logging.info('Building: %s', arch) Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:113: def CollectCommon(aar, arch): On 2017/01/23 14:39:30, kjellander_webrtc wrote: > aar -> aar_file to make it more clear what this object is. Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:125: jni_dir = 'jni' On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Remove this variable, it doesn't add anything useful. Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:130: os.path.join(abi_dir, so_file)) On 2017/01/23 14:39:31, kjellander_webrtc wrote: > + indentation to alight with the above parenthesis. Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:134: if not args.arch: On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Add a parser.error message if len(args.arch) < 1. Wouldn't that be the case when using the default architectures. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:135: args.arch = DEFAULT_ARCHS On 2017/01/23 14:39:31, kjellander_webrtc wrote: > Can't you set this at line 39 instead? No, see above. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:137: logging.basicConfig(level=logging.INFO if not args.debug else logging.DEBUG) On 2017/01/23 14:39:30, kjellander_webrtc wrote: > level=logging.DEBUG if args.debug else logging.INFO > is easier to read IMO. Done. https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:143: CollectCommon(aar, args.arch[0]) On 2017/01/23 14:39:31, kjellander_webrtc wrote: > This makes me wonder what different it makes which is the first architecture in > the list? If it can matter, it'd be better to pass some kind of primary arch and > "additional archs" so it's obvious what happens. There is no difference. The common outputs are the same regardless of the architecture. I just arbitrarily choose to take them from the first one.
lgtm with comments addressed. Nice work, I'll be happy to wire up a bot running this. That bot should just perform a checkout then invoke the script, right? Nothing else should be needed except of course archiving the produced .aar file (on Google storage)? https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... File webrtc/build/android/build_aar.py (right): https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:134: if not args.arch: On 2017/01/23 15:29:03, sakal wrote: > On 2017/01/23 14:39:31, kjellander_webrtc wrote: > > Add a parser.error message if len(args.arch) < 1. > > Wouldn't that be the case when using the default architectures. Yeah, this assumed it was possible to use default=DEFAULT_ARCHS which turned out to not be true, so just ignore my comment here :) https://codereview.webrtc.org/2653533004/diff/20001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:143: CollectCommon(aar, args.arch[0]) On 2017/01/23 15:29:03, sakal wrote: > On 2017/01/23 14:39:31, kjellander_webrtc wrote: > > This makes me wonder what different it makes which is the first architecture > in > > the list? If it can matter, it'd be better to pass some kind of primary arch > and > > "additional archs" so it's obvious what happens. > > There is no difference. The common outputs are the same regardless of the > architecture. I just arbitrarily choose to take them from the first one. Please add a comment explaining that to make it obvious for future readers. https://codereview.webrtc.org/2653533004/diff/60001/webrtc/build/android/buil... File webrtc/build/android/build_aar.py (right): https://codereview.webrtc.org/2653533004/diff/60001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:35: NEEDED_SO_FILES = [ nit: put the list contents on one line
The CQ bit was checked by sakal@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/2653533004/#ps80001 (title: "Changes according to kjellander's comments. #2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by sakal@webrtc.org
Description was changed from ========== Android: Script for building .aar-files. BUG=webrtc:7023 ========== to ========== Android: Script for building libwebrtc.aar. BUG=webrtc:7023 ==========
The CQ bit was checked by sakal@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by sakal@webrtc.org
The CQ bit was checked by sakal@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/2653533004/#ps100001 (title: "Fix typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Yes, getting a bot running this would be nice. I think the only requirement is an Android version of the checkout. https://codereview.webrtc.org/2653533004/diff/60001/webrtc/build/android/buil... File webrtc/build/android/build_aar.py (right): https://codereview.webrtc.org/2653533004/diff/60001/webrtc/build/android/buil... webrtc/build/android/build_aar.py:35: NEEDED_SO_FILES = [ On 2017/01/24 06:47:00, kjellander_webrtc wrote: > nit: put the list contents on one line Done.
one last question (can be addressed in follow-up if you decide to do changes). https://codereview.webrtc.org/2653533004/diff/100001/webrtc/build/android/bui... File webrtc/build/android/build_aar.py (right): https://codereview.webrtc.org/2653533004/diff/100001/webrtc/build/android/bui... webrtc/build/android/build_aar.py:37: DEFAULT_ARCHS = ['armeabi-v7a', 'x86'] Should we not build more configs by default? Isn't ARM64 reasonably widespread now?
https://codereview.webrtc.org/2653533004/diff/100001/webrtc/build/android/bui... File webrtc/build/android/build_aar.py (right): https://codereview.webrtc.org/2653533004/diff/100001/webrtc/build/android/bui... webrtc/build/android/build_aar.py:37: DEFAULT_ARCHS = ['armeabi-v7a', 'x86'] On 2017/01/24 08:59:13, kjellander_webrtc wrote: > Should we not build more configs by default? Isn't ARM64 reasonably widespread > now? I believe arm64 processors are able to run armeabi-v7a code. I don't know if there are any real speed gains for using arm64-v8a code.
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485246008642380,
"parent_rev": "e04064deb2d7a49493bdeac0c87f7e0aa0fa141c", "commit_rev":
"a4a753857e6df10abcadaf8276eeff9bce56d156"}
Message was sent while issue was closed.
Description was changed from ========== Android: Script for building libwebrtc.aar. BUG=webrtc:7023 ========== to ========== Android: Script for building libwebrtc.aar. BUG=webrtc:7023 Review-Url: https://codereview.webrtc.org/2653533004 Cr-Commit-Position: refs/heads/master@{#16232} Committed: https://chromium.googlesource.com/external/webrtc/+/a4a753857e6df10abcadaf827... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/a4a753857e6df10abcadaf827... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
