Bug 31228 – Windows environment variables should be set automatically

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Marwan Al Jubeh]

The WebKitOutputDir and WebKitLibrariesDir environment variables have to be set to be able to build inside Visual Studio. They currently have to be set manually. They should be set automatically as part of running update-webkit. This has previously been suggested here:
http://trac.webkit.org/wiki/ImprovingLifeOnWindows#SettheWebKitOutputDirandWebKitLibrariesDirenvironmentvariablesautomatically

Also, the Cygwin environment variable should be set automatically to enable extra support (i.e., termios) for UNIX-like ttys in the Windows console. This is needed for some programs to work, like emacs for example.

[Marwan Al Jubeh]

Created attachment 42701 [details]
Automatically set the environment variables by editing HKCU\Environment.

[Marwan Al Jubeh]

Created attachment 42704 [details]
Automatically set the environment variables by editing HKCU\Environment.

fixed minor issues with variable scopes.

[Adam Roben (:aroben)]

Comment on attachment 42704 [details]
Automatically set the environment variables by editing HKCU\Environment.

Thanks for taking this on!

> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> [phone]  Marwan Al Jubeh  <[e-mail]>
> +
> +        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.

[Marwan Al Jubeh]

Created attachment 43043 [details]
Automatically set the environment variables by editing HKCU\Environment.

(In reply to comment #3)
> > +sub isWindowsNT()
> > +{
> > +    return ENV{'OS'} eq 'Windows_NT';
> > +}
> 
> We shouldn't add this function if we don't use it.

The reason why I added this function is because I was expecting this patch to work for all Windows versions from the Windows NT family (which includes all the most recent versions of Windows, including Windows 7). However, at the time I was unable to find any documentation to support that and didn't have access except to Windows XP, Vista and Windows 7.

However, in the past few days I managed to dig up some documentation from Microsoft's websites which confirmed my expectations.

According to those two websites, we should be able to safely assume that we can use HKEY_CURRENT_USER\Environment to modify user environment variables for all the Operating Systems in the Windows NT family (in addition to Windows 98 and Windows ME as well):

http://support.microsoft.com/kb/104011
http://msdn.microsoft.com/en-us/library/system.environmentvariabletarget(VS.100).aspx

As for the function isWindowsNT() itself, I verified that it works for Windows XP, Vista and Windows 7. And according to the following websites, it should work for Windows 2000, Windows 2003 and others:
http://support.microsoft.com/kb/190899
http://www.scriptlogic.com/support/CustomScripts/environmentVariableReference.html

Therefore, I decided to get rid of the other functions that detect the version of Windows and just use this one because it encapsulates all of them. If necessary, I can add functions that test for Windows 98 and Windows ME, but I would be surprised if anyone still uses them.

I also changed WebkitSite so that it would reflect the changes introduced by the patch.

[Adam Roben (:aroben)]

(In reply to comment #4)
> According to those two websites, we should be able to safely assume that we can
> use HKEY_CURRENT_USER\Environment to modify user environment variables for all
> the Operating Systems in the Windows NT family (in addition to Windows 98 and
> Windows ME as well):
> 
> http://support.microsoft.com/kb/104011
> http://msdn.microsoft.com/en-us/library/system.environmentvariabletarget(VS.100).aspx

If HKCU\Environment works for all those versions of Windows, it seems like we don't need to check the Windows version at all. I don't think we need to worry about supporting versions of Windows older than Windows 98!

[Adam Roben (:aroben)]

Comment on attachment 43043 [details]
Automatically set the environment variables by editing HKCU\Environment.

> +        if ($restartNeeded) {
> +            print "Please restart your computer before attempting to build inside Visual Studio.\n\n";
> +        }

I think logging out and back in would also work. Maybe we should change the message to say that?

And it sounds like we could skip the isWindowsNT() check entirely (see comment 5).

r=me even if those two things don't get changed. Thanks for fixing this!

[Adam Barth]

Comment on attachment 43043 [details]
Automatically set the environment variables by editing HKCU\Environment.

aroben has more comments.

[Marwan Al Jubeh]

(In reply to comment #5)
> If HKCU\Environment works for all those versions of Windows, it seems like we
> don't need to check the Windows version at all. I don't think we need to worry
> about supporting versions of Windows older than Windows 98!

I think that it would still be a good idea to check the version of Windows. This is because if a version of Windows that doesn't support HKCU\Environment is ever released, having update-webkit not set the environment variables and issue a warning telling the user to do so would be much less harmful and easier to detect and fix than having it not issue the warning and not compile. I know that this is very unlikely, but I would prefer to err on the side of caution.

[Eric Seidel (no email)]

Aroben, do you have more comments, or can we land this?

[Adam Roben (:aroben)]

Comment on attachment 43043 [details]
Automatically set the environment variables by editing HKCU\Environment.

Sure, let's land it.

[WebKit Commit Bot]

Comment on attachment 43043 [details]
Automatically set the environment variables by editing HKCU\Environment.

Clearing flags on attachment: 43043

Committed r51932: <http://trac.webkit.org/changeset/51932>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.