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

Detect VS2017 #1103

Closed
wants to merge 1 commit into from
Closed

Detect VS2017 #1103

wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented Feb 2, 2017

Based on @bzoz's #1101 but without python `comtypes'

Also bumped gyp to latest version and updated out patch files

@refack
Copy link
Contributor Author

refack commented Feb 3, 2017

(changed COM tool from an exe to a powershell script)

@joaocgreis
Copy link
Member

Node-gyp must support Windows 7/2008R2 while Node does, and those versions of Windows ship with PowerShell 2.0. The module you use here seems to require at least version 3. We could ask users to update PowerShell, but there have been issues in the past from users locked in corporate environments with no permissions to install anything, so this might be troublesome to most and impossible to some.

Using an executable opens an issue of trust in the committed file. We could get around this by setting up a job in node's release infrastructure to compile and sign an executable to use here, but only if must be.

@refack what issue do you see with comtypes? I'd rather not add the dependency, but so far it seems to be the easiest for all users. Either way we decide to go, thanks for submitting this, it's great to be able to broaden discussion by having more alternatives!

@refack
Copy link
Contributor Author

refack commented Feb 7, 2017

I'll check if I can back port the script...
Keep in mind, it's a fall back to try to detect VS2017 specificaly, doesn't it's installation upgrades powershell anyway?

  • as for the exe, I get it.
    ** as for comtypes I think it's major overkill, and the "prevailing winds" is to reduce dependency on python, not increase it for this "tiny" COM query 🤔😕

@bzoz
Copy link
Contributor

bzoz commented Feb 8, 2017

@refack I just checked this on Windows 7 - after installing VS2017 and selecting Desktop Development with C++, powershell is still at version 2

@refack
Copy link
Contributor Author

refack commented Feb 8, 2017

Yep just did that too 😣 (also encountered the ExecutionPolicy issue)
But i'm not giving up... I'll find a way!

@refack
Copy link
Contributor Author

refack commented Feb 8, 2017

P.S. same config (VS2017 build tools on Windows 7SP1):

gyp ERR! stack Error: Command failed: c:\bin\dev\python27\python.EXE \\REFAELUX\code\3party\node-gyp\find_vs2017.py
gyp ERR! stack Traceback (most recent call last):
gyp ERR! stack   File "\\REFAELUX\code\3party\node-gyp\find_vs2017.py", line 173, in <module>
gyp ERR! stack     main()
gyp ERR! stack   File "\\REFAELUX\code\3party\node-gyp\find_vs2017.py", line 158, in main
gyp ERR! stack     installs = GetVS2017CPPBasePath()
gyp ERR! stack   File "\\REFAELUX\code\3party\node-gyp\find_vs2017.py", line 106, in GetVS2017CPPBasePath
gyp ERR! stack     iface = CreateObject(GUID('{177F0C4A-1CD3-4DE7-A32C-71DBBB9FA36D}'))
gyp ERR! stack   File "\\REFAELUX\code\3party\node-gyp\comtypes\comtypes\client\__init__.py", line 238, in CreateObject
gyp ERR! stack     obj = comtypes.CoCreateInstance(clsid, clsctx=clsctx, interface=interface)
gyp ERR! stack   File "\\REFAELUX\code\3party\node-gyp\comtypes\comtypes\__init__.py", line 1225, in CoCreateInstance
gyp ERR! stack     _ole32.CoCreateInstance(byref(clsid), punkouter, clsctx, byref(iid), byref(p))
gyp ERR! stack   File "_ctypes/callproc.c", line 945, in GetResult
gyp ERR! stack WindowsError: [Error -2147221164] Class not registered

Apparently the COM interface doesn't get registered

@refack
Copy link
Contributor Author

refack commented Feb 8, 2017

Unless you install the .NET 4.6 SDK

@joaocgreis
Copy link
Member

@refack any luck with PowerShell v2?

@refack
Copy link
Contributor Author

refack commented Feb 13, 2017

@joaocgreis just cracked it (and added an undocumented Registry approach as backup)

@joaocgreis
Copy link
Member

I've tested, and I could not get this to work because of spaces in the path. Also, the registry key approach does not provide the SDK.

@heaths this looks for the path in the registry at HKLM\Software\Microsoft\VisualStudio\SxS\VS7. Can we trust the value in this registry key? Is it likely to be removed in the future?

@heaths
Copy link

heaths commented Feb 15, 2017

Paths could always have spaces. Make sure the path is quoted and you shouldn't have a problem. No registry values are supported for VS2017. Whatever you found is probably just a left-over that should've been removed. MSIs are singletons and may have nothing to do with where VS instances are installed.

@refack
Copy link
Contributor Author

refack commented Feb 15, 2017

Fixed path escaping issue, and now the code works with Powershell "core".
Registry "hack" is a backup measure anyway ...

P.S. I refactored my code into a package (it's has no dependecies of it's own) what might be the push back of adding it as a dependency?

@refack refack force-pushed the detect-VS2017 branch 2 times, most recently from a9cabb9 to 0376bd6 Compare February 16, 2017 20:52
@refack
Copy link
Contributor Author

