Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670743004:
Add support for extra GN args to Android build script. (Closed)
|
Created:
3 years, 10 months ago by kjellander_webrtc Modified:
3 years, 10 months ago Reviewers:
sakal CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd support for extra GN args to Android build script.
By using the --extra-gn-args flag, it is now possible to
specify additional GN arguments for the build. This is needed
in order to pass a non-default Goma directory (needed for the bots).
Example use: --extra-gn-args goma_dir=\"/path/to/goma\"
You can also pass multiple args (separated by spaces).
BUG=chromium:684387
NOTRY=True
TESTED=Did a local successful run.
Review-Url: https://codereview.webrtc.org/2670743004
Cr-Commit-Position: refs/heads/master@{#16458}
Committed: https://chromium.googlesource.com/external/webrtc/+/6b3fcfd82318aea0cd4ce1705f510ece0057491a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Updated after review #
Total comments: 3
Patch Set 3 : Fixed argparse oddness #Messages
Total messages: 19 (10 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add support for extra GN args to Android build script. By using the --extra-gn-args flag, it is now possible to specify additional GN arguments for the build. This is needed in order to pass a non-default Goma directory (needed for the bots). BUG=chromium:684387 ========== to ========== Add support for extra GN args to Android build script. By using the --extra-gn-args flag, it is now possible to specify additional GN arguments for the build. This is needed in order to pass a non-default Goma directory (needed for the bots). Example use: --extra-gn-args=goma_dir=\"/path/to/goma\" BUG=chromium:684387 ==========
kjellander@webrtc.org changed reviewers: + sakal@webrtc.org
Description was changed from ========== Add support for extra GN args to Android build script. By using the --extra-gn-args flag, it is now possible to specify additional GN arguments for the build. This is needed in order to pass a non-default Goma directory (needed for the bots). Example use: --extra-gn-args=goma_dir=\"/path/to/goma\" BUG=chromium:684387 ========== to ========== Add support for extra GN args to Android build script. By using the --extra-gn-args flag, it is now possible to specify additional GN arguments for the build. This is needed in order to pass a non-default Goma directory (needed for the bots). Example use: --extra-gn-args=goma_dir=\"/path/to/goma\" BUG=chromium:684387 NOTRY=True TESTED=Did a local successful run. ==========
https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... File tools-webrtc/android/build_aar.py (right): https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:57: parser.add_argument('--extra-gn-args', action='append', default=[], I would prefer this be a regular string parameter. You can pass multiple parameters: --extra-gn-args='is_debug=false is_component_build=true' https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:128: k + '=' + _EncodeForGN(v) for k, v in gn_args.items()] + extra_gn_args) I would prefer a system where you can override existing args but if that is too much work this is ok.
https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... File tools-webrtc/android/build_aar.py (right): https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:57: parser.add_argument('--extra-gn-args', action='append', default=[], On 2017/02/06 09:24:11, sakal wrote: > I would prefer this be a regular string parameter. You can pass multiple > parameters: > --extra-gn-args='is_debug=false is_component_build=true' Since I have action='append' it is possible to pass multiple arguments by specifying the same flag multiple times, like --extra-gn-args=foo=\"bar\" --extra-gn-args='baz=\"wee\" I think that's better than string. I tried passing two flags as a string and it actually also works: --extra-gn-args='goma_dir="/usr/local/google/home/kjellander/goma" foo="bar"' See https://docs.python.org/2.7/library/argparse.html#action for more info. https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:128: k + '=' + _EncodeForGN(v) for k, v in gn_args.items()] + extra_gn_args) On 2017/02/06 09:24:11, sakal wrote: > I would prefer a system where you can override existing args but if that is too > much work this is ok. argparse doesn't support dicts, so I think it's a little too much work, unfortunately. https://codereview.webrtc.org/2670743004/diff/40001/tools-webrtc/android/buil... File tools-webrtc/android/build_aar.py (right): https://codereview.webrtc.org/2670743004/diff/40001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:51: parser.add_argument('--arch', action='append', default=DEFAULT_ARCHS, I fixed this for you.
https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... File tools-webrtc/android/build_aar.py (right): https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:57: parser.add_argument('--extra-gn-args', action='append', default=[], On 2017/02/06 09:24:11, sakal wrote: > I would prefer this be a regular string parameter. You can pass multiple > parameters: > --extra-gn-args='is_debug=false is_component_build=true' Since I have action='append' it is possible to pass multiple arguments by specifying the same flag multiple times, like --extra-gn-args=foo=\"bar\" --extra-gn-args='baz=\"wee\" I think that's better than string. I tried passing two flags as a string and it actually also works: --extra-gn-args='goma_dir="/usr/local/google/home/kjellander/goma" foo="bar"' See https://docs.python.org/2.7/library/argparse.html#action for more info. https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:128: k + '=' + _EncodeForGN(v) for k, v in gn_args.items()] + extra_gn_args) On 2017/02/06 09:24:11, sakal wrote: > I would prefer a system where you can override existing args but if that is too > much work this is ok. argparse doesn't support dicts, so I think it's a little too much work, unfortunately. https://codereview.webrtc.org/2670743004/diff/40001/tools-webrtc/android/buil... File tools-webrtc/android/build_aar.py (right): https://codereview.webrtc.org/2670743004/diff/40001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:51: parser.add_argument('--arch', action='append', default=DEFAULT_ARCHS, I fixed this for you.
lgtm if you verify your "--arch" fix actually works. https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... File tools-webrtc/android/build_aar.py (right): https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:57: parser.add_argument('--extra-gn-args', action='append', default=[], On 2017/02/06 14:44:12, kjellander_webrtc wrote: > On 2017/02/06 09:24:11, sakal wrote: > > I would prefer this be a regular string parameter. You can pass multiple > > parameters: > > --extra-gn-args='is_debug=false is_component_build=true' > > Since I have action='append' it is possible to pass multiple arguments by > specifying the same flag multiple times, like --extra-gn-args=foo=\"bar\" > --extra-gn-args='baz=\"wee\" > I think that's better than string. > I tried passing two flags as a string and it actually also works: > --extra-gn-args='goma_dir="/usr/local/google/home/kjellander/goma" foo="bar"' > > See https://docs.python.org/2.7/library/argparse.html#action for more info. I find passing arguments one by one a little cumbersome but if you prefer it ok. https://codereview.webrtc.org/2670743004/diff/20001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:128: k + '=' + _EncodeForGN(v) for k, v in gn_args.items()] + extra_gn_args) On 2017/02/06 14:44:12, kjellander_webrtc wrote: > On 2017/02/06 09:24:11, sakal wrote: > > I would prefer a system where you can override existing args but if that is > too > > much work this is ok. > > argparse doesn't support dicts, so I think it's a little too much work, > unfortunately. Yeah, you would have to manually parse the list into a dictionary. Let's keep as is then. https://codereview.webrtc.org/2670743004/diff/40001/tools-webrtc/android/buil... File tools-webrtc/android/build_aar.py (right): https://codereview.webrtc.org/2670743004/diff/40001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:51: parser.add_argument('--arch', action='append', default=DEFAULT_ARCHS, On 2017/02/06 14:44:12, kjellander_webrtc wrote: > I fixed this for you. Nice if this works but I think I tested this already. Can you try passing in "--arch" argument. In my testing it was appended to the default values.
Improved with nargs instead. PTAL https://codereview.webrtc.org/2670743004/diff/40001/tools-webrtc/android/buil... File tools-webrtc/android/build_aar.py (right): https://codereview.webrtc.org/2670743004/diff/40001/tools-webrtc/android/buil... tools-webrtc/android/build_aar.py:51: parser.add_argument('--arch', action='append', default=DEFAULT_ARCHS, On 2017/02/06 14:52:57, sakal wrote: > On 2017/02/06 14:44:12, kjellander_webrtc wrote: > > I fixed this for you. > > Nice if this works but I think I tested this already. Can you try passing in > "--arch" argument. In my testing it was appended to the default values. You're right (I didn't test it properly). action needs to be set to the default ('store') and use the nargs attribute as well. Having append here will only append to the default value, which is not what we want! I tested PS#3 by passing --arch x86 then by passing --arch x86 armv7a then by not passing it at all. All worked as expected.
Cool, this looks better, lgtm.
The CQ bit was checked by kjellander@webrtc.org
The CQ bit was unchecked by kjellander@webrtc.org
Description was changed from ========== Add support for extra GN args to Android build script. By using the --extra-gn-args flag, it is now possible to specify additional GN arguments for the build. This is needed in order to pass a non-default Goma directory (needed for the bots). Example use: --extra-gn-args=goma_dir=\"/path/to/goma\" BUG=chromium:684387 NOTRY=True TESTED=Did a local successful run. ========== to ========== Add support for extra GN args to Android build script. By using the --extra-gn-args flag, it is now possible to specify additional GN arguments for the build. This is needed in order to pass a non-default Goma directory (needed for the bots). Example use: --extra-gn-args goma_dir=\"/path/to/goma\" You can also pass multiple args (separated by spaces). BUG=chromium:684387 NOTRY=True TESTED=Did a local successful run. ==========
The CQ bit was checked by kjellander@webrtc.org
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": 1486458494839400,
"parent_rev": "6b34124a6d4733cf2d95986b4920bccec8b39397", "commit_rev":
"6b3fcfd82318aea0cd4ce1705f510ece0057491a"}
Message was sent while issue was closed.
Description was changed from ========== Add support for extra GN args to Android build script. By using the --extra-gn-args flag, it is now possible to specify additional GN arguments for the build. This is needed in order to pass a non-default Goma directory (needed for the bots). Example use: --extra-gn-args goma_dir=\"/path/to/goma\" You can also pass multiple args (separated by spaces). BUG=chromium:684387 NOTRY=True TESTED=Did a local successful run. ========== to ========== Add support for extra GN args to Android build script. By using the --extra-gn-args flag, it is now possible to specify additional GN arguments for the build. This is needed in order to pass a non-default Goma directory (needed for the bots). Example use: --extra-gn-args goma_dir=\"/path/to/goma\" You can also pass multiple args (separated by spaces). BUG=chromium:684387 NOTRY=True TESTED=Did a local successful run. Review-Url: https://codereview.webrtc.org/2670743004 Cr-Commit-Position: refs/heads/master@{#16458} Committed: https://chromium.googlesource.com/external/webrtc/+/6b3fcfd82318aea0cd4ce1705... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/6b3fcfd82318aea0cd4ce1705... |
