Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3008973002:
Add --build_dir arg to build_aar.py (Closed)
|
Created:
3 years, 3 months ago by korniltsev Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd --build_dir arg to build_aar.py
This will enable incremental build for aar
BUG=None
Review-Url: https://codereview.webrtc.org/3008973002
Cr-Commit-Position: refs/heads/master@{#19689}
Committed: https://chromium.googlesource.com/external/webrtc/+/0b510a994692df0137b39d38427e7c7311bdad27
Patch Set 1 #
Total comments: 6
Patch Set 2 : review fixes #
Total comments: 3
Patch Set 3 : review fixes #
Total comments: 1
Patch Set 4 : Rename tmp_dir to build_dir #Patch Set 5 : Rebase on master, rename tmp_dir to build_dir in GenerateLicense functino #Messages
Total messages: 27 (10 generated)
korniltsev.anatoly@gmail.com changed reviewers: + kjellander@webrtc.org
kjellander@webrtc.org changed reviewers: + sakal@webrtc.org
+sakal https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... tools_webrtc/android/build_aar.py:167: if args.build_dir == '': I prefer: if not args.build_dir: args.build_dir = tempfile.mkdtemp() and then use args.build_dir instead of the tmp_dir variable everywhere below.
https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... tools_webrtc/android/build_aar.py:49: parser.add_argument('--build_dir', default='', I don't think it is necessary to specify default here.
https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... tools_webrtc/android/build_aar.py:49: parser.add_argument('--build_dir', default='', Also, can we use build-dir for consistency wit the other variables?
https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... tools_webrtc/android/build_aar.py:49: parser.add_argument('--build_dir', default='', On 2017/08/31 08:07:49, sakal wrote: > I don't think it is necessary to specify default here. Done. https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... tools_webrtc/android/build_aar.py:49: parser.add_argument('--build_dir', default='', On 2017/08/31 08:09:00, sakal wrote: > Also, can we use build-dir for consistency wit the other variables? Done. https://codereview.webrtc.org/3008973002/diff/1/tools_webrtc/android/build_aa... tools_webrtc/android/build_aar.py:167: if args.build_dir == '': On 2017/08/31 08:03:06, kjellander_webrtc wrote: > I prefer: > > if not args.build_dir: > args.build_dir = tempfile.mkdtemp() > > and then use args.build_dir instead of the tmp_dir variable everywhere below. "if not" is better. Done. But I'd like keep args.build_dir unmodified because I need original value down the script
https://codereview.webrtc.org/3008973002/diff/20001/tools_webrtc/android/buil... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3008973002/diff/20001/tools_webrtc/android/buil... tools_webrtc/android/build_aar.py:170: tmp_dir = args.build_dir nit: I would to call this build_dir because it is not always a temporary directory. You could also write: build_dir = args.build_dir if args.build_dir else tempfile.mkdtemp()
https://codereview.webrtc.org/3008973002/diff/20001/tools_webrtc/android/buil... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3008973002/diff/20001/tools_webrtc/android/buil... tools_webrtc/android/build_aar.py:170: tmp_dir = args.build_dir On 2017/08/31 08:42:07, sakal wrote: > nit: I would to call this build_dir because it is not always a temporary > directory. You could also write: > > build_dir = args.build_dir if args.build_dir else tempfile.mkdtemp() +1 to that.
Done. Should I rename tmp_dir to build_dir in all other fuctions? https://codereview.webrtc.org/3008973002/diff/20001/tools_webrtc/android/buil... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3008973002/diff/20001/tools_webrtc/android/buil... tools_webrtc/android/build_aar.py:170: tmp_dir = args.build_dir On 2017/08/31 08:42:07, sakal wrote: > nit: I would to call this build_dir because it is not always a temporary > directory. You could also write: > > build_dir = args.build_dir if args.build_dir else tempfile.mkdtemp() Done.
lgtm with nits fixed https://codereview.webrtc.org/3008973002/diff/40001/tools_webrtc/android/buil... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3008973002/diff/40001/tools_webrtc/android/buil... tools_webrtc/android/build_aar.py:88: def _GetOutputDirectory(tmp_dir, arch): nit: I think we should rename rest of the tmp_dir references as well.
lgtm I realize I never setup a trybot for this archiving, so you'll have to monitor https://build.chromium.org/p/client.webrtc.fyi/builders/Android%20Archive after submitting this to make sure it doesn't break. Would that be OK?
On 2017/08/31 11:06:13, kjellander_webrtc wrote: > lgtm > > I realize I never setup a trybot for this archiving, so you'll have to monitor > https://build.chromium.org/p/client.webrtc.fyi/builders/Android%20Archive after > submitting this to make sure it doesn't break. Would that be OK? Ok
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/23643) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/28955) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/27977) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
I just realized my recent change conflicts with this CL. Can you rebase please and I can land this for your.
Rebased on master.
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/3008973002/#ps80001 (title: "Rebase on master, rename tmp_dir to build_dir in GenerateLicense functino")
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": 1504622773611870,
"parent_rev": "9c5e511806ef64c4cede52722c8a7ae7ae50ff56", "commit_rev":
"0b510a994692df0137b39d38427e7c7311bdad27"}
Message was sent while issue was closed.
Description was changed from ========== Add --build_dir arg to build_aar.py This will enable incremental build for aar BUG=None ========== to ========== Add --build_dir arg to build_aar.py This will enable incremental build for aar BUG=None Review-Url: https://codereview.webrtc.org/3008973002 Cr-Commit-Position: refs/heads/master@{#19689} Committed: https://chromium.googlesource.com/external/webrtc/+/0b510a994692df0137b39d384... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/0b510a994692df0137b39d384... |
