Use HOSTFXR_PATH to resolve native hostfxr by mateoatr · Pull Request #18911 · dotnet/sdk

mateoatr · 2021-07-14T18:35:22Z

The runtime property HOSTFXR_PATH is set by the muxer when executing an SDK and stores hostfxr's fullpath. Use this property in the NativeWrapper to resolve hostfxr using a fullpath instead of relying on dlopen/LoadLibrary behavior.

This fixes dotnet/runtime#54965.

dotnet sdk check before changes:

System.DllNotFoundException: Unable to load shared library 'hostfxr' or one of its dependencies. In order to help diagnose loading problems, consider setting the DYLD_PRINT_LIBRARIES environment variable: dlopen(libhostfxr, 1): image not found
   at Microsoft.DotNet.NativeWrapper.Interop.hostfxr_get_dotnet_environment_info(String dotnet_root, IntPtr reserved, hostfxr_get_dotnet_environment_info_result_fn result, IntPtr result_context)
   at Microsoft.DotNet.NativeWrapper.NETBundlesNativeWrapper.GetDotnetEnvironmentInfo(String dotnetExeDirectory) in Microsoft.DotNet.NativeWrapper.dll:token 0x600000f+0x20
   at Microsoft.DotNet.Tools.Sdk.Check.SdkCheckCommand.Execute() in dotnet.dll:token 0x60004fa+0x13
   at Microsoft.DotNet.Cli.DotNetTopLevelCommandBase.RunCommand(String[] args) in dotnet.dll:token 0x60008ca+0x5e
   at Microsoft.DotNet.Tools.Sdk.SdkCommand.Run(String[] args) in dotnet.dll:token 0x60004e8+0x6
   at Microsoft.DotNet.Cli.Program.ProcessArgs(String[] args, TimeSpan startupTime, ITelemetry telemetryClient) in dotnet.dll:token 0x6000933+0x2e8
   at Microsoft.DotNet.Cli.Program.Main(String[] args) in dotnet.dll:token 0x6000931+0x6f

After changes (tested locally on an M1):

.NET SDKs:
Version                         Status                                       
-----------------------------------------------------------------------------
6.0.100-preview.5.21302.13      Patch 6.0.100-preview.6.21355.2 is available.

Try out the newest .NET SDK features with .NET 6.0.100-preview.6.21355.2.

.NET Runtimes:
Name                          Version                       Status                                      
--------------------------------------------------------------------------------------------------------
Microsoft.NETCore.App         6.0.0-preview.5.21301.5       Patch 6.0.0-preview.6.21352.12 is available.
Microsoft.AspNetCore.App      6.0.0-preview.5.21301.17      Patch 6.0.0-preview.6.21352.12 is available.


The latest versions of .NET can be installed from https://aka.ms/dotnet-core-download. For more information about .NET lifecycles, see https://aka.ms/dotnet-core-support.

ghost · 2021-07-14T18:35:25Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sfoslund · 2021-07-14T19:29:44Z

@mateoatr will there be any changes needed on the SDK side for this?

elinor-fung · 2021-07-14T19:35:26Z

-            {
-                PreloadWindowsLibrary("hostfxr.dll");
-            }
+            HostFxrPath = (string)AppContext.GetData("HOSTFXR_PATH");


Check that HostFxrPath? I don't know if the code here can assume that the hosting bits launching it have the HOSTFXR_PATH change.

Technically it should be able to make that assumption (SDK is bound to a version of runtime), but I agree it would be a good idea to somehow handle the error more gracefully.

mateoatr · 2021-07-15T16:55:25Z

@mateoatr will there be any changes needed on the SDK side for this?

I don't think we need any more changes than the ones in this PR.

vitek-karas · 2021-07-16T11:35:23Z

+                if (!string.IsNullOrEmpty(HostFxrPath))
+                    NativeLibrary.SetDllImportResolver(typeof(Interop).Assembly, HostFxrDllImportResolver);


We might want to fail if he hostfxr path is not available. Mainly because we know it is somewhat random if it will work without the resolver. In short we think this should never happen, but it's better to have diagnostics for this.

vitek-karas · 2021-07-16T11:36:12Z

+            if (libraryName != Constants.HostFxr || !NativeLibrary.TryLoad(HostFxrPath, out IntPtr handle))
+                return IntPtr.Zero;


Same here - if it's possible to have some diagnostics... I think it would be better to fail with clear error if we fail to load the library (for whatever reason).

