[webkit-reviews] review granted: [Bug 39296] build-webkit doesn't recognize Platform SDK on win64 : [Attachment 56499] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Tony Gentilcore
<tonyg at chromium.org>'s request for review:
Bug 39296: build-webkit doesn't recognize Platform SDK on win64
https://bugs.webkit.org/show_bug.cgi?id=39296

Attachment 56499: Patch
https://bugs.webkit.org/attachment.cgi?id=56499&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
Thanks for going back and creating a new patch. I have some minor nits:

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index
4f06670e4bcfa8824b6e9034642e578ba69ceb0a..f7afb31e53adb55d7c10e2d9549493a78f2c8
4a4 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,17 @@
> +2010-05-18  Tony Gentilcore	<tonyg at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Look in registry64 for Platform SDK.

I would change this to read "Look in /proc/registry64 for the Platform SDK on
64-bit Windows."

> +
> +	   The build-webkit script failed for me on Vista 64. A web search
turned
> +	   up this blog post with a patch that worked for me:
> +	   http://www.nicholaswilson.me.uk/2010/04/hacking-webkit-fail/
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=39296

I would move the bug URL line so that it appears after the "Reviewed by NOBODY
(OOPS!)." line:

> +
> +	   * Scripts/webkitdirs.pm:
> +
>  2010-05-08  Robert Hogan  <robert at roberthogan.net>
>  
>	   Reviewed by Simon Hausmann.
> diff --git a/WebKitTools/Scripts/webkitdirs.pm
b/WebKitTools/Scripts/webkitdirs.pm
> index
c512009ecdecd2f9b6c8fc9e82665a2144b0614e..648bce19bdd8233007d97a5abc24e1d82440d
03e 100644
> --- a/WebKitTools/Scripts/webkitdirs.pm
> +++ b/WebKitTools/Scripts/webkitdirs.pm
> @@ -1051,9 +1051,14 @@ sub setupCygwinEnv()
>  
>  sub dieIfWindowsPlatformSDKNotInstalled
>  {
> -    my $windowsPlatformSDKRegistryEntry =
"/proc/registry/HKEY_LOCAL_MACHINE/SOFTWARE/Microsoft/MicrosoftSDK/InstalledSDK
s/D2FF9F89-8AA2-4373-8A31-C838BF4DBBE1";
> -
> -    return if -e $windowsPlatformSDKRegistryEntry;
> +    my $registry32 = "/proc/registry";
> +    my $registry64 = "/proc/registry64";

The names of these variables seem a bit vague, maybe rename them to
$registry32Path, $registry64Path?

I would append "/" to the end of both of these values then remove the "/" at
the beginning of the value for $windowsPlatformSDKRegistryEntry so that
$windowsPlatformSDKRegistryEntry better resembles a Windows registry path when
we print it in the error message "Cannot find ...."

> +    my $windowsPlatformSDKRegistryEntry =
"/HKEY_LOCAL_MACHINE/SOFTWARE/Microsoft/MicrosoftSDK/InstalledSDKs/D2FF9F89-8AA
2-4373-8A31-C838BF4DBBE1";

As aforementioned, I would remove the leading "/" in the value for
$windowsPlatformSDKRegistryEntry.

> +
> +    # FIXME It would be better to detect if this is a 64-bit Windows build
> +    # and only check the appropriate entry. But for now we just blindly
check
> +    # both.
> +    return if (-e $registry32 . $windowsPlatformSDKRegistryEntry) || (-e
$registry64 . $windowsPlatformSDKRegistryEntry);
>  
>      print "*************************************************************\n";

>      print "Cannot find '$windowsPlatformSDKRegistryEntry'.\n";

Since we are no longer using a fixed file system path, I would suggest changing
the error message line to read:

print "Cannot find registry entry '$windowsPlatformSDKRegistryEntry'.\n";

r=me.