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.

Sign up for GitHub

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

Jump to bottom

Fix hostfx resolving issue in some Mac machines #236

Merged
merged 8 commits into from
Aug 29, 2023
Merged

Fix hostfx resolving issue in some Mac machines #236

merged 8 commits into from
Aug 29, 2023
+52 −27

Conversation

lifengl
Copy link
Member

@lifengl lifengl commented Aug 26, 2023 •
edited
Loading

This PR is to fix hostfx resolver issue on some Mac machines. There are a few issues fixed in this PR:

1, the libraryName is actually the string from DllImport metadata, so it would be hostfxr
2, enumerate directory will get a list of full path, the semantic version parser doesn't seem to handle it.
3, DOTNET_HOST_PATH is actually path to the dotnet command file, which is different from DOTNET_ROOT, that was a misunderstanding in the code which leads it to set it incorrectly.

This PR is to address:

dotnet/msbuild#9038

Update the fix:
1db2f2a 
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.
@lifengl lifengl changed the title Add diagnostic logging, and address issues in the code. Fix hostfx resolving issue in some Mac machines Aug 27, 2023
@lifengl lifengl marked this pull request as ready for review August 27, 2023 01:23
@lifengl
Copy link
Member Author

lifengl commented Aug 27, 2023

cc @chzuluag

Lifeng Lu added 5 commits August 26, 2023 18:36
use StringComparison.OrdinalIgnoreCase to compare file name
7d8bbfd 
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.
a2f280e
Further hardern the logic inside HostFxrResolver
9361ae8 
Adds more logging and handles empty folder.
delete unnecessary logging.
fb0e177
Additional logging.
70715d3
@lifengl lifengl requested a review from YuliiaKovalova August 27, 2023 23:45
@lifengl
Copy link
Member Author

lifengl commented Aug 27, 2023

@lifengl
Copy link
Member Author

lifengl commented Aug 28, 2023

Both Christian and CTI has verified the PR fixed the issue on Mac (which can be produced on their x64 mac machine earlier.)

ladipro
ladipro reviewed Aug 28, 2023
View reviewed changes
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Thank you, I've left a few comments inline. I understand we're under time pressure but please consider throwing exceptions rather than printing diagnostics to stderr.

@lifengl
Copy link
Member Author

lifengl commented Aug 28, 2023

@YuliiaKovalova , @ladipro : can your team help to merge it when you feel it is ready. (I don't have permission to do it.)

@YuliiaKovalova YuliiaKovalova merged commit 67a2a25 into microsoft:master Aug 29, 2023
YuliiaKovalova added a commit that referenced this pull request Aug 29, 2023
@YuliiaKovalova @dependabot
@Forgind @MichalPavlik @ladipro @lifengl
Fix hostfxr, respect DOTNET_ROOT, bump to net6.0, set DOTNET_HOST
17625c6 
* 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] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
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] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
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] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
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] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Forgind <Forgind@users.noreply.github.com>

* 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] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
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] <support@github.com>

* 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] <support@github.com>

* 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] <support@github.com>

* 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] <support@github.com>

* 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] <support@github.com>

* 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] <support@github.com>
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] <support@github.com>
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] <support@github.com>
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 <Forgind@users.noreply.github.com>

* Validate dotnet executable exists (#202)

* Validate dotnet executable exists

* PR comment

* Update src/MSBuildLocator/DotNetSdkLocationHelper.cs

Co-authored-by: Ladi Prosek <ladi.prosek@gmail.com>

* Simplify logic

Also avoids an unnecessary File.Exists check on Windows

---------

Co-authored-by: Forgind <Forgind@users.noreply.github.com>
Co-authored-by: Ladi Prosek <ladi.prosek@gmail.com>

* 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] <support@github.com>
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] <support@github.com>
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 <Forgind@users.noreply.github.com>

* 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] <support@github.com>
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 <lifengl@microsoft.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Forgind <Forgind@users.noreply.github.com>
Co-authored-by: MichalPavlik <michalpavlik@outlook.com>
Co-authored-by: Forgind <12969783+Forgind@users.noreply.github.com>
Co-authored-by: Ladi Prosek <ladi.prosek@gmail.com>
Co-authored-by: Lifeng Lu <lulifeng@hotmail.com>
Co-authored-by: Lifeng Lu <lifengl@microsoft.com>
@JoeRobich JoeRobich mentioned this pull request Aug 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ladipro ladipro

@YuliiaKovalova YuliiaKovalova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lifengl @ladipro @YuliiaKovalova