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

Fix: find visual studio arm64 support #2810

Closed

Conversation

StefanStojanovic
Copy link
Contributor

Checklist
Description of change

This enables running find visual studio script on Windows on ARM64.

Find-VisualStudio.cs uses certain COM classes and interfaces to fetch information about installed Visual Studio versions. As it turns out, these components are not registered correctly for ARM64 when installed, thus they cannot be used from the ARM64 process. Because of that, when searching for Visual Studio installations on Windows on ARM64, PowerShell from SysWOW64 is used, since then Find-VisualStudio.cs can create instances of COM class.

This issue was noticed when running Node.js native test suites on Windows on ARM. The change proposed here fixes this issue as seen from this run https://ci.nodejs.org/job/node-test-binary-windows-native-suites/17121/

The same problem was previously reported in other repositories, eg. microsoft/vs-setup-samples#15 and rust-lang/rust#83043

@anonrig
Copy link
Member

anonrig commented Mar 7, 2023

cc @nodejs/gyp

@gengjiawen gengjiawen requested a review from rvagg March 8, 2023 01:31
// SysWOW64 PowerShell 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.
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

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 ?

@cclauss cclauss added the Windows label Mar 8, 2023
@StefanStojanovic StefanStojanovic force-pushed the mefi-find-vs-arm64-fix branch 2 times, most recently from 3dceb59 to 2acb06f Compare March 10, 2023 20:44
This enables running find visual studio script on Windows on ARM64
@cclauss
Copy link
Contributor

cclauss commented Mar 13, 2023

@nodejs/platform-windows-arm

@StefanStojanovic
Copy link
Contributor Author

VS2019 runs emulated on ARM64 and we were using it to develop before. However, after asking Microsoft about this, we found that only VS2022 is officially supported on ARM64, and that's the minimum version that we should support.

Taking into consideration the feedback from @gengjiawen, we can drop this.

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