Skip to content

Validate dotnet executable exists#202

Merged
YuliiaKovalova merged 4 commits into
microsoft:masterfrom
Forgind:validate-target-exists
Aug 4, 2023
Merged

Validate dotnet executable exists#202
YuliiaKovalova merged 4 commits into
microsoft:masterfrom
Forgind:validate-target-exists

Conversation

@Forgind
Copy link
Copy Markdown
Contributor

@Forgind Forgind commented Mar 16, 2023

Fixes #201

If a user has a symlink called dotnet (or dotnet.exe on Windows) on their PATH, but that symlink points to a file that does not exist, we will find that symlink, stop processing the path, and ultimately fail. We should notice that the symlink target does not exist and continue processing PATH, only failing if we fail to find an executable anywhere on the PATH.

elinor-fung
elinor-fung reviewed Mar 16, 2023
View reviewed changes
Comment thread src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated
dotnetPath = filePath;
break;
dotnetPath = Path.GetDirectoryName(isWindows ? filePath : realpath(filePath) ?? filePath);
if (File.Exists(dotnetPath))
Copy link
Copy Markdown
Member

@elinor-fung elinor-fung Mar 16, 2023

Choose a reason for hiding this comment

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

Would you want to validate the file path realpath(filePath) exists instead of just the directory?

ladipro
ladipro reviewed Mar 20, 2023
View reviewed changes
Comment thread src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated
Comment thread src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated
dotnetPath = filePath;
break;
filePath = Path.GetDirectoryName(isWindows ? filePath : realpath(filePath) ?? filePath);
if (File.Exists(Path.Combine(filePath, exeName)))
Copy link
Copy Markdown
Member

@ladipro ladipro Mar 20, 2023

Choose a reason for hiding this comment

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

Are we now checking the same file twice on Windows?

Copy link
Copy Markdown
Contributor Author

@Forgind Forgind Mar 20, 2023

Choose a reason for hiding this comment

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

Yes, so it could be optimized somewhat, but I care very little about performance on this code path, since it should only execute once per SDK you have installed...so like twice or thrice for the average user.

@Forgind @ladipro
Update src/MSBuildLocator/DotNetSdkLocationHelper.cs
8c4fa97 
Co-authored-by: Ladi Prosek <ladi.prosek@gmail.com>
rainersigwald
rainersigwald reviewed Mar 31, 2023
View reviewed changes
Comment thread src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated
{
dotnetPath = filePath;
break;
filePath = Path.GetDirectoryName(isWindows ? filePath : realpath(filePath) ?? filePath);
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald Mar 31, 2023

Choose a reason for hiding this comment

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

I don't think I understand the isWindows check here. You can have dangling symlinks on Windows just as much, right?

Copy link
Copy Markdown
Contributor Author

@Forgind Forgind Apr 3, 2023

Choose a reason for hiding this comment

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

That is an excellent question. You can in theory, but I don't think I've ever seen someone who has a path to a symlink to dotnet.exe on their path rather than dotnet.exe itself on windows. The real reason I did this for unix and not windows is that that's what the SDK does like here:

https://github.com/dotnet/sdk/blob/733eec58c350f139bbedbdccb38df128ca60b8c3/src/Resolvers/Microsoft.DotNet.NativeWrapper/EnvironmentProvider.cs#L80-L85

Since I used the SDK as a reference when initially trying to understand how to call hostfxr, I just went with it.

Comment thread src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated
Comment on lines +105 to +106
filePath = Path.GetDirectoryName(isWindows ? filePath : realpath(filePath) ?? filePath);
if (File.Exists(Path.Combine(filePath, exeName)))
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald Mar 31, 2023

Choose a reason for hiding this comment

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

I don't think I get this get the directory of the realpath then recombine with the exe name thing either. Why not just check the result of realpath?

Copy link
Copy Markdown
Contributor Author

@Forgind Forgind Apr 3, 2023

Choose a reason for hiding this comment

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

Fair enough. Fixed.

@Forgind
Simplify logic
df5174c 
Also avoids an unnecessary File.Exists check on Windows
@YuliiaKovalova
Copy link
Copy Markdown
Contributor

YuliiaKovalova commented Jun 15, 2023

Hi @rainersigwald,

I can see that comments were resolved. Do you plan to complete this PR?

@YuliiaKovalova YuliiaKovalova merged commit aa44a35 into microsoft:master Aug 4, 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@YuliiaKovalova YuliiaKovalova YuliiaKovalova approved these changes

@rainersigwald rainersigwald Awaiting requested review from rainersigwald

@ladipro ladipro Awaiting requested review from ladipro

+1 more reviewer

@elinor-fung elinor-fung elinor-fung left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DotNetSdkLocationHelper does not validate symlink target exists

5 participants

@Forgind @YuliiaKovalova @rainersigwald @ladipro @elinor-fung