search

dotnet / machinelearning

ML.NET is an open source and cross-platform machine learning framework for .NET.
https://dot.net/ml
MIT License
9.04k stars 1.89k forks source link

ML.net build failing with Visual Studio 2019 Version 16.0 #1967

Closed Anipik closed 5 years ago

Anipik commented 5 years ago

I just rebooted my machine. So while setting up the visual studio, I installed the latest Visual Studio 2019. The visual studio version for this 16.0

So when I try to build the repo, I get an error as Error: Visual Studio 2015 or 2017 required.

My guess is that the problem is here https://github.com/dotnet/machinelearning/blob/master/src/Native/build.cmd#L48

This check fails as the value of the version is 16.0

I changed this line to if %VisualStudioVersion% geq 15.0 (

and the build works fine then.

is this an appropriate fix and anything else also needs to be done here ?

cc @danmosemsft @safern @eerhardt @TomFinley

codemzs commented 5 years ago

You may want to do an explicit equality comparison like we do for VS 2015 and VS 2017, not just greater than equal to, what if another version gets released that needs to have different setup?, in addition to that you may want to consider changing/adding few more things, for example, change the error message here below:

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

Just like how we have setup lines for VS2017, we probably will need something for VS 2019, no? :VS2017 :: Setup vars for VS2017 set PlatformToolset=v141 set VSVersion=15 2017 if NOT "%BuildArch%" == "arm64" ( :: Set the environment for the native build call "%VS150COMNTOOLS%....\VC\Auxiliary\Build\vcvarsall.bat" %VCBuildArch% ) goto :SetupDirs

All in all, we need to have explicit references for VS 2019 just like we have for VS 2017/2015.

Anipik commented 5 years ago

Actually i tried changing i to set __VSVersion=16 2019 for 2019 VS but cmake fails to recognise it and i get this as an error

EXEC : CMake error : Could not create named generator Visual Studio 16 2019 Win64 [C:\git\machinelearning\src\Native\build.proj]

i am not sure whts the correct set of values for vsversion and platformToolset for vs2019

rrmistry commented 5 years ago

If you're trying to build using build.cmd on Windows (at the root level of this repository) then there is a much simpler fix.

I traced down the call sequence to:

  1. .\build.cmd
  2. src\Native\build.cmd
  3. %ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe

The relevant parameters for vswhere.exe are:

-prerelease    Also searches prereleases. By default, only releases are searched.
-version arg   A version range for instances to find. Example: [15.0,16.0) will find versions 15.*.
-latest        Return only the newest version and last installed.

Making an inferrence on this, when I changed this file: https://github.com/dotnet/machinelearning/blob/a570da14a41f2870eb8f61d84496a58422398253/src/Native/build.cmd#L38

And removed the -prerelease cmd argument, it defaulted to VS 2017.

This is obviously a temporary solution until VS 2019 becomes an official version, but then ML.NET might move to VS 2019 anyways :smirk:

Anipik commented 5 years ago

Actually there is no vs 2017 on my machine

rrmistry commented 5 years ago

Actually there is no vs 2017 on my machine

Just to be safe, it would be best to install VS 2017. Just to ensure consistent behavior with other developers. VS 2019 may have different sets of C++ optimizations so you won't be reproducing the exact build / test environment as intended by authors.

TomFinley commented 5 years ago

Just to be safe, it would be best to install VS 2017. Just to ensure consistent behavior with other developers. VS 2019 may have different sets of C++ optimizations so you won't be reproducing the exact build / test environment as intended by authors.

This issue here (until properly investigated) is what would lead me to prefer a VS 2019 build being something a user has to explicitly invoke, with the default behavior being to insist on the older version. Over the years, as we've moved from one version of VS to the next, it was the inconsistency of native code from one compilation to the next and the resulting change in expected values in tests that was the biggest barrier to new VS adoption. (To be clear, I don't know that VS 2019 will give us the same problem. But, if it doesn't, it would be the first version of VS that did not. Guilty until proven innocent, and whatnot. 😉)

But for now, while making VS 2019 compilation possible, should we keep that as something the user has to explicitly flag, if they want to use? So, if VS 2017 and 2019 pre-release are both on the machine, it defaults to 2017, if only 2019 then it complains, and in either situation it compiles on 2019 if the flag is set? What you think @Anipik and @rrmistry, good idea, bad idea?

rrmistry commented 5 years ago

I would prefer the same. No objections on my part for:

if VS 2017 and 2019 pre-release are both on the machine, it defaults to 2017, if only 2019 then it complains, and in either situation it compiles on 2019 if the flag is set

where a new flag is required to be implemented for the build / tools / run batch scripts.

Sure would make debugging things a lot easier too when the project is officially planned to move to VS 2019.

rrmistry commented 5 years ago

By the way, I just wanted to convey my sincere thanks for all the hard work that everyone has been putting into this project. And wish everybody a very Merry Christmas! 🎄

@TomFinley , I have never contributed to a open source project before, but I would like to start.

I did read through CONTRIBUTING.md but I'm not sure what's the best way to handle this.

I can offer to do a deeper investigation and produce findings as suggested, or go through ROADMAP.md and offer support towards greatest needs.

We can connect in private over gitter if you would like. Any tips or advise would be much obliged 🤠

Again, thanks for everything!

Anipik commented 5 years ago

Yes @TomFinley I agree with you. We will have to fix the compile for for 2019

safern commented 5 years ago

This is what I did in core fix to fix this: https://github.com/dotnet/corefx/pull/33969 we use a very similar mechanism so you could use that as a guidance.

codemzs commented 5 years ago

Thanks @safern , this is what I was expecting the fix to be.

TomFinley commented 5 years ago

Great, thanks all, and thanks to you @rrmistry ! This is a great attitude. I'm not an expert in the build systems but @safern definitely is. 😄 Though, this time of year most people are on vacation so maybe we won't be as responsive as some other times. Not sure if you wanted to tackle it?

rrmistry commented 5 years ago

Thanks @TomFinley ! I'm on-and-off the holiday spirits myself 🎄 🎁 🎆
I'll reach out to @safern to discuss best way to move forward. Thanks!