refack commented Feb 16, 2017

As my detection package is getting traction, and bug fixes, I believe it's worth the add.

@bnoordhuis @kunalspathak I think now it's mature enough to cc you :)

@refack refack mentioned this pull request Feb 16, 2017
@heaths
Copy link

heaths commented Feb 16, 2017

While I appreciate the community working together to fill gaps - being a long-time open source contributor myself - I'm curious how using the batch scripts are easier than acquiring the PowerShell module:

install-module vssetup -scope currentuser # only needed once
$installationPath = get-vssetupinstance | select-vssetupinstance -latest | select-object -expand InstallationPath

I will be keeping these up to date with new functionality as we bring it online.

@AndrewPardoe
Copy link

First, I agree with Heath: The PowerShell solution is probably best for most users.

I had a discussion with the Boost folks that lays out the options. This thread may be interesting to you: boostorg/build#157

@refack
Copy link
Contributor Author

refack commented Feb 17, 2017

@heaths It's a change to the underlying system. While reasonable for developers, node-gyp is an automation module that runs as part of setup of different systems.
Also one tenet of the node.js way is "Keep everything local / Leave no trace" emblemized with the ubiquitous node_molues directory (with all it's pros and cons).
Second: it require access to the internet, and permission to install such things. Again, not an assumption node-gyp can take.
If you can escalate these concerns, that would be great. I believe the "C++ Build Tools" configuration is an example of helping these use cases.

@heaths
Copy link

heaths commented Feb 17, 2017

@refack FWIW, I'm working to finish up a single native binary that can return information in a number of formats. A prototype actually ships in the tools subdirectory of the Microsoft.VisualStudio.Setup.Configuration.Native NuGet package, which is what several VC projects are using. You might take a look at that, as the eventual new project will also grow over time. These are also fully supported.

@refack
Copy link
Contributor Author

refack commented Feb 17, 2017

@heaths That's sounds promising. Maybe also also consider implementing IDispatch so we could use it directly with Powershell's New-Object -ComObject and cscript / JScript?

@refack
Copy link
Contributor Author

refack commented Feb 17, 2017

@heaths Where's the best place to leave feedback on "Microsoft.VisualStudio.Setup.Configuration.Native"? I have one request; we need the version number for installed SDKs so we could populate the <WindowsTargetPlatformVersion>10.0.14393.0</WindowsTargetPlatformVersion> field in vcxproj files

@refack
Copy link
Contributor Author

refack commented Feb 17, 2017

P.S. IMHO nuget is very node.jsy 😜

@AndrewPardoe
Copy link

AndrewPardoe commented Feb 17, 2017

@refack, given that NuGet and node.js were designed around the same time, that's not a surprise. They both reflect how the world was thinking about packages at the time.

Given node.js is a year earlier, there might be inspiration, but I'm not the one to comment on that.

@heaths
Copy link

heaths commented Feb 17, 2017

@refack re: PowerShell, people should just use the VSSetup module. Even with IDispatch support, enumerating instances through the IEnumXXX-style interface is a PITA and doesn't jive with PowerShell. The point of the module was to make something easily acquirable and easy to use. If you don't want to take a dependency on a network connection, it's license to redistribute.

We can consider adding IDispatch for scripting support, but PowerShell was meant to replace all that. It would be a lower priority item.

re: feedback, consider it heard. We can consider how to do that, but the data on which the query API works doesn't store that. The more data we store, we slow the query API down. That's only one type of question to ask, but I'm sure other people have others. We can't really store every conceivable answer. Instead, it would be better that the Windows SDK - which is actually a system singleton (i.e. it's still MSI-based and will be for the foreseeable future since it's so big and used by things outside VS) - write something like that. I will pass along feedback.

For now, you could do something like the following as long as their package ID pattern holds:

get-vssetupinstance | select -expand Packages | where Id -match 'Win10SDK_\d' | select Version

@refack
Copy link
Contributor Author

refack commented Feb 17, 2017

@heaths Thanks again for listening. (as for figuring out the SDK that's almost exaclty what I'm doing in my CS script)

@joaocgreis
Copy link
Member

@heaths we must support Windows 7 and 2008R2 as shipped, with Powershell v2. I did not manage to get the VSSetup module to work with it, is there a way? I'm not thrilled about landing a binary either - even if we can trust the binary's content we might run into issues with antivirus software because node-gyp must be usable as downloaded from the internet (e.g. after updating npm). Currently, the solution here, using the COM API, seems to be the best one.

@refack can you move the gyp bump to a different PR, or is it required for this? Also, the commit with the Gyp floating patch that you had before looked good, can you bring it back (or is there a reason not to)?

About using a package: @refack can you commit to always supporting the same versions of Windows that node does? @nodejs/node-gyp are there other reasons why we might not want this as a package?

@refack
Copy link
Contributor Author

refack commented Feb 20, 2017

I can do the minimal patch, give me a few minutes

@refack
Copy link
Contributor Author

refack commented Feb 21, 2017

Did a minimal patch, no package.
But I shall return
image
Muhhhhaaaaaaa

@heaths
Copy link

heaths commented Feb 24, 2017

I have published a release of vswhere - a single-file, native executable you can redistribute - at https://github.com/Microsoft/vswhere. It currently emits data in plan text and JSON and would be fairly easy to use. For example, to find the root path to the latest version installed for native tools:

for /r "usebackq tokens=1* delims=: " %%i in (`vswhere.exe -latest -requires Microsoft.VisualStudio.Workload.NativeDesktop`) do (
  if /i "%%i"=="installationPath" set dir=%%j
)

rem %dir% now contains the root installation path, if available

@refack
Copy link
Contributor Author

refack commented Feb 24, 2017

I tweaked the .bat file to be more "polite" if it can't find the COM.
cc @joaocgreis

@joaocgreis
Copy link
Member

Reviewed and opened #1130 with a slightly modified version.

@refack
Copy link
Contributor Author

refack commented Feb 27, 2017

I brough back windows-autoconf, since it now has general MSVS detection logic, which enabled me to remove almost half of build.js.
I have it CI on a windows machine at AppVeyor AppVeyor
I'm working on eliminating more of node-gyp's VS decection code, and adding it's test suite as a dependency for passing the CI...

@refack
Copy link
Contributor Author

refack commented Mar 5, 2017

@bnoordhuis @joaocgreis
Can we have vote this vs node-gyp#1130 ? See all the red lines, code you don't need to worry about ;)
cc @nodejs/node-gyp @nodejs/build

@refack
Copy link
Contributor Author

refack commented Mar 7, 2017

CI now passes node-gyp test suite AppVeyor

@refack
Copy link
Contributor Author

refack commented Mar 11, 2017

Getting better coverage from AppVeyor VS2015 and vs2017 pass node-gyp's test suite

@refack refack closed this Mar 17, 2017
@refack refack deleted the detect-VS2017 branch March 17, 2017 03:31
@refack
Copy link
Contributor Author

refack commented Apr 15, 2017

Great news we landed VS2017 support to GYP https://chromium-review.googlesource.com/c/453201/

@AndrewPardoe
Copy link

The officially supported vswhere tool is now available as part of the install starting today with Visual Studio 15.2 preview 2: https://blogs.msdn.microsoft.com/heaths/2017/04/21/vswhere-is-now-installed-with-visual-studio-2017/. This doesn't fix all the existing installs of VS 2017 but going forward you can now detect the VS install locations and whether C++ tools are available from a scripted environment.

@refack
Copy link
Contributor Author

refack commented Apr 20, 2017

@AndrewPardoe
Copy link

@refack I wasn't, but I'm searching for all the places online where vswhere was mentioned and trying to link to this post : ) Thank you for the list!

