Bug 175275

Summary: [Win] Detect Visual Studio 2017 location
Product: WebKit Reporter: Don Olmstead <don.olmstead>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, commit-queue, dbates, Hironori.Fujii, lforschler, pvollan, stephan.szabo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 175279, 179091    
Bug Blocks:    
Attachments:
Description Flags
[WIP] Use vswhere to determine visual studio location
none
[WIP] Use vswhere to determine visual studio location
none
Use vswhere to determine visual studio location
none
Use vswhere to determine visual studio location
none
Use vswhere to determine visual studio location
none
Use vswhere to determine visual studio location
none
Use vswhere to determine visual studio location - 2017 only
none
Use vswhere to determine visual studio location - choose generator
none
Use vswhere to determine visual studio location - choose generator
none
Alternate patch for getting visual studio location w/vswhere
none
Use vswhere to determine visual studio location
none
Use vswhere to determine visual studio location - simplified
none
Use vswhere to determine visual studio location - removed unneeded numeric conversions
none
Use vswhere to determine visual studio location - make new modules require only in new paths
ysuzuki: review+
Use vswhere to determine visual studio location - move requires into sub none

Description Don Olmstead 2017-08-07 11:28:48 PDT
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.
Comment 1 Don Olmstead 2017-08-07 12:53:40 PDT
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.
Comment 2 Stephan Szabo 2017-08-31 10:42:07 PDT
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.
Comment 3 Stephan Szabo 2017-08-31 17:12:43 PDT
Created attachment 319545 [details]
[WIP] Use vswhere to determine visual studio location

Update to WIP vswhere patch with encoding changes
Comment 4 Stephan Szabo 2017-09-08 10:56:11 PDT
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.
Comment 5 Stephan Szabo 2017-09-08 12:52:45 PDT
Created attachment 320291 [details]
Use vswhere to determine visual studio location

Updating vswhere for isAnyWindows rather than in wincairo block.
Comment 6 Stephan Szabo 2017-09-13 11:22:32 PDT
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.
Comment 7 Stephan Szabo 2017-10-12 11:14:24 PDT
Created attachment 323538 [details]
Use vswhere to determine visual studio location

Update for changes to webkitdirs.pm
Comment 8 Fujii Hironori 2017-10-15 22:40:01 PDT
You use unnecessary single-quote like:

> $vsWhereFoundInstallation->{'shortVersion'}

But, other codes in webkitdirs.pm look like:

> $device->{state}
Comment 9 Stephan Szabo 2017-10-16 11:05:14 PDT
Created attachment 323909 [details]
Use vswhere to determine visual studio location - 2017 only
Comment 10 Stephan Szabo 2017-10-16 12:57:25 PDT
Created attachment 323930 [details]
Use vswhere to determine visual studio location - choose generator
Comment 11 Stephan Szabo 2017-10-16 15:50:39 PDT
Choose generator patch should pick the 2015 or 2017 generator to cmake based on the version of visual studio found by vswhere.
Comment 12 Per Arne Vollan 2017-10-17 09:38:10 PDT
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]
Comment 13 Per Arne Vollan 2017-10-17 09:56:25 PDT
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
Comment 14 Stephan Szabo 2017-10-17 11:19:12 PDT
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.
Comment 15 Per Arne Vollan 2017-10-23 16:45:28 PDT
(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 :)
Comment 16 Per Arne Vollan 2017-10-25 20:09:36 PDT
Do we have a fallback in case we for some reason were unable to download vswhere?
Comment 17 Stephan Szabo 2017-10-26 10:55:24 PDT
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?
Comment 18 Per Arne Vollan 2017-10-26 11:37:01 PDT
(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.
Comment 19 Stephan Szabo 2017-10-26 15:52:36 PDT
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.
Comment 20 Stephan Szabo 2017-10-26 20:02:46 PDT
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.
Comment 21 Per Arne Vollan 2017-10-27 09:07:55 PDT
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?
Comment 22 Stephan Szabo 2017-10-27 10:27:02 PDT
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.
Comment 23 Stephan Szabo 2017-10-27 10:52:00 PDT
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.
Comment 24 Per Arne Vollan 2017-10-27 12:02:26 PDT
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?
Comment 25 Stephan Szabo 2017-10-27 14:12:32 PDT
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.
Comment 26 Stephan Szabo 2017-10-27 15:25:29 PDT
Created attachment 325206 [details]
Use vswhere to determine visual studio location - removed unneeded numeric conversions
Comment 27 Per Arne Vollan 2017-10-27 15:58:01 PDT
Comment on attachment 325206 [details]
Use vswhere to determine visual studio location - removed unneeded numeric conversions

Thanks, looks good! R=me.
Comment 28 WebKit Commit Bot 2017-10-27 20:49:13 PDT
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>
Comment 29 WebKit Commit Bot 2017-10-27 20:49:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Fujii Hironori 2017-10-28 16:54:55 PDT
*** Bug 178327 has been marked as a duplicate of this bug. ***
Comment 31 Lucas Forschler 2017-10-30 13:59:14 PDT
Note: this broke webkitbot and WKR, as the perl libraries added in webkitdirs.pm were not installed (I have fixed the WKR/WebKitBot nodes)
Comment 32 Yusuke Suzuki 2017-10-30 22:40:58 PDT
(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
Comment 33 Per Arne Vollan 2017-10-31 07:48:22 PDT
(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!
Comment 34 Yusuke Suzuki 2017-10-31 16:55:22 PDT
Could you remove these dependencies in build-webkit/build-jsc in macOS?
Comment 35 WebKit Commit Bot 2017-10-31 17:26:14 PDT
Re-opened since this is blocked by bug 179091
Comment 36 Stephan Szabo 2017-11-01 10:55:14 PDT
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.
Comment 37 Yusuke Suzuki 2017-11-01 11:05:01 PDT
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.
Comment 38 Stephan Szabo 2017-11-01 11:23:19 PDT
Created attachment 325598 [details]
Use vswhere to determine visual studio location - move requires into sub
Comment 39 WebKit Commit Bot 2017-11-01 13:44:58 PDT
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>
Comment 40 Radar WebKit Bug Importer 2017-11-15 13:11:50 PST
<rdar://problem/35568974>