Skip to content

Conversation

@stevapple
Copy link
Contributor

@stevapple stevapple commented Feb 5, 2023

Use vswhere tool to find MSVC tools. (Partially address #5771)

Motivation:

After #5720, SwiftPM gains implicit dependency of link on Windows, but cannot detect it from the context without opts-in like Developer command prompt.

Although the installation guide never claims that non-developer environment is supported, SwiftPM actually works since it's ported. Some automation workflows and tools like Swift for Visual Studio Code already depends on the behavior. Breaking the compatibility makes huge pain for the Swift on Windows ecosystem.

Modifications:

  • UserToolchain.findTool is taught to call vswhere on Windows and now knows about MSVC installation.
  • UserToolchain.determineLibrarian and UserToolchain.determineSwiftCompilers gets new arguments to help determine the context.
  • \UserToolchain.useXcrun is reused for vswhere because they're much of serving the same purpose.

Result:

SwiftPM will be usable (again) from non-developer shells like Windows Powershell.

}

if useXcrun {
#if os(Windows)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is #if os(Windows) check enough, or do we need to gate on host triple too?

Comment on lines 59 to 60
/// Only use search paths, do not fall back to `xcrun` or `vswhere`.
let useXcrun: Bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still undecided if we want to rename the variable. I didn't do this at the point because neither have I got a short and clear name (useExternalTool sounds verbose and confusing), nor am I going to make vswhere the default like xcrun is now. It can be prioritized in later pitches.

}
}

if useXcrun {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to introduce this approach as a fall-back from the beginning, and then makes it the default (like Clang does) if everything is working as expected.

@MaxDesiatov MaxDesiatov requested a review from compnerd February 5, 2023 16:22
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I strongly feel that this is the wrong way to handle this. If the install location changes this breaks. If I install outside of ProgramFiles(x86) this breaks. This has a bigger issue where we now are divergent from clang - clang has its own logic for selecting the VSCode version, which we must match or run the risk in mixing pieces incorrectly.

At the very least, this should be using clang to locate the tools, not vswhere.

@stevapple
Copy link
Contributor Author

I believe you have very deep misunderstandings on how vswhere works and how Clang finds MSVC toolsets. @compnerd

  1. vswhere is more powerful and robust than Clang’s internal selector, and LLVM is already using it in its Windows installer, instead of replicating Clang code.
  2. vswhere is guaranteed to be there, in %ProgramFiles(x86)%/Microsoft Visual Studio/Installer, ever since Visual Studio 2017. As the path suggests, it’s a part of the installer bundle, not Visual Studio itself. The installer will be automatically installed at first run and the installation path cannot be changed.
  3. The vswhere and Clang typically gives the same result. Clang has additional logic to handle MSVC version in triple, which is unsupported in Swift yet. It also prefers LINK and LIB environment variables, which I believe should be part of PackageModel: partially address #5719 #5981 instead.
  4. Clang is also searching for Visual Studio installation, not the standalone MSVC toolset. If you use fancy way to install MSVC instead of vs_installer.exe, you should set VCINSTALLDIR in the environment. Installation detection won’t work.

return tool
}
#endif
}
Copy link
Member

Choose a reason for hiding this comment

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

This makes the behaviour different when targeting Windows from Windows and Windows from another environment just making things more complicated and confusing for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what useXcrun is already doing for Darwin to Darwin case, which I believe is reasonable — just like xcrun is only available on macOS, Visual Studio is basically a Windows thing.

@compnerd
Copy link
Member

compnerd commented Feb 6, 2023

I believe you have very deep misunderstandings on how vswhere works and how Clang finds MSVC toolsets. @compnerd

I'd really rather that you would stop with the personal attacks. This seems to be a repetitive thing and really does not help create a pleasant environment.

  1. vswhere is more powerful and robust than Clang’s internal selector, and LLVM is already using it in its Windows installer, instead of replicating Clang code.

Would you care to elaborate how it is "more powerful and robust"? Other than it is controlled by Microsoft and they can coordinate changes, I don't see how it is any more robust. Use in the installer is one thing, use in the tooling is another.

  1. vswhere is guaranteed to be there, in %ProgramFiles(x86)%/Microsoft Visual Studio/Installer, ever since Visual Studio 2017. As the path suggests, it’s a part of the installer bundle, not Visual Studio itself. The installer will be automatically installed at first run and the installation path cannot be changed.

Is this documented and stated as a public API? The alternative is to use the Visual Studio COM APIs, which are documented public API (and avoids the path issues).

  1. The vswhere and Clang typically gives the same result. Clang has additional logic to handle MSVC version in triple, which is unsupported in Swift yet. It also prefers LINK and LIB environment variables, which I believe should be part of PackageModel: partially address #5719 #5981 instead.

Sounds like you agree - typically. The problem case is where they do not agree, and if that happens, we get into a state that is difficult to diagnose. I think that honoring LINK and LIB is the right thing to do, and I believe that they should already be honored through clang. If not, that is certainly an oversight and not intentional. Would you be up to add some additional test cases to ensure that we do properly honor them?

  1. Clang is also searching for Visual Studio installation, not the standalone MSVC toolset. If you use fancy way to install MSVC instead of vs_installer.exe, you should set VCINSTALLDIR in the environment. Installation detection won’t work.

