Bug 175275 – [Win] Detect Visual Studio 2017 location

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Don Olmstead]

Currently the build scripts try to find a Visual Studio install through a standard location. With Visual Studio 2017 there is now a way to detect where a VS install is. This is similar in concept to xcode-select.

At present the build scripts look for the standard location of VS Community Edition. There is also a build tools variant that does not install the IDE which is what we're using for our WinCairo buildbot and will not be picked up at that location.

Information on how to find a VS install is documented at https://blogs.msdn.microsoft.com/vcblog/2017/03/06/finding-the-visual-c-compiler-tools-in-visual-studio-2017/. One option is a redistributable executable vswhere, whose source is MIT licensed and available at https://github.com/Microsoft/vswhere. Checking in a built version of this executable is probably the easiest way forward.

[Don Olmstead]

After talking to Brent on IRC it seems like the safest bet is to actually go and download the vswhere executable during the build and then running that. Executables are built and distributed through GitHub releases so that should be used.

[Stephan Szabo]

Created attachment 319477 [details]
[WIP] Use vswhere to determine visual studio location

Work in progress patch for grabbing vswhere and using it in webkitdirs for getting the install dir, version and msbuild location.

vswhere is limited with its handling of "legacy" pre-2017 versions, as you get a subset of data for those and cannot search by product or requirements, but it will give you a version and install dir.

[Stephan Szabo]

Created attachment 319545 [details]
[WIP] Use vswhere to determine visual studio location

Update to WIP vswhere patch with encoding changes

[Stephan Szabo]

Created attachment 320278 [details]
Use vswhere to determine visual studio location

Now for 2017, will check without products after trying each prioritized product for any other installation that provides MSBuild.

[Stephan Szabo]

Created attachment 320291 [details]
Use vswhere to determine visual studio location

Updating vswhere for isAnyWindows rather than in wincairo block.

[Stephan Szabo]

Created attachment 320661 [details]
Use vswhere to determine visual studio location

Added chmod to update-vswhere script in order to allow execution when downloaded from Cygwin.

[Stephan Szabo]

Created attachment 323538 [details]
Use vswhere to determine visual studio location

Update for changes to webkitdirs.pm

[Fujii Hironori]

You use unnecessary single-quote like:

> $vsWhereFoundInstallation->{'shortVersion'}

But, other codes in webkitdirs.pm look like:

> $device->{state}

[Stephan Szabo]

Created attachment 323909 [details]
Use vswhere to determine visual studio location - 2017 only

[Stephan Szabo]

Created attachment 323930 [details]
Use vswhere to determine visual studio location - choose generator

[Stephan Szabo]

Choose generator patch should pick the 2015 or 2017 generator to cmake based on the version of visual studio found by vswhere.

[Per Arne Vollan]

I have tested the latest patch, and it seems my old MSBuild is being used:

Using Visual Studio: /cygdrive/c/Program Files (x86)/Microsoft Visual Studio/2017/Community
Using Visual Studio 15.3

Using MSBuild: /cygdrive/c/Program Files (x86)/MSBuild/14.0/Bin

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.Cpp.Platform.targets(57,5): error MSB8020: The build tools for v141 (Platform Toolset = 'v141') cannot be found. To build using the v141 build tools, please install v141 build tools.  Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\Projects\WebKit3\OpenSource\WebKitBuild\Release\ZERO_CHECK.vcxproj]

[Per Arne Vollan]

VSWhere thinks MSBuild is located in:

C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/MSBuild/15.3/Bin

but it seems to be located in:

C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/MSBuild/15.0/Bin

[Stephan Szabo]

Created attachment 324032 [details]
Use vswhere to determine visual studio location - choose generator

MSBuild path can use major.0 rather than major.minor, so check for both options.

[Per Arne Vollan]

(In reply to Stephan Szabo from comment #14)
> Created attachment 324032 [details]
> Use vswhere to determine visual studio location - choose generator
> 
> MSBuild path can use major.0 rather than major.minor, so check for both
> options.

Yes, that fixed the problem I had :)

[Per Arne Vollan]

Do we have a fallback in case we for some reason were unable to download vswhere?

[Stephan Szabo]

Currently, there isn't one as the build-jsc and build-webkit scripts have a "or die" on the update lines.

Without dying there, the webkitdirs.pm functions should (and appear to) fall back to the older 2015 constant values if vswhere is missing, with some noise about being unable to run it and being unable to get the location.

So, it seems like we could relax the "or die". Although we might then want to have the 2017/community fallback back in as well?

[Per Arne Vollan]