duqingnian pushed a commit to duqingnian/gyp that referenced this pull request Apr 26, 2024
Patch Set 4:

Sorry, but this is not the way to do that. Registry key approach was discussed downstream in nodejs, Heath Stewart who is responsible for the VS installer said explicitly that the registry entry is not supported (nodejs/node-gyp#1103 (comment))

In node-gyp we used Rafael's powershell script that can query VS COM server (nodejs/node-gyp#1130). We used that to find VS and set environment variables. IMHO we should do the same thing here.

Patch-set: 4
duqingnian pushed a commit to duqingnian/gyp that referenced this pull request Apr 26, 2024
Patch Set 4:

> Patch Set 4:
> 
> Sorry, but this is not the way to do that. Registry key approach was discussed downstream in nodejs, Heath Stewart who is responsible for the VS installer said explicitly that the registry entry is not supported (nodejs/node-gyp#1103 (comment))
> 
> In node-gyp we used Rafael's powershell script that can query VS COM server (nodejs/node-gyp#1130). We used that to find VS and set environment variables. IMHO we should do the same thing here.

We could also assume the default install location and allow users to set an environment variable if they have a non-default install. That is what Chrome is doing - the complexity of the COM solution didn't seem justified, at least initially.

Patch-set: 4
Reviewer: Gerrit User 1128439 <1128439@3ce6091f-6c88-37e8-8c75-72f92ae8dfba>
duqingnian pushed a commit to duqingnian/gyp that referenced this pull request Apr 26, 2024
Patch Set 4:

> Patch Set 4:
> 
> Sorry, but this is not the way to do that. Registry key approach was discussed downstream in nodejs, Heath Stewart who is responsible for the VS installer said explicitly that the registry entry is not supported (nodejs/node-gyp#1103 (comment))
> 
> In node-gyp we used Rafael's powershell script that can query VS COM server (nodejs/node-gyp#1130). We used that to find VS and set environment variables. IMHO we should do the same thing here.

Obviously, but I like this solution for this project, it's good enough...
...
...
...
...
...
...
You know what, I'll make another PR with the full script

Patch-set: 4
Reviewer: Gerrit User 1188132 <1188132@3ce6091f-6c88-37e8-8c75-72f92ae8dfba>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants