Fix resolving hostfxr #230

YuliiaKovalova · 2023-08-22T17:46:14Z

Fixes:
dotnet/msbuild#9038
#210
#200
#181

This PR solves 2 problems:

Dotnet path resolution
Loading hostfx.(dylib|so) for MAC OS and Linux

In order to resolve dotnet path, we probe different sources:
Case: MSBuild.Locator is called from dotnet.exe, check DOTNET_ROOT env var -> current process path
Case: MSBuild.Locator is called in apphost, check DOTNET_ROOT env var -> DOTNET_HOST_PATH -> DOTNET_MSBUILD_SDK_RESOLVER_CLI_DIR -> PATH
If the path can't be extracted, an exception is thrown.

When DotnetPath is resolved, we create a subscription on ResolvingUnmanagedDll event when hostfx is requested, if it isn't not loaded in context (the possible reason is described here dotnet/runtime#83774).
The path to hostfxr.dylib|so is built based on the pattern <dotnet_path>/host/fxr//libhostfxr.(dylib|so)

YuliiaKovalova · 2023-08-22T19:15:29Z

@baronfel please take a look

Forgind

I'm not necessarily opposed to it, but this feels like a lot of changes for not very many impactful changes. I generally prefer more targeted changes.

Also, can you separate the framework changes into a separate PR? That feels meaningfully distinct.

src/MSBuildLocator/DotNetSdkLocationHelper.cs

ladipro · 2023-08-23T07:49:58Z

Also, can you separate the framework changes into a separate PR? That feels meaningfully distinct.

+1 on a separate PR for the target framework change.

Also, please describe the changes in the initial comment. Ideally everyone would be able to understand how the PR is fixing the issue by reading the description.

YuliiaKovalova · 2023-08-23T08:44:02Z

Also, can you separate the framework changes into a separate PR? That feels meaningfully distinct.

+1 on a separate PR for the target framework change.

Also, please describe the changes in the initial comment. Ideally everyone would be able to understand how the PR is fixing the issue by reading the description.

I have moved this change to a separate PR:
#231 please take a look.

and the second part of comment is relevant - I will describe my steps in detail.

ladipro

Thank you, I've left a few comments inline.

src/MSBuildLocator/DotNetSdkLocationHelper.cs

ladipro

Looks good! One minor comment inline and please obtain at least one more approval.

src/MSBuildLocator/DotNetSdkLocationHelper.cs

rainersigwald

The only thing blocking IMO is the NuGet dependency at the package level.

src/MSBuildLocator/DotNetSdkLocationHelper.cs

src/MSBuildLocator/Utils/SemanticVersionParser.cs

src/MSBuildLocator/DotNetSdkLocationHelper.cs

Forgind · 2023-08-24T19:11:40Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

-                // 32-bit architecture
-                dotnetPath ??= FindDotnetFromEnvironmentVariable("DOTNET_ROOT(x86)", exeName);
+                var loadContext = AssemblyLoadContext.GetLoadContext(Assembly.GetExecutingAssembly());
+                if (loadContext != null)


When is loadContext null? My default assumption would be that there's always at least the default context...is that wrong?

Probably runtime has some cases for returning null, since SDK also validates it
https://github.com/dotnet/sdk/blob/285888bebe0735d3ad8a4cd0dc1cd1037ba17ed6/src/ApiCompat/Microsoft.DotNet.ApiCompat.Shared/RoslynResolver.cs#L26

