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.

Sign up for GitHub

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

Jump to bottom

Fix: find visual studio arm64 support #2810

Closed
wants to merge 1 commit into from
Closed

Fix: find visual studio arm64 support #2810

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion lib/find-visualstudio.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,19 @@ VisualStudioFinder.prototype = {
// Invoke the PowerShell script to get information about Visual Studio 2017
// or newer installations
findVisualStudio2017OrNewer: function findVisualStudio2017OrNewer (cb) {
var ps = path.join(process.env.SystemRoot, 'System32',
// 32-bit PowerShell (available in SysWOW64) is needed on ARM64 because of
// the COM classes and interfaces used by Find-VisualStudio.cs, which are
// not registered for being used from ARM64 processes.
//
// This fix is required for the clean installation of Windows 11. Old
// versions (e.g. Windows 10 upgraded to 11) work without it.
//
// Since VS v17.4 and later is natively supported on ARM64 it seems to
// register the ARM64 COM components correctly, but VS 2019 and 2017 will
// still have problems being detected. Once support for VS 2017 and 2019
// is dropped, this change can be reverted.
var systemDirectory = process.arch === 'arm64' ? 'SysWOW64' : 'System32'
Copy link
Member

Choose a reason for hiding this comment

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

It used to work for arm64. Should some compatibility need to do here ? Like SysWOW64 available in all windows arm64 distribution.

Copy link
Member

Choose a reason for hiding this comment

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

Also from what I tested, currently works

I am on windows virtual machine with latest visual studio

PS C:\Users\jiawengeng\node-gyp\lib> node -p process.arch 
arm64
PS C:\Users\jiawengeng\node-gyp\lib> node .\find-visualstudio.js
info find VS using VS2022 (17.5.33424.131) found at:
info find VS "C:\Program Files\Microsoft Visual Studio\2022\Enterprise"
info find VS run with --verbose for detailed information

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 used to work for arm64.

That makes sense to me. I ran the following command from bash on two ARM64 machines:

  • Command:
file /c/Windows/*/WindowsPowerShell/v1.0/powershell.exe
  • Machine 1: Originally installed Windows 10, was upgraded to Windows 11
/c/Windows/SysArm32/WindowsPowerShell/v1.0/powershell.exe: PE32 executable (console) Intel 80386, for MS Windows
/c/Windows/SysWOW64/WindowsPowerShell/v1.0/powershell.exe: PE32 executable (console) Intel 80386, for MS Windows
/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe: PE32 executable (console) Intel 80386, for MS Windows
  • Machine 2: Originally installed Windows 11
/c/Windows/SysArm32/WindowsPowerShell/v1.0/powershell.exe: PE32 executable (console) ARMv7 Thumb, for MS Windows
/c/Windows/SysWOW64/WindowsPowerShell/v1.0/powershell.exe: PE32 executable (console) Intel 80386, for MS Windows
/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe: PE32+ executable (console) Aarch64, for MS Windows

Can you please run the same command and check what the output will be on your machine? Only Intel 80386 should be able to find Visual Studio, so as expected the fix proposed in this PR is not required on machine 1 but it is for machine 2.

Should some compatibility need to do here ? Like SysWOW64 available in all windows arm64 distribution.

After you do a clean install, SysWOW64 is present on Windows on ARM.

Also from what I tested, currently works

I am on windows virtual machine with latest visual studio

This is in line with nodejs/node#46995 (comment). Since VS v17.4 and later is natively supported on ARM64 I assume it registers the ARM64 COM components correctly, but VS 2019 and 2017 will still have problems being detected.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to the code too? It would be beneficial in the long run to have this knowledge on the code as well.

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've updated the comment in the code with the most important info from my previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gengjiawen is there something else I can do to help this move forward?

Copy link
Member

@gengjiawen gengjiawen Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am Neutral on this since I am skeptical on vs support on arm in older versions based on my experience (Rarely fix bugs and lack support from official). I am more inclined drop VS2019 on arm instead patch on it.

@rvagg Second Opinion ?

var ps = path.join(process.env.SystemRoot, systemDirectory,
'WindowsPowerShell', 'v1.0', 'powershell.exe')
var csFile = path.join(__dirname, 'Find-VisualStudio.cs')
var psArgs = [
Expand Down