Bug 28400 – [Chromium] Add compile-only canary-style Chromium slave to the waterfall

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dimitri Glazkov (Google)]

Needless to say, having a Chromium slave on build.webkit.org would be pretty useful to WebKit committers.

The problem is that we are still quite a ways away from being able a fully functional Chromium WebKit slave on build.webkit.org (see bug 28398 for details).

I wonder if we could compromise here by temporarily hooking up an existing pre-configured  Chromium slave (http://src.chromium.org/viewvc/chrome/trunk/tools/buildbot/) and modifying WebKit master to feed it Chromiumey commands?

Although not a replacement for the proper build slave, this solution allows us to gain immediate benefit of observing the breakage as it happens, right there on the waterfall, rather than having to look at http://tinyurl.com/webkit-canary or worse yet, letting Chromium people put out the fires.

What do you think? Patch coming up.

[Dimitri Glazkov (Google)]

Created attachment 34979 [details]
Add canary-style Chromium build slave, v1.

 .../build.webkit.org-config/config.json            |    9 ++++-
 .../build.webkit.org-config/master.cfg             |   37 +++++++++++++++++++-
 WebKitTools/ChangeLog                              |   10 +++++
 3 files changed, 54 insertions(+), 2 deletions(-)

[Mark Rowe (bdash)]

Comment on attachment 34979 [details]
Add canary-style Chromium build slave, v1.

Has this been tested?

 283         if platform == "chromium":
 284             self.addStep(UpdateChromiumSource)

I don't see how platform can ever be equal to "chromium" here, which would suggest that none of the Chromium-specific bits would ever be executed.

Where is the output from this build placed?  Does it end up below the WebKit source root?

I'm still not particularly fond of adding all this Chromium-specific gunk to what is otherwise a cross-platform setup.  What is the ETA for getting Chromium building in the same manner as every other WebKit port?

[Dimitri Glazkov (Google)]

Comment on attachment 34979 [details]
Add canary-style Chromium build slave, v1.

I apologize. I will set up a master and test this.

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> I'm still not particularly fond of adding all this Chromium-specific gunk to
> what is otherwise a cross-platform setup.  What is the ETA for getting Chromium
> building in the same manner as every other WebKit port?

I am trying to lead this effort to completion in the next few months. I don't have a specific date, but it is "as soon as possible" in my book. It's not a good situation for the Chromium folks, either.

[Dimitri Glazkov (Google)]

Created attachment 35064 [details]
Add canary-style Chromium build slave, v2.

 .../build.webkit.org-config/config.json            |   12 ++++-
 .../build.webkit.org-config/master.cfg             |   48 ++++++++++++++++++++
 WebKitTools/ChangeLog                              |   10 ++++
 3 files changed, 68 insertions(+), 2 deletions(-)

[Dimitri Glazkov (Google)]

Comment on attachment 35064 [details]
Add canary-style Chromium build slave, v2.

I tested this one :). Can you take another look?

[Dimitri Glazkov (Google)]

Comment on attachment 35064 [details]
Add canary-style Chromium build slave, v2.

Will fix issues discussed on #webkit, new patch coming up.

[Dimitri Glazkov (Google)]

Created attachment 35083 [details]
Add canary-style Chromium build slave, v3.

 .../build.webkit.org-config/config.json            |   12 ++++-
 .../build.webkit.org-config/master.cfg             |   51 ++++++++++++++++++++
 WebKitTools/ChangeLog                              |   10 ++++
 3 files changed, 71 insertions(+), 2 deletions(-)

[Dimitri Glazkov (Google)]

Comment on attachment 35083 [details]
Add canary-style Chromium build slave, v3.

Addressed #webkit comments:

* Changed builder to "ChromiumBuild"
* Escaped backslash
* Added revision-bound gclient sync

[Mark Rowe (bdash)]

Comment on attachment 35083 [details]
Add canary-style Chromium build slave, v3.

> diff --git a/WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg b/WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg
> index 4d92436..6ad9143 100644
> --- a/WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg
> +++ b/WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg
> @@ -47,6 +47,48 @@ class CheckOutSource(source.SVN):
>          source.SVN.__init__(self, baseURL=self.baseURL, defaultBranch="trunk", mode=self.mode, *args, **kwargs)
>  
>  
> +# FIXME: Remove this step once Chromium WebKit port build system is decoupled from
> +# Chromium (https://bugs.webkit.org/show_bug.cgi?id=28396)
> +class UpdateChromiumSource(shell.ShellCommand):
> +    command = ["gclient", "sync"]
> +    name = "update-chromium"
> +    description = ["updating chromium source"]
> +    descriptionDone = ["updated chromium source"]
> +    haltOnFailure = True

