Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342973002:
Teach bot_update to remove partially deleted git repos. (Closed)
|
Created:
4 years, 3 months ago by Vadim Sh. 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. |
DescriptionTeach bot_update to remove partially deleted git repos.
Also add logging to 'remove', helps to debug issues with 'build.dead' filling
up with crap.
R=iannucci@chromium.org, hinoka@chromium.org
BUG=647046
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/18ca30ca804679ee624a52e73017d234a8c0008f
Patch Set 1 #
Total comments: 8
Patch Set 2 : Teach bot_update to remove partially deleted git repos. #Messages
Total messages: 17 (8 generated)
See the bug for more info. tl;dr cleanup_disk did a bad job of removing "old" checkouts and deleted all files in ".git/" except ".git/objects/*". It confuses bot_update.py. https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:513: git('remote', 'set-url', 'origin', mirror_dir, cwd=sln_dir) This fails with ===Running git.bat remote set-url origin C:\\b\\c\\git_cache\chromium.googlesource.com-chromium-src (attempt #1)=== In directory: C:\b\c\b\winx64_high_dpi_perf_bisect\src fatal: Not a git repository (or any of the parent directories): .git ===Failed in 0.0 mins=== See the bug.
martiniss@chromium.org changed reviewers: + martiniss@chromium.org
lgtm
The CQ bit was checked by vadimsh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dnj@chromium.org changed reviewers: + dnj@chromium.org
lgtm w/ comments https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:301: print 'Removing %s => %s' % (target, dest) nit: I think this is a bit confusing, since this isn't actually removing things here, and removals don't normally include two paths. Maybe "Marking for removal: %s => %s" ? https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:463: return not path.exists(os.path.join(repo_dir, '.git', 'config')) This is a good check. WDYT about adding something like: ... or $("git -C <repo_dir> status").return_code != 0 This will actually let Git try and run in each directory, maybe detecting other types of corruption. https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:513: git('remote', 'set-url', 'origin', mirror_dir, cwd=sln_dir) On 2016/09/15 21:24:48, Vadim Sh. wrote: > This fails with > > ===Running git.bat remote set-url origin > C:\\b\\c\\git_cache\chromium.googlesource.com-chromium-src (attempt #1)=== > In directory: C:\b\c\b\winx64_high_dpi_perf_bisect\src > fatal: Not a git repository (or any of the parent directories): .git > ===Failed in 0.0 mins=== > > See the bug. Acknowledged.
The CQ bit was unchecked by vadimsh@chromium.org
tandrii@chromium.org changed reviewers: + tandrii@chromium.org
lgtm https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:463: return not path.exists(os.path.join(repo_dir, '.git', 'config')) On 2016/09/15 21:44:34, dnj wrote: > This is a good check. WDYT about adding something like: > > ... or $("git -C <repo_dir> status").return_code != 0 > > This will actually let Git try and run in each directory, maybe detecting other > types of corruption. +1, but could be separate CL.
https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:301: print 'Removing %s => %s' % (target, dest) On 2016/09/15 21:44:34, dnj wrote: > nit: I think this is a bit confusing, since this isn't actually removing things > here, and removals don't normally include two paths. > > Maybe "Marking for removal: %s => %s" ? Done. https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:463: return not path.exists(os.path.join(repo_dir, '.git', 'config')) On 2016/09/15 21:44:34, dnj wrote: > This is a good check. WDYT about adding something like: > > ... or $("git -C <repo_dir> status").return_code != 0 > > This will actually let Git try and run in each directory, maybe detecting other > types of corruption. I'm afraid running 'git', it can complicate life, e.g. it can discover parent '.git' (I've seen it doing that with some broken checkouts). And --git-dir is magic. I really don't like modifying this script beyond absolute minimum. Not unless I can reliably verify my modification work, without breaking all bots in the process. And I can't. So I'll be modifying it in absolute minimum way.
The CQ bit was checked by vadimsh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org, martiniss@chromium.org, tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/2342973002/#ps20001 (title: "Teach bot_update to remove partially deleted git repos.")
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 ========== Teach bot_update to remove partially deleted git repos. Also add logging to 'remove', helps to debug issues with 'build.dead' filling up with crap. R=iannucci@chromium.org, hinoka@chromium.org BUG=647046 ========== to ========== Teach bot_update to remove partially deleted git repos. Also add logging to 'remove', helps to debug issues with 'build.dead' filling up with crap. R=iannucci@chromium.org, hinoka@chromium.org BUG=647046 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/18ca30ca804679... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/18ca30ca804679...
Message was sent while issue was closed.
https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2342973002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:463: return not path.exists(os.path.join(repo_dir, '.git', 'config')) On 2016/09/15 21:56:26, Vadim Sh. wrote: > On 2016/09/15 21:44:34, dnj wrote: > > This is a good check. WDYT about adding something like: > > > > ... or $("git -C <repo_dir> status").return_code != 0 > > > > This will actually let Git try and run in each directory, maybe detecting > other > > types of corruption. > > I'm afraid running 'git', it can complicate life, e.g. it can discover parent > '.git' (I've seen it doing that with some broken checkouts). And --git-dir is > magic. I think we could ensure this doesn't happen with: git_dir = git( '--git-dir', os.path.join(repo_dir, '.git'), '--work-tree', repo_dir, 'status') > > I really don't like modifying this script beyond absolute minimum. Not unless I > can reliably verify my modification work, without breaking all bots in the > process. And I can't. So I'll be modifying it in absolute minimum way. Fair enough. It'd be nice if we had some way to actually canary this :/ |
