Bug 107714 – [Windows] Update WebKitDirs.pm for new Windows paths

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brent Fulgham]

Some environment variable names were changed in Bug 106949 to reduce clutter and crazy 5-level "up directory" pathing.

The WebKit script tools need to be updated to make sure the developer's environment has the proper variables set:

WEBKITLIBRARIES -> WEBKIT_LIBRARIES
WEBKITOUTPUTDIR -> WEBKIT_OUTPUTDIR
WEBKIT_TESTFONTS (stayed the same)

Added WEBKIT_SOURCE to point to Source directory of webkit checkout

[Brent Fulgham]

Created attachment 187508 [details]
Patch

[Brent Fulgham]

This first patch updates webkitdirs.pm so that the new environment variables are properly set for both VS2005 and VS2010 builds.

A future update will remove the VS2005 support, but not until the build machines have switched to the new compiler.

[Brent Fulgham]

Created attachment 187511 [details]
Patch

[Brent Fulgham]

I made a mistake in the setting of the WebKit_Source environment variable.  It needs to be of the actual "Source" sub-directory of the WebKit source tree.

[Daniel Bates]

Comment on attachment 187511 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187511&action=review

For completeness, we should look to update the following web pages that reference the environment variables WEBKITOUTPUTDIR or WEBKITLIBRARIESDIR:

Building WebKit, <http://www.webkit.org/building/build.html>
Apple Windows Port/Building on Windows, <https://trac.webkit.org/wiki/BuildingOnWindows>
Cairo-based Windows Port/Building Cairo on Windows, <https://trac.webkit.org/wiki/BuildingCairoOnWindows>
Building the Qt port on Linux, <https://trac.webkit.org/wiki/BuildingQtOnLinux>
Building Qt On OSX , <https://trac.webkit.org/wiki/BuildingQtOnOSX>
TinOS's WebKit Port/Building on Windows Embedded CE, <https://trac.webkit.org/wiki/TinOS> (is the information on this page still applicable?)
Setting Up Development Environment For N9, <https://trac.webkit.org/wiki/SettingUpDevelopmentEnvironmentForN9> 
EFL Port of WebKit, <https://trac.webkit.org/wiki/EFLWebKit>

> Tools/ChangeLog:3
> +        Update WebKitDirs.pm for new Windows Paths

Nit: Paths => paths

> Tools/Scripts/webkitdirs.pm:190
> +    $baseProductDir = $ENV{"WEBKITOUTPUTDIR"}; #Fixme: Switch to WEBKIT_OUTPUTDIR in future

Nit: "#Fixme:" => "# FIXME: "

(Notice the space character after the '#' and the use of capitals).

I also suggest making this comment a full sentence by adding the word "the" before the word "future" and ending the comment with a period.

Do we have an idea of a timetable for addressing this FIXME? If we don't have a timetable or the timetable is far out then we may want to file a bug to address this FIXME and reference the bug number in this comment.

> Tools/Scripts/webkitdirs.pm:1646
> +sub windowsSourceSourceDir()

I wish we could up with a better name for this function given that the word "Source" has two different meanings: the top-level WebKit directory and the sub-directory whose name is Source. Although the name of this function is clarified by reading its implementation, which is concise.

> Tools/Scripts/webkitdirs.pm:1697
> +            # VS2005 Version.  This will go away in a future commit.

Nit: Version => version

(The word "Version" also appears on line 1711).

> Tools/Scripts/webkitdirs.pm:1719
> +        if (!$ENV{'WEBKIT_LIBRARIES'}) {
> +            # New VS2010 support. Will replace VS2005 version in the future.
> +            print "Warning: You must set the 'WebKit_Libraries' environment variable\n";
> +            print "         to be able build WebKit from within Visual Studio.\n";
> +            print "         Make sure that 'WebKit_Libraries' points to the\n";
> +            print "         'WebKitLibraries/win' directory, not the 'WebKitLibraries/' directory.\n\n";
> +        }
>          if (!$ENV{'WEBKITOUTPUTDIR'}) {
> +            # VS2005 Version.  This will go away in a future commit.
>              print "Warning: You must set the 'WebKitOutputDir' environment variable\n";
>              print "         to be able build WebKit from within Visual Studio.\n\n";
>          }
> +        if (!$ENV{'WEBKIT_OUTPUTDIR'}) {
> +            # New VS2010 support. Will replace VS2005 version in the future.
> +            print "Warning: You must set the 'WebKit_OutputDir' environment variable\n";
> +            print "         to be able build WebKit from within Visual Studio.\n\n";
> +        }

