Skip to content
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

Automatically detect MSVC tools on Windows via vswhere #11496

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Nov 25, 2021

Simmilar to #11495, except it uses vswhere to detect the installation path. vswhere is looked up in the following sequence:

  • The same directory as the compiler currently running
  • The default location (%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe), valid since VS2017 15.2
  • PATH

This does not fall back to the COM interfaces. Everything other than retrieving the list of Visual Studio installations is exactly the same. Note that as long as a sufficiently new version of Visual Studio is installed, everything will work even if we don't redistribute vswhere.exe ourselves.

@oprypin
Copy link
Member

oprypin commented Nov 25, 2021

Hm so this cuts off less than half of the size of the previous pull request, so I'm still not sure about it, when this is the alternative (and I know it could be directly ported to Crystal): https://github.com/neatorobito/scoop-crystal/blob/3b33560/scripts/crystal.ps1

@HertzDevil
Copy link
Contributor Author

I think it's best not to simply invoke the MSVC developer command prompt on behalf of the user even if it is only for a single linker command. Explicitly capturing the paths ourselves means the linker command as outputted by the --verbose compiler flag also works outside the MSVC prompt.

@oprypin
Copy link
Member

oprypin commented Nov 26, 2021

I see. That's a good point.

Then we'll need to keep considering this option. Or even the other pull request as the main option.
I said

this cuts off less than half of the size of the previous pull request

If we're already going to all these lengths, may as well also reimplement vswhere :/

@straight-shoota
Copy link
Member

Yeah, considering a big part of the diff to #11495 is lib bindings, I think it would be acceptable to go the little extra way so we don't depend on vswhere.exe.

On the other hand, is there an actual downside of using vswhere.exe?

@oprypin
Copy link
Member

oprypin commented Nov 26, 2021

On the other hand, is there an actual downside of using vswhere.exe?

Certainly! You need to most likely ship it with Crystal

@oprypin
Copy link
Member

oprypin commented Nov 26, 2021

Or, well, maybe not, according to the main post of this PR

@straight-shoota
Copy link
Member

I understand it is available in %ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe if you have VS 2017 or newer installed. So we shouldn't have to ship it. I'm not even sure if anything below VS 2019 works with Crystal.

Copy link
Member

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

Looking great overall!

src/compiler/crystal/compiler.cr Show resolved Hide resolved
src/crystal/system/win32/visual_studio.cr Show resolved Hide resolved
src/crystal/system/win32/visual_studio.cr Outdated Show resolved Hide resolved
src/crystal/system/win32/visual_studio.cr Outdated Show resolved Hide resolved
src/crystal/system/win32/visual_studio.cr Outdated Show resolved Hide resolved
src/crystal/system/win32/visual_studio.cr Outdated Show resolved Hide resolved
src/compiler/crystal/compiler.cr Outdated Show resolved Hide resolved
src/compiler/crystal/compiler.cr Outdated Show resolved Hide resolved
src/crystal/system/win32/visual_studio.cr Outdated Show resolved Hide resolved
src/crystal/system/win32/windows_sdk.cr Outdated Show resolved Hide resolved
# cross-compiling and therefore we should attempt detecting MSVC's
# standard paths
{% if flag?(:msvc) %}
if msvc_path = Crystal::System::VisualStudio.find_latest_msvc_path
Copy link
Member

Choose a reason for hiding this comment

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

The not so nice thing is that now we're unconditionally invoking vswhere.exe on every invokation of Crystal.

And on my VM it takes almost a minute to finish, consistently.

> .\crystal2.exe eval 'p Time.local'
{"finding latest msvc path": 2021-11-30 15:50:13.593808900 -08:00 Local}
{"finding vs installations": 2021-11-30 15:50:13.615752800 -08:00 Local}
{"finding vswhere": 2021-11-30 15:50:13.617481600 -08:00 Local}
{"found vswhere": 2021-11-30 15:50:13.619154100 -08:00 Local}
{"called vswhere": 2021-11-30 15:50:14.535144200 -08:00 Local}
{"found vs installations": 2021-11-30 15:50:14.537205000 -08:00 Local}
{"checking a version": 2021-11-30 15:50:14.539411300 -08:00 Local}
{"finding win sdk libpath": 2021-11-30 15:50:14.541482200 -08:00 Local}
{"found win sdk libpath": 2021-11-30 15:50:14.543453600 -08:00 Local}
LINK : C:\Users\User\AppData\Local\crystal\cache\crystal-run-eval.tmp.exe not found or not built by the last incremental link; performing full link
2021-11-30 15:50:14.857111400 -08:00 Local

