Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2280213002:
Delete lots of svn logic from bot_update (Closed)
|
Created:
4 years, 3 months ago by agable Modified:
4 years, 3 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionDelete lots of svn logic from bot_update
R=hinoka@chromium.org
BUG=472386
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/082267a659f94bf7bca5cb144e203ca0913abb3a
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments #Patch Set 3 : Rebase #
Total comments: 12
Patch Set 4 : Rebase #Patch Set 5 : Respond to iannucci's comments #Messages
Total messages: 17 (5 generated)
lgtm https://codereview.chromium.org/2280213002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (left): https://codereview.chromium.org/2280213002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:92: BUILDSPEC_RE = (r'^/chrome-internal/trunk/tools/buildspec/' If this doesn't work / isn't invoked to begin with, just delete this, and all the other buildspec related logic. https://codereview.chromium.org/2280213002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2280213002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:1422: except Inactive: I don't think "Inactive" is referenced anymore after deleting L1527
https://codereview.chromium.org/2280213002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (left): https://codereview.chromium.org/2280213002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:92: BUILDSPEC_RE = (r'^/chrome-internal/trunk/tools/buildspec/' On 2016/08/26 at 21:59:54, hinoka wrote: > If this doesn't work / isn't invoked to begin with, just delete this, and all the other buildspec related logic. Deleted a bunch of buildspec code now. https://codereview.chromium.org/2280213002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2280213002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:1422: except Inactive: On 2016/08/26 at 21:59:54, hinoka wrote: > I don't think "Inactive" is referenced anymore after deleting L1527 Right, deleted.
Oh god the official continuous builders are still using the buildspec munging logic: https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/w... Trying to fix.
On 2016/08/29 at 19:43:21, agable wrote: > Oh god the official continuous builders are still using the buildspec munging logic: https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/w... > > Trying to fix. Ok, filed https://chromereviews.googleplex.com/497917013. That will need to land before this can.
hinoka@google.com changed reviewers: + hinoka@google.com
What's the status of this?
On 2016/08/31 at 20:47:11, hinoka wrote: > What's the status of this? Other CL landed, this one is waiting until after I've turned off the svn servers entirely (later today) to ensure I don't start two fires at once.
iannucci@chromium.org changed reviewers: + iannucci@chromium.org
lgtm https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:509: first_solution = True this variable is no longer used https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:518: print 'Warning: %s' % ('path %r not recognized' % parsed_path,) remove this entire condition... if we need it at all (we don't) this is the wrong place to put it anyway. https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:524: if deps_value and '$$' in deps_value: this is never used in recipes: https://cs.chromium.org/search/?q=%22$$%22+file:.*%5C.json&sq=package:chromiu... https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:556: build_dir = os.getcwd() can we move this os.getcwd() up and re-use it in the previous line too? https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:890: def get_commit_position(git_path, revision='HEAD'): TODO; use git-footers tool for this https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:1090: parse.add_option('--patch_url', help='DEPRECATED') I already removed this, but note that this is a resource of the bot_update module. You should have removed this and then removed the respective invocation from api.py (which is the sole user of this script).
The CQ bit was checked by agable@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@chromium.org, iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2280213002/#ps80001 (title: "Respond to iannucci's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Delete lots of svn logic from bot_update R=hinoka@chromium.org BUG=472386 ========== to ========== Delete lots of svn logic from bot_update R=hinoka@chromium.org BUG=472386 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/082267a659f94b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/082267a659f94b...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2320653003/ by bpastene@chromium.org. The reason for reverting is: Everything's purple :(.
Message was sent while issue was closed.
https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:509: first_solution = True On 2016/09/07 at 19:45:55, iannucci wrote: > this variable is no longer used Removed. https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:518: print 'Warning: %s' % ('path %r not recognized' % parsed_path,) On 2016/09/07 at 19:45:55, iannucci wrote: > remove this entire condition... if we need it at all (we don't) this is the wrong place to put it anyway. Removed. https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:524: if deps_value and '$$' in deps_value: On 2016/09/07 at 19:45:55, iannucci wrote: > this is never used in recipes: https://cs.chromium.org/search/?q=%22$$%22+file:.*%5C.json&sq=package:chromiu... Removed whole conditional stanza. https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:556: build_dir = os.getcwd() On 2016/09/07 at 19:45:55, iannucci wrote: > can we move this os.getcwd() up and re-use it in the previous line too? Done. https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:890: def get_commit_position(git_path, revision='HEAD'): On 2016/09/07 at 19:45:55, iannucci wrote: > TODO; use git-footers tool for this Added https://codereview.chromium.org/2280213002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:1090: parse.add_option('--patch_url', help='DEPRECATED') On 2016/09/07 at 19:45:55, iannucci wrote: > I already removed this, but note that this is a resource of the bot_update module. You should have removed this and then removed the respective invocation from api.py (which is the sole user of this script). Yeah good call. Rebase removed it. |
