Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1976243002:
Add support for WebRTC patches to Chromium trybots (Closed)
|
Created:
4 years, 7 months ago by kjellander_chromium Modified:
4 years, 7 months ago Reviewers:
tandrii(chromium) 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. |
DescriptionAdd support for WebRTC patches to Chromium trybots
Test case being added in https://codereview.chromium.org/1984623002/
NOTICE:
* Only patches from the webrtc/ subdirectory in a standalone
WebRTC checkout will work, since the root is set to src/third_party.
This can be changed once http://crbug.com/611808 is fixed.
* The Chromium checkout will be synced to the DEPS-pinned revision
of WebRTC. Changing this to HEAD will be addressed in a future CL.
BUG=438952
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300588
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changed patch root and added TODO #Patch Set 3 : Changed HEAD to None to make our special case work. #Messages
Total messages: 21 (8 generated)
kjellander@chromium.org changed reviewers: + tandrii@chromium.org
Hi Andrii (update your username if you're not OOO). I saw your PSA and wanted to get this longstanding request from our team going. Unfortunately I think we need to add a hack somewhere in order for the patches to apply, since src/third_party/webrtc doesn't mape to the root of our repo (so patches created in standalone WebRTC checkouts won't apply to src/third_party/webrtc). See https://bugs.chromium.org/p/chromium/issues/detail?id=611808 for more details. Can you help me out how to make such a hack, or do we have to wait for that bug to be resolved?
On 2016/05/13 18:11:46, kjellander (chromium) wrote: > Hi Andrii (update your username if you're not OOO). > I saw your PSA and wanted to get this longstanding request from our team going. > Unfortunately I think we need to add a hack somewhere in order for the patches > to apply, since src/third_party/webrtc doesn't mape to the root of our repo (so > patches created in standalone WebRTC checkouts won't apply to > src/third_party/webrtc). See > https://bugs.chromium.org/p/chromium/issues/detail?id=611808 for more details. > > Can you help me out how to make such a hack, or do we have to wait for that bug > to be resolved? I have no idea how to patch with different root. The apply_issue.py doesn't seem to have an option, and with Gerrit refs, it's way more complicated. So, I think we'd very much prefer to have 611808 fixed first.
On 2016/05/16 07:26:34, tandrii(chromium)-traveling wrote: > On 2016/05/13 18:11:46, kjellander (chromium) wrote: > > Hi Andrii (update your username if you're not OOO). > > I saw your PSA and wanted to get this longstanding request from our team > going. > > Unfortunately I think we need to add a hack somewhere in order for the patches > > to apply, since src/third_party/webrtc doesn't mape to the root of our repo > (so > > patches created in standalone WebRTC checkouts won't apply to > > src/third_party/webrtc). See > > https://bugs.chromium.org/p/chromium/issues/detail?id=611808 for more details. > > > > Can you help me out how to make such a hack, or do we have to wait for that > bug > > to be resolved? > > I have no idea how to patch with different root. The apply_issue.py doesn't seem > to have an option, and with Gerrit refs, it's way more complicated. > So, I think we'd very much prefer to have 611808 fixed first. to be clear: we can probably hack somewhere to indicate to apply_issue that actual root is not src/third_party/webrtc, but src/third_party. However, there are WebRTC patches that touch files other than webrtc/*, like say codereview.settings or https://chromium.googlesource.com/external/webrtc/+/master/tools/. Were you thinking of just educating webrtc devs to not send such patches to chromium trybots? Because apply_issue may actually succeed patching them, producing weird results.
On 2016/05/16 07:31:46, tandrii(chromium)-traveling wrote: > On 2016/05/16 07:26:34, tandrii(chromium)-traveling wrote: > > On 2016/05/13 18:11:46, kjellander (chromium) wrote: > > > Hi Andrii (update your username if you're not OOO). > > > I saw your PSA and wanted to get this longstanding request from our team > > going. > > > Unfortunately I think we need to add a hack somewhere in order for the > patches > > > to apply, since src/third_party/webrtc doesn't mape to the root of our repo > > (so > > > patches created in standalone WebRTC checkouts won't apply to > > > src/third_party/webrtc). See > > > https://bugs.chromium.org/p/chromium/issues/detail?id=611808 for more > details. > > > > > > Can you help me out how to make such a hack, or do we have to wait for that > > bug > > > to be resolved? > > > > I have no idea how to patch with different root. The apply_issue.py doesn't > seem > > to have an option, and with Gerrit refs, it's way more complicated. > > So, I think we'd very much prefer to have 611808 fixed first. > to be clear: we can probably hack somewhere to indicate to apply_issue that > actual root is not src/third_party/webrtc, but src/third_party. However, there > are WebRTC patches that touch files other than webrtc/*, like say > codereview.settings or > https://chromium.googlesource.com/external/webrtc/+/master/tools/. > Were you thinking of just educating webrtc devs to not send such patches to > chromium trybots? Because apply_issue may actually succeed patching them, > producing weird results. Oh, and now i actually read the bug https://bugs.chromium.org/p/chromium/issues/detail?id=438952 and it seems education is what you want. Let me think.
i think we can make it work - see comment. https://codereview.chromium.org/1976243002/diff/1/recipe_modules/gclient/conf... File recipe_modules/gclient/config.py (right): https://codereview.chromium.org/1976243002/diff/1/recipe_modules/gclient/conf... recipe_modules/gclient/config.py:175: p['webrtc'] = ('src/third_party/webrtc', 'HEAD') based on this code https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/recipe_... i think having here just 'src/third_party' might as well work. Make sure to add a new test based on v8 in example.py of gclient (this) module and see expectations.
Description was changed from ========== Add support for WebRTC patches to Chromium trybots BUG=438952 ========== to ========== Add support for WebRTC patches to Chromium trybots Test case being added in https://codereview.chromium.org/1984623002/ BUG=438952 ==========
On 2016/05/16 07:31:46, tandrii(chromium)-traveling wrote: > On 2016/05/16 07:26:34, tandrii(chromium)-traveling wrote: > > On 2016/05/13 18:11:46, kjellander (chromium) wrote: > > > Hi Andrii (update your username if you're not OOO). > > > I saw your PSA and wanted to get this longstanding request from our team > > going. > > > Unfortunately I think we need to add a hack somewhere in order for the > patches > > > to apply, since src/third_party/webrtc doesn't mape to the root of our repo > > (so > > > patches created in standalone WebRTC checkouts won't apply to > > > src/third_party/webrtc). See > > > https://bugs.chromium.org/p/chromium/issues/detail?id=611808 for more > details. > > > > > > Can you help me out how to make such a hack, or do we have to wait for that > > bug > > > to be resolved? > > > > I have no idea how to patch with different root. The apply_issue.py doesn't > seem > > to have an option, and with Gerrit refs, it's way more complicated. > > So, I think we'd very much prefer to have 611808 fixed first. > to be clear: we can probably hack somewhere to indicate to apply_issue that > actual root is not src/third_party/webrtc, but src/third_party. However, there > are WebRTC patches that touch files other than webrtc/*, like say > codereview.settings or > https://chromium.googlesource.com/external/webrtc/+/master/tools/. > Were you thinking of just educating webrtc devs to not send such patches to > chromium trybots? Because apply_issue may actually succeed patching them, > producing weird results. Yes, we're going to be super happy if this can work in any case. Even if hard-to-understand errors are produced when patches contains changes outside webrtc/ that's fine (patching will fail due to missing source files to patch so it should be pretty obvious what's wrong also)
Changed the root and created test case in https://codereview.chromium.org/1984623002/ https://codereview.chromium.org/1976243002/diff/1/recipe_modules/gclient/conf... File recipe_modules/gclient/config.py (right): https://codereview.chromium.org/1976243002/diff/1/recipe_modules/gclient/conf... recipe_modules/gclient/config.py:175: p['webrtc'] = ('src/third_party/webrtc', 'HEAD') On 2016/05/16 07:37:29, tandrii(chromium)-traveling wrote: > based on this code > https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/recipe_... > > i think having here just 'src/third_party' might as well work. Make sure to add > a new test based on v8 in example.py of gclient (this) module and see > expectations. Chatted offline. It seems you're right: I created a test case in https://codereview.chromium.org/1984623002/ using ./recipes.py -O depot_tools=~/dev/infra/depot_tools simulation_test train of a build checkout.
The CQ bit was checked by kjellander@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/1976243002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976243002/20001
lgtm
Description was changed from ========== Add support for WebRTC patches to Chromium trybots Test case being added in https://codereview.chromium.org/1984623002/ BUG=438952 ========== to ========== Add support for WebRTC patches to Chromium trybots Test case being added in https://codereview.chromium.org/1984623002/ NOTICE: Only patches from the webrtc/ subdirectory in a standalone WebRTC checkout will work, since the root is set to src/third_party. This can be changed once http://crbug.com/611808 is fixed. BUG=438952 ==========
The CQ bit was unchecked by tandrii@chromium.org
Description was changed from ========== Add support for WebRTC patches to Chromium trybots Test case being added in https://codereview.chromium.org/1984623002/ NOTICE: Only patches from the webrtc/ subdirectory in a standalone WebRTC checkout will work, since the root is set to src/third_party. This can be changed once http://crbug.com/611808 is fixed. BUG=438952 ========== to ========== Add support for WebRTC patches to Chromium trybots Test case being added in https://codereview.chromium.org/1984623002/ NOTICE: * Only patches from the webrtc/ subdirectory in a standalone WebRTC checkout will work, since the root is set to src/third_party. This can be changed once http://crbug.com/611808 is fixed. * The Chromium checkout will be synced to the DEPS-pinned revision of WebRTC. Changing this to HEAD will be addressed in a future CL. BUG=438952 ==========
New patch set.
The CQ bit was checked by tandrii@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976243002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976243002/40001
Message was sent while issue was closed.
Description was changed from ========== Add support for WebRTC patches to Chromium trybots Test case being added in https://codereview.chromium.org/1984623002/ NOTICE: * Only patches from the webrtc/ subdirectory in a standalone WebRTC checkout will work, since the root is set to src/third_party. This can be changed once http://crbug.com/611808 is fixed. * The Chromium checkout will be synced to the DEPS-pinned revision of WebRTC. Changing this to HEAD will be addressed in a future CL. BUG=438952 ========== to ========== Add support for WebRTC patches to Chromium trybots Test case being added in https://codereview.chromium.org/1984623002/ NOTICE: * Only patches from the webrtc/ subdirectory in a standalone WebRTC checkout will work, since the root is set to src/third_party. This can be changed once http://crbug.com/611808 is fixed. * The Chromium checkout will be synced to the DEPS-pinned revision of WebRTC. Changing this to HEAD will be addressed in a future CL. BUG=438952 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300588 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300588 |
