Bug 51996 – Windows bots need to archive/unarchive builds to/from configuration-specific directories

• Component: New Bugs
• Status: RESOLVED FIXED

[Steve Falkenburg]

Windows bots need to archive/unarchive builds to/from configuration-specific directories

[Steve Falkenburg]

Created attachment 78131 [details]
Patch

[Steve Falkenburg]

Created attachment 78132 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 78132 [details]
Patch

It's amazing how little is shared here between platforms.  I CC'd wms as he knows the master.cfg

[Eric Seidel (no email)]

I believe mrowe wrote these scripts originally.

[Adam Roben (:aroben)]

Comment on attachment 78132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78132&action=review

> Tools/BuildSlaveSupport/built-product-archive:65
>          configurationBuildDirectory = os.path.join(buildDirectory, configuration.title())
>          return subprocess.call(["ditto", "-c", "-k", "--keepParent", "--sequesterRsrc", configurationBuildDirectory, archiveFile])
>      elif platform == 'win':
> -        binDirectory = os.path.join(buildDirectory, "bin")
> +        binDirectory = os.path.join(buildDirectory, configuration.title(), "bin")

Can you pull the configurationBuildDirectory definition out a level so that we can use it in the "win" case?

> Tools/BuildSlaveSupport/built-product-archive:123
> +        configurationBuildDirectory = os.path.join(buildDirectory, configuration.title())

This is duplicating code that already exists in the "mac" case. Can you share it?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:209
> +        if platform == "win":
> +            rootArgument = ['--root=WebKitBuild/' + self.getProperty('configuration') + '/bin']
> +        else:
> +            rootArgument = ['--root=WebKitBuild/bin']
>          if self.skipBuild:
> -            self.setCommand(self.command + ['--root=WebKitBuild/bin'])
> +            self.setCommand(self.command + rootArgument)

Why don't we need to do this for RunWebKitTests.start, too?

[Steve Falkenburg]

Created attachment 78153 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 78153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78153&action=review

> Tools/BuildSlaveSupport/built-product-archive:122
> +        os.mkdir(configurationBuildDirectory)

Why didn't we need this mkdir before?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:207
> +        if platform == "win":
> +            rootArgument = ['--root=WebKitBuild/' + self.getProperty('configuration') + '/bin']
> +        else:
> +            rootArgument = ['--root=WebKitBuild/bin']

Seems like we should be using os.path.join here, even though we weren't using it before.

I'm surprised that WebKitBuild/bin is really what we want on non-Windows platforms. I know that's what we had before, though.

[Steve Falkenburg]

Committed r75192: <http://trac.webkit.org/changeset/75192>

[William Siegrist]

The patch has an undefined variable. I rolled the master back for now. 

http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/23235/steps/layout-test/logs/err.html

[Adam Roben (:aroben)]

Until this is fixed, the test bots are running old versions of WebKit/DRT!

[Steve Falkenburg]

Created attachment 78184 [details]
Patch

[William Siegrist]

Comment on attachment 78184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78184&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:204
> +        if self.getProperty('platform') == "win":

We should probably only call getProperty once for platform and store it in a local variable?

[Steve Falkenburg]

Created attachment 78189 [details]
Patch

[Steve Falkenburg]

Committed r75211: <http://trac.webkit.org/changeset/75211>

[William Siegrist]

Buildbot master updated. Looks good.