Forgind · 2023-08-24T19:13:20Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+
+        private static IntPtr HostFxrResolver(Assembly assembly, string libraryName)
+        {
+            var hostFxrLibName = "libhostfxr";


I thought you'd implied it could be libhostfxr or hostfxr by having a regex accepting either—was that wrong?

I have checked that for osx and linux envs - the lib always has this name.
it's called hostfxr on Windows.

src/MSBuildLocator/DotNetSdkLocationHelper.cs

Forgind · 2023-08-24T19:45:38Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

-                throw new InvalidOperationException("Failed to find an appropriate version of .NET Core MSBuild. Call to hostfxr_resolve_sdk2 failed. There may be more details in stderr.");
+                string? dotnetExePath = GetCurrentProcessPath();
+                var isRunFromDotnetExecutable = !string.IsNullOrEmpty(dotnetExePath) 
+                    && Path.GetFileName(dotnetExePath).Equals(ExeName, StringComparison.InvariantCultureIgnoreCase);


This looks like something that should be platform-specific, right?

It's an execution context-specific - if MsBuild.Locator is invoked in the scope of dotnet.exe, isRunFromDotnetExecutable is true.

I meant whether it should be IgnoreCase or not should be platform-specific?

src/MSBuildLocator/Utils/VersionComparer.cs

Forgind · 2023-08-24T19:47:36Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+                {
+                    dotnetPath = FindDotnetPathFromEnvVariable("DOTNET_HOST_PATH") 
+                        ?? FindDotnetPathFromEnvVariable("DOTNET_MSBUILD_SDK_RESOLVER_CLI_DIR")
+                        ?? GetDotnetPathFromPATH();


We should have one last place where we just look in the expected places, right? Like /usr/bin/dotnet or whatever on linux?

Chet listed the lookup sequence - based on the execution context of MsBuild.Locator, we check different places. More details are in the PR description.

src/MSBuildLocator/DotNetSdkLocationHelper.cs

* Bump Microsoft.NET.Test.Sdk from 15.9.0 to 17.3.1 (#180) Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 15.9.0 to 17.3.1. - [Release notes](https://github.com/microsoft/vstest/releases) - [Commits](microsoft/vstest@v15.9.0...v17.3.1) --- updated-dependencies: - dependency-name: Microsoft.NET.Test.Sdk dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump xunit from 2.4.1 to 2.4.2 (#172) Bumps [xunit](https://github.com/xunit/xunit) from 2.4.1 to 2.4.2. - [Release notes](https://github.com/xunit/xunit/releases) - [Commits](xunit/xunit@2.4.1...2.4.2) --- updated-dependencies: - dependency-name: xunit dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump xunit.runner.visualstudio from 2.4.1 to 2.4.5 (#155) Bumps [xunit.runner.visualstudio](https://github.com/xunit/visualstudio.xunit) from 2.4.1 to 2.4.5. - [Release notes](https://github.com/xunit/visualstudio.xunit/releases) - [Commits](https://github.com/xunit/visualstudio.xunit/commits) --- updated-dependencies: - dependency-name: xunit.runner.visualstudio dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump Shouldly from 4.0.3 to 4.1.0 (#187) Bumps [Shouldly](https://github.com/shouldly/shouldly) from 4.0.3 to 4.1.0. - [Release notes](https://github.com/shouldly/shouldly/releases) - [Changelog](https://github.com/shouldly/shouldly/blob/master/BREAKING%20CHANGES.txt) - [Commits](shouldly/shouldly@v4.0.3...4.1.0) --- updated-dependencies: - dependency-name: Shouldly dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Forgind <[email protected]> * Bump Microsoft.VisualStudio.Setup.Configuration.Interop (#186) Bumps Microsoft.VisualStudio.Setup.Configuration.Interop from 1.16.30 to 3.3.2180. --- updated-dependencies: - dependency-name: Microsoft.VisualStudio.Setup.Configuration.Interop dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump Microsoft.VisualStudio.SDK.EmbedInteropTypes Bumps Microsoft.VisualStudio.SDK.EmbedInteropTypes from 15.0.21 to 15.0.36. --- updated-dependencies: - dependency-name: Microsoft.VisualStudio.SDK.EmbedInteropTypes dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Enabling CodeQL (#190) Enables CodeQL in build pipeline * Update xml doc comments (#193) Update xml doc comments * Bump Shouldly from 4.1.0 to 4.2.1 Bumps [Shouldly](https://github.com/shouldly/shouldly) from 4.1.0 to 4.2.1. - [Release notes](https://github.com/shouldly/shouldly/releases) - [Changelog](https://github.com/shouldly/shouldly/blob/master/BREAKING%20CHANGES.txt) - [Commits](shouldly/shouldly@4.1.0...4.2.1) --- updated-dependencies: - dependency-name: Shouldly dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump Microsoft.VisualStudio.Setup.Configuration.Interop Bumps Microsoft.VisualStudio.Setup.Configuration.Interop from 3.3.2180 to 3.6.2115. --- updated-dependencies: - dependency-name: Microsoft.VisualStudio.Setup.Configuration.Interop dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump Nerdbank.GitVersioning from 3.5.107 to 3.6.133 Bumps [Nerdbank.GitVersioning](https://github.com/dotnet/Nerdbank.GitVersioning) from 3.5.107 to 3.6.133. - [Release notes](https://github.com/dotnet/Nerdbank.GitVersioning/releases) - [Commits](dotnet/Nerdbank.GitVersioning@v3.5.107...v3.6.133) --- updated-dependencies: - dependency-name: Nerdbank.GitVersioning dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump Microsoft.NET.Test.Sdk from 17.3.1 to 17.6.2 Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.3.1 to 17.6.2. - [Release notes](https://github.com/microsoft/vstest/releases) - [Changelog](https://github.com/microsoft/vstest/blob/main/docs/releases.md) - [Commits](microsoft/vstest@v17.3.1...v17.6.2) --- updated-dependencies: - dependency-name: Microsoft.NET.Test.Sdk dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Update Releasing_MSBuildLocator.md * Bump xunit.runner.visualstudio from 2.4.5 to 2.5.0 (#223) Bumps [xunit.runner.visualstudio](https://github.com/xunit/visualstudio.xunit) from 2.4.5 to 2.5.0. - [Release notes](https://github.com/xunit/visualstudio.xunit/releases) - [Commits](xunit/visualstudio.xunit@v2.4.5...2.5.0) --- updated-dependencies: - dependency-name: xunit.runner.visualstudio dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump Microsoft.NET.Test.Sdk from 17.6.2 to 17.6.3 (#221) Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.6.2 to 17.6.3. - [Release notes](https://github.com/microsoft/vstest/releases) - [Changelog](https://github.com/microsoft/vstest/blob/main/docs/releases.md) - [Commits](microsoft/vstest@v17.6.2...v17.6.3) --- updated-dependencies: - dependency-name: Microsoft.NET.Test.Sdk dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump xunit from 2.4.2 to 2.5.0 (#222) Bumps [xunit](https://github.com/xunit/xunit) from 2.4.2 to 2.5.0. - [Commits](xunit/xunit@2.4.2...2.5.0) --- updated-dependencies: - dependency-name: xunit dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Make Unregister a no-op Fixes #77 (#204) * Make Unregister a no-op * Change comment * Specify not to show Unregister * Sort usings --------- Co-authored-by: Forgind <[email protected]> * Validate dotnet executable exists (#202) * Validate dotnet executable exists * PR comment * Update src/MSBuildLocator/DotNetSdkLocationHelper.cs Co-authored-by: Ladi Prosek <[email protected]> * Simplify logic Also avoids an unnecessary File.Exists check on Windows --------- Co-authored-by: Forgind <[email protected]> Co-authored-by: Ladi Prosek <[email protected]> * Bump Microsoft.NET.Test.Sdk from 17.6.3 to 17.7.0 (#226) Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.6.3 to 17.7.0. - [Release notes](https://github.com/microsoft/vstest/releases) - [Changelog](https://github.com/microsoft/vstest/blob/main/docs/releases.md) - [Commits](microsoft/vstest@v17.6.3...v17.7.0) --- updated-dependencies: - dependency-name: Microsoft.NET.Test.Sdk dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump Microsoft.VisualStudio.Setup.Configuration.Interop (#227) Bumps Microsoft.VisualStudio.Setup.Configuration.Interop from 3.6.2115 to 3.7.2175. --- updated-dependencies: - dependency-name: Microsoft.VisualStudio.Setup.Configuration.Interop dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Respect DOTNET_ROOT (#225) * Respect DOTNET_ROOT * DOTNET_ROOT is a folder. Also, DOTNET_ROOT(x86) * PR Feedback --------- Co-authored-by: Forgind <[email protected]> * Bump Microsoft.NET.Test.Sdk from 17.7.0 to 17.7.1 (#228) Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.7.0 to 17.7.1. - [Release notes](https://github.com/microsoft/vstest/releases) - [Changelog](https://github.com/microsoft/vstest/blob/main/docs/releases.md) - [Commits](microsoft/vstest@v17.7.0...v17.7.1) --- updated-dependencies: - dependency-name: Microsoft.NET.Test.Sdk dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * upgrade core version to net6.0 (#231) * upgrade framework version + fix issue with path extraction from environment variable * fix review comments * Fix resolving hostfxr for Mac OS and Linux envs (#230) * add setting of "DOTNET_HOST_PATH" env variable (#235) * Fix hostfx resolving issue in some Mac machines (#236) * Add diagnostic logging, and address issues in the code. * Update the fix: 1, it turns out DotnetPath is the folder path. It just sets DOTNET_HOST_PATH incorrectly DOTNET_HOST_PATH is a file path, which is different than DOTNET_ROOT 2, Fix DOTNET_HOST_PATH handling, which broke the application when it sets to a folder. * use StringComparison.OrdinalIgnoreCase to compare file name maybe should use platform dependent comparison, but it looks like the rest of code is doing that. * do not create new instance on each call. * Further hardern the logic inside HostFxrResolver Adds more logging and handles empty folder. * delete unnecessary logging. * Additional logging. * Throw errors instead of logging it. --------- Co-authored-by: Lifeng Lu <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Forgind <[email protected]> Co-authored-by: MichalPavlik <[email protected]> Co-authored-by: Forgind <[email protected]> Co-authored-by: Ladi Prosek <[email protected]> Co-authored-by: Lifeng Lu <[email protected]> Co-authored-by: Lifeng Lu <[email protected]>

YuliiaKovalova requested review from rainersigwald, ladipro, baronfel and Forgind August 22, 2023 19:15

YuliiaKovalova marked this pull request as ready for review August 22, 2023 19:17

Forgind reviewed Aug 22, 2023

View reviewed changes

JoeRobich mentioned this pull request Aug 23, 2023

Rosyln failing to start on Mac x64 with 'image not found' error for hostfxr.dylib dotnet/vscode-csharp#6198

Closed

YuliiaKovalova requested a review from Forgind August 23, 2023 10:13

YuliiaKovalova force-pushed the dev/ykovalova/fix_macos_resolving branch 2 times, most recently from 5607a03 to 5759980 Compare August 23, 2023 10:38

Fix hostfxr resolution

b53cd42

YuliiaKovalova force-pushed the dev/ykovalova/fix_macos_resolving branch from bddd4f1 to b53cd42 Compare August 23, 2023 12:24

YuliiaKovalova added 2 commits August 23, 2023 14:25

merge

06e4571

fix review comments

c8dccd1

YuliiaKovalova mentioned this pull request Aug 23, 2023

Use dotnet from current process if possible #203

Closed

ladipro reviewed Aug 23, 2023

View reviewed changes

JoeRobich mentioned this pull request Aug 23, 2023

dotnet-format 8.0 rc1 is failing on Alpine 3.17 dotnet/format#1945

Closed

YuliiaKovalova requested a review from ladipro August 24, 2023 06:36

ladipro requested changes Aug 24, 2023

View reviewed changes

YuliiaKovalova requested a review from ladipro August 24, 2023 11:40

ladipro approved these changes Aug 24, 2023

View reviewed changes

src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated Show resolved Hide resolved

fix version sorting, Lazy loading for dotnet_path, cleanup

2a6bb81

YuliiaKovalova force-pushed the dev/ykovalova/fix_macos_resolving branch from 4f275d1 to 2a6bb81 Compare August 24, 2023 12:32

rainersigwald requested changes Aug 24, 2023

View reviewed changes

add SemanticVersion parser and cover it with UTs

fce84f4

YuliiaKovalova requested a review from rainersigwald August 24, 2023 18:00

rainersigwald approved these changes Aug 24, 2023

View reviewed changes

src/MSBuildLocator/Utils/SemanticVersionParser.cs Outdated Show resolved Hide resolved

src/MSBuildLocator/Utils/SemanticVersionParser.cs Outdated Show resolved Hide resolved

src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated Show resolved Hide resolved

Forgind reviewed Aug 24, 2023

View reviewed changes

fix review comments

cede57e

ladipro reviewed Aug 25, 2023

View reviewed changes

src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated Show resolved Hide resolved

YuliiaKovalova force-pushed the dev/ykovalova/fix_macos_resolving branch from 6c38067 to cede57e Compare August 25, 2023 07:44

YuliiaKovalova added 2 commits August 25, 2023 09:47

fix issue with combining fileName and extension

4ced06a

update check for hostFxrAssembly presence

94f96d7

YuliiaKovalova mentioned this pull request Aug 25, 2023

Updating locator from 1.4.1 to 1.5.3 breaks x86 processes #182

Open

YuliiaKovalova merged commit 7aabcd9 into microsoft:master Aug 25, 2023
2 checks passed

JoeRobich mentioned this pull request Aug 30, 2023

Upgrade MSBuildLocator to version 1.6.1 dotnet/format#1948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix resolving hostfxr #230

Fix resolving hostfxr #230

YuliiaKovalova commented Aug 22, 2023 •

edited

Loading

YuliiaKovalova commented Aug 22, 2023

Forgind left a comment

ladipro commented Aug 23, 2023

YuliiaKovalova commented Aug 23, 2023

ladipro left a comment

ladipro left a comment

rainersigwald left a comment

Forgind Aug 24, 2023

YuliiaKovalova Aug 25, 2023

Forgind Aug 24, 2023

YuliiaKovalova Aug 25, 2023 •

edited

Loading

Forgind Aug 24, 2023

YuliiaKovalova Aug 25, 2023

Forgind Aug 25, 2023

Forgind Aug 24, 2023

YuliiaKovalova Aug 25, 2023

Fix resolving hostfxr #230

Fix resolving hostfxr #230

Conversation

YuliiaKovalova commented Aug 22, 2023 • edited Loading

YuliiaKovalova commented Aug 22, 2023

Forgind left a comment

Choose a reason for hiding this comment

ladipro commented Aug 23, 2023

YuliiaKovalova commented Aug 23, 2023

ladipro left a comment

Choose a reason for hiding this comment

ladipro left a comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuliiaKovalova Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuliiaKovalova commented Aug 22, 2023 •

edited

Loading

YuliiaKovalova Aug 25, 2023 •

edited

Loading