Find correct x64 host on arm64 machines #207
Conversation
There was a problem hiding this comment.
Made some suggestions. Also cc @MarcoRossignoli and @vitek-karas for best practice on locating dotnet as I believe they have components which also do this.
|
We don't have a specific component but we implemented the search for the |
| @@ -69,18 +110,26 @@ private static string GetProgramFilesFolder(bool is64bit = false) | |||
| /// </summary> | |||
| /// <param name="is64bit">If 64-bit bitness is required</param> | |||
| /// <returns></returns> | |||
| private static string GetGlobalHost(bool is64bit = false) | |||
| private string GetGlobalHost(bool is64bit = false) | |||
| { | |||
| string folder = GetProgramFilesFolder(is64bit); | |||
There was a problem hiding this comment.
I know this is not related to this PR exactly, but this is technically wrong. There's no guarantee that the runtime will be in program files. Ever since 3.1 we added the ability to install to custom "global" location and the location is written in registry. See this doc for the arch specific details: https://github.com/dotnet/designs/blob/main/accepted/2021/install-location-per-architecture.md
and this for the "custom global location" details: https://github.com/dotnet/designs/blob/main/accepted/2020/install-locations.md
There was a problem hiding this comment.
Yes, that is a known issue. I have called this out in the comments, in this source file. We do not currently support custom global host locations, but I believe this will be done before 7.0 RTM.
deployment-tools/src/clickonce/launcher/HostFinder.cs
Lines 18 to 23 in f8a0ab6
|
I think the interesting bit is mostly this function: https://github.com/microsoft/vstest/blob/67d7546f17112c0f07a908554f7e3fd5bc09f265/src/Microsoft.TestPlatform.CoreUtilities/Helpers/DotnetHostHelper.cs#L282 |
src/clickonce/launcher/HostFinder.cs
Outdated
| { | ||
| get | ||
| { | ||
| string proc = Environment.GetEnvironmentVariable("PROCESSOR_ARCHITECTURE"); |
There was a problem hiding this comment.
Can you use System.Runtime.InteropServices.RuntimeInformation.ProcessArchitecture instead of the environment variable?
There was a problem hiding this comment.
Can you use System.Runtime.InteropServices.RuntimeInformation.ProcessArchitecture instead of the environment variable?
It seems that the API returns architecture of the currently running app: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.runtimeinformation.processarchitecture?view=net-6.0&viewFallbackFrom=netframework-4.6.2#system-runtime-interopservices-runtimeinformation-processarchitecture
For ClickOnce deployment, this is not guaranteed to match the OS architecture, even though Launcher is built as MSIL.
There was a problem hiding this comment.
For OS Architecture, there is the RuntimeInformation.OSArchitecture property.
There was a problem hiding this comment.
For OS Architecture, there is the RuntimeInformation.OSArchitecture property.
That property is available in v4.7.1 and later. Launcher targets v4.5, to ensure we can run on as many systems as possible.
There was a problem hiding this comment.
I think the following code can be used as a model, or simply copied:
There was a problem hiding this comment.
There was a problem hiding this comment.
I've pushed the commit that fixes this.
| { | ||
| try | ||
| { | ||
| if (NativeMethods.Kernel32.IsWow64Process2(new IntPtr(-1), out _, out ushort nativeMachine)) |
There was a problem hiding this comment.
Can we call GetCurrentProcess instead of hardcoding to -1?
There was a problem hiding this comment.
I realize it will return a pseudo handle of -1 also.
Fixes: #138
Launcher will find proper x64 host location on arm64 machines.
Since .NET 6, x64 .NET, on arm64 systems, is installed to
\Program Files\dotnet\x64.