The descriptionDone should really include the SVN revision number so that it is clear from the waterfall page which revision was built.  The SVN status step uses a descriptionDone of the form ["updated", "rXXXX"] where XXXX is the value of got_revision (the revision that the "svn update" ended up pulling).

> +
> +    def start(self):
> +        os = self.getProperty("fullPlatform").split('-')[1]
> +        if os == "win":
> +            self.command[0] = "gclient.bat"
> +        revision = self.getProperty("revision")
> +        if revision:
> +            self.command.append("--revision=src/third_party/WebKit@" + revision)

We should be using ShellCommand.setCommand like is done elsewhere in the file rather than mutating self.command directly.  In theory the step should also be setting up got_revision as other source-fetching steps do.  It'll be needed for the descriptionDone property as mentioned above.

> +# FIXME: Remove this step once Chromium WebKit port build system is decoupled from
> +# Chromium (https://bugs.webkit.org/show_bug.cgi?id=28396)
> +class CompileChromiumWebKit(shell.ShellCommand):
> +    command = ["python", "../../../scripts/slave/compile.py"]
> +    name = "build-chromium"
> +    description = ["compiling"]
> +    descriptionDone = ["compiled"]
> +    haltOnFailure = True
> +
> +    def start(self):
> +        os = self.getProperty("fullPlatform").split('-')[1]
> +        command = self.command
> +        if not self.getProperty("revision"):
> +            # Buildbot will set revision to None when a build is forced. Use it
> +            # as a signal to clobber.
> +            command.append("--clobber")
> +        if os == "win":
> +            command.extend(["--solution=webkit.sln", "--build-dir=src\\webkit", "--", "/project", "webcore"])
> +        elif os == "mac":
> +            command.extend(["--solution=__solution__", "--build-dir=src/build", "--", "-project", "../webkit/webkit.xcodeproj", "-target", "webcore"])

Same comment as above about self.command.

[Dimitri Glazkov (Google)]

(In reply to comment #10)

> The descriptionDone should really include the SVN revision number so that it is
> clear from the waterfall page which revision was built.  The SVN status step
> uses a descriptionDone of the form ["updated", "rXXXX"] where XXXX is the value
> of got_revision (the revision that the "svn update" ended up pulling).

Let me see how I can figure this out. I don't know if I can, but will try.

> 
> > +
> > +    def start(self):
> > +        os = self.getProperty("fullPlatform").split('-')[1]
> > +        if os == "win":
> > +            self.command[0] = "gclient.bat"
> > +        revision = self.getProperty("revision")
> > +        if revision:
> > +            self.command.append("--revision=src/third_party/WebKit@" + revision)
> 
> We should be using ShellCommand.setCommand like is done elsewhere in the file
> rather than mutating self.command directly.  In theory the step should also be
> setting up got_revision as other source-fetching steps do.  It'll be needed for
> the descriptionDone property as mentioned above.
> 
> > +# FIXME: Remove this step once Chromium WebKit port build system is decoupled from
> > +# Chromium (https://bugs.webkit.org/show_bug.cgi?id=28396)
> > +class CompileChromiumWebKit(shell.ShellCommand):
> > +    command = ["python", "../../../scripts/slave/compile.py"]
> > +    name = "build-chromium"
> > +    description = ["compiling"]
> > +    descriptionDone = ["compiled"]
> > +    haltOnFailure = True
> > +
> > +    def start(self):
> > +        os = self.getProperty("fullPlatform").split('-')[1]
> > +        command = self.command
> > +        if not self.getProperty("revision"):
> > +            # Buildbot will set revision to None when a build is forced. Use it
> > +            # as a signal to clobber.
> > +            command.append("--clobber")
> > +        if os == "win":
> > +            command.extend(["--solution=webkit.sln", "--build-dir=src\\webkit", "--", "/project", "webcore"])
> > +        elif os == "mac":
> > +            command.extend(["--solution=__solution__", "--build-dir=src/build", "--", "-project", "../webkit/webkit.xcodeproj", "-target", "webcore"])
> 
> Same comment as above about self.command.

Thanks! Will fix.

[Dimitri Glazkov (Google)]

Comment on attachment 35083 [details]
Add canary-style Chromium build slave, v3.

New patch coming up ...

[Dimitri Glazkov (Google)]

Created attachment 35158 [details]
Add canary-style Chromium build slave, v4.

 .../build.webkit.org-config/config.json            |   12 +++-
 .../build.webkit.org-config/master.cfg             |   63 ++++++++++++++++++++
 WebKitTools/ChangeLog                              |   10 +++
 3 files changed, 83 insertions(+), 2 deletions(-)

[Dimitri Glazkov (Google)]

Comment on attachment 35158 [details]
Add canary-style Chromium build slave, v4.

Comments addressed. Took me a while to get this one right. I also have a variation that doesn't use the super-fugly regex, just startswith and line loop.

[Adam Barth]

From the blackout:

--- Comment #15 from Eric Seidel <[e-mail]>  2009-08-20 10:38:50 PDT ---
(From update of attachment 35158 [details])
I wonder if the master shouldn't just import some chromium module with these
hacks in it, instead of inlining them.  Anyway, looks sane enough to me, but
you need a review from bdash.

Does buildbot really uses non-standard python style?
 58     haltOnFailure = True

How about here?
 63         got_revision = "??" # This matches SVN unknown revision response

No way to avoid the ../../..?
 83     command = ["python", "../../../scripts/slave/compile.py"]

So all force builds should be clobber builds?  I thought buildbot had a clobber
option?  Or maybe that's just chromium's special buildbot.

This should be inside some sort of "build-chromium" script instead of in ever
thing that wants to call compile.py:
        if os == "win":
 97             command.extend(["--solution=webkit.sln",
"--build-dir=src\\webkit", "--", "/project", "webcore"])
 98         elif os == "mac":
 99             command.extend(["--solution=__solution__",
"--build-dir=src/build", "--", "-project", "../webkit/webkit.xcodeproj",
"-target", "webcore"])

I gues it's becasue we're trying to build only WebCore...?

Sad:
" ".join(architectures)

Again, looks OK to me.  bdash is your man.

[Dimitri Glazkov (Google)]

> Does buildbot really uses non-standard python style?
>  58     haltOnFailure = True

yup.
 
> How about here?
>  63         got_revision = "??" # This matches SVN unknown revision response

Well, the naming here is to match the name of the property. But I can rename that.

> No way to avoid the ../../..?
>  83     command = ["python", "../../../scripts/slave/compile.py"]

Not without more angst :)

> So all force builds should be clobber builds?  I thought buildbot had a clobber
> option?  Or maybe that's just chromium's special buildbot.

Yup. It's something nsylvain bolted on to build.chromium.org.
 
> This should be inside some sort of "build-chromium" script instead of in ever
> thing that wants to call compile.py:
>         if os == "win":
>  97             command.extend(["--solution=webkit.sln",
> "--build-dir=src\\webkit", "--", "/project", "webcore"])
>  98         elif os == "mac":
>  99             command.extend(["--solution=__solution__",
> "--build-dir=src/build", "--", "-project", "../webkit/webkit.xcodeproj",
> "-target", "webcore"])
> 
> I gues it's becasue we're trying to build only WebCore...?

Yes, and I really want to minimize the customness here. This will be going away in a matter of months.

> Sad:
> " ".join(architectures)
> 
> Again, looks OK to me.  bdash is your man.

[Dimitri Glazkov (Google)]

How are we doing here? :)

[Mark Rowe (bdash)]

Comment on attachment 35158 [details]
Add canary-style Chromium build slave, v4.

> diff --git a/WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg b/WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg
> index 4d92436..5af0698 100644
> --- a/WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg
> +++ b/WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg
> @@ -14,6 +14,7 @@ from buildbot.status.builder import SUCCESS, FAILURE, WARNINGS, SKIPPED
>  from twisted.internet import defer
>  
>  import simplejson
> +import re

This should be alphabetical.

> +    def createSummary(self, log):
> +        scraper = re.compile(r"^________ running '[^\n]+third_party[/\\]WebKit[^\n]+$\n(?:^[UA]\W+[^\n]+$\n)*^(?:Updated to|At) revision (\d+)", re.DOTALL | re.MULTILINE)
> +        revs = scraper.findall(log.getText())
> +        got_revision = "??" # This matches SVN unknown revision response

These two variable names don't follow our coding style.  "revs" should presumably be "revisions".  "got_revision" should be "gotRevision"..

> +        if len(revs):
> +            got_revision = ("r%s" % revs[-1])

The parens are unnecessary here.

> +    def start(self):
> +        os = self.getProperty("fullPlatform").split('-')[1]
> +        command = self.command[:]
> +        if not self.getProperty("revision"):
> +            # Buildbot will set revision to None when a build is forced. Use it
> +            # as a signal to clobber.
> +            command.append("--clobber")

This introduces a difference in behavior from the other configurations.  While it may make sense to add this behavior, we shouldn't be doing it in such a way that it is specific to a single configuration.

r=me

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/47787.

[Dimitri Glazkov (Google)]

Clobber option removal landed as http://trac.webkit.org/changeset/47789