Very strangely, calling vswhere manually in the same terminal somehow doesn't take so long!

> Get-Date -Format HH:mm:ss.fff; & 'C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe' -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -format json -sort | out-null; Get-Date -Format HH:mm:ss.fff
15:54:21.255
15:54:21.318

Does anyone have any idea about the difference?

diff --git a/src/compiler/crystal/compiler.cr b/src/compiler/crystal/compiler.cr
index ebfa4a292..bbbf6998a 100644
--- a/src/compiler/crystal/compiler.cr
+++ b/src/compiler/crystal/compiler.cr
@@ -357,8 +357,11 @@ module Crystal
         # cross-compiling and therefore we should attempt detecting MSVC's
         # standard paths
         {% if flag?(:msvc) %}
+          p "finding latest msvc path": ::Time.local
           if msvc_path = Crystal::System::VisualStudio.find_latest_msvc_path
+            p "finding win sdk libpath": ::Time.local
             if win_sdk_libpath = Crystal::System::WindowsSDK.find_win10_sdk_libpath
+              p "found win sdk libpath": ::Time.local
               host_bits = {{ flag?(:bits64) ? "x64" : "x86" }}
               target_bits = program.has_flag?("bits64") ? "x64" : "x86"

diff --git a/src/crystal/system/win32/visual_studio.cr b/src/crystal/system/win32/visual_studio.cr
index f26254c69..773f78af9 100644
--- a/src/crystal/system/win32/visual_studio.cr
+++ b/src/crystal/system/win32/visual_studio.cr
@@ -16,8 +16,11 @@ module Crystal::System::VisualStudio
   def self.find_latest_msvc_path : ::Path?
     # ported from https://github.com/microsoft/vswhere/wiki/Find-VC
     # Copyright (C) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
+    p "finding vs installations": ::Time.local
     if vs_installations = get_vs_installations
+      p "found vs installations": ::Time.local
       vs_installations.each do |installation|
+        p "checking a version": ::Time.local
         version_path = ::File.join(installation.directory, "VC", "Auxiliary", "Build", "Microsoft.VCToolsVersion.default.txt")
         next unless ::File.file?(version_path)

@@ -30,8 +33,11 @@ module Crystal::System::VisualStudio
   end

   private def self.get_vs_installations : Array(Installation)?
+    p "finding vswhere": ::Time.local
     if vswhere_path = find_vswhere
+      p "found vswhere": ::Time.local
       vc_install_json = `#{::Process.quote(vswhere_path)} -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -sort -format json`.chomp
+      p "called vswhere": ::Time.local
       return if !$?.success? || vc_install_json.empty?

       Array(Installation).from_json(vc_install_json)

Copy link
Member

Choose a reason for hiding this comment

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

Both for this reason and because maybe it's nice anyway to use cl.exe if it's already in path, do you think we should change the behavior to be like that - don't try to locate anything if cl.exe is already available?

It's just a question, not a blocking comment. My approval still stands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but it can fail if e.g. we use link.exe directly instead because that often refers to coreutils link from Git Bash (such as on CI), so PATH should have the lowest priority.

Copy link
Member

Choose a reason for hiding this comment

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

@oprypin It's almost a second. Not minute. But yeah, that's still quite a bunch.

As an option, we could accept CL as an environment variable and if that is set, we don't need to look up the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might be able to detect the MSVC developer command prompt too by checking for the VSCMD_VER environment variable. In that case it is probably reasonable to use the defaults.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that in a follow-up

Copy link
Member

Choose a reason for hiding this comment

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

@straight-shoota Of course I meant a second. No idea how that slip happened.

Copy link
Member

Choose a reason for hiding this comment

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

Time is relative, so... 🤷 🤣

@oprypin oprypin added this to the 1.3.0 milestone Dec 1, 2021
@straight-shoota straight-shoota merged commit c30ce92 into crystal-lang:master Dec 2, 2021
@HertzDevil HertzDevil deleted the feature/windows-detect-msvc2 branch December 3, 2021 00:38
oprypin added a commit to crystal-lang/install-crystal that referenced this pull request Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants