Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 6, 2019
Merged

Support buld using VS2019 #2388

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/Native/build.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,30 @@ set "VSCMD_START_DIR=%__currentScriptDir%"
call "%_VSCOMNTOOLS%\VsDevCmd.bat"

:RunVCVars
if "%VisualStudioVersion%"=="15.0" (
if "%VisualStudioVersion%"=="16.0" (
goto :VS2019
) else if "%VisualStudioVersion%"=="15.0" (
goto :VS2017
) else if "%VisualStudioVersion%"=="14.0" (
goto :VS2015
)

:MissingVersion
:: Can't find VS 2015 or 2017
echo Error: Visual Studio 2015 or 2017 required
:: Can't find VS 2015, 2017 or 2019
echo Error: Visual Studio 2015, 2017 or 2019 required
echo Please see https://github.com/dotnet/machinelearning/tree/master/Documentation for build instructions.
exit /b 1

:VS2019
:: Setup vars for VS2019
set __PlatformToolset=v142
set __VSVersion=15 2017
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Member

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?

https://github.com/dotnet/corefx/blob/66985ca21f89e4d2230f5fcb99f4e8b3d618c9af/src/Native/build-native.cmd#L73-L79

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@safern - thoughts?

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Member

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.

if NOT "%__BuildArch%" == "arm64" (
:: Set the environment for the native build
call "%VS160COMNTOOLS%..\..\VC\Auxiliary\Build\vcvarsall.bat" %__VCBuildArch%
)
goto :SetupDirs

:VS2017
:: Setup vars for VS2017
set __PlatformToolset=v141
Expand Down