WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Updated per Tony's comments
(12.15 KB, patch)
2012-03-29 17:51 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
Fixed
(12.10 KB, patch)
2012-03-30 10:50 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1
2012-03-29 15:29:13 PDT
Created
attachment 134683
[details]
Patch
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
Comment on
attachment 134683
[details]
Patch
Attachment 134683
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/12230004
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
Created
attachment 134837
[details]
Fixed
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
Landed build fixes in
http://trac.webkit.org/changeset/112782
and
http://trac.webkit.org/changeset/112783
.
Ryosuke Niwa
Comment 18
2012-03-31 14:38:27 PDT
More build fixes landed in
http://trac.webkit.org/changeset/112802
and
http://trac.webkit.org/changeset/112804
.
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.
Top of Page
Format For Printing
XML
Clone This Bug