[webkit-reviews] review denied: [Bug 31228] Windows environment variables should be set automatically : [Attachment 42704] Automatically set the environment variables by editing HKCU\Environment.

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Marwan Al Jubeh
<marwan.aljubeh at gmail.com>'s request for review:
Bug 31228: Windows environment variables should be set automatically
https://bugs.webkit.org/show_bug.cgi?id=31228

Attachment 42704: Automatically set the environment variables by editing
HKCU\Environment.
https://bugs.webkit.org/attachment.cgi?id=42704&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
Thanks for taking this on!

> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> +2009-11-07  Marwan Al Jubeh	<marwan.aljubeh at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	Fixes: https://bugs.webkit.org/show_bug.cgi?id=31228
> +	
> +	   Set the WebKitOutputDir, WebKitLibrariesDir and Cygwin environment
variables automatically
> +	in Windows as part of running update_webkit. This has been tested and
works on Windows XP,
> +	Windows Vista and Windows 7. However, I expect that the same procedure
should work for
> +	other versions of Windows.

There are some tabs in here. Our pre-commit hook won't allow patches with tabs
to be committed. Please replace the tabs with spaces.

> +	   * Scripts/update-webkit:
> +	   * Scripts/webkitdirs.pm:

It would be good to add file- or function-level comments about the changes you
made.

> +++ WebKitTools/Scripts/update-webkit (working copy)
> @@ -81,6 +81,8 @@ if (-d "../Internal") {
>      system("perl", "WebKitTools/Scripts/update-webkit-auxiliary-libs") == 0
or die;
>  }
>  
> +setupWindowsEnv() if isCygwin();

I think you probably want to check isAppleWinWebKit() instead of isCygwin().
There are other ports that use Cygwin that don't use the same environment
variables as Apple's Windows port.

> +sub determineWindowsVersion()
> +{
> +    return if $windowsVersion;
> +    chomp($windowsVersion = `regtool get
'\\HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Windows
NT\\CurrentVersion\\ProductName'`);
> +}
> +
> +sub windowsVersion()
> +{
> +    determineWindowsVersion();
> +    return $windowsVersion;
> +}
> +
> +sub isWindowsXP()
> +{
> +    return substr(windowsVersion(),0, 20) eq "Microsoft Windows XP";
> +}
> +
> +sub isWindowsVista()
> +{
> +    return substr(windowsVersion(),0, 13) eq "Windows Vista";
> +}
> +
> +sub isWindows7()
> +{
> +    return substr(windowsVersion(), 0, 9) eq "Windows 7";
> +}

It seems like it would be safer to check the CurrentVersion value instead of
the ProductName value.

> +sub isWindowsNT()
> +{
> +    return ENV{'OS'} eq 'Windows_NT';
> +}

We shouldn't add this function if we don't use it.

> +sub setupWindowsEnv()
> +{
> +    return if !isCygwin();

You can use "unless isCygwin()" instead of "if !isCygwin()". But again I think
isAppleWinWebKit() is more appropriate than isCygwin(). Maybe we should call
this function setupAppleWinEnv() instead of setupWindowsEnv() to make it
clearer which port it applies to.

> +    # FIXME: The following if statement can probably be replaced with the
simpler:
> +    # if (isWindowsNT()) {
> +    # or, if this works for all versions of windows, then the if statement
can simply be removed.
> +    
> +    if (isWindowsXP() || isWindowsVista() || isWindows7()) {
> +	   my $restart_needed = 0;

Please use camelCase for variable names.

> +	   if (!$ENV{'CYGWIN'}) {
> +	       # This environment variable makes cygwin enable extra support
(i.e., termios)
> +	       # for UNIX-like ttys in the Windows console
> +	       print "Adding the Environment Variable: Cygwin\n\n";

I think it would be useful to print out the value you're setting the variable
to, too.

> +	       system("regtool -s set
'\\HKEY_CURRENT_USER\\Environment\\CYGWIN' 'tty'");

The multi-parameter version of system() would be better to use, like this:

system qw(regtool -s set \\HKEY_CURRENT_USER\\Environment\\CYGWIN tty);

> +	   }
> +
> +	   if (!$ENV{'WEBKITLIBRARIESDIR'}) {
> +	       # This environment variable must be set to be able to build
inside Visual Studio.
> +	       print "Adding the Environment Variable: WebKitLibrariesDir\n";
> +	       system("regtool -s set
'\\HKEY_CURRENT_USER\\Environment\\WEBKITLIBRARIESDIR' '" .
windowsLibrariesDir() . "'");
> +	       $restart_needed = 1;
> +	   }
> +
> +	   if (!$ENV{'WEBKITOUTPUTDIR'}) {
> +	       # This environment variable must be set to be able to build
inside Visual Studio.
> +	       print "Adding the Environment Variable: WebKitOutputDir\n";
> +	       system("regtool -s set
'\\HKEY_CURRENT_USER\\Environment\\WEBKITOUTPUTDIR' '" . windowsOutputDir() .
"'");
> +	       $restart_needed = 1;
> +	   }

It would be nice to reduce the code duplication in this function. Something
like this could work:

my %variablesToSet = ();

$variablesToSet{CYGWIN} = "tty" unless $ENV{CYGWIN};
$variablesToSet{WEBKITLIBRARIESDIR} = ... unless $ENV{WEBKITLIBRARIESDIR};
...

foreach my $variable (keys %variablesToSet) {
    system qw(regtool -s set), "HKEY_CURRENT_USER\\Environment\\" . $variable,
$variablesToSet{$variable};
}

> +	   if ($restart_needed) {
> +	       # A system restart is needed for the changes in environment
variables to take effect.
> +	       print "Please restart your computer before attempting to build
inside Visual Studio.\n\n";
> +	   }

I don't think the comment here is needed.

> +	   if(!$ENV{'WEBKITOUTPUTDIR'}) {

You're missing a space after "if" here.

This is looking pretty good! r- so we can address the above issues.