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

GitHub Actions: Enable os: windows-latest #8

Closed
wants to merge 2 commits into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Dec 2, 2019

No description provided.

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Great! Thanks for this. Same as #7, this still breaks the build, no?

@targos
Copy link
Member

targos commented Mar 15, 2020

I can reproduce the test failure on Windows.

@ryzokuken
Copy link
Contributor

ryzokuken commented Mar 15, 2020

@targos can you please rebase this? If not, I'd be happy to.

@targos
Copy link
Member

targos commented Apr 3, 2020

I checked with my computer and it seems that part of the code is really broken on recent versions of Visual Studio. The registry values that are checked do not exist on my machine.
Related: https://developercommunity.visualstudio.com/content/problem/2813/cant-find-registry-entries-for-visual-studio-2017.html

@cclauss
Copy link
Contributor Author

cclauss commented Apr 3, 2020

I am not a Windows user so it is difficult for me to fix this so please try to make progress on it. Given the simple mods, please feel free to create another PR and close this one if that is easier.

@ryzokuken
Copy link
Contributor

@targos could you please look into this issue? Would it take a non-trivial amount of effort to fix? It seems like the upstream PR at nodejs/node#32698 is blocked on this.

@targos
Copy link
Member

targos commented Apr 14, 2020

I have looked into this and it requires work that I can't do myself.

This is not related to the error we saw in your PR

@ryzokuken
Copy link
Contributor

@targos it's not related but I think we require CI on Windows in order to fix that issue.

@vweevers
Copy link

nodejs/node-gyp#1762 (comment) suggested gyp can use vswhere for VS detection:

This [node-gyp's visual studio detection] is only needed for node-gyp AFAICT, so for Node core and other projects GYP3 could simply use VSWhere, require the C++ Workload, and pick the latest version. That would make it quite a lot simpler.

@targos
Copy link
Member

targos commented Apr 14, 2020

Is there a common location for vswhere.exe? It is not on my PATH by default and I have Visual Studio Community 2019.

@targos
Copy link
Member

targos commented Apr 14, 2020

OK, found it: https://github.com/microsoft/vswhere

%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe

@targos
Copy link
Member

targos commented Apr 14, 2020

I suppose we need to run something like this:

C:\Program Files (x86)\Microsoft Visual Studio\Installer>vswhere.exe -version [16.0,17.0) -property installationPath
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community

@targos
Copy link
Member

targos commented Apr 14, 2020

Something we could do is bundle the executable with gyp-next. It is one of the recommended ways (vswhere is MIT-licensed).

/cc @joaocgreis what's the oldest version of Visual Studio that we need to support in node-gyp?
Assuming that we have vswhere.exe distributed with gyp-next, is it able to find all supported versions?

@ryzokuken
Copy link
Contributor

Something we could do is bundle the executable with gyp-next. It is one of the recommended ways (vswhere is MIT-licensed).

Sounds like a pretty decent idea to me.

@cclauss cclauss closed this Oct 17, 2020
@cclauss cclauss deleted the Enable-os-windows-latest branch October 17, 2020 06:44
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.

None yet

4 participants