Bug 196622 – [Win] build-webkit should be ready for Visual Studio 2019

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Stephan Szabo]

Because later versions of Visual Studio 2017 did not successfully compile the WinCairo port and Visual Studio 2019 has now been released, update to support VS2019.

Use the VS2019 generator for WinCairo, continue using the VS2017 generator for other ports
When looking for Visual Studio instances with vswhere pass a version argument to ask for VS2019 for WinCairo and 2017 for other ports
When looking for MSBuild, look in "MSBuild\Current\bin" (the new expected location) and fallback to "MSBuild\15.0\bin" if we can't find an MSBuild.exe in the first location.

[Stephan Szabo]

Created attachment 366748 [details]
[WinCairo] Update to VS2019

[Ross Kirsling]

🥳🎉🎊

[Don Olmstead]

Do we also need to set the cmake minimum version to 3.14 for us?

[Stephan Szabo]

I'm not sure we can reasonably do that only for the wincairo port because I think that's pretty much got to be the first thing in the script.

[Stephan Szabo]

Although the build error on win seems unrelated, it does look like the quoting is off for the vswhere version expression, fixing.

[Stephan Szabo]

Created attachment 366762 [details]
[WinCairo] Update to VS2019

[Don Olmstead]

Comment on attachment 366762 [details]
[WinCairo] Update to VS2019

r=me assuming bots pass. Not sure if you want to put the version range into a sub but I don't have a strong opinion on that.

[Fujii Hironori]

Comment on attachment 366762 [details]
[WinCairo] Update to VS2019

I think this doesn’t work. https://bugs.webkit.org/show_bug.cgi?id=190797

[Stephan Szabo]

It seems to me that the same condition of using different versions could happen with VSINSTALLDIR as well as cmake doesn't always obey that variable as far as I can see from https://gitlab.kitware.com/cmake/cmake/blob/master/Source/cmVSSetupHelper.cxx

[Stephan Szabo]

That aside, is the vswhere portion the only portion you're objecting to? There are three pieces to this:

1) Change the generator to the 2019 generator for wincairo
2) Add the version argument for vswhere to match the generator chosen
3) Check the new location for msbuild under the found installation because the scripts call to msbuild directly?

If the second were unacceptable, are the first and third okay (with the use of the environment variable for choosing the installation)?

[Fujii Hironori]

Comment on attachment 366762 [details]
[WinCairo] Update to VS2019

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

> Tools/Scripts/webkitdirs.pm:669
> +        $versionRange = "[15.0,16.0)";

There is no reason to force AppleWin to use VS2017. WinCairo and AppleWin ports should use '-latest'. Only PlayStation port has the reason to use VS2017 at the moment.

[Fujii Hironori]

(In reply to Stephan Szabo from comment #9)
> It seems to me that the same condition of using different versions could
> happen with VSINSTALLDIR as well as cmake doesn't always obey that variable
> as far as I can see from
> https://gitlab.kitware.com/cmake/cmake/blob/master/Source/cmVSSetupHelper.cxx

What is your idea addressing the version mismatch issue (Bug 190797 comment 21)?

[Fujii Hironori]

Comment on attachment 366762 [details]
[WinCairo] Update to VS2019

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

> Tools/Scripts/webkitdirs.pm:2270
> +        push @args, '-DCMAKE_GENERATOR_TOOLSET="host=x64"';

VS2019 can build VS2017 project. You don’t need to upgrade at the moment for someone wants to use vs2017.

[Fujii Hironori]

Wincairo buildbot is still using vs2017.

[Don Olmstead]

(In reply to Fujii Hironori from comment #14)
> Wincairo buildbot is still using vs2017.

I have docker images that have 2019 ready to go so this won't be a problem. EWS bots are already using them

[Stephan Szabo]

(In reply to Fujii Hironori from comment #13)
> Comment on attachment 366762 [details]
> [WinCairo] Update to VS2019
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366762&action=review
> 
> > Tools/Scripts/webkitdirs.pm:2270
> > +        push @args, '-DCMAKE_GENERATOR_TOOLSET="host=x64"';
> 
> VS2019 can build VS2017 project. You don’t need to upgrade at the moment for
> someone wants to use vs2017.

Don't we still have the problem that the most current versions of VS2017 don't build the wincairo port successfully? That was the point of forcing the upgrade for wincairo to avoid those issues.

[Stephan Szabo]

(In reply to Fujii Hironori from comment #12)
> (In reply to Stephan Szabo from comment #9)
> > It seems to me that the same condition of using different versions could
> > happen with VSINSTALLDIR as well as cmake doesn't always obey that variable
> > as far as I can see from
> > https://gitlab.kitware.com/cmake/cmake/blob/master/Source/cmVSSetupHelper.cxx
> 
> What is your idea addressing the version mismatch issue (Bug 190797 comment
> 21)?

Can we agree on the following?
A) If one runs the vcvars script in the shell, we shouldn't use vswhere to find one.
B) Therefore, the only time we use vswhere is when the vcvars environment is not set up and we have to try to find one
C) If vcvars was not run, setting VSINSTALLDIR is insufficient, so the use of VSINSTALLDIR alone in the script is itself insufficient to guarantee matching:
===
C:\github\tests\cmake-basic-test\foo>set VSINSTALLDIR=C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\

C:\github\tests\cmake-basic-test\foo>cmake -G "Visual Studio 15" ..
-- The C compiler identification is MSVC 19.14.26433.0
-- The CXX compiler identification is MSVC 19.14.26433.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/VC/Tools/MSVC/14.14.26428/bin/Hostx86/x86/cl.exe
===

If we can agree on the above, then my thought sequence would be:
D) We'd like to cover as many cases as possible
E) And we would like to make it easy to be able to switch between ports without requiring the user the also switch between shells.
F) So, we try to find a compiler that'll match as best we can for the case that the user isn't giving us a configured environment
G) And we allow users to use vcvars shells to cover any cases we've not yet covered explicitly.
H) And we try to over time minimize the cases where the difference occurs.

