With this patch, it is possible to pass extra arguments to vswhere which is used internally to find the location of MS VisualStudio.
Created attachment 352897 [details] PATCH
Created attachment 352898 [details] PATCH Example: # to use version greater or equeal than 15.7 but lesser than 15.8 > perl build-webkit --wincairo --vswhereargs='-version [15.7,15.8)' # to use VisualStudio Community over VisualStudio Professional > perl build-webkit --wincairo --vswhereargs='-products Microsoft.VisualStudio.Product.Community'
Created attachment 352901 [details] PATCH
Comment on attachment 352901 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=352901&action=review Seems fine once builds pass. :) > Tools/ChangeLog:12 > + # to use version greater or equeal than 15.7 but lesser than 15.8 Nit: "greater than or equal to 15.7 but less than 15.8"
> > Tools/ChangeLog:12 > > + # to use version greater or equeal than 15.7 but lesser than 15.8 > > Nit: "greater than or equal to 15.7 but less than 15.8" Ouch. Yes, that's right. GTE. Real programmer doesn't mistake this English. Shame on me. I'll fix it after seeing the result of build.
Is there a reason why this is needed? https://bugs.webkit.org/show_bug.cgi?id=189987 should be resolved now. I don't think we should be figuring out semantic versioning because VC had problems. We should always be able to build with VC and I think the VC++ now has us in their test suite so hopefully this shouldn't happen again.
(In reply to Don Olmstead from comment #6) > Is there a reason why this is needed? > https://bugs.webkit.org/show_bug.cgi?id=189987 should be resolved now. I > don't think we should be figuring out semantic versioning because VC had > problems. We should always be able to build with VC and I think the VC++ now > has us in their test suite so hopefully this shouldn't happen again. Well this shouldn't but "History repeats itself". VC changes its implementation on 15.8 and that causes https://bugs.webkit.org/show_bug.cgi?id=189987 . Also it's handy for us to have a way to test preview installation. I don't see any downside of this patch.
Created attachment 352905 [details] FIX example sentence in ChangeLog
(In reply to Basuke Suzuki from comment #7) > (In reply to Don Olmstead from comment #6) > > Is there a reason why this is needed? > > https://bugs.webkit.org/show_bug.cgi?id=189987 should be resolved now. I > > don't think we should be figuring out semantic versioning because VC had > > problems. We should always be able to build with VC and I think the VC++ now > > has us in their test suite so hopefully this shouldn't happen again. > > Well this shouldn't but "History repeats itself". VC changes its > implementation on 15.8 and that causes > https://bugs.webkit.org/show_bug.cgi?id=189987 . > > Also it's handy for us to have a way to test preview installation. I don't > see any downside of this patch. My only complaint is that vswhere is used internally so exposing things doesn't seem to be something people would do on a regular basis. I can see the utility of this but it seems like a pretty small use case. I don't see any problems with the contents of the patch.
Don, is wincairo bot fails actually because of this patch?
(In reply to Basuke Suzuki from comment #10) > Don, is wincairo bot fails actually because of this patch? No, it's just the MIDL issue again (bug 187725).
AppleWin port doesn't use vswhere. Bug 189412 – download-github-release.py hits errors on Windows bots WinCairo BuildBot and EWS bots explicitly set env vars by using Select-VSEnvironment outside of build-webkit. https://github.com/WebKitForWindows/docker-webkit-dev/blob/master/ews/windowsservercore/WebKit-EWS/Run-EWS.ps1#L33 https://github.com/WebKitForWindows/docker-webkit-dev/blob/master/buildbot/windowsservercore/WebKit-BuildWorker/Run-BuildbotWorker.ps1#L54 build-webkit prefers VSINSTALLDIR env var to vswhere. https://github.com/WebKit/webkit/blob/19cf5c15f58dcbe9843f2ffa2ede30a96bc97bbd/Tools/Scripts/webkitdirs.pm#L696 I think vswhere should be removed from build-webkit.
FYI, https://github.com/Microsoft/vswhere > vswhere is included with the installer as of Visual Studio 2017 version 15.2 and later, and can > be found at the following location: %ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe. So, I'm using it like following: > for /F "delims=" %%I in ('"%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe" -latest -property installationPath') do set VSPATH=%%I > call "%VSPATH%\VC\Auxiliary\Build\vcvars64.bat"
Comment on attachment 352905 [details] FIX example sentence in ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=352905&action=review > Tools/Scripts/webkitdirs.pm:671 > + undef $@; If we want to use eval{}, it may be a good idea to import Try::Tiny module into the tree and use it instead, its usage is much more clear and less error prone > Tools/Scripts/webkitdirs.pm:673 > + return undef unless (scalar @$installations); I think following code would be easier to read: unless (@$installations) { return; } Though you may want to omit it completely, because sorting empty array requires no work > Tools/Scripts/webkitdirs.pm:683 > + return $vsWhereFoundInstallation; return $sorted[0] What you really want is to return first element of array, not to modify that array for possible future use (what pop does) > Tools/Scripts/webkitdirs.pm:686 > +sub visualStudioProductIdPriority($) See below about prototypes > Tools/Scripts/webkitdirs.pm:1972 > +sub buildVisualStudioProject($;$;$) In modern perl community usage of prototypes is considered harmful, unless it is done to imitate psecific behavior of builtin functions, or to implement embedded DSL. See e.g. http://www.modernperlbooks.com/mt/2009/08/the-problem-with-prototypes.html Also, correct spelling of this one is ($;$$)
Comment on attachment 352905 [details] FIX example sentence in ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=352905&action=review Thanks for review! >> Tools/Scripts/webkitdirs.pm:671 >> + undef $@; > > If we want to use eval{}, it may be a good idea to import Try::Tiny module into the tree and use it instead, its usage is much more clear and less error prone Okay, I'll try. How can I import perl module into tree? >> Tools/Scripts/webkitdirs.pm:673 >> + return undef unless (scalar @$installations); > > I think following code would be easier to read: > > unless (@$installations) { > return; > } > > Though you may want to omit it completely, because sorting empty array requires no work Okay to remove this at this. And I learned accessing out of indexed value is undef. Fancy. >> Tools/Scripts/webkitdirs.pm:683 >> + return $vsWhereFoundInstallation; > > return $sorted[0] > > What you really want is to return first element of array, not to modify that array for possible future use (what pop does) Okay. Right. >> Tools/Scripts/webkitdirs.pm:1972 >> +sub buildVisualStudioProject($;$;$) > > In modern perl community usage of prototypes is considered harmful, unless it is done to imitate psecific behavior of builtin functions, or to implement embedded DSL. See e.g. http://www.modernperlbooks.com/mt/2009/08/the-problem-with-prototypes.html > > Also, correct spelling of this one is ($;$$) Got it. I will remove that.
Respond to my comment. Why do you need this change?
(In reply to Fujii Hironori from comment #16) > Respond to my comment. Why do you need this change? See my previous comment to Don. WinCairo uses vswhere so that it is better way for choosing instance of VisualStudio. If you want to delete that, that should be another discussion.
(In reply to Basuke Suzuki from comment #17) > See my previous comment to Don. WinCairo uses vswhere so that it is better > way for choosing instance of VisualStudio. This is not right. WinCairo doesn't necessarily use vswhere. (Comment 12) It use VSINSTALLDIR if VSINSTALLDIR is set. Otherwise, it uses vswhere. You can simply use vcvars64.bat. What is the difference using command line switch and using vcvars64.bat?
(In reply to Fujii Hironori from comment #18) > (In reply to Basuke Suzuki from comment #17) > > See my previous comment to Don. WinCairo uses vswhere so that it is better > > way for choosing instance of VisualStudio. > > This is not right. WinCairo doesn't necessarily use vswhere. (Comment 12) > It use VSINSTALLDIR if VSINSTALLDIR is set. Otherwise, it uses vswhere. So that WinCairo uses vswhere if env var is not set. And that means vswhere is the default choice because that var isn't defined by default. But is this conversation meaningful? There's a feature which somebody uses and this patch improve the value of that usage. If you think the feature is harmful, open a bug for that and let's discuss about that. This bug is not the right place for removing vswhere. I don't think I have to probe my way is better than your way on this bug.
I think removing vswhere is the simplest solution for Bug 189412. OK, I will do it in Bug 189412.
Did you test your patch? It doesn't work as you expect. I installed two instances: > installationVersion: 15.7.27703.2047 > productPath: C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\Common7\IDE\devenv.exe > displayName: Visual Studio Professional 2017 > installationVersion: 15.8.28010.2048 > productPath: C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\devenv.exe > displayName: Visual Studio Community 2017 vswhere ouputs as expected. > PS C:\webkit\ga> vswhere -property installationPath -version "[15.7,15.8)" > C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional Here is the build log: > PS C:\webkit\ga> perl Tools\Scripts\build-webkit --release --wincairo --generate-project-only --no-ninja --vswhereargs='-version [15.7,15.8)' > Updating WinCairoRequirements.zip... > Found existing release: v2018.10.09 > Seeking latest release from WebKitForWindows/WinCairoRequirements... > Found release to download: v2018.10.09 > Already up-to-date! > Updating vswhere.exe... > Found existing release: 2.5.2 > Seeking latest release from Microsoft/vswhere... > Found release to download: 2.5.2 > Already up-to-date! > + cmake -DPORT="WinCairo" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Release -G "Visual Studio 15 2017 Win64" -DCMAKE_GENERATOR_TOOLSET="host=x64" -DSHOW_BINDINGS_GENERATION_PROGRESS=1 -DDEVELOPER_MODE=ON "C:/webkit/ga" > -- The C compiler identification is MSVC 19.15.26732.1 > -- The CXX compiler identification is MSVC 19.15.26732.1 > -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.15.26726/bin/Hostx64/x64/cl.exe > -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.15.26726/bin/Hostx64/x64/cl.exe -- works It picks the compiler of Community. webkitdirs.pm is using vswhere to find msbuild, but it is used only for building generated solution file. CMake looks for VS installation by its own logic.
(In reply to Fujii Hironori from comment #21) > Did you test your patch? It doesn't work as you expect. > > I installed two instances: > > > installationVersion: 15.7.27703.2047 > > productPath: C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\Common7\IDE\devenv.exe > > displayName: Visual Studio Professional 2017 > > > installationVersion: 15.8.28010.2048 > > productPath: C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\devenv.exe > > displayName: Visual Studio Community 2017 > > vswhere ouputs as expected. > > > PS C:\webkit\ga> vswhere -property installationPath -version "[15.7,15.8)" > > C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional > > Here is the build log: > > > PS C:\webkit\ga> perl Tools\Scripts\build-webkit --release --wincairo --generate-project-only --no-ninja --vswhereargs='-version [15.7,15.8)' > > Updating WinCairoRequirements.zip... > > Found existing release: v2018.10.09 > > Seeking latest release from WebKitForWindows/WinCairoRequirements... > > Found release to download: v2018.10.09 > > Already up-to-date! > > Updating vswhere.exe... > > Found existing release: 2.5.2 > > Seeking latest release from Microsoft/vswhere... > > Found release to download: 2.5.2 > > Already up-to-date! > > + cmake -DPORT="WinCairo" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Release -G "Visual Studio 15 2017 Win64" -DCMAKE_GENERATOR_TOOLSET="host=x64" -DSHOW_BINDINGS_GENERATION_PROGRESS=1 -DDEVELOPER_MODE=ON "C:/webkit/ga" > > -- The C compiler identification is MSVC 19.15.26732.1 > > -- The CXX compiler identification is MSVC 19.15.26732.1 > > -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.15.26726/bin/Hostx64/x64/cl.exe > > -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.15.26726/bin/Hostx64/x64/cl.exe -- works > > It picks the compiler of Community. > > webkitdirs.pm is using vswhere to find msbuild, but it is used only for > building generated solution file. > CMake looks for VS installation by its own logic. That is the expected behaviour. Vswhere is for choosing msbuild. That’s the result I wanted.
(In reply to Basuke Suzuki from comment #22) > (In reply to Fujii Hironori from comment #21) > > webkitdirs.pm is using vswhere to find msbuild, but it is used only for > > building generated solution file. > > CMake looks for VS installation by its own logic. > > That is the expected behaviour. Vswhere is for choosing msbuild. That’s the > result I wanted. I'm confusing. Why do you want to choose msbuild? You don't need to choose compiler? What problem does your patch solve?
I got the answer to my question. Different compilers are used in CMake configuration time (chosen by CMake) and building time (chosen by vswhere).
Using vswhere causes the compiler version mismatch problem (Bug 190797 Comment 24). CMake looks for VS by its own logic. We should use vcvars64.bat to safely choose a VS installation. If you have multiple VS installations, there are mulptile "x64 Native Tools Command Prompt for Visual Studio 2017" menu items in Windows Start menu. You can use which you want to choose VS.
Comment on attachment 352905 [details] FIX example sentence in ChangeLog This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.