RESOLVED FIXED 82666
Chromium bots should upload archived built files
https://bugs.webkit.org/show_bug.cgi?id=82666
Summary Chromium bots should upload archived built files
Ryosuke Niwa
Reported 2012-03-29 15:09:39 PDT
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.
Attachments
Patch (10.95 KB, patch)
2012-03-29 15:29 PDT, Ryosuke Niwa
no flags
Updated per Tony's comments (12.15 KB, patch)
2012-03-29 17:51 PDT, Ryosuke Niwa
no flags
Fixed (12.10 KB, patch)
2012-03-30 10:50 PDT, Ryosuke Niwa
no flags
Ryosuke Niwa
Comment 1 2012-03-29 15:29:13 PDT
Ryosuke Niwa
Comment 2 2012-03-29 15:34:53 PDT
Maybe Qt and GTK want to use copyBuildFiles to avoid archiving intermediate files as well.
Build Bot
Comment 3 2012-03-29 15:53:37 PDT
Ryosuke Niwa
Comment 4 2012-03-29 15:57:47 PDT
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 5 2012-03-29 17:26:16 PDT
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 6 2012-03-29 17:31:48 PDT
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
Comment 7 2012-03-29 17:51:30 PDT
Created attachment 134704 [details] Updated per Tony's comments
Tony Chang
Comment 8 2012-03-30 10:10:58 PDT
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 9 2012-03-30 10:35:56 PDT
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
Comment 10 2012-03-30 10:40:21 PDT
Effing python. The stupid function is deprecated in 3.0 :(
Ryosuke Niwa
Comment 11 2012-03-30 10:50:41 PDT
Tony Chang
Comment 12 2012-03-30 11:43:43 PDT
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
Comment 13 2012-03-30 11:44:27 PDT
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
Comment 14 2012-03-30 12:16:04 PDT
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 15 2012-03-30 23:50:13 PDT
Comment on attachment 134837 [details] Fixed Clearing flags on attachment: 134837 Committed r112781: <http://trac.webkit.org/changeset/112781>
Ryosuke Niwa
Comment 16 2012-03-30 23:50:20 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 17 2012-03-31 00:22:14 PDT
Ryosuke Niwa
Comment 18 2012-03-31 14:38:27 PDT
Peter Beverloo
Comment 19 2012-04-02 09:29:06 PDT
Out of interest, why didn't you enable this for the Chromium Android builder?
Ryosuke Niwa
Comment 20 2012-04-03 01:40:17 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.