What this says to me is that the vswhere approach is the degenerate case then. We can support just the one way of you specifying the VCINSTALLDIR environment variable, but I don't think that is much better than requiring that you run from within the VSDevEnv environment.

@tomerd tomerd added the windows label Feb 6, 2023
@stevapple
Copy link
Contributor Author

I'd really rather that you would stop with the personal attacks. This seems to be a repetitive thing and really does not help create a pleasant environment.

I believe I didn't use offensive words here, and if you feel offended I sincerely apologize.

For me, Swift is a general-purpose tool, and I'm always trying to lower the barrier of using Swift in general cases. Edge cases are important, but I just don't think a feature can be in a perfect stage when it first gets in the mainstream. That's not software engineering in these days.

Would you care to elaborate how it is "more powerful and robust"?

I'll try to explain how Clang handles MSVC detection. Since that's going to be long, see the following comment.

Is this documented and stated as a public API?

Yes it is: https://learn.microsoft.com/en-us/visualstudio/install/tools-for-managing-visual-studio-instances?view=vs-2022

The alternative is to use the Visual Studio COM APIs, which are documented public API (and avoids the path issues).

Basically that's how vswhere works. You can browse the source code at https://github.com/microsoft/vswhere.

Sounds like you agree - typically. The problem case is where they do not agree, and if that happens, we get into a state that is difficult to diagnose. I think that honoring LINK and LIB is the right thing to do, and I believe that they should already be honored through clang. If not, that is certainly an oversight and not intentional. Would you be up to add some additional test cases to ensure that we do properly honor them?

Will be mentioned in the following comment.

What this says to me is that the vswhere approach is the degenerate case then. We can support just the one way of you specifying the VCINSTALLDIR environment variable, but I don't think that is much better than requiring that you run from within the VSDevEnv environment.

I believe it's not. Like you already mentioned, requiring VCINSTALLDIR doesn't make difference from requiring VSDevEnv. vswhere is not in Path by default, but it can be called from a static path, which should work at almost any situations, including VS Code, PowerShell and other developer tools.

@stevapple
Copy link
Contributor Author

Now let's dive in how Clang detects MSVC. Visual Studio has a refreshed interface since its 2017 release, and older ones are called "Legacy Instances". Swift has never supported legacy Visual Studio — and I suppose it never will.

Clang detects MSVC from the following functions in order:

  • findVCToolChainViaCommandLine: This function checks if Windows SDK path, VC tools path and MSVC tools version are set from command-line arguments, and searches for a toolchain accordingly. This is currently a missing piece of Swift, as it simply ignores related flags and MSVC version set by the triple. I think that's worth a bug report because, for triples with MSVC version, Swift may diverge from Clang. That's not something we can resolve before the Swift compiler supports it.
  • findVCToolChainViaEnvironment: This function checks if VCToolsInstallDir or VCINSTALLDIR is set, and searches for a toolchain there. Clang uses VCToolsInstallDir to judge if a Visual Studio instance is legacy, but SwiftPM doesn't have to. Clang also searches for Path here — that's why I think VS detection should shadow regular env search once we prove it's fully ready.
  • findVCToolChainViaSetupConfig: This calls the Visual Studio Setup APIs via COM, and is mostly identical to what vswhere does. The only difference is, Clang finds the newest Visual Studio and checks if it has MSVC tools installed; vswhere can find the newest Visual Studio with MSVC tools directly. That's where I think vswhere shines with robustness: If you have VS 2022 without MSVC and VS 2019 with it, vswhere can find it while Clang cannot.
  • findVCToolChainViaRegistry: This function checks the default Visual Studio installation from registry. This is a legacy approach so we won't need it for Swift.

@xwu
Copy link
Contributor

xwu commented Feb 12, 2023

@stevapple Is it possible to exhaustively enumerate all the cases where your approach would differ from Clang and demonstrate that they can be diagnosed?

@stevapple
Copy link
Contributor Author

@xwu I could only talk about the latest Clang implementation, which is already, exhaustively I believe, elaborated in #6120 (comment). Do you have any question?

Since Clang is evolving itself, the behavior may change, so using Clang API should be better, but I didn't know how to do that in SwiftPM easily. I'd like to first introduce a solution to make sure Swift 5.8 release won't break our clients.

@dafedidi
Copy link

Adding a potential solution that might be intersting instead of vswhere.

This way, we can reduce one more dependencies and implement it directly (it's around 500 lines without any dependancies). Let me know if it could help @compnerd @stevapple

@stevapple
Copy link
Contributor Author

@dafedidi I’m sorry but it seems such solution would not make things better… To use it we need to handle COM calls from Swift (which is possible though SwiftCOM but that’s another dependency), and it’s basically re-implementing a less-robust vswhere, which is not we want.

As I had stated, vswhere is not a typical “new dependency”. It’s documented and guaranteed to be there as long as you’re able to run Swift today. The risk of using vswhere to detect MSVC is that it introduces potential divergence with Clang.

@compnerd
Copy link
Member

@dafedidi - I think that using the COM interfaces would definitely be something I would be in favour of. We already do that in the installer. But that is a well documented interface that has guaranteed stability unlike executing a "random" executable and parsing the output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants