Bug 126317 - webkitdirs.pm does not use environment variable VSINSTALLDIR
Summary: webkitdirs.pm does not use environment variable VSINSTALLDIR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-30 17:43 PST by David Delaune
Modified: 2014-01-02 11:59 PST (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Delaune 2013-12-30 17:43:43 PST
Hi,

This change: https://bugs.webkit.org/show_bug.cgi?id=125998

Makes no sense whatsoever... it hard-codes the Visual Studio path using the environment variables %PROGRAMFILES(X86)% or %PROGRAMFILES% and yet ignores the environment variable %VSINSTALLDIR%.

This change incorrectly assumes that Visual Studio is always installed to (%PROGRAMFILES(X86)% or %PROGRAMFILES% or "C:\\Program Files").
Visual Studio may be installed on any drive... C through Z.
In my case... my OS is on drive C: and Visual Studio is installed on D: and this change completely breaks compiling.

What exactly was fixed here? Looks like a fix for 1 person?
Comment 1 Brent Fulgham 2013-12-30 18:20:02 PST
Visual Studio no longer sets VSINSTALLDIR, so checking for this incorrectly causes the build system to use VS2005 or earlier. We do not want that.

This fix was needed to support our build farm. I'm surprised it caused any issues for you; do you not have VS2013 installed?

What error messages did you get when attempting to build?
Comment 2 David Delaune 2013-12-30 21:55:53 PST
Hi,

What makes you think Visual Studio has been changed to no longer set the %VSINSTALLDIR% variable?

Visual Studio 2013 (Version 12) does set the %VSINSTALLDIR% environment variable.

Best Wishes,
-David Delaune
Comment 3 David Delaune 2013-12-31 12:47:11 PST
(In reply to comment #1) 
> This fix was needed to support our build farm. I'm surprised it caused any issues for you; do you not have VS2013 installed?

So it is exactly as I suspected. This quick hack was added to fix a single-user scenario while potentially breaking compilations for the rest of the world.

Someone should invest 5 minutes looking into this. It only takes a few minutes to peruse vcvarsall.bat where you will find that depending on the architecture it will call \VC\bin\vcvars[*].bat which calls "%VS120COMNTOOLS%VCVarsQueryRegistry.bat" to set %VSINSTALLDIR%.

or perhaps if you only have 10 seconds to invest you could open a Visual Studio command prompt and type 'echo %VSINSTALLDIR%'

If you really want to check the version then you should consider taking advantage of the %VISUALSTUDIOVERSION% environment variable.

Best Wishes,
-David Delaune
Comment 4 Brent Fulgham 2013-12-31 17:06:14 PST
It sounds like we have to execute the VSVARSALL.BAT script to (temporarily) set the VSINSTALLDIR environment variable. I don't see how executing this script as part of our build process is any better than coding the path to the required Visual Studio installation.

We could certainly manually create a VSINSTALLDIR environment variable, but I'm not sure what problem we are solving by doing so.

Can you be specific about the issue you are actually having?

* Are you trying to build with something besides VS3013?
* Did you install VS2013 in an unusual location and are having a problem? If so, what errors are you seeing?

I work with several other Windows developers, who had no problem with this change. Please provide specific error messages and build logs so we can deal with the actual problem you are encountering.
Comment 5 David Delaune 2013-12-31 20:34:29 PST
Hey Brent,

Surely you remember me? I am the guy who worked and created the WinCairoRequirements Visual Studio project that you graciously uploaded to github.

(In reply to comment #4)
> It sounds like we have to execute the VSVARSALL.BAT script
> to (temporarily) set the VSINSTALLDIR environment variable.
> I don't see how executing this script as part of our build
> process is any better than coding the path to the required
> Visual Studio installation.


How is it even possible that you are not understanding this? The patch hard codes the path to Visual Studio to either the OS drive or the C drive. If Visual Studio is installed anywhere else they will be unable to build.


> 
> We could certainly manually create a VSINSTALLDIR environment
> variable, but I'm not sure what problem we are solving by doing so.


The VSVARSALL.BAT script batch file sets several environment variables, PATH, INCLUDE, LIB and LIBPATH. It also adds the path to Windows SDK and corresponding toolsets.


> 
> Can you be specific about the issue you are actually having?
> 


Unable to compile without fixing webkitdirs.pm library.


> * Are you trying to build with something besides VS3013?


I am obviously compiling with Visual Studio 2013 (Version 12)

> * Did you install VS2013 in an unusual location and are having
> a problem? If so, what errors are you seeing?


Define unusual. My workstation has Visual Studio 2013 installed on the D: drive. The operating system is on the C: drive. I mention this in the first comment. I find it disturbing that you do not know what errors occur when the build script cannot find the compiler. I am beginning to wonder if you are being pretensive.

> 
> I work with several other Windows developers, who had no problem with this change. Please provide specific error messages and build logs so we can deal with the actual problem you are encountering.

They would certainly be able to compile if Visual Studio installed on either the C: drive or OS drive.

Best Wishes,
-David Delaune
Comment 6 Roger Fong 2014-01-02 11:59:49 PST
Rolled out r160846 here: https://bugs.webkit.org/show_bug.cgi?id=126395