Did you intend to emit warnings about missing environment variables WEBKIT_LIBRARIES and WEBKIT_OUTPUTDIR when building in an environment that defines WEBKITOUTPUTDIR and WEBKITLIBRARIESDIR?

[Brent Fulgham]

Created attachment 187647 [details]
Patch

[Brent Fulgham]

Thanks for taking the time to review this change!

(In reply to comment #5)
> For completeness, we should look to update the following web pages that reference the environment variables WEBKITOUTPUTDIR or WEBKITLIBRARIESDIR:

Good point.  I created Bug 109478 to track this.

> Do we have an idea of a timetable for addressing this FIXME? If we don't have a timetable or the timetable is far out then we may want to file a bug to address this FIXME and reference the bug number in this comment.

I created Bug 109472 to track removing the VS2005 cruft once the build system cuts over to VS2010+.

> > Tools/Scripts/webkitdirs.pm:1646
> > +sub windowsSourceSourceDir()
> 
> I wish we could up with a better name for this function given that the word "Source" has two different meanings: the top-level WebKit directory and the sub-directory whose name is Source. Although the name of this function is clarified by reading its implementation, which is concise.

Yes, I couldn't think of anything better.  The existing "windowsSourceDir" could perhaps be renamed to "windowsSourceRootDir", or maybe just "windowsRootDir", but I didn't think it was appropriate to change the existing naming.

> Did you intend to emit warnings about missing environment variables WEBKIT_LIBRARIES and WEBKIT_OUTPUTDIR when building in an environment that defines WEBKITOUTPUTDIR and WEBKITLIBRARIESDIR?

Yes.  The library doesn't currently distinguish what kind of build (VS2005 or VS2010) is currently being done, so I thought it would be best to just require both sets of variables, thereby ensuring everyone will be capable of building using either toolset.  Existing users will already have the 'old' environment variables set, and will now get the 'new' values added to their environment.  New users will (regrettably) get the 'old' values in addition to the 'new', until we cut over, but that seems like a small price for the convenience this script provides.

All of your other comments have been incorporated into my latest patch.

[Daniel Bates]

(In reply to comment #7)
> [...]
> New users will (regrettably) get the 'old' values in addition to the 'new', until we cut over, but that seems like a small price for the convenience this script provides.

OK. It would be nice if we could determine whether Visual Studio 2010 is installed so that we don't instruct people with such installations to set the deprecated environment variables WEBKITOUTPUTDIR and WEBKITLIBRARIESDIR. Having said that, as we work to deprecate Visual Studio 2005 we will be able to clean up WebKitDirs.pm.

[Daniel Bates]

Comment on attachment 187647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187647&action=review

> Tools/Scripts/webkitdirs.pm:1702
>              print "Warning: You must set the 'WebKitLibrariesDir' environment variable\n";
>              print "         to be able build WebKit from within Visual Studio.\n";
>              print "         Make sure that 'WebKitLibrariesDir' points to the\n";
>              print "         'WebKitLibraries/win' directory, not the 'WebKitLibraries/' directory.\n\n";

For your consideration I suggest we make it clear that this warning is applicable when building with Visual Studio 2005 by explicitly mentioning "Visual Studio 2005" similar to what you did in the Visual Studio 2010 warning messages (below).

> Tools/Scripts/webkitdirs.pm:1715
> +            # VS2005 version.  This will be removed as part of https://bugs.webkit.org/show_bug.cgi?id=109472.
>              print "Warning: You must set the 'WebKitOutputDir' environment variable\n";
>              print "         to be able build WebKit from within Visual Studio.\n\n";

Ditto.

[Brent Fulgham]

Committed r142692: <http://trac.webkit.org/changeset/142692>