Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12221058:
Allow trybot changes to run against WebKit@HEAD. (Closed)
|
Created:
7 years, 10 months ago by tony Modified:
7 years, 10 months ago CC:
chromium-reviews, Mike Stip (use stip instead), cmp, Isaac (away), kjellander+cc_chromium.org, iannucci, hinoka Visibility:
Public. |
DescriptionAllow trybot changes to run against WebKit@HEAD.
The buildbots already know how to run against specific WebKit
revisions, this is how the WebKit canary bot run tests.
Make it possible so that a try job's patch file can request to
use WebKit at ToT. This allows the bot to gclient sync to
ToT WebKit before applying the patch.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183566
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #Patch Set 4 : #Messages
Total messages: 21 (0 generated)
This shouldn't have any visible impact on the bots. When someone sends a patch where the first line says "third_party/WebKit@HEAD", we'll sync to head. I plan on changing my wk-try patch to modify the diff rather than messing with src/DEPS.
https://codereview.chromium.org/12221058/diff/1/scripts/master/chromium_step.py File scripts/master/chromium_step.py (right): https://codereview.chromium.org/12221058/diff/1/scripts/master/chromium_step.... scripts/master/chromium_step.py:112: wk_revision = 'HEAD' are you sure "HEAD" works here? don't the try servers still need svn revisions? In the gclient spec I use, I usually have to set 'webkit_rev' = '' to get the equivalent of HEAD. https://codereview.chromium.org/12221058/diff/1/scripts/master/factory/chromi... File scripts/master/factory/chromium_factory.py (right): https://codereview.chromium.org/12221058/diff/1/scripts/master/factory/chromi... scripts/master/factory/chromium_factory.py:879: self._InitWebkitLatestFactorySettings(factory_properties) Does this imply that *all* try jobs will try to pull against ToT? Are we sure we want that by default instead of something like LKGR or the version specified in DEPS?
https://codereview.chromium.org/12221058/diff/1/scripts/master/chromium_step.py File scripts/master/chromium_step.py (right): https://codereview.chromium.org/12221058/diff/1/scripts/master/chromium_step.... scripts/master/chromium_step.py:112: wk_revision = 'HEAD' On 2013/02/07 03:42:33, Dirk Pranke wrote: > are you sure "HEAD" works here? don't the try servers still need svn revisions? > In the gclient spec I use, I usually have to set 'webkit_rev' = '' to get the > equivalent of HEAD. I kind of agree I'd prefer a regexp to allow numbers and eventually git hash. HEAD in this context is very svn-specific.
On 2013/02/07 14:01:53, Marc-Antoine Ruel wrote: > https://codereview.chromium.org/12221058/diff/1/scripts/master/chromium_step.py > File scripts/master/chromium_step.py (right): > > https://codereview.chromium.org/12221058/diff/1/scripts/master/chromium_step.... > scripts/master/chromium_step.py:112: wk_revision = 'HEAD' > On 2013/02/07 03:42:33, Dirk Pranke wrote: > > are you sure "HEAD" works here? don't the try servers still need svn > revisions? > > In the gclient spec I use, I usually have to set 'webkit_rev' = '' to get the > > equivalent of HEAD. > > I kind of agree I'd prefer a regexp to allow numbers and eventually git hash. > HEAD in this context is very svn-specific. I was actually thinking that HEAD was git-specific and wouldn't work w/ svn. Looks like it's common to both, and so it's less of an issue for me.
New patch up. https://codereview.chromium.org/12221058/diff/1/scripts/master/chromium_step.py File scripts/master/chromium_step.py (right): https://codereview.chromium.org/12221058/diff/1/scripts/master/chromium_step.... scripts/master/chromium_step.py:112: wk_revision = 'HEAD' On 2013/02/07 14:01:53, Marc-Antoine Ruel wrote: > On 2013/02/07 03:42:33, Dirk Pranke wrote: > > are you sure "HEAD" works here? don't the try servers still need svn > revisions? > > In the gclient spec I use, I usually have to set 'webkit_rev' = '' to get the > > equivalent of HEAD. > > I kind of agree I'd prefer a regexp to allow numbers and eventually git hash. > HEAD in this context is very svn-specific. HEAD does work, but I've switched it to a regex to make it more flexible going forward. https://codereview.chromium.org/12221058/diff/1/scripts/master/factory/chromi... File scripts/master/factory/chromium_factory.py (right): https://codereview.chromium.org/12221058/diff/1/scripts/master/factory/chromi... scripts/master/factory/chromium_factory.py:879: self._InitWebkitLatestFactorySettings(factory_properties) On 2013/02/07 03:42:33, Dirk Pranke wrote: > Does this imply that *all* try jobs will try to pull against ToT? Are we sure we > want that by default instead of something like LKGR or the version specified in > DEPS? No, part of the changes in chromium_step.py is to remove the 'webkit_revision':'$$WK_REV$$', from gclient config if the patch doesn't have the magic first line. I tested this manually, but you can also try it by running git-try --host meatwad.sfo --port 8328 and looking at the waterfall at http://meatwad.sfo:8028/waterfall?show=linux_layout_rel (tests fail because it's precise).
lgtm
maruel, does this look OK to you?
The more I think about it, the more I'd prefer to only have trychange.py sends the diff against gclient's observed revision and the actual code. That is, independent of any local change to src/DEPS. Right now this whole thing is highly webkit specific and I can see how skia and others may want something similar and it'll reduce in a large amount of copy-pasted build properties. To clarify, I wonder if we are crossing the line to do a generic implementation or we keep a subproject specific implementation. I'm unsure, cc'ing more people to decide for me. https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... File scripts/master/chromium_step.py (left): https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... scripts/master/chromium_step.py:104: wk_revision = revision Why are you ditching revision? Won't this break canary slave? https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... scripts/master/chromium_step.py:111: nacl_revision = revision In particular, this doesn't get changed. https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... File scripts/master/chromium_step.py (right): https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... scripts/master/chromium_step.py:138: ',"webkit_revision":"$$WK_REV$$"', '') I'd remove the initial comma, since this assumes a specific ordering of the keys. https://codereview.chromium.org/12221058/diff/8001/scripts/master/factory/chr... File scripts/master/factory/chromium_factory.py (right): https://codereview.chromium.org/12221058/diff/8001/scripts/master/factory/chr... scripts/master/factory/chromium_factory.py:879: self._InitWebkitLatestFactorySettings(factory_properties) # This permits simpler webkit specific try jobs.
szager also proposed the idea of making a big diff against the DEPS version of WebKit and the current patch. This will fail to patch most of the time since there will often be layout test results (binary png files) in the diff. It will also unexpectedly pass if the user who created the patch is behind ToT and there are conflicts with the patch. I think we want the trybot to use actual ToT to see these conflicts. I agree that it is unfortunate that this is WebKit specific. I'm open to suggestions on how to make this more generic. https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... File scripts/master/chromium_step.py (left): https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... scripts/master/chromium_step.py:104: wk_revision = revision On 2013/02/08 19:36:40, Marc-Antoine Ruel wrote: > Why are you ditching revision? Won't this break canary slave? No, this value was never used. The code in the try block (line 108) would always set the wk_revision based on the property. Also, it doesn't make sense to set the webkit revision to the chromium revision. $$WK_REV$$ is currently only used by the testers in the builder/tester configuration on the webkit.chromium canary bots. https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... scripts/master/chromium_step.py:111: nacl_revision = revision On 2013/02/08 19:36:40, Marc-Antoine Ruel wrote: > In particular, this doesn't get changed. I suspect this is only used by some builder/tester case as well. https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... File scripts/master/chromium_step.py (right): https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... scripts/master/chromium_step.py:138: ',"webkit_revision":"$$WK_REV$$"', '') On 2013/02/08 19:36:40, Marc-Antoine Ruel wrote: > I'd remove the initial comma, since this assumes a specific ordering of the > keys. If we don't remove the comma, the python dictionary will have ,, which won't parse. I guess I could make a separate pass to collapse duplicate commas? https://codereview.chromium.org/12221058/diff/8001/scripts/master/factory/chr... File scripts/master/factory/chromium_factory.py (right): https://codereview.chromium.org/12221058/diff/8001/scripts/master/factory/chr... scripts/master/factory/chromium_factory.py:879: self._InitWebkitLatestFactorySettings(factory_properties) On 2013/02/08 19:36:40, Marc-Antoine Ruel wrote: > # This permits simpler webkit specific try jobs. Done.
On 2013/02/08 19:50:33, tony wrote: > szager also proposed the idea of making a big diff against the DEPS version of > WebKit and the current patch. This will fail to patch most of the time since > there will often be layout test results (binary png files) in the diff. > > It will also unexpectedly pass if the user who created the patch is behind ToT > and there are conflicts with the patch. I think we want the trybot to use > actual ToT to see these conflicts. > > I agree that it is unfortunate that this is WebKit specific. I'm open to > suggestions on how to make this more generic. I don't have much idea sadly. That's why I dragged this review a bit, to see if I could come up with something better but I don't have much ideas. lgtm https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... File scripts/master/chromium_step.py (right): https://codereview.chromium.org/12221058/diff/8001/scripts/master/chromium_st... scripts/master/chromium_step.py:138: ',"webkit_revision":"$$WK_REV$$"', '') On 2013/02/08 19:50:33, tony wrote: > On 2013/02/08 19:36:40, Marc-Antoine Ruel wrote: > > I'd remove the initial comma, since this assumes a specific ordering of the > > keys. > > If we don't remove the comma, the python dictionary will have ,, which won't > parse. I guess I could make a separate pass to collapse duplicate commas? Don't bother
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tony@chromium.org/12221058/4003
Message was sent while issue was closed.
Change committed as 181719
New version of this patch. It was reverted because I was wrong about the |revision| parameter in startVC(). It's the revision that triggered the change, so if a webkit change triggered the build (like on the webkit canary bots), it is the webkit revision. I removed the other code changes from before and only overwrite wk_revision if there's a patch with a revision in it (shouldn't impact the webkit canary bots at all).
I would like to see this locally tested with a webkit bot before being committed.
Here're my local tests: http://meatwad.sfo:8014/waterfall?show=WebKit%20Linux . The last run includes my changes.
On 2013/02/19 22:51:48, tony wrote: > Here're my local tests: http://meatwad.sfo:8014/waterfall?show=WebKit%2520Linux . > The last run includes my changes. Specifically, build 21 includes my changes and build 20 and build 19 do not.
Also note that it is an accident that the old code syncs to the correct revision :) If you look on the chromium.webkit waterfall, you'll see the update step with, "Please fix your script, having invalid --revision flags will soon considered an error." The code is trying to gclient sync to the webkit revision (!!) but happens to work because gclient throws out the malformed revision.
OK -- I am fine w/ this.
lgtm
Message was sent while issue was closed.
Committed patchset #4 manually as r183566 (presubmit successful). |
