Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1997023002:
recipe engine: add remote_run command (Closed)
|
Created:
4 years, 7 months ago by Paweł Hajdan Jr. Modified:
4 years, 7 months ago CC:
chromium-reviews, infra-reviews+recipes-py_chromium.org Base URL:
https://github.com/luci/recipes-py.git@master Target Ref:
refs/heads/master Project:
recipes-py Visibility:
Public. |
Descriptionrecipe engine: add remote_run command
BUG=chromium:459840
Committed: https://github.com/luci/recipes-py/commit/68ad74bd9cb97cf110b5d6457ffac8c2861ad1b6
Patch Set 1 #
Total comments: 8
Patch Set 2 : fixes #Patch Set 3 : 80cols #
Total comments: 28
Patch Set 4 : fixes #Patch Set 5 : trybots #Patch Set 6 : review #Patch Set 7 : trybots #
Messages
Total messages: 46 (19 generated)
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997023002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997023002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
phajdan.jr@chromium.org changed reviewers: + iannucci@chromium.org, martiniss@chromium.org, nodir@chromium.org
I'm just moving in small steps here. Gitiles support would be next - added to fetch.py, and a subclass in package.py for recipes.cfg support. I'm not sure what's the best way to test the new code in this CL. Suggestions are welcome.
Design looks fine. For testing, I think two types of tests would be good: 1. Real unittests for recipe_engine/fetch.py, in recipe_engine/unittests/fetch_test.py, which uses mocking and gets full code coverage. 2. One integration test in unittests/<something>. Use the unittest/repo_test_util.py fake git repo, and do a local git clone (git lets you do that, I think, right?). https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py File recipe_engine/remote_run.py (right): https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py... recipe_engine/remote_run.py:24: cmd = [ My original design here had been to just do the fetch, and then return from here and proceed with the regular logic in recipes.py. So, there wouldn't be a subprocess call here. Not sure exactly if it's worth doing that, which might add some complication, and a bit of confusion to the command logic in recipes.py, but just mentioning it in case you hadn't thought of it.
iannucci@google.com changed reviewers: + iannucci@google.com
some comments, I defer to martiniss for final review, but lg as a first CL. https://codereview.chromium.org/1997023002/diff/1/recipe_engine/fetch.py File recipe_engine/fetch.py (right): https://codereview.chromium.org/1997023002/diff/1/recipe_engine/fetch.py#newc... recipe_engine/fetch.py:25: cmd = ['git.bat'] I think we need a full path here, right? Otherwise I think on windows it requires shell=True, which... bleh. Also, can we only use subprocess42 now, since that's now in third_party? It has a similar interface but spawns less threads and generally behaves better. edit: I see this was ported from elsewhere, not new code. My comment still stands, but should be done in a followup CL. https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py File recipe_engine/remote_run.py (right): https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py... recipe_engine/remote_run.py:26: os.path.join(checkout_dir, proto_file.read().recipes_path, 'recipes.py'), s/proto_file/recipes_cfg ? proto_file will get confusing if we ever use proto for anything else (which we probably will) https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py... recipe_engine/remote_run.py:44: shutil.rmtree(args.workdir) ignore errors?
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997023002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997023002/20001
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997023002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997023002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/1997023002/diff/1/recipe_engine/fetch.py File recipe_engine/fetch.py (right): https://codereview.chromium.org/1997023002/diff/1/recipe_engine/fetch.py#newc... recipe_engine/fetch.py:25: cmd = ['git.bat'] On 2016/05/20 at 18:04:22, iannucci1 wrote: > I think we need a full path here, right? Otherwise I think on windows it requires shell=True, which... bleh. This is existing code as you noticed, and point #2 is how should recipe engine know the "full path" to git? You mean explicitly resolve PATH env here (not sure what the advantages would be), or something else? In theory we could have e.g. explicit command line flag for that, just not sure how much is that worth it. > Also, can we only use subprocess42 now, since that's now in third_party? It has a similar interface but spawns less threads and generally behaves better. Good point. Switched now. https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py File recipe_engine/remote_run.py (right): https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py... recipe_engine/remote_run.py:24: cmd = [ On 2016/05/20 at 17:56:31, martiniss wrote: > My original design here had been to just do the fetch, and then return from here and proceed with the regular logic in recipes.py. So, there wouldn't be a subprocess call here. > > Not sure exactly if it's worth doing that, which might add some complication, and a bit of confusion to the command logic in recipes.py, but just mentioning it in case you hadn't thought of it. Given it's going to be the entry point replacing kitchen, I think this has to be inside this command. There's no "something else" to call the real run later. Does that make sense? :) https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py... recipe_engine/remote_run.py:26: os.path.join(checkout_dir, proto_file.read().recipes_path, 'recipes.py'), On 2016/05/20 at 18:04:22, iannucci1 wrote: > s/proto_file/recipes_cfg ? > > proto_file will get confusing if we ever use proto for anything else (which we probably will) Done. https://codereview.chromium.org/1997023002/diff/1/recipe_engine/remote_run.py... recipe_engine/remote_run.py:44: shutil.rmtree(args.workdir) On 2016/05/20 at 18:04:22, iannucci1 wrote: > ignore errors? Done.
lgtm https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/remote_ru... File recipe_engine/remote_run.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/remote_ru... recipe_engine/remote_run.py:24: cmd = [ So, under the alternative logic, you would return from real_main here, and not subprocess to run here. You would then go back to recipes.py, which would do the regular fetch and call to run. So no need for a subprocess call, although the control flow would be a bit strange. I'm fine with this though.
lgtm
Description was changed from ========== recipe engine: add remote_run command BUG=chromium:613026 ========== to ========== recipe engine: add remote_run command BUG=chromium:459840 ==========
https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py File recipe_engine/fetch.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. is this file about git? it seems so. Maybe rename to git.py? Especially if you agree to rename fetch_from_git to ensure_checkout https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:38: def _cleanup_pyc(path): why cleanup_pyc is in a file related to fetching? https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:40: This ensures we always use the fresh code. there should be a blank line between first line of docstirng and the rest https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:48: def fetch_from_git(repo, revision, checkout_dir, allow_fetch): it is strange to see "allow_fetch" parameter in a function that starts with "fetch_". It smells. I'd rename it to ensure_git_checkout. Note that the function was called "checkout" in the past, probably because it does not always fetch https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:50: Network operations are performed only if |allow_fetch| is True. here too https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:55: if allow_fetch: inverse condition and unindent the _run_git call https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:63: what if checkout dir exists but points to a different repo url? https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:68: if allow_fetch: same here https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:74: _cleanup_pyc(checkout_dir) pyc files have nothing to do with git. This function should be called by callers of fetch_from_git https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/remote_ru... File recipe_engine/remote_run.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/remote_ru... recipe_engine/remote_run.py:18: def real_main(args): a more elegant solution would be to use a context manager for creation and deletion of temp dir @contextlib.contextmanager def init_workdir(args): # make temp dir try: yield finally: # delete temp dir def main(args): with init_workdir(args): # code of real main https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/remote_ru... recipe_engine/remote_run.py:33: def main(args): document that args is result of parsing by remote_run parser defined in recipes.py https://codereview.chromium.org/1997023002/diff/40001/recipes.py File recipes.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipes.py#newcode306 recipes.py:306: remote_run_p.add_argument( since this is a long term solution, add --fetch-ref that defaults to "refs/heads/master"
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997023002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997023002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Recipes-py Presubmit on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Recipes-py%20Presubmit/...)
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997023002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997023002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py File recipe_engine/fetch.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/20 at 23:03:49, nodir wrote: > is this file about git? it seems so. Maybe rename to git.py? Especially if you agree to rename fetch_from_git to ensure_checkout This won't be necessarily exclusive to git. For example, we're already planning to add fetch from gitiles. It'll be a tarball, so nothing really git-specific, and only very loosely related (because of gitiles). https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:38: def _cleanup_pyc(path): On 2016/05/20 at 23:03:49, nodir wrote: > why cleanup_pyc is in a file related to fetching? Good point. Moved back. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:40: This ensures we always use the fresh code. On 2016/05/20 at 23:03:49, nodir wrote: > there should be a blank line between first line of docstirng and the rest Done. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:48: def fetch_from_git(repo, revision, checkout_dir, allow_fetch): On 2016/05/20 at 23:03:49, nodir wrote: > it is strange to see "allow_fetch" parameter in a function that starts with "fetch_". It smells. I'd rename it to ensure_git_checkout. Note that the function was called "checkout" in the past, probably because it does not always fetch Yeah, I was also thinking it's somewhat ugly but meh. Renamed to ensure_git_checkout. FWIW, as for the autoroller we'll need support for returning latest updates, we may well get objects here (e.g. Git and Gitiles) and this would likely be named checkout again. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:50: Network operations are performed only if |allow_fetch| is True. On 2016/05/20 at 23:03:49, nodir wrote: > here too Done. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:55: if allow_fetch: On 2016/05/20 at 23:03:49, nodir wrote: > inverse condition and unindent the _run_git call Done. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:63: On 2016/05/20 at 23:03:49, nodir wrote: > what if checkout dir exists but points to a different repo url? Good point. Out of scope for this CL (this is just existing code moved here), but added a TODO. FWIW, if it's different repo it's very unlikely that below rev-parse would succeed and point to different contents than expected. Of course it'd most likely fail, and fixing the TODO would mean recovering from that gracefully. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:68: if allow_fetch: On 2016/05/20 at 23:03:49, nodir wrote: > same here Done. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:74: _cleanup_pyc(checkout_dir) On 2016/05/20 at 23:03:49, nodir wrote: > pyc files have nothing to do with git. This function should be called by callers of fetch_from_git Yup, done.
some of my other comments were ignored https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py File recipe_engine/fetch.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/24 16:58:06, Paweł Hajdan Jr. wrote: > On 2016/05/20 at 23:03:49, nodir wrote: > > is this file about git? it seems so. Maybe rename to git.py? Especially if you > agree to rename fetch_from_git to ensure_checkout > > This won't be necessarily exclusive to git. For example, we're already planning > to add fetch from gitiles. It'll be a tarball, so nothing really git-specific, > and only very loosely related (because of gitiles). Acknowledged. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:63: On 2016/05/24 16:58:06, Paweł Hajdan Jr. wrote: > On 2016/05/20 at 23:03:49, nodir wrote: > > what if checkout dir exists but points to a different repo url? > > Good point. Out of scope for this CL (this is just existing code moved here), > but added a TODO. > > FWIW, if it's different repo it's very unlikely that below rev-parse would > succeed and point to different contents than expected. Of course it'd most > likely fail, and fixing the TODO would mean recovering from that gracefully. It is likely to succeed because default value of revision is FETCH_HEAD and git rev-parse --verify will verify that FETCH_HEAD points to an existing commit, which is very likely to be the case. As a result, this step will succeed, and a trooper will be very unhappy about this code.
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997023002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997023002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Recipes-py Presubmit on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Recipes-py%20Presubmit/...)
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997023002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997023002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
Thanks for the feedback. I indeed missed some comments before, sorry. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py File recipe_engine/fetch.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/fetch.py#... recipe_engine/fetch.py:63: On 2016/05/24 at 17:46:31, nodir wrote: > It is likely to succeed because default value of revision is FETCH_HEAD and git rev-parse --verify will verify that FETCH_HEAD points to an existing commit, which is very likely to be the case. As a result, this step will succeed, and a trooper will be very unhappy about this code. Yeah, fair enough. I added code to verify it in this CL + a test. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/remote_ru... File recipe_engine/remote_run.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/remote_ru... recipe_engine/remote_run.py:18: def real_main(args): On 2016/05/20 at 23:03:49, nodir wrote: > a more elegant solution would be to use a context manager for creation and deletion of temp dir > > @contextlib.contextmanager > def init_workdir(args): > # make temp dir > try: > yield > finally: > # delete temp dir > > def main(args): > with init_workdir(args): > # code of real main Good point, done. https://codereview.chromium.org/1997023002/diff/40001/recipe_engine/remote_ru... recipe_engine/remote_run.py:33: def main(args): On 2016/05/20 at 23:03:49, nodir wrote: > document that args is result of parsing by remote_run parser defined in recipes.py Whoa, does any other command do this? I'm not convinced a comment like that would be useful. https://codereview.chromium.org/1997023002/diff/40001/recipes.py File recipes.py (right): https://codereview.chromium.org/1997023002/diff/40001/recipes.py#newcode306 recipes.py:306: remote_run_p.add_argument( On 2016/05/20 at 23:03:50, nodir wrote: > since this is a long term solution, add --fetch-ref that defaults to "refs/heads/master" What would be semantics of that? How about we only add it when we need it and implement it? Also, it's not obvious to me how it'd apply to gitiles fetch. Overall, it's probably worthwhile to consider this but necessarily in scope of this CL. WDYT?
lgtm
The CQ bit was checked by phajdan.jr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org, martiniss@chromium.org Link to the patchset: https://codereview.chromium.org/1997023002/#ps120001 (title: "trybots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997023002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997023002/120001
Message was sent while issue was closed.
Description was changed from ========== recipe engine: add remote_run command BUG=chromium:459840 ========== to ========== recipe engine: add remote_run command BUG=chromium:459840 Committed: https://github.com/luci/recipes-py/commit/68ad74bd9cb97cf110b5d6457ffac8c2861... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://github.com/luci/recipes-py/commit/68ad74bd9cb97cf110b5d6457ffac8c2861... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
