[webkit-reviews] review granted: [Bug 39296] build-webkit doesn't recognize Platform SDK on win64 : [Attachment 56499] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 19 15:08:14 PDT 2010
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.
More information about the webkit-reviews
mailing list