Bug 39296 – build-webkit doesn't recognize Platform SDK on win64

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Gentilcore]

build-webkit doesn't recognize Platform SDK on win64

[Tony Gentilcore]

Created attachment 56383 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 56383 [details]
Patch

> -    return if -e $windowsPlatformSDKRegistryEntry;
> +    return if ((-e $windowsPlatformSDKRegistryEntry) || (-e $windows64PlatformSDKRegistryEntry));

Normally we'd omit the outer parentheses here.

r=me

[Daniel Bates]

Comment on attachment 56383 [details]
Patch


>  {
>      my $windowsPlatformSDKRegistryEntry = "/proc/registry/HKEY_LOCAL_MACHINE/SOFTWARE/Microsoft/MicrosoftSDK/InstalledSDKs/D2FF9F89-8AA2-4373-8A31-C838BF4DBBE1";
> +    my $windows64PlatformSDKRegistryEntry = "/proc/registry64/HKEY_LOCAL_MACHINE/SOFTWARE/Microsoft/MicrosoftSDK/InstalledSDKs/D2FF9F89-8AA2-4373-8A31-C838BF4DBBE1";

Is there no way to detect that we are building Windows 64? Also, these paths diff only in there prefix. It seems extraneous to repeat the value twice just because of the difference in prefix.

>  
> -    return if -e $windowsPlatformSDKRegistryEntry;
> +    return if ((-e $windowsPlatformSDKRegistryEntry) || (-e $windows64PlatformSDKRegistryEntry));

I also agree with Adam Roben about omitting the outer parentheses here.

[Tony Gentilcore]

Created attachment 56499 [details]
Patch

[Tony Gentilcore]

(In reply to comment #3)
> (From update of attachment 56383 [details])
> 
> >  {
> >      my $windowsPlatformSDKRegistryEntry = "/proc/registry/HKEY_LOCAL_MACHINE/SOFTWARE/Microsoft/MicrosoftSDK/InstalledSDKs/D2FF9F89-8AA2-4373-8A31-C838BF4DBBE1";
> > +    my $windows64PlatformSDKRegistryEntry = "/proc/registry64/HKEY_LOCAL_MACHINE/SOFTWARE/Microsoft/MicrosoftSDK/InstalledSDKs/D2FF9F89-8AA2-4373-8A31-C838BF4DBBE1";
> 
> Is there no way to detect that we are building Windows 64? 

I'm sure there is a way, but I'm not a windows environment or perl expert. I just had to run the layout tests on win and this got me building. I've added a FIXME comment.

> Also, these paths diff only in there prefix. It seems extraneous to repeat the value twice just because of the difference in prefix.

I've cleaned up that part.

> 
> >  
> > -    return if -e $windowsPlatformSDKRegistryEntry;
> > +    return if ((-e $windowsPlatformSDKRegistryEntry) || (-e $windows64PlatformSDKRegistryEntry));
> 
> I also agree with Adam Roben about omitting the outer parentheses here.

Done.

[Daniel Bates]

Comment on attachment 56499 [details]
Patch

Thanks for going back and creating a new patch. I have some minor nits:

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 4f06670e4bcfa8824b6e9034642e578ba69ceb0a..f7afb31e53adb55d7c10e2d9549493a78f2c84a4 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,17 @@
> [phone]  Tony Gentilcore  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          Reviewed by Simon Hausmann.
> diff --git a/WebKitTools/Scripts/webkitdirs.pm b/WebKitTools/Scripts/webkitdirs.pm
> index c512009ecdecd2f9b6c8fc9e82665a2144b0614e..648bce19bdd8233007d97a5abc24e1d82440d03e 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/InstalledSDKs/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-8AA2-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.

[Tony Gentilcore]

Created attachment 56535 [details]
Patch

[Tony Gentilcore]

(In reply to comment #6)
> (From update of attachment 56499 [details])
> Thanks for going back and creating a new patch. I have some minor nits:
> 
> > diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> > index 4f06670e4bcfa8824b6e9034642e578ba69ceb0a..f7afb31e53adb55d7c10e2d9549493a78f2c84a4 100644
> > --- a/WebKitTools/ChangeLog
> > +++ b/WebKitTools/ChangeLog
> > @@ -1,3 +1,17 @@
> > [phone]  Tony Gentilcore  <[e-mail]>
> > +
> > +        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."

Done.

> 
> > +
> > +        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:
> 

The prepare-ChangeLog script puts the description before the bug URLs. The majority of ChangeLog entries seem to follow that pattern as well. Can you explain the rationale?

> > +
> > +        * Scripts/webkitdirs.pm:
> > +
> >  2010-05-08  Robert Hogan  <[e-mail]>
> >  
> >          Reviewed by Simon Hausmann.
> > diff --git a/WebKitTools/Scripts/webkitdirs.pm b/WebKitTools/Scripts/webkitdirs.pm
> > index c512009ecdecd2f9b6c8fc9e82665a2144b0614e..648bce19bdd8233007d97a5abc24e1d82440d03e 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/InstalledSDKs/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?

Done.

> 
> 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-8AA2-4373-8A31-C838BF4DBBE1";
> 
> As aforementioned, I would remove the leading "/" in the value for $windowsPlatformSDKRegistryEntry.
> 

Done.

> > +
> > +    # 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";
> 

Done.

> r=me.

[Daniel Bates]

Comment on attachment 56535 [details]
Patch

> -    return if -e $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.

You are missing a ':' after FIXME in the comment, which is necessary for some editors, such as Xcode, to notice that this is a special comment.

Since the last line of this comment is only one word, I would suggest collapsing it into the second line and push the word "and" (at the start of the second line) into the first line so that the two lines are roughly the same size.

Also, I think it might be a bit clearer to write the first sentence as "It would be better to detect whether we are using 32-bit or 64-bit Windows ...":

# FIXME: It would be better to detect whether we are using 32- or 64-bit Windows
# and only check the appropriate entry. But for now we just blindly check both.

[Daniel Bates]

I inadvertently left this out of my last comment.

(In reply to comment #8)
> (In reply to comment #6)
> 
> The prepare-ChangeLog script puts the description before the bug URLs. The majority of ChangeLog entries seem to follow that pattern as well. Can you explain the rationale?
> 

The rationale is that someone does not have to read through the entire description to find the bug URL.

From my understanding of <http://webkit.org/coding/contributing.html#changelogs>, and the example it references <http://trac.webkit.org/changeset/43259> the order is bug name, bug URL, then bug description. A more recent example of this ordering is change-set 59798: <http://trac.webkit.org/changeset/59798>.

[Daniel Bates]

(In reply to comment #10)
> I inadvertently left this out of my last comment.
> 
> (In reply to comment #8)
> > (In reply to comment #6)
> > 
> > The prepare-ChangeLog script puts the description before the bug URLs. The majority of ChangeLog entries seem to follow that pattern as well. Can you explain the rationale?
> > 
> 
> The rationale is that someone does not have to read through the entire description to find the bug URL.
> 
> From my understanding of <http://webkit.org/coding/contributing.html#changelogs>, and the example it references <http://trac.webkit.org/changeset/43259> the order is bug name, bug URL, then bug description. A more recent example of this ordering is change-set 59798: <http://trac.webkit.org/changeset/59798>.

I meant "description", not "bug description", as the description in the change log does not need (and probably should not be) to be a verbatim copy of the description in the bug.

[Tony Gentilcore]

Created attachment 56543 [details]
Patch

[Tony Gentilcore]

(In reply to comment #9)
> (From update of attachment 56535 [details])
> > -    return if -e $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.
> 
> You are missing a ':' after FIXME in the comment, which is necessary for some editors, such as Xcode, to notice that this is a special comment.
> 
> Since the last line of this comment is only one word, I would suggest collapsing it into the second line and push the word "and" (at the start of the second line) into the first line so that the two lines are roughly the same size.
> 
> Also, I think it might be a bit clearer to write the first sentence as "It would be better to detect whether we are using 32-bit or 64-bit Windows ...":
> 
> # FIXME: It would be better to detect whether we are using 32- or 64-bit Windows
> # and only check the appropriate entry. But for now we just blindly check both.

Done.

[Tony Gentilcore]

(In reply to comment #11)
> (In reply to comment #10)
> > I inadvertently left this out of my last comment.
> > 
> > (In reply to comment #8)
> > > (In reply to comment #6)
> > > 
> > > The prepare-ChangeLog script puts the description before the bug URLs. The majority of ChangeLog entries seem to follow that pattern as well. Can you explain the rationale?
> > > 
> > 
> > The rationale is that someone does not have to read through the entire description to find the bug URL.
> > 
> > From my understanding of <http://webkit.org/coding/contributing.html#changelogs>, and the example it references <http://trac.webkit.org/changeset/43259> the order is bug name, bug URL, then bug description. A more recent example of this ordering is change-set 59798: <http://trac.webkit.org/changeset/59798>.
> 
> I meant "description", not "bug description", as the description in the change log does not need (and probably should not be) to be a verbatim copy of the description in the bug.

Ah, that makes sense to me now, thanks for the explanation. Fixed.

[Daniel Bates]

Comment on attachment 56543 [details]
Patch

Thanks Tony. Looks good to me.

[WebKit Commit Bot]

Comment on attachment 56543 [details]
Patch

Clearing flags on attachment: 56543

Committed r59906: <http://trac.webkit.org/changeset/59906>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.