Skip to content

Find correct x64 host on arm64 machines #207

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

Merged
merged 3 commits into from
Jun 14, 2022
Merged
Changes from 1 commit
Commits
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
71 changes: 60 additions & 11 deletions src/clickonce/launcher/HostFinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@

namespace Microsoft.Deployment.Launcher
{
internal static class HostFinder
internal class HostFinder
{
private readonly string applicationFilePath;
private string arch;

/// <summary>
/// Gets full path to .NET host appropriate for activating the application.
///
/// .NET host'slocation can be obtained from multiple locations.
/// .NET host's location can be obtained from multiple locations.
///
/// Current code searches in default, global shared runtime, location only:
/// %ProgramFiles%\dotnet and %ProgramFiles(x86)%\dotnet
Expand All @@ -28,10 +31,48 @@ internal static class HostFinder
/// 3) Global location
/// </summary>
/// <param name="applicationFilePath">Full path to application</param>
/// <returns></returns>
/// <returns>Path to host</returns>
internal static string GetHost(string applicationFilePath)
{
string arch = GetProcessorArchitectureFromAssembly(applicationFilePath);
HostFinder hf = new HostFinder(applicationFilePath);
return hf.GetHost();
}

/// <summary>
/// Checks if running on Arm64 system
/// </summary>
private bool IsArm64System
{
get
{
string proc = Environment.GetEnvironmentVariable("PROCESSOR_ARCHITECTURE");

Choose a reason for hiding this comment

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

Can you use System.Runtime.InteropServices.RuntimeInformation.ProcessArchitecture instead of the environment variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

For OS Architecture, there is the RuntimeInformation.OSArchitecture property.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed the commit that fixes this.

if (!string.IsNullOrEmpty(proc) && proc.ToLower() == "arm64")
{
return true;
}

proc = Environment.GetEnvironmentVariable("PROCESSOR_ARCHITEW6432");
if (!string.IsNullOrEmpty(proc) && proc.ToLower() == "arm64")
{
return true;
}

return false;
}
}

private HostFinder(string path)
{
applicationFilePath = path;
}

/// <summary>
/// Gets full path to .NET host appropriate for activating the application.
/// </summary>
/// <returns>Path to host</returns>
private string GetHost()
{
arch = GetProcessorArchitectureFromAssembly(applicationFilePath);

if (arch == "x86")
{
Expand All @@ -55,7 +96,7 @@ internal static string GetHost(string applicationFilePath)
/// </summary>
/// <param name="is64bit">If 64-bit bitness is required</param>
/// <returns></returns>
private static string GetProgramFilesFolder(bool is64bit = false)
private string GetProgramFilesFolder(bool is64bit = false)
{
return is64bit ?
(Environment.Is64BitProcess ?
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Jun 7, 2022

Choose a reason for hiding this comment

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

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.

/// Current code searches in default, global shared runtime, location only:
/// %ProgramFiles%\dotnet and %ProgramFiles(x86)%\dotnet
///
/// Consider adding support for other non-standard registrations of host location.
/// 1) Environment variables: DOTNET_ROOT or DOTNET_ROOT(x86)
/// 2) Registry: HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\{arch}\[InstallLocation]

string host = !string.IsNullOrEmpty(folder) ? Path.Combine(folder, "dotnet\\dotnet.exe") : string.Empty;
string relativeHostPath = "dotnet\\dotnet.exe";

// On Arm64 systems, x64 host is in "x64" sub-folder
if (is64bit && arch == "amd64" && IsArm64System)
{
relativeHostPath = "dotnet\\x64\\dotnet.exe";
}

string host = !string.IsNullOrEmpty(folder) ? Path.Combine(folder, relativeHostPath) : string.Empty;
return File.Exists(host) ? host : string.Empty;
}

/// <summary>
/// Gets full path to x86 .NET host.
/// </summary>
/// <returns>X86 host</returns>
private static string GetX86Host()
private string GetX86Host()
{
return GetGlobalHost();
}
Expand All @@ -89,7 +138,7 @@ private static string GetX86Host()
/// Gets full path to 64-bit .NET host.
/// </summary>
/// <returns>64-bit host</returns>
private static string Get64bitHost()
private string Get64bitHost()
{
return Environment.Is64BitOperatingSystem ? GetGlobalHost(true) : string.Empty;
}
Expand All @@ -99,7 +148,7 @@ private static string Get64bitHost()
/// </summary>
/// <param name="path">Assembly path</param>
/// <returns></returns>
private static string GetProcessorArchitectureFromAssembly(string path)
private string GetProcessorArchitectureFromAssembly(string path)
{
string processorArchitecture = string.Empty;

Expand All @@ -124,7 +173,7 @@ private static string GetProcessorArchitectureFromAssembly(string path)
processorArchitecture.ToLowerInvariant();
}

private static Guid GetGuidOfType(Type type)
private Guid GetGuidOfType(Type type)
{
var guidAttr = (GuidAttribute)Attribute.GetCustomAttribute(type, typeof(GuidAttribute), false);
return new Guid(guidAttr.Value);
Expand Down