(In reply to Stephan Szabo from comment #17)
> Currently, there isn't one as the build-jsc and build-webkit scripts have a
> "or die" on the update lines.
> 
> Without dying there, the webkitdirs.pm functions should (and appear to) fall
> back to the older 2015 constant values if vswhere is missing, with some
> noise about being unable to run it and being unable to get the location.
> 
> So, it seems like we could relax the "or die". Although we might then want
> to have the 2017/community fallback back in as well?

Yes, I think we could have a fallback to 2017/community also.

[Stephan Szabo]

Created attachment 325068 [details]
Alternate patch for getting visual studio location w/vswhere

Putting this up to check happiness on bots (w/2015), it's an alternate version which should be more resilient to failing to get vswhere.

It checks the location in your environment if it's there, then uses vswhere for each 2017 and then 2015, then tries a default location for each 2017 (like the old community one but for each) then 2015.

[Stephan Szabo]

Created attachment 325105 [details]
Use vswhere to determine visual studio location

After discussion, add the 2017 fallbacks to the original patch version.

While testing noticed that if you had VSINSTALLDIR set to a 2015 version, you'd still potentially get a 2017 msbuild location and version if vswhere found that (while leaving the install dir pointing to 2015). Similarly, if you had it pointed to a 2017 install and did not have vswhere available, you'd get version 14 because it didn't handle the directory format. This version tries to make it consistent when using VSINSTALLDIR from the environment.

[Per Arne Vollan]

Comment on attachment 325105 [details]
Use vswhere to determine visual studio location

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

> Tools/Scripts/webkitdirs.pm:824
>  sub visualStudioVersion
>  {
>      return $vsVersion if defined $vsVersion;
>  
> -    my $installDir = visualStudioInstallDir();
> +    if ($ENV{'VSINSTALLDIR'}) {
> +        $vsVersion = visualStudioVersionFromInstallDir($ENV{'VSINSTALLDIR'});
> +    }
>  
> -    $vsVersion = ($installDir =~ /Microsoft Visual Studio ([0-9]+\.[0-9]*)/) ? $1 : "14";
> +    if (!defined($vsVersion)) {
> +        pickCurrentVisualStudioInstallation();
> +        if (defined($vsWhereFoundInstallation)) {
> +            $vsVersion = $vsWhereFoundInstallation->{shortVersion};
> +        }
> +    }
> +
> +    if (!defined($vsVersion)) {
> +        pickLegacyVisualStudioInstallation();
> +        if (defined($vsWhereLegacyInstallation)) {
> +            $vsVersion = $vsWhereLegacyInstallation->{shortVersion};
> +        }
> +    }
> +
> +    if (!defined($vsVersion)) {
> +        my $installDir = visualStudioInstallDir();
> +        $vsVersion = visualStudioVersionFromInstallDir($installDir);
> +    }
> +
> +    if (!defined($vsVersion)) {
> +        $vsVersion = "14";
> +    }
>  

Could we implement the function visualStudioVersion as

sub visualStudioVersion
{
    return $vsVersion if defined $vsVersion;

    $installDir = visualStudioInstallDir();
    $vsVersion = visualStudioVersionFromInstallDir($installDir);

    print "Using Visual Studio $vsVersion\n";
    return $vsVersion;
}

since much of this logic is already implemented in the function visualStudioInstallDir?

[Stephan Szabo]

I wasn't sure if it's safe for us to assume that if we get an install directory from an install found by vswhere that it'll be in one of the formats that we can parse a version out of.

[Stephan Szabo]

Created attachment 325179 [details]
Use vswhere to determine visual studio location - simplified

A version of the patch that gets the version from the directory only and similarly simplifies out the msbuild location to just use the version to pick one of the two formats.

[Per Arne Vollan]

Comment on attachment 325179 [details]
Use vswhere to determine visual studio location - simplified

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

> Tools/Scripts/update-vswhere.py:40
> +if result == download.Status.COULD_NOT_FIND:
> +    sys.exit(1)

Are there other results that might indicate an error?

[Stephan Szabo]

The status defined in download-latest-github-release seem to be
    DOWNLOADED = 0
    UP_TO_DATE = 1
    COULD_NOT_FIND = 2

It could fail for other reasons but what I have seen of those appear to come back as raised errors.

[Stephan Szabo]

Created attachment 325206 [details]
Use vswhere to determine visual studio location - removed unneeded numeric conversions

[Per Arne Vollan]

Comment on attachment 325206 [details]
Use vswhere to determine visual studio location - removed unneeded numeric conversions

Thanks, looks good! R=me.

[WebKit Commit Bot]

Comment on attachment 325206 [details]
Use vswhere to determine visual studio location - removed unneeded numeric conversions

Clearing flags on attachment: 325206

Committed r224143: <https://trac.webkit.org/changeset/224143>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

*** Bug 178327 has been marked as a duplicate of this bug. ***

[Lucas Forschler]

Note: this broke webkitbot and WKR, as the perl libraries added in webkitdirs.pm were not installed (I have fixed the WKR/WebKitBot nodes)

[Yusuke Suzuki]

(In reply to Lucas Forschler from comment #31)
> Note: this broke webkitbot and WKR, as the perl libraries added in
> webkitdirs.pm were not installed (I have fixed the WKR/WebKitBot nodes)

Yeah, it includes Encode/Locale, which is not bundled in macOS standard perl.
https://github.com/mozilla/arewefastyet/issues/182

[Per Arne Vollan]

(In reply to Lucas Forschler from comment #31)
> Note: this broke webkitbot and WKR, as the perl libraries added in
> webkitdirs.pm were not installed (I have fixed the WKR/WebKitBot nodes)

Thanks for fixing this, Lucas!

[Yusuke Suzuki]

Could you remove these dependencies in build-webkit/build-jsc in macOS?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 179091

[Stephan Szabo]

Created attachment 325591 [details]
Use vswhere to determine visual studio location - make new modules require only in new paths

Move the three new modules as requires in the vswhere code paths so they shouldn't be loaded except when trying to find visual studio.

[Yusuke Suzuki]

Comment on attachment 325591 [details]
Use vswhere to determine visual studio location - make new modules require only in new paths

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

> Tools/Scripts/webkitdirs.pm:600
> +    require Encode;
> +    require Encode::Locale;
> +    require JSON::PP;

Let's extract this part to a subroutine.

> Tools/Scripts/webkitdirs.pm:639
> +    require Encode;
> +    require Encode::Locale;
> +    require JSON::PP;

Ditto.

[Stephan Szabo]

Created attachment 325598 [details]
Use vswhere to determine visual studio location - move requires into sub

[WebKit Commit Bot]

Comment on attachment 325598 [details]
Use vswhere to determine visual studio location - move requires into sub

Clearing flags on attachment: 325598

Committed r224291: <https://trac.webkit.org/changeset/224291>

[Radar WebKit Bug Importer]

<rdar://problem/35568974>