[Webkit-unassigned] [Bug 31228] Windows environment variables should be set automatically

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31228


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42704|review?                     |review-
               Flag|                            |




--- Comment #3 from Adam Roben (aroben) <aroben at apple.com>  2009-11-09 09:10:28 PDT ---
(From update of attachment 42704)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.