[Stephan Szabo]

As an example of one option for H would be to find the vcvars for the found instance of visual studio and run that in the subshell that's running cmake when we found one via vswhere.

[Fujii Hironori]

(In reply to Stephan Szabo from comment #16)
> 
> Don't we still have the problem that the most current versions of VS2017
> don't build the wincairo port successfully? That was the point of forcing
> the upgrade for wincairo to avoid those issues.

For example, if I will have a compiler issue in vs2019, I will want to compile with vs2017 for investigation. We should have a way to choose compiler for such case.

[Stephan Szabo]

(In reply to Fujii Hironori from comment #19)
> (In reply to Stephan Szabo from comment #16)
> > 
> > Don't we still have the problem that the most current versions of VS2017
> > don't build the wincairo port successfully? That was the point of forcing
> > the upgrade for wincairo to avoid those issues.
> 
> For example, if I will have a compiler issue in vs2019, I will want to
> compile with vs2017 for investigation. We should have a way to choose
> compiler for such case.

How common is this case? I don't have a good sense of whether it's common enough for it to make sense for the script to support rather than calling cmake manually with a different generator specified.

If it is common, then specifying the 15 generator unconditionally would seem not be the desired choice (as cmake appears to use the specified generator to influence its choice of installation) but probably instead to choose the generator based on the chosen installation. In a case where you explicitly wanted to use 2017 for investigation, would it be safe to only choose that if you were in a 2017 vcvars environment? And should we warn about it for users who wouldn't know that they may get a failing compilation due to recent 2017 instances not building the tree?

[Fujii Hironori]

(In reply to Stephan Szabo from comment #17)

> E) And we would like to make it easy to be able to switch between ports
> without requiring the user the also switch between shells.

I’m against (E).
I’m suggesting invoking vcvars in outside of build-WebKit to choose compiler.

[Stephan Szabo]

(In reply to Fujii Hironori from comment #21)
> (In reply to Stephan Szabo from comment #17)
> 
> > E) And we would like to make it easy to be able to switch between ports
> > without requiring the user the also switch between shells.
> 
> I’m against (E).
> I’m suggesting invoking vcvars in outside of build-WebKit to choose compiler.

I can't tell if you're suggesting removing all the fallbacks for when vcvars wasn't run (for example VSINSTALLDIR is not set). 

If you aren't then we still have to deal with those cases somehow.

If you are suggesting removing the fallbacks, that seems to change a long standing behavior of the scripts as I see those going back at least 6 years to VS8.

In either case, it seems like current cmake (3.14) uses the generator version as input to how at least some of the code paths choose a Visual Studio instance, so it's unclear to me that leaving the script using the 2017 generator while having both 2017 and 2019 installed will have the desired behavior.

[Fujii Hironori]

(In reply to Stephan Szabo from comment #22)
> I can't tell if you're suggesting removing all the fallbacks for when vcvars
> wasn't run (for example VSINSTALLDIR is not set). 
> If you aren't then we still have to deal with those cases somehow.

We shouldn't remove the fallback. build-webkit should choose the latest installed VS as the fallback.
I'm not sure which is better to choose the latest VS by using vswhere or using CMake (I think CMake can find the latest installed VS. Right?).
 
> In either case, it seems like current cmake (3.14) uses the generator
> version as input to how at least some of the code paths choose a Visual
> Studio instance, so it's unclear to me that leaving the script using the
> 2017 generator while having both 2017 and 2019 installed will have the
> desired behavior.

Good point. I'll check.

[Fujii Hironori]

I was completely misunderstanding how CMake Visual Studio Generators choose a compiler.
I tested by installing the following three instances:

1. VS2019 Professional 16.0.0
2. VS2017 Professional 15.7.6
3. VS2017 Build Tool 15.8.9

Without -G switch, it chooses the latest ... (1)
This can not be overridden by invoking vcvars ... CMake error
With -G "Visual Studio 15 2017", it choose the latest VS2017 ... (3)
This can be overridden by invoking (2)'s vcvars ... (2)

[Fujii Hironori]

With -G "Visual Studio 15 2017", it chooses 3's compiler for CMake configuration time.
If I explicitly invoke 2's msbuild, then it compiles with 2's compiler.

https://gist.github.com/fujii/6d8bf93131dcb4f4af45eb6efed1e11f

[Fujii Hironori]

Created attachment 366919 [details]
Patch

[Fujii Hironori]

Currently, vswhere is used only for finding msbuild.
If that is true, we should do the following.

  Find MSBuild · Microsoft/vswhere Wiki
  https://github.com/Microsoft/vswhere/wiki/Find-MSBuild

Furthermore, cmake --build should be used if possible.

[Stephan Szabo]

(In reply to Fujii Hironori from comment #27)
> Currently, vswhere is used only for finding msbuild.
> If that is true, we should do the following.
> 
>   Find MSBuild · Microsoft/vswhere Wiki
>   https://github.com/Microsoft/vswhere/wiki/Find-MSBuild

I had it doing that in an earlier version, but when looking at the callers to get the install directory I saw setupCygwinEnv was using the install directory to find a devenv.com. I missed that the computed value is then unused in the function.

> Furthermore, cmake --build should be used if possible.

That would be a much better general case, but Don thought that would cause failures for some apple internal builds so we'd need to get alignment there.

[Stephan Szabo]

My state with that patch building --no-ninja (having a 2017 pro and community and 2019 community) and trimming down libraries looked for to jsc libs in the playstation case.

Non-vcvars cmd, --wincairo: Fails trying to use the 2017 msbuild (maybe because of the installation type prioritization in the perl right now)
Non-vcvars cmd, --playstation: Starts build

2017pro-vcvars cmd, --wincairo: Fails because cmake fails to find the compiler
2017pro-vcvars cmd, --playstation: Fails, trying to run 2017 msbuild, but "The specified solution configuration \"Release|x64\" is invalid."

2019-vcvars cmd, --wincairo: Starts build
2019-vcvars cmd, --playstation: Fails, trying to run 2019 msbuild, but "The specified solution configuration \"Release|x64\" is invalid."

If I remove the code that prioritizes installation type I get:
Non-vcvars cmd, --wincairo: Starts build
Non-vcvars cmd, --playstation: Fails, trying to run 2019 msbuild, but "Please check to make sure that you have specified a valid combination of Configuration and Platform for this project.  Configuration='Release'  Platform='ORBIS'." (edited)

If I add a /p:Platform=ORBIS to getMSBuildPlatformArgument in the perl, then 2017pro-vcvars cmd, --playstation starts a build.

[Fujii Hironori]

(In reply to Stephan Szabo from comment #28)
> > Furthermore, cmake --build should be used if possible.
> 
> That would be a much better general case, but Don thought that would cause
> failures for some apple internal builds so we'd need to get alignment there.

AppleWin internal builds don't use build-webkit script. It is irrelevant.

[Stephan Szabo]

Created attachment 367054 [details]
Move to Visual Studio 2019 - with final fallback updated to 2019

Based on the above results:

I found that we can use an environment variable to avoid the last change for a vs2017 vcvars shell, that I'd forgotten about the final fallback if vswhere also does not return an instance, and that if we only look for 2017 in PlayStation port the non-vcvars shell seemed to work locally for both that and wincairo.

[Fujii Hironori]

Comment on attachment 367054 [details]
Move to Visual Studio 2019 - with final fallback updated to 2019

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

> Tools/Scripts/webkitdirs.pm:665
> +    if (isPlayStation()) {

Does PlayStation port need to use vs2017 msbuild even through it uses a compiler from PlayStation sdk?

> Tools/Scripts/webkitdirs.pm:723
> +            "Microsoft Visual Studio", "2019", $productType);

If AppleWin internal builds are using this fallback, we can’t change at the moment.

[Stephan Szabo]

(In reply to Fujii Hironori from comment #32)
> Comment on attachment 367054 [details]
> Move to Visual Studio 2019
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367054&action=review
> 
> > Tools/Scripts/webkitdirs.pm:665
> > +    if (isPlayStation()) {
> 
> Does PlayStation port need to use vs2017 msbuild even through it uses a
> compiler from PlayStation sdk?

If we're using --no-ninja, as far as I can tell, yes, as building the solution relies on the platform support having been added to Visual Studio (and it's as far as I know not yet available for 2019).

> > Tools/Scripts/webkitdirs.pm:723
> > +            "Microsoft Visual Studio", "2019", $productType);
> 
> If AppleWin internal builds are using this fallback, we can’t change at the
> moment.

Ah, good point, will reverse that.

[Stephan Szabo]

Created attachment 367073 [details]
Move to Visual Studio 2019

Reset the final fallback back to 2017.

[Stephan Szabo]

Comment on attachment 367054 [details]
Move to Visual Studio 2019 - with final fallback updated to 2019

It was brought up that actually we may not need to worry about internal builds using build-webkit so un-obsoleting the one with the final fallback switched over to 2019.

[Fujii Hironori]

Comment on attachment 367054 [details]
Move to Visual Studio 2019 - with final fallback updated to 2019

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

>>> Tools/Scripts/webkitdirs.pm:665
>>> +    if (isPlayStation()) {
>> 
>> Does PlayStation port need to use vs2017 msbuild even through it uses a compiler from PlayStation sdk?
> 
> If we're using --no-ninja, as far as I can tell, yes, as building the solution relies on the platform support having been added to Visual Studio (and it's as far as I know not yet available for 2019).

Really good point.
For example, if I have

1. VS 2017 Professional 15.7.6 + PlayStation VS Extension
2. VS 2017 Community 15.8.9

CMake is going to choose the newer (2) instance for PlayStation port even though it doesn't have PlayStation VS Extension.
We should invoke vcvars.bat before build-webkit for this case.

[Stephan Szabo]

(In reply to Fujii Hironori from comment #36)
> Comment on attachment 367054 [details]
> Move to Visual Studio 2019 - with final fallback updated to 2019
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367054&action=review
> 
> >>> Tools/Scripts/webkitdirs.pm:665
> >>> +    if (isPlayStation()) {
> >> 
> >> Does PlayStation port need to use vs2017 msbuild even through it uses a compiler from PlayStation sdk?
> > 
> > If we're using --no-ninja, as far as I can tell, yes, as building the solution relies on the platform support having been added to Visual Studio (and it's as far as I know not yet available for 2019).
> 
> Really good point.
> For example, if I have
> 
> 1. VS 2017 Professional 15.7.6 + PlayStation VS Extension
> 2. VS 2017 Community 15.8.9
> 
> CMake is going to choose the newer (2) instance for PlayStation port even
> though it doesn't have PlayStation VS Extension.
> We should invoke vcvars.bat before build-webkit for this case.

Yes, for that case (which I also have on one of my boxes) I think it could become necessary to run vcvars (and then the platform may need to be specified as well in either direct arg or envvar). I'm just a bit concerned that we can do better for the probably more common case of one instance per major version.

[Per Arne Vollan]

(In reply to Stephan Szabo from comment #35)
> Comment on attachment 367054 [details]
> Move to Visual Studio 2019 - with final fallback updated to 2019
> 
> It was brought up that actually we may not need to worry about internal
> builds using build-webkit so un-obsoleting the one with the final fallback
> switched over to 2019.

We are also using build-webkit for AppleWin development.

[Fujii Hironori]

(In reply to Per Arne Vollan from comment #38)
> We are also using build-webkit for AppleWin development.

Yeah. I think we can remove the fallback. Filed in Bug 196760.

[Fujii Hironori]

Created attachment 367193 [details]
Patch

[Fujii Hironori]

r? This patch is needed for AppleWin and WinCairo to be ready for VS2019.

[Fujii Hironori]

Created attachment 367924 [details]
Patch

[Fujii Hironori]

Committed r245087: <https://trac.webkit.org/changeset/245087>

[Radar WebKit Bug Importer]

<rdar://problem/50606874>