elinor-fung · 2021-08-03T23:42:40Z

+            {
+                return new Command("check", LocalizableStrings.AppFullName);
+            }
+            catch (FileNotFoundException e)


It seems weird that we'd have to catch the exception on just getting the command. I'd expect it to be when the command is actually executed?

Is this because it is in the static constructor for the Interop class / something else using the class before the command execution?

I think exceptions in static constructors also come through as TypeInitializationException.

It seems weird that we'd have to catch the exception on just getting the command.

Agreed. I moved this to the place where we actually execute the command. Also catching TypeInitializationException now -- I wasn't aware of this.

elinor-fung · 2021-08-03T23:58:24Z

+            }
+            catch (ArgumentException e)
+            {
+                throw new GracefulException(string.Format(LocalizableStrings.HostFxrCouldNotBeLoaded, e.Message));


The direct map of FileNotFoundException / ArgumentException to RuntimePropertyNotFound / HostFxrCouldNotBeLoaded also seems unnatural.

Could other issues result in FileNotFoundException / ArgumentException?

I don't know how I would make the connection between the exceptions caught here and the ones thrown in NativeWrapper Interop

Also, I think the messages are flipped. ArgumentException is being thrown for the runtime property, not loading hostfxr.

Could other issues result in FileNotFoundException / ArgumentException?

Good point. I added a new type of exception for the exceptions related to the hostfxr path resolution thrown in NativeWrapper.Interop. Fixed the messages as well.

…mpty

Throw from HostFxrDllImportResolver

…nto sdkCheckDllImport

vitek-karas

Looks good!

mateoatr requested review from elinor-fung, sfoslund and vitek-karas July 14, 2021 18:35

elinor-fung reviewed Jul 14, 2021

View reviewed changes

sfoslund mentioned this pull request Jul 15, 2021

dotnet sdk check cannot find "hostfxr" on macOS M1 #18946

Closed

vitek-karas reviewed Jul 16, 2021

View reviewed changes

elinor-fung reviewed Aug 4, 2021

View reviewed changes

Mateo Torres Ruiz added 5 commits August 4, 2021 16:26

Use HOSTFXR_PATH property to load hostfxr

e5a01c3

Keep RunningOnWindows field

b7ca20f

PR feedback.

08eb5fe

Throw exception if HostFxr not found or runtime property not set or e…

5ea0ddd

…mpty

PR feedback

b8e970e

mateoatr force-pushed the sdkCheckDllImport branch from 5f225fc to b8e970e Compare August 5, 2021 16:25

Mateo Torres Ruiz and others added 5 commits August 5, 2021 09:32

Catch TypeInitializationException

4325c7c

Modify hostfxr exception messages

273b5ba

Throw from HostFxrDllImportResolver

Merge branch 'main' into sdkCheckDllImport

b7b23da

Try to load using library name

7ae66b0

Merge branch 'sdkCheckDllImport' of https://github.com/mateoatr/sdk i…

31586c1

…nto sdkCheckDllImport

mateoatr requested review from elinor-fung and vitek-karas August 9, 2021 23:06

Use ResolvingUnmanagedDll ALC event instead of SetDllImportResolver

fc4cf4c

elinor-fung reviewed Aug 10, 2021

View reviewed changes

Comment thread src/Resolvers/Microsoft.DotNet.NativeWrapper/Interop.cs Outdated

Use the executing assembly load context

1acdaaf

vitek-karas approved these changes Aug 10, 2021

View reviewed changes

Comment thread src/Cli/dotnet/commands/dotnet-sdk/check/Program.cs Outdated

Rethrow as GracefulException

4e38bd7

mateoatr merged commit 5ca6ab9 into dotnet:main Aug 10, 2021

mateoatr mentioned this pull request Aug 16, 2021

dotnet sdk check command fails in dotnet/sdk:6.0-alpine dotnet/dotnet-docker#3051

Closed

		if (!string.IsNullOrEmpty(HostFxrPath))
		NativeLibrary.SetDllImportResolver(typeof(Interop).Assembly, HostFxrDllImportResolver);

		if (libraryName != Constants.HostFxr \|\| !NativeLibrary.TryLoad(HostFxrPath, out IntPtr handle))
		return IntPtr.Zero;

Conversation

mateoatr commented Jul 14, 2021

Uh oh!

ghost commented Jul 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfoslund commented Jul 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mateoatr commented Jul 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elinor-fung Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elinor-fung Aug 4, 2021 •

edited

Loading