Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1927403003:
Avoid computing patch_root in get_files_affected_by_patch. (Closed)
|
Created:
4 years, 7 months ago by tandrii(chromium) Modified:
4 years, 7 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionAvoid computing patch_root in get_files_affected_by_patch.
Context: See https://codereview.chromium.org/1934533002/#msg19 for context.
Allows: https://codereview.chromium.org/1933223002
Next - testing that it works for skia, upgrading old users to new method,
and finally killing old code.
R=machenbach@chromium.org,rmistry@chromium.org
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300395
Patch Set 1 #
Total comments: 2
Patch Set 2 : rework #Patch Set 3 : Real rework + downstream #
Total comments: 6
Patch Set 4 : review #
Messages
Total messages: 35 (15 generated)
The CQ bit was checked by tandrii@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/1927403003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927403003/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: + phajdan.jr@chromium.org
LGTM See https://codereview.chromium.org/1934533002/#msg19 for context.
lgtm
On 2016/04/29 14:08:44, Michael Achenbach wrote: > lgtm also consider moving to the git recipe module
The CQ bit was checked by tandrii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927403003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927403003/1
The CQ bit was unchecked by tandrii@chromium.org
Description was changed from ========== Copy get_files_affected_by_patch to gclient module. Next - testing that it works for skia, upgrading old users to new method, and finally killing old code. R=machenbach@chromium.org,rmistry@chromium.org BUG= ========== to ========== Copy get_files_affected_by_patch to gclient module. Context: See https://codereview.chromium.org/1934533002/#msg19 for context. Next - testing that it works for skia, upgrading old users to new method, and finally killing old code. R=machenbach@chromium.org,rmistry@chromium.org BUG= ==========
https://codereview.chromium.org/1927403003/diff/1/recipe_modules/gclient/api.py File recipe_modules/gclient/api.py (right): https://codereview.chromium.org/1927403003/diff/1/recipe_modules/gclient/api.... recipe_modules/gclient/api.py:382: patch_root = self.calculate_patch_root(patch_project, gclient_config) Maybe patch_root should be a parameter to this method instead of patch_project, gclient_config. Then the patch_root should be calculated elsewhere, using the method from gclient.
The CQ bit was checked by tandrii@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/1927403003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927403003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Copy get_files_affected_by_patch to gclient module. Context: See https://codereview.chromium.org/1934533002/#msg19 for context. Next - testing that it works for skia, upgrading old users to new method, and finally killing old code. R=machenbach@chromium.org,rmistry@chromium.org BUG= ========== to ========== Avoid computing patch_root in get_files_affected_by_patch. Context: See https://codereview.chromium.org/1934533002/#msg19 for context. Allows: https://codereview.chromium.org/1933223002 Next - testing that it works for skia, upgrading old users to new method, and finally killing old code. R=machenbach@chromium.org,rmistry@chromium.org BUG= ==========
The CQ bit was checked by tandrii@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/1927403003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927403003/40001
Now it's ready, still backwards compatible, but you can see proposed downstream changes that show that skia's situation is corrected: https://codereview.chromium.org/1933223002 https://codereview.chromium.org/1927403003/diff/1/recipe_modules/gclient/api.py File recipe_modules/gclient/api.py (right): https://codereview.chromium.org/1927403003/diff/1/recipe_modules/gclient/api.... recipe_modules/gclient/api.py:382: patch_root = self.calculate_patch_root(patch_project, gclient_config) On 2016/04/29 14:11:53, Michael Achenbach wrote: > Maybe patch_root should be a parameter to this method instead of patch_project, > gclient_config. Then the patch_root should be calculated elsewhere, using the > method from gclient. Good idea. Will do that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comments: https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/gclient/... File recipe_modules/gclient/__init__.py (right): https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/gclient/... recipe_modules/gclient/__init__.py:2: 'git', Why the gclient changes? Do we need to account for transitive deps? Why not put this into tryserver/__init__.py? https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/tryserve... File recipe_modules/tryserver/api.py (right): https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/tryserve... recipe_modules/tryserver/api.py:176: if self.m.platform.is_win: nit: Some tabs sneaked in? https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/tryserve... File recipe_modules/tryserver/example.py (right): https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/tryserve... recipe_modules/tryserver/example.py:53: api.properties(test_patch_root='sub/project')) Will the patch root also on windows be passed with slashes? They seem to stay in the expectations and should maybe become backslashes?
LGTM
The CQ bit was checked by tandrii@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/1927403003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927403003/60001
Thanks for review! https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/gclient/... File recipe_modules/gclient/__init__.py (right): https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/gclient/... recipe_modules/gclient/__init__.py:2: 'git', On 2016/05/02 06:44:32, Michael Achenbach wrote: > Why the gclient changes? Do we need to account for transitive deps? Why not put > this into tryserver/__init__.py? oversight - remnant from PS#1. https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/tryserve... File recipe_modules/tryserver/api.py (right): https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/tryserve... recipe_modules/tryserver/api.py:176: if self.m.platform.is_win: On 2016/05/02 06:44:32, Michael Achenbach wrote: > nit: Some tabs sneaked in? Done. https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/tryserve... File recipe_modules/tryserver/example.py (right): https://codereview.chromium.org/1927403003/diff/40001/recipe_modules/tryserve... recipe_modules/tryserver/example.py:53: api.properties(test_patch_root='sub/project')) On 2016/05/02 06:44:32, Michael Achenbach wrote: > Will the patch root also on windows be passed with slashes? They seem to stay in > the expectations and should maybe become backslashes? hm, good point! Done.
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1927403003/#ps60001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927403003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927403003/60001
Message was sent while issue was closed.
Description was changed from ========== Avoid computing patch_root in get_files_affected_by_patch. Context: See https://codereview.chromium.org/1934533002/#msg19 for context. Allows: https://codereview.chromium.org/1933223002 Next - testing that it works for skia, upgrading old users to new method, and finally killing old code. R=machenbach@chromium.org,rmistry@chromium.org BUG= ========== to ========== Avoid computing patch_root in get_files_affected_by_patch. Context: See https://codereview.chromium.org/1934533002/#msg19 for context. Allows: https://codereview.chromium.org/1933223002 Next - testing that it works for skia, upgrading old users to new method, and finally killing old code. R=machenbach@chromium.org,rmistry@chromium.org BUG= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300395 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300395
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
