Summary: | [Win] build-webkit should be ready for Visual Studio 2019 | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephan Szabo <stephan.szabo> | ||||||||||||||||
Component: | Tools / Tests | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, dbates, don.olmstead, ews-watchlist, Hironori.Fujii, lforschler, pvollan, ross.kirsling, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 196760 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Stephan Szabo
2019-04-04 13:28:52 PDT
Created attachment 366748 [details]
[WinCairo] Update to VS2019
🥳🎉🎊 Do we also need to set the cmake minimum version to 3.14 for us? 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. Although the build error on win seems unrelated, it does look like the quoting is off for the vswhere version expression, fixing. Created attachment 366762 [details]
[WinCairo] Update to VS2019
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.
Comment on attachment 366762 [details] [WinCairo] Update to VS2019 I think this doesn’t work. https://bugs.webkit.org/show_bug.cgi?id=190797 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 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)? 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. (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)? 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. Wincairo buildbot is still using vs2017. (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 (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. (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. 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. (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. (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? (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. (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. (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. 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) 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 Created attachment 366919 [details]
Patch
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. (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. 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. (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. 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.
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. (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. Created attachment 367073 [details]
Move to Visual Studio 2019
Reset the final fallback back to 2017.
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.
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. (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. (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. (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. Created attachment 367193 [details]
Patch
r? This patch is needed for AppleWin and WinCairo to be ready for VS2019. Created attachment 367924 [details]
Patch
Committed r245087: <https://trac.webkit.org/changeset/245087> |