HaxeFoundation / hxcpp

Runtime files for c++ backend for haxe
Other
298 stars 189 forks source link

Update Visual Studio detection scripts #1008

Closed tobil4sk closed 1 year ago

tobil4sk commented 2 years ago

It looks like the scripts for detecting MSVC installations are out of date, because they have specific checks for VS2017 but not for VS2019 or VS2022. We should update them so they work for newer versions.

See: https://github.com/HaxeFoundation/hxcpp/blob/master/toolchain/msvc-setup.bat https://github.com/HaxeFoundation/hxcpp/blob/master/toolchain/msvc64-setup.bat

justin-espedal commented 2 years ago

Quick recap:

  1. VS2017 detection was originally added in.
  2. It was shortly replaced with a vswhere-based lookup.
  3. After a few months, an explicit VS2017 lookup was added back in as a fallback, though I don't understand why exactly. In what situation does vswhere fail to work?

In general, VS2017 and all newer versions should be detected with the vswhere tool, since all of these install vswhere as part of their setup process. It's not clear to me why VS2017 is explicitly included in the script this way. I have only VS2019 installed, and it works fine for me.

I'd be interested to hear why a fallback was desired at all for VS2017.

If somebody who can merge PRs looks into this, I'd appreciate this being looked at as well: https://github.com/HaxeFoundation/hxcpp/pull/796.

hughsando commented 2 years ago

I can't recall exactly, but vswhere may have come as a separate download or service pack at some stage, so there may have been a transition. As I understand it, the latest stuff seems to just work. Are you finding a need for additional fallbacks?

tobil4sk commented 2 years ago

This came up after troubleshooting with a user from the Haxe discord who wasn't able to get the compilation to work. I believe they had installed vs2019 via winget:

winget install --id=Microsoft.VisualStudio.2019.BuildTools  -e

However, HXCPP was giving errors about how it it wasn't able to locate vs2017.

hughsando commented 2 years ago

I don't have a problem with fallbacks - especially with a well defined used case - so if they fix things then good. They seem slightly brittle to me (specific install directory etc) so a generic solution would be better if we had one. Do you know if it is possible to install vswhere using winget too? That might be a neat solution. Another solution might setting NO_AUTO_MSVC, which will just try "cl.exe" if it can be found in the path. We might also consider a new haxelib "hxcpp_vswhere" that includes the vswhere.exe so we can always find it.

tobil4sk commented 2 years ago

Do you know if it is possible to install vswhere using winget too?

Looks like the answer to that is no unfortunately, for now at least: https://github.com/microsoft/vswhere/issues/222.

Here is the wiki page with the available installation methods: https://github.com/microsoft/vswhere/wiki/Installing

Perhaps a solution would be to add proper instructions on how to install a minimum working msvc version somewhere in the manual. If it is clear to users that they need to have vswhere so MSVC can be located, then it's less of an issue.

tobil4sk commented 2 years ago

Although, it looks like vswhere doesn't find build tools by default, unless an extra flag is added: https://github.com/microsoft/vswhere/pull/8

tobil4sk commented 2 years ago

~-products * is in theory what we need to pass so that build tools are searched as well... but we already have that... Perhaps the -required flag is causing the build tools to be excluded?~ https://github.com/HaxeFoundation/hxcpp/blob/master/toolchain/msvc64-setup.bat#L12

nvm, acording to #1010, it works fine as long as vswhere is installed.

tobil4sk commented 2 years ago

Ok, so apparently the winget package does already exist:

winget install -e --id Microsoft.VisualStudio.Locator

See: https://winget.run/pkg/Microsoft/VisualStudio.Locator

I assumed it wasn't because it wasn't mentioned in the documentation.

In that case, we might just want to update the docs to mention that vswhere is required, and also update the error message here: https://github.com/HaxeFoundation/hxcpp/blob/master/toolchain/msvc64-setup.bat#L15-L21