Bug 51790 – [WinCairo] Patch to download the WinCairo dependencies as part of build-webkit.

• Component: New Bugs
• Status: RESOLVED FIXED

[Carl Lobo]

Patch to download the WinCairo dependancies as part of build-webkit.

[Carl Lobo]

Created attachment 77741 [details]
Patch

[WebKit Review Bot]

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.

[Carl Lobo]

Created attachment 77742 [details]
Patch

[Eric Seidel (no email)]

Bleh.  i can't read perl anymore.

[Daniel Bates]

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?

[Eric Seidel (no email)]

Comment on attachment 77742 [details]
Patch

copy-paste is not cool.

[Carl Lobo]

Created attachment 77881 [details]
Patch

[Carl Lobo]

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.

[Carl Lobo]

Need to fix this properly...

review=-

[Carl Lobo]

Created attachment 77970 [details]
Patch

[Carl Lobo]

Properly tested out and refractored. Hope this meets the guidelines. Sorry about all the spam.

[Brent Fulgham]

Hey!  I should be on this one. :-)

[Brent Fulgham]

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.

[Adam Roben (:aroben)]

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

[Brent Fulgham]

Created attachment 89265 [details]
Patch

[Brent Fulgham]

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.

[Adam Roben (:aroben)]

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.

[Brent Fulgham]

Created attachment 89269 [details]
Patch

[Brent Fulgham]

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.)

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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;

[Brent Fulgham]

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!

[Brent Fulgham]

Committed r83639: <http://trac.webkit.org/changeset/83639>

[Brent Fulgham]

Corrected typo in build-webkit:

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