-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Support buld using VS2019 #2388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CMake shipped with VS does not support VS2019 yet, so run generator for VS 2017. It will still build using VS 2019
Codecov Report
@@ Coverage Diff @@
## master #2388 +/- ##
==========================================
- Coverage 71.24% 71.24% -0.01%
==========================================
Files 783 783
Lines 140733 140733
Branches 16086 16086
==========================================
- Hits 100266 100262 -4
- Misses 36014 36017 +3
- Partials 4453 4454 +1
|
Fixes to #1967 |
:VS2019 | ||
:: Setup vars for VS2019 | ||
set __PlatformToolset=v142 | ||
set __VSVersion=15 2017 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017 [](start = 19, length = 4)
if you look into this commit:
https://github.com/dotnet/corefx/pull/33969/files
they set VSVersion to vs2019 and platformToolsSet to v141
I'm not sure about toolset but vsversion as vs2019 make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__PlatformToolset=v142
is what shipped with VS2019 by default, so I choose this option. v141 could be installed there, but as well and v140 so I choose default for VS2019.
CMake 3.13 shipped with VS2019 does not support "Visual Studio 16 2019" generator, so that's why this one strange value for __VSVersion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be following what corefx
does, shouldn't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@safern - thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that the current Cmake version that is out doesn't yet support this because the VS2017 generator uses the v141 toolset and VS2019 Preview2 had a breaking change which changed the toolset version file name, so cmake wouldn't find the compiler. Then on preview3 that was fixed. And @kant2002 is correct, v142 is the toolset for VS2019.
In corefx we haven't been able to yet build with VS 2019 Preview2 or Preview3, until cmake 3.14 is out the door and points to the right toolset version, but this is the needed change and in corefx we will have to move the toolset to v142.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@safern So you approve this PR, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the __VSVersion string that we set in corefx in build-native.cmd we override it when we call cmake with the generator to use VS2017 generator for now. That will be fixed once cmake 3.14 is out of the door and supports VS2019 generator. https://github.com/dotnet/corefx/blob/master/src/Native/Windows/gen-buildsys-win.bat#L17
So basically the values that we're setting to in build-native.cmd are ignored.
Thanks @kant2002 for submitting this PR. Would you please file an issue and associate the issue with this PR? |
@shauheen as @Ivanidzo4ka point out this is related to #1967 and I update description so that issue would be automatically closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. As an FYI, we have this issues in corefx tracking cmake support for VS2019.
https://github.com/dotnet/corefx/issues/34413
https://github.com/dotnet/corefx/issues/34613
Thank you very much @kant2002 ! |
CMake shipped with VS does not support VS2019 yet, so run generator for VS 2017. It will still build using VS 2019
Closes: #1967