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.
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.
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.
Created attachment 319545 [details] [WIP] Use vswhere to determine visual studio location Update to WIP vswhere patch with encoding changes
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.
Created attachment 320291 [details] Use vswhere to determine visual studio location Updating vswhere for isAnyWindows rather than in wincairo block.
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.
Created attachment 323538 [details] Use vswhere to determine visual studio location Update for changes to webkitdirs.pm
You use unnecessary single-quote like: > $vsWhereFoundInstallation->{'shortVersion'} But, other codes in webkitdirs.pm look like: > $device->{state}
Created attachment 323909 [details] Use vswhere to determine visual studio location - 2017 only
Created attachment 323930 [details] Use vswhere to determine visual studio location - choose generator
Choose generator patch should pick the 2015 or 2017 generator to cmake based on the version of visual studio found by vswhere.
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]
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
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.
(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 :)
Do we have a fallback in case we for some reason were unable to download vswhere?
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?
(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.
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.
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 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?
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.
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 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?
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.
Created attachment 325206 [details] Use vswhere to determine visual studio location - removed unneeded numeric conversions
Comment on attachment 325206 [details] Use vswhere to determine visual studio location - removed unneeded numeric conversions Thanks, looks good! R=me.
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>
All reviewed patches have been landed. Closing bug.
*** Bug 178327 has been marked as a duplicate of this bug. ***
Note: this broke webkitbot and WKR, as the perl libraries added in webkitdirs.pm were not installed (I have fixed the WKR/WebKitBot nodes)
(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
(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!
Could you remove these dependencies in build-webkit/build-jsc in macOS?
Re-opened since this is blocked by bug 179091
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 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.
Created attachment 325598 [details] Use vswhere to determine visual studio location - move requires into sub
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>
<rdar://problem/35568974>