Bug 82666 – Chromium bots should upload archived built files

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

I want Chromium test/perf bots to just extra built archives instead of building on their own.

This is the first half of this transition. The patch for this bug will make builders create zip files and then upload them to the master.
The second half (which will be posted on a follow up bug) will make testers extract the uploaded zip files.

[Ryosuke Niwa]

Created attachment 134683 [details]
Patch

[Ryosuke Niwa]

Maybe Qt and GTK want to use copyBuildFiles to avoid archiving intermediate files as well.

[Build Bot]

Comment on attachment 134683 [details]
Patch

Attachment 134683 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12230004

[Ryosuke Niwa]

Note EWS can never check the correctness of this patch...

(In reply to comment #3)
> (From update of attachment 134683 [details])
> Attachment 134683 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/12230004

===============================================================================
WebKitSupportLibrary.zip is out-of-date.
Please download WebKitSupportLibrary.zip from:

    https://developer.apple.com/opensource/internet/webkit_sptlib_agree.html

and place it in:

    /home/buildbot/WebKit

Then run build-webkit again.
===============================================================================

[Tony Chang]

Comment on attachment 134683 [details]
Patch

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

Seems OK in general.

> Tools/BuildSlaveSupport/built-product-archive:34
> +buildDirectory = None

Nit: Prefix with an underscore to signify that this is only used within the module.

> Tools/BuildSlaveSupport/built-product-archive:73
> +def removeDirectoryIfExists(thinDirectory):
> +    if os.path.isdir(thinDirectory):
> +        shutil.rmtree(thinDirectory)

Are we intentionally not using webkitpy code here?  Maybe reusing some of webkitpy.common would be nice.

> Tools/BuildSlaveSupport/built-product-archive:78
> +def copyBuildFiles(source, destination):
> +    shutil.copytree(source, destination,
> +        ignore=shutil.ignore_patterns('.svn', '*.a', '*.d', '*.dSYM', '*.o', '*.ilk', '*.pdb'))

I think we want to ignore .lib and .obj too (MSVC extensions).

> Tools/BuildSlaveSupport/built-product-archive:91
> +    os.path.walk(directoryToZip, addToArchive, None)

Use os.walk instead of os.path.walk.

> Tools/Scripts/webkit-build-directory:65
> +# FIXME: Check if extra flags are valid or not.
> +Getopt::Long::Configure('pass_through'); # Let --blackberry, etc... be handled by webkitdirs
> +my $chromium = 0;

Why do we need $chromium here?

[Ryosuke Niwa]

Comment on attachment 134683 [details]
Patch

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

>> Tools/BuildSlaveSupport/built-product-archive:73
>> +        shutil.rmtree(thinDirectory)
> 
> Are we intentionally not using webkitpy code here?  Maybe reusing some of webkitpy.common would be nice.

Yes. It seems like not directly depending on Tools/Scripts is the convention used in this directory.

>> Tools/BuildSlaveSupport/built-product-archive:78
>> +        ignore=shutil.ignore_patterns('.svn', '*.a', '*.d', '*.dSYM', '*.o', '*.ilk', '*.pdb'))
> 
> I think we want to ignore .lib and .obj too (MSVC extensions).

Good point. Will do.

>> Tools/BuildSlaveSupport/built-product-archive:91
>> +    os.path.walk(directoryToZip, addToArchive, None)
> 
> Use os.walk instead of os.path.walk.

Okay. Will do.

>> Tools/Scripts/webkit-build-directory:65
>> +my $chromium = 0;
> 
> Why do we need $chromium here?

Oh oops, that's a left over. Will remove.

[Ryosuke Niwa]

Created attachment 134704 [details]
Updated per Tony's comments

[Tony Chang]

Comment on attachment 134704 [details]
Updated per Tony's comments

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

> Tools/BuildSlaveSupport/built-product-archive:91
> +    os.walk(directoryToZip, addToArchive, None)

Err, did you try this? os.walk is an iterator. See the example here:
http://docs.python.org/library/os.html#os.walk

[Ryosuke Niwa]

Comment on attachment 134704 [details]
Updated per Tony's comments

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

>> Tools/BuildSlaveSupport/built-product-archive:91
>> +    os.walk(directoryToZip, addToArchive, None)
> 
> Err, did you try this? os.walk is an iterator. See the example here:
> http://docs.python.org/library/os.html#os.walk

Why is that better tahn os.path.walk?

[Ryosuke Niwa]

Effing python. The stupid function is deprecated in 3.0 :(

[Ryosuke Niwa]

Created attachment 134837 [details]
Fixed

[Tony Chang]

Comment on attachment 134837 [details]
Fixed

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

> Tools/BuildSlaveSupport/built-product-archive:87
> +    for path, dirNames, fileNames in os.walk(directoryToZip):
> +        relativePath = os.path.relpath(path, directoryToZip)
> +        for dirName in dirNames:
> +            archiveZip.write(os.path.join(path, dirName), os.path.join(relativePath, dirName))

This looks like you're writing directory names only.  I think you can ignore dirNames and just use fileNames here.  Please test this (you locally modify the code to run on OSX) before landing.

[Tony Chang]

It's also unfortunate that this file uses camelCaseNames instead of variables_with_underscores, but that would be a separate style only change.

[Ryosuke Niwa]

Thanks for the review.

(In reply to comment #12)
> (From update of attachment 134837 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134837&action=review
> 
> > Tools/BuildSlaveSupport/built-product-archive:87
> > +    for path, dirNames, fileNames in os.walk(directoryToZip):
> > +        relativePath = os.path.relpath(path, directoryToZip)
> > +        for dirName in dirNames:
> > +            archiveZip.write(os.path.join(path, dirName), os.path.join(relativePath, dirName))
> 
> This looks like you're writing directory names only.  I think you can ignore dirNames and just use fileNames here.  Please test this (you locally modify the code to run on OSX) before landing.

Oops, fixed.

(In reply to comment #13)
> It's also unfortunate that this file uses camelCaseNames instead of variables_with_underscores, but that would be a separate style only change.

Yeah, we could fix that later I guess.

[Ryosuke Niwa]

Comment on attachment 134837 [details]
Fixed

Clearing flags on attachment: 134837

Committed r112781: <http://trac.webkit.org/changeset/112781>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Landed build fixes in http://trac.webkit.org/changeset/112782 and http://trac.webkit.org/changeset/112783.

[Ryosuke Niwa]

More build fixes landed in http://trac.webkit.org/changeset/112802 and http://trac.webkit.org/changeset/112804.

[Peter Beverloo]

Out of interest, why didn't you enable this for the Chromium Android builder?

[Ryosuke Niwa]

(In reply to comment #19)
> Out of interest, why didn't you enable this for the Chromium Android builder?

Because I don't know what kind of tools are available on Chromium Android builder and how builds are setup. Also, you guys don't have any testers setup so this whole change (making testers not build) is irrelevant.