Bug 31442 – Chromium Build Slaves

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

[Yaar Schnitman]

Created attachment 43106 [details]
Patch

These 3 new build slaves obsolete the existing chromium slave, and build the webkit chromium port.

[Yaar Schnitman]

Created attachment 43109 [details]
buildbot configuration changes

[Dimitri Glazkov (Google)]

Comment on attachment 43109 [details]
buildbot configuration changes

>  
> +class InstallChromiumDependencies(shell.ShellCommand):
> +    name = "gclient"
> +    description = ["updating chromium dependencies"]
> +    descriptionDone = ["updated chromium dependencies"]
> +    command = ["perl", "./WebKitTools/Scripts/update-webkit-chromium"]

this should be ["perl", "./WebKitTools/Scripts/update-webkit", "--chromium"]

right?

[Mark Rowe (bdash)]

Comment on attachment 43109 [details]
buildbot configuration changes

> diff --git a/WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json b/WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json
> index 2b8faf6..04e7ad9 100644
> --- a/WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json
> +++ b/WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json
> @@ -25,7 +25,9 @@
>  
>                      { "name": "szeged-linux-1", "platform": "qt"},
>  
> -                    { "name": "google-slave-1", "platform": "chromium-win" }
> +                    { "name": "chromium-win-slave-1", "platform": "chromium" },
> +                    { "name": "chromium-mac-slave-1", "platform": "chromium" },
> +                    { "name": "chromium-linux-slave-1", "platform": "chromium” }

Per my recent email these aren’t the slave names that will be used.


>                    ],
>  
>      "builders":   [ { "name": "Tiger Intel Release", "type": "BuildAndTest", "builddir": "tiger-intel-release",
> @@ -98,9 +100,22 @@
>                        "slavenames": ["szeged-linux-1"]
>                      },
>                      {
> -                      "name": "Chromium Win Release", "type": "ChromiumBuild", "builddir": "chromium-win-release",
> -                      "platform": "chromium-win", "configuration": "release", "architectures": ["i386"],
> -                      "slavenames": ["google-slave-1"]
> +                      "name": "Chromium Win Release", "type": "Build", "builddir": "chromium-win-release",
> +                      "platform": "chromium", "configuration": "release", "architectures": ["i386"],
> +                      "triggers": ["chromium-win-release"],
> +                      "slavenames": ["chromium-win-slave-1"]
> +                    },
> +                    {
> +                      "name": "Chromium Mac Release", "type": "Build", "builddir": "chromium-mac-release",
> +                      "platform": "chromium", "configuration": "release", "architectures": ["i386"],
> +                      "triggers": ["chromium-mac-release"],
> +                      "slavenames": ["chromium-mac-slave-1"]
> +                    },
> +                    {
> +                      "name": "Chromium Linux Release", "type": "Build", "builddir": "chromium-linux-release",
> +                      "platform": "chromium", "configuration": "release", "architectures": ["i386"],
> +                      "triggers": ["chromium-linux-release"],
> +                      "slavenames": ["chromium-linux-slave-1"]
>                      }
>                    ],

The “triggers” values here seem bogus given that they don’t attempt to trigger anything and there’s no corresponding triggerable configuration.

> +class InstallChromiumDependencies(shell.ShellCommand):
> +    name = "gclient"
> +    description = ["updating chromium dependencies"]
> +    descriptionDone = ["updated chromium dependencies"]
> +    command = ["perl", "./WebKitTools/Scripts/update-webkit-chromium"]
> +    haltOnFailure = True

This script doesn’t exist.  Is that the right command to run?

>  def appendCustomBuildFlags(step, platform):
>      if platform in ('gtk', 'wx', 'qt'):
>          step.setCommand(step.command + ['--' + platform])
> @@ -119,6 +78,8 @@ class CompileWebKit(shell.Compile):
>      def start(self):
>          platform = self.getProperty('platform')
>          buildOnly = self.getProperty('buildOnly')
> +        if platform == 'chromium':
> +            self.setCommand(self.command + ['--chromium'])
>          if platform == 'mac' and buildOnly:
>              self.setCommand(self.command + ['DEBUG_INFORMATION_FORMAT=dwarf-with-dsym’])

This should be handled in the same way it is for other platforms: in appendCustomBuildFlags.

[Yaar Schnitman]

Created attachment 43115 [details]
buildbot configuration changes

[Yaar Schnitman]

Thanks for reviewing this. New patch is attached. See comments below:

> (From update of attachment 43109 [details])
> > diff --git a/WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json b/WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json
> > index 2b8faf6..04e7ad9 100644
> > --- a/WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json
> > +++ b/WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json
> > @@ -25,7 +25,9 @@
> >  
> >                      { "name": "szeged-linux-1", "platform": "qt"},
> >  
> > -                    { "name": "google-slave-1", "platform": "chromium-win" }
> > +                    { "name": "chromium-win-slave-1", "platform": "chromium" },
> > +                    { "name": "chromium-mac-slave-1", "platform": "chromium" },
> > +                    { "name": "chromium-linux-slave-1", "platform": "chromium” }
> 
> Per my recent email these aren’t the slave names that will be used.
>
Fixed. 
> 
> >                    ],
> >  
> >      "builders":   [ { "name": "Tiger Intel Release", "type": "BuildAndTest", "builddir": "tiger-intel-release",
> > @@ -98,9 +100,22 @@
> >                        "slavenames": ["szeged-linux-1"]
> >                      },
> >                      {
> > -                      "name": "Chromium Win Release", "type": "ChromiumBuild", "builddir": "chromium-win-release",
> > -                      "platform": "chromium-win", "configuration": "release", "architectures": ["i386"],
> > -                      "slavenames": ["google-slave-1"]
> > +                      "name": "Chromium Win Release", "type": "Build", "builddir": "chromium-win-release",
> > +                      "platform": "chromium", "configuration": "release", "architectures": ["i386"],
> > +                      "triggers": ["chromium-win-release"],
> > +                      "slavenames": ["chromium-win-slave-1"]
> > +                    },
> > +                    {
> > +                      "name": "Chromium Mac Release", "type": "Build", "builddir": "chromium-mac-release",
> > +                      "platform": "chromium", "configuration": "release", "architectures": ["i386"],
> > +                      "triggers": ["chromium-mac-release"],
> > +                      "slavenames": ["chromium-mac-slave-1"]
> > +                    },
> > +                    {
> > +                      "name": "Chromium Linux Release", "type": "Build", "builddir": "chromium-linux-release",
> > +                      "platform": "chromium", "configuration": "release", "architectures": ["i386"],
> > +                      "triggers": ["chromium-linux-release"],
> > +                      "slavenames": ["chromium-linux-slave-1"]
> >                      }
> >                    ],
> 
> The “triggers” values here seem bogus given that they don’t attempt to trigger
> anything and there’s no corresponding triggerable configuration.
> 
I know, but BuildFactory require triggers, so I had to put something there. Once we also have testing harness, we will connect them to the triggers. 

> > +class InstallChromiumDependencies(shell.ShellCommand):
> > +    name = "gclient"
> > +    description = ["updating chromium dependencies"]
> > +    descriptionDone = ["updated chromium dependencies"]
> > +    command = ["perl", "./WebKitTools/Scripts/update-webkit-chromium"]
> > +    haltOnFailure = True
> 
> This script doesn’t exist.  Is that the right command to run?

Actually, update-webkit-chromium does exist and is reused by "update-webkit --chromium" too.

> 
> >  def appendCustomBuildFlags(step, platform):
> >      if platform in ('gtk', 'wx', 'qt'):
> >          step.setCommand(step.command + ['--' + platform])
> > @@ -119,6 +78,8 @@ class CompileWebKit(shell.Compile):
> >      def start(self):
> >          platform = self.getProperty('platform')
> >          buildOnly = self.getProperty('buildOnly')
> > +        if platform == 'chromium':
> > +            self.setCommand(self.command + ['--chromium'])
> >          if platform == 'mac' and buildOnly:
> >              self.setCommand(self.command + ['DEBUG_INFORMATION_FORMAT=dwarf-with-dsym’])
> 
> This should be handled in the same way it is for other platforms: in
> appendCustomBuildFlags.

Done.

[Mark Rowe (bdash)]

(In reply to comment #5)
> Thanks for reviewing this. New patch is attached. See comments below:
> 
> > The “triggers” values here seem bogus given that they don’t attempt to trigger
> > anything and there’s no corresponding triggerable configuration.
> > 
> I know, but BuildFactory require triggers, so I had to put something there.
> Once we also have testing harness, we will connect them to the triggers. 

A better fix would be to make the triggers optional.  This would convert some Chromium-specific logic in master.cfg in to logic that is specific to whether or not BuildFactory has anything to trigger.  

> 
> > > +class InstallChromiumDependencies(shell.ShellCommand):
> > > +    name = "gclient"
> > > +    description = ["updating chromium dependencies"]
> > > +    descriptionDone = ["updated chromium dependencies"]
> > > +    command = ["perl", "./WebKitTools/Scripts/update-webkit-chromium"]
> > > +    haltOnFailure = True
> > 
> > This script doesn’t exist.  Is that the right command to run?
> 
> Actually, update-webkit-chromium does exist and is reused by "update-webkit
> --chromium" too.

My mistake.

[Mark Rowe (bdash)]

And at least one of the slave names doesn’t match the ones from my email.

[Mark Rowe (bdash)]

Comment on attachment 43115 [details]
buildbot configuration changes

Marking as r- per last two comments.  There’s no point marking this as commit-queue? since it needs to be manually landed anyway.

[Yaar Schnitman]

Created attachment 43118 [details]
buildbot configuration changes

[Yaar Schnitman]

Created attachment 43120 [details]
buildbot configuration changes

[Yaar Schnitman]

Comment on attachment 43120 [details]
buildbot configuration changes

Fixed the win slave names and made triggers optional.

[Mark Rowe (bdash)]

Comment on attachment 43120 [details]
buildbot configuration changes


>  class BuildFactory(Factory):
> -    def __init__(self, platform, configuration, architectures, triggers):
> +    def __init__(self, platform, configuration, architectures, triggers=None):
>          Factory.__init__(self, platform, configuration, architectures, True)
>          self.addStep(CompileWebKit)
> +        if platform == "chromium":
> +            # FIXME: Chromium should archive and trigger tests too.
> +            return
>          self.addStep(ArchiveBuiltProduct)
>          self.addStep(UploadBuiltProduct)
> -        self.addStep(trigger.Trigger, schedulerNames=triggers)
> +        if triggers:
> +            self.addStep(trigger.Trigger, schedulerNames=triggers)

Since there’s no point in archiving or uploading the built product unless you’re going to do something that uses it, this would make more sense if it were expressed as:

self.addStep(CompileWebKit)
if triggers:
    self.addStep(ArchiveBuiltProduct)
    self.addStep(UploadBuiltProduct)
    self.addStep(trigger.Trigger, schedulerNames=triggers)

It also removes logic that special-cases Chromium.

I’m going to say r=me but this should be fixed before it is landed.

[Yaar Schnitman]

Created attachment 43464 [details]
buildbot configuration changes

[Darin Adler]

Comment on attachment 43464 [details]
buildbot configuration changes

r=me based on Mark's earlier review and comment

[Yaar Schnitman]

Thank you!

[Eric Seidel (no email)]

Comment on attachment 43464 [details]
buildbot configuration changes

Yaar is not a committer quite yet, so marking cq+.  I beleive Mark Rowe will have to push the config to the master manually once this is in.  You can work that out with him. :)

[WebKit Commit Bot]

Comment on attachment 43464 [details]
buildbot configuration changes

Clearing flags on attachment: 43464

Committed r51167: <http://trac.webkit.org/changeset/51167>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.