Bug 49503 – update-webkit-support-libs should fall back to existing WebKitSupportLibrary version if there is no internet connectivity

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

On a computer with an existing Apple Windows WebKit setup and no internet connectivity, update-webkit-support-libs dies with the following message:
===============================================================================
WebKitSupportLibrary.zip is out-of-date.
Please download WebKitSupportLibrary.zip from:

    http://developer.apple.com/opensource/internet/webkit_sptlib_agree.html

and place it in:

    /home/Administrator/WebKit

Then run build-webkit again.
===============================================================================

Moreover, build-webkit fails because update-webkit-support-libs fails. Therefore, it is not possible to build WebKit on Windows using the build-webkit script without internet connectivity.

[Daniel Bates]

Additional Remarks:

Notice, update-webkite-support-libs performs version checking against http://developer.apple.com so as to inform a person when a new version of the support libraries are available. Without internet connectivity the script considers the existing support libraries to be out-of-date. Hence, update-webkit-support-libs dies with the aforementioned message. Instead, we should fall back to the existing version of the support libraries (if present), possibly with some kind of warning message, so that it is possible to run build-webkit without an internet connection.

[Daniel Bates]

Created attachment 73837 [details]
Work-in-progress patch

[Daniel Bates]

Created attachment 73850 [details]
Patch

[Daniel Bates]

Created attachment 73851 [details]
Patch

Removed extraneous new line.

[Adam Roben (:aroben)]

Comment on attachment 73851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73851&action=review

It's great to make this script not rely on the network. I think it might be clearer if the "were we able to check the expected version" check happened at a higher level somehow. Maybe just combining your sequential ifs into if/else would make it clearer, i.e.:

if ($wasSupportLibsURLReachable) {
    ...do one thing...
} else {
    ...do another...
}

> WebKitTools/Scripts/update-webkit-support-libs:51
> +my $supportLibsURL = "http://developer.apple.com/opensource/internet/$versionFile";

This name seems confusing. It's not the URL of the support libs; it's the URL of the version file.

> WebKitTools/Scripts/update-webkit-support-libs:60
> +my $wasSupportLibsURLReachable = WEXITSTATUS($?) == 0;

This variable has the same naming issue.

> WebKitTools/Scripts/update-webkit-support-libs:68
> +if ($wasSupportLibsURLReachable && $extractedVersion eq $expectedVersion) {
> +    # Check whether the extracted library is up-to-date. If it is, we don't have anything to do.

I think this comment should be above the if, not below it.

> WebKitTools/Scripts/update-webkit-support-libs:81
> +if ($wasSupportLibsURLReachable && $zipFileVersion ne $expectedVersion) {
> +    # Check whether the downloaded library is up-to-date. If it isn't, the user needs to download it.

I think this comment should be above the if, not below it.

[Daniel Bates]

Created attachment 74165 [details]
Patch

Updated patch based on Adam Roben's comments.

Renamed $supportLibsURL to $versionFileURL and moved common code to check for presence of WebKitSupportLibrary.zip and extract its version into method zipFileVersionOrDieAndInstructToDownload(). I am open to suggestions.

[Daniel Bates]

Created attachment 74168 [details]
Patch

Fixed formatting issues.

[Adam Roben (:aroben)]

Comment on attachment 74168 [details]
Patch

What about something like (vastly summarized):

my $expectedVersion = downloadExpectedVersionNumber();
exit if $expectedVersion && $expectedVersion eq $extractedVersion;

my $zipFileVersion = zipFileVersion();
die if $expectedVersion && $expectedVersion ne $extractedVersion;

exit if $zipFileVersion eq $extractedVersion;

// extract the zip


Another option is to set $extractedVersion to an invalid version ("-1", "NOTAVERSION", etc.) when we can't download it. Then we wouldn't need the "if $extractedVersion" tests.

[Daniel Bates]

Comment on attachment 74168 [details]
Patch

Clearing review flag based on Adam Roben's comments.

[Daniel Bates]

Created attachment 74248 [details]
Patch

Updated patch based on Adam Roben's comments.

[Daniel Bates]

Comment on attachment 74248 [details]
Patch

Clearing flags on attachment: 74248

Committed r72326: <http://trac.webkit.org/changeset/72326>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.