| Summary: | [WinCairo] Patch to download the WinCairo dependencies as part of build-webkit. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carl Lobo <carllobo> | ||||||||||||||
| Component: | New Bugs | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | aroben, bfulgham, carllobo, dbates, eric, webkit.review.bot | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Windows 2000 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Carl Lobo
2011-01-01 05:11:02 PST
Created attachment 77741 [details]
Patch
Attachment 77741 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/build-webkit', u'Tools/Scripts/update-webkit-cairo-libs', u'Tools/Scripts/webkitdirs.pm']" exit_code: 1
Tools/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 77742 [details]
Patch
Bleh. i can't read perl anymore. Comment on attachment 77742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77742&action=review > Tools/Scripts/build-webkit:449 > + if(isWinCairo()) { Nit: There should be a space between the "if" and '('. > Tools/Scripts/update-webkit-cairo-libs:29 > +# Updates a development environment to the new Cairo Requirements. This makes me sad. This entire file is almost identical to the script Tools/Scripts/update-webkit-auxiliary-libs. Can we extract the common functionality? Comment on attachment 77742 [details]
Patch
copy-paste is not cool.
Created attachment 77881 [details]
Patch
I reworked it without the copy-paste (my apologies again for that). I've intentionally left the update-webkit-auxillary-libs and update-webkit-cairo-libs as: 1. It would be a more intuitive place to look if the URL or filename changes. 2. Leaves the option to download from more than one source - specific at least to cairo. Need to fix this properly... review=- Created attachment 77970 [details]
Patch
Properly tested out and refractored. Hope this meets the guidelines. Sorry about all the spam. Hey! I should be on this one. :-) Comment on attachment 77970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77970&action=review This looks like a really great change -- thanks for working on it! I have a few comments below that I hope you will consider. I'm sorry I didn't see this patch earlier. Please feel free to cc me on any WinCairo changes in the future. > Tools/ChangeLog:11 > + If we are going to do this (and I hope we do!), we should probably land this on a more 'official' location than my private .Mac account. > Tools/Scripts/build-webkit:454 > + } General comment: It seems like build-webkit and friends should have a variable for the Scripts location. We've changed this once, and all of this hardcoded pathing seems pretty fragile. > Tools/Scripts/update-webkit-cairo-libs:4 > +# You should have yourself here. I'm not sure you need to include Apple for this one-line implementation. > Tools/Scripts/update-webkit-dependancy:1 > +#!/usr/bin/perl -w General note: It would be helpful if you could show this file as being 'moved' from 'Tools/Scripts/update-webkit-auxiliary-libs' to this new file. (e.g., "svn cp Tools/Scripts/update-webkit-auxiliary-libs Tools/Scripts/update-webkit-dependancy'. Then modify the new (copied) file to show the deletion of the unused stuff, and the small additional changes you made. That way, we could see (easily) if any of the loading logic below is different. Comment on attachment 77970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77970&action=review > Tools/ChangeLog:16 > + * Tools/Scripts/update-webkit-dependancy: added Typo: dependancy should be dependency Created attachment 89265 [details]
Patch
I updated (hijacked?) this patch to work a little bit better. Thanks so much for working on this -- this will be a huge help to anyone interested in working on the WinCairo port. 1. update-webkit needed to know how to get these files (not just build-webkit). 2. I updated my 'requirements.zip' file on the server to be called "WinCairoRequirements.zip", and landed a current WinCairoRequirements.header file so it will work like the Apple stuff when I update it. 3. I purged items in the WinCairoRequirements.zip that duplicated stuff in the "WebKitAuxilliaryLibraries.zip". The WinCairo stuff is basically an add-on to the regular Apple stuff. After making these changes I was able to successfully update and install the WinCairo requirements using the script changes. Comment on attachment 89265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89265&action=review > Tools/Scripts/update-webkit-auxiliary-libs:38 > +system("perl", "Tools/Scripts/update-webkit-dependency", $auxiliaryLibsURL, "win") == 0 or die; Are you sure that the working directory is always the same when this script is invoked? Using $FindBin::Bin to locate the other script might be safer. > Tools/Scripts/update-webkit-dependency:150 > +sub getLibraryName Please give this function a prototype (like lastModifiedToUnixTime has). > Tools/Scripts/update-webkit-dependency:157 > + my $path = shift; > + my $filename; > + my $i = rindex($path, "/"); > + $filename = substr($path, $i + 1); > + $filename = substr($filename, 0, rindex($filename, ".zip")); > + return $filename; Please use 4-space indents. I think a regular expression might be clearer: $path =~ m#/([^/])\.zip$#; return $1; > Tools/Scripts/update-webkit-wincairo-libs:38 > +system("perl", "Tools/Scripts/update-webkit-dependency", $winCairoLibsURL, ".") == 0 or die; Same comments here about working directory. Created attachment 89269 [details]
Patch
Comment on attachment 89265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89265&action=review >> Tools/Scripts/update-webkit-auxiliary-libs:38 >> +system("perl", "Tools/Scripts/update-webkit-dependency", $auxiliaryLibsURL, "win") == 0 or die; > > Are you sure that the working directory is always the same when this script is invoked? Using $FindBin::Bin to locate the other script might be safer. Cool! I didn't know about this utility. >> Tools/Scripts/update-webkit-dependency:157 >> + return $filename; > > Please use 4-space indents. > > I think a regular expression might be clearer: > > $path =~ m#/([^/])\.zip$#; > return $1; I ended up using File::Spec, because I couldn't figure out the syntax for getting the full file name (for some reason your suggestion kept returning '' on my system.) Comment on attachment 89269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89269&action=review > Tools/Scripts/build-webkit:512 > + if(isWinCairo()) { Missing space after "if". > Tools/Scripts/update-webkit-dependency:56 > +sub getLibraryName; You should add the prototype here, too. > Tools/Scripts/update-webkit-dependency:154 > + my $path = shift; > + my ($volume, $directory, $file) = File::Spec->splitpath($path); Hm, splitpath doesn't seem right, since $path is really a URL. You should rename $path to $url, and use the regex solution I think. Comment on attachment 89265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89265&action=review >>> Tools/Scripts/update-webkit-dependency:157 >>> + return $filename; >> >> Please use 4-space indents. >> >> I think a regular expression might be clearer: >> >> $path =~ m#/([^/])\.zip$#; >> return $1; > > I ended up using File::Spec, because I couldn't figure out the syntax for getting the full file name (for some reason your suggestion kept returning '' on my system.) That's because I forgot a +: $path =~ m#/([^/]+)\.zip$#; return $1; Comment on attachment 89265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89265&action=review >>>> Tools/Scripts/update-webkit-dependency:157 >>>> + return $filename; >>> >>> Please use 4-space indents. >>> >>> I think a regular expression might be clearer: >>> >>> $path =~ m#/([^/])\.zip$#; >>> return $1; >> >> I ended up using File::Spec, because I couldn't figure out the syntax for getting the full file name (for some reason your suggestion kept returning '' on my system.) > > That's because I forgot a +: > > $path =~ m#/([^/]+)\.zip$#; > return $1; Oh yeah! I was putting the '+' outside the capture parens, so I was only getting the last letter. Doh! Committed r83639: <http://trac.webkit.org/changeset/83639> Corrected typo in build-webkit: Committed r83657: <http://trac.webkit.org/changeset/83657> |