Bug 51790 - [WinCairo] Patch to download the WinCairo dependencies as part of build-webkit.
Summary: [WinCairo] Patch to download the WinCairo dependencies as part of build-webkit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 2000
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-01 05:11 PST by Carl Lobo
Modified: 2011-04-12 15:59 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.60 KB, patch)
2011-01-01 05:14 PST, Carl Lobo
no flags Details | Formatted Diff | Diff
Patch (8.64 KB, patch)
2011-01-01 05:33 PST, Carl Lobo
no flags Details | Formatted Diff | Diff
Patch (14.51 KB, patch)
2011-01-04 05:26 PST, Carl Lobo
no flags Details | Formatted Diff | Diff
Patch (15.08 KB, patch)
2011-01-04 22:51 PST, Carl Lobo
no flags Details | Formatted Diff | Diff
Patch (19.38 KB, patch)
2011-04-12 13:37 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (19.49 KB, patch)
2011-04-12 14:15 PDT, Brent Fulgham
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carl Lobo 2011-01-01 05:11:02 PST
Patch to download the WinCairo dependancies as part of build-webkit.
Comment 1 Carl Lobo 2011-01-01 05:14:28 PST
Created attachment 77741 [details]
Patch
Comment 2 WebKit Review Bot 2011-01-01 05:16:30 PST
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.
Comment 3 Carl Lobo 2011-01-01 05:33:32 PST
Created attachment 77742 [details]
Patch
Comment 4 Eric Seidel 2011-01-03 23:44:14 PST
Bleh.  i can't read perl anymore.
Comment 5 Daniel Bates 2011-01-04 00:28:13 PST
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 6 Eric Seidel 2011-01-04 00:32:36 PST
Comment on attachment 77742 [details]
Patch

copy-paste is not cool.
Comment 7 Carl Lobo 2011-01-04 05:26:47 PST
Created attachment 77881 [details]
Patch
Comment 8 Carl Lobo 2011-01-04 05:30:36 PST
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.
Comment 9 Carl Lobo 2011-01-04 06:08:23 PST
Need to fix this properly...

review=-
Comment 10 Carl Lobo 2011-01-04 22:51:10 PST
Created attachment 77970 [details]
Patch
Comment 11 Carl Lobo 2011-01-04 22:52:30 PST
Properly tested out and refractored. Hope this meets the guidelines. Sorry about all the spam.
Comment 12 Brent Fulgham 2011-04-10 20:55:41 PDT
Hey!  I should be on this one. :-)
Comment 13 Brent Fulgham 2011-04-10 21:06:50 PDT
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 14 Adam Roben (:aroben) 2011-04-11 06:28:32 PDT
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
Comment 15 Brent Fulgham 2011-04-12 13:37:47 PDT
Created attachment 89265 [details]
Patch
Comment 16 Brent Fulgham 2011-04-12 13:43:12 PDT
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 17 Adam Roben (:aroben) 2011-04-12 13:48:00 PDT
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.
Comment 18 Brent Fulgham 2011-04-12 14:15:50 PDT
Created attachment 89269 [details]
Patch
Comment 19 Brent Fulgham 2011-04-12 14:16:21 PDT
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 20 Adam Roben (:aroben) 2011-04-12 14:19:01 PDT
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 21 Adam Roben (:aroben) 2011-04-12 14:19:51 PDT
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 22 Brent Fulgham 2011-04-12 14:23:46 PDT
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!
Comment 23 Brent Fulgham 2011-04-12 14:29:06 PDT
Committed r83639: <http://trac.webkit.org/changeset/83639>
Comment 24 Brent Fulgham 2011-04-12 15:59:46 PDT
Corrected typo in build-webkit:

Committed r83657: <http://trac.webkit.org/changeset/83657>