macOS: Include global tools in zsh $PATH #48893
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the macOS shell path instructions by consolidating the logic into a unified MacOSEnvironmentPath implementation that handles both bash and zsh shells. Key changes include:
- Renaming and replacing OsxBashEnvironmentPath and OsxZshEnvironmentPathInstruction with MacOSEnvironmentPath.
- Updating tests to cover both bash and zsh environments uniformly.
- Adjusting environment variable reads and file write operations to align with the new unified implementation.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.DotNet.ShellShim.Tests/OsxZshEnvironmentPathInstructionTests.cs | Removed obsolete zsh instruction tests |
| test/Microsoft.DotNet.ShellShim.Tests/OsxEnvironmentPathTests.cs | Updated tests to use MacOSEnvironmentPath and cover multiple shell types |
| test/Microsoft.DotNet.ShellShim.Tests/LinuxEnvironmentPathTests.cs | Added SHELL setup for consistency |
| test/Microsoft.DotNet.ShellShim.Tests/EnvironmentPathFactoryTests.cs | Updated inline data and assertions to use MacOSEnvironmentPath |
| src/Cli/dotnet/ShellShim/ZshDetector.cs | Refactored for concise null-safe check |
| src/Cli/dotnet/ShellShim/MacOSEnvironmentPath.cs | Replaced OsxBashEnvironmentPath with unified handling for bash and zsh |
| src/Cli/dotnet/ShellShim/EnvironmentPathFactory.cs | Adjusted factory to return MacOSEnvironmentPath, removing the zsh-specific branch |
Comments suppressed due to low confidence (2)
src/Cli/dotnet/ShellShim/MacOSEnvironmentPath.cs:73
- [nitpick] Consider extracting the conditional message formatting into a local variable or helper method to improve readability and reduce nested string.Format calls.
ZshDetector.IsZshTheUsersShell(_environmentProvider) ? string.Format( CommonLocalizableStrings.EnvironmentPathOSXZshManualInstructions, _packageExecutablePath.Path) : string.Format( CommonLocalizableStrings.EnvironmentPathOSXBashManualInstructions, _packageExecutablePath.Path));
src/Cli/dotnet/ShellShim/EnvironmentPathFactory.cs:49
- Ensure that merging the zsh-specific logic into the unified MacOSEnvironmentPath meets all runtime requirements for both bash and zsh shells, as this design change may affect behavior expected by existing consumers.
environmentPath = new MacOSEnvironmentPath(
There was a problem hiding this comment.
At least one failure looked real because you changed the path, and the new path didn't look right to me? Also, why are you targeting 9.0.1xx? This issue has been open for years, so you'd have to take it to servicing. I'm not convinced that it's worth it in this case.
| @@ -55,7 +55,7 @@ private bool PackageExecutablePathExists() | |||
|
|
|||
| return value | |||
| .Split(':') | |||
| .Any(p => p == _packageExecutablePath.Path || p == _packageExecutablePath.PathWithTilde); | |||
| .Any(p => p.Equals(_packageExecutablePath.Path, StringComparison.OrdinalIgnoreCase)); | |||
There was a problem hiding this comment.
Why OrdinalIgnoreCase on mac? I thought it was case-sensitive normally?
There was a problem hiding this comment.
Ohh suggested by Copilot, didn't pay much attention. You're right :)
There was a problem hiding this comment.
macOS can be set to case insensitive
Yes, I was trying to get the full expanded path instead of the path with tilde (~), but am not sure how it got resolved into the one on the test.
Should I instead target main? cc @Forgind |
There was a problem hiding this comment.
Kudos for tackling this challenge. Question:
What change does this PR implement - is it fixing using dotnet shellenv in a config file? I assume it does not add the expanded HOME to the path, since that PR was attempted and rejected, Write the full path of the dotnet tools directory in /etc/paths.d/dotnet-cli-tools by 0xced · Pull Request #35411 · dotnet/sdk. Does it also remove the ~ from the path only on zsh, to fix the issue of running shellenv? That is my initial understanding based on the PR.
So, we would ask people to add this to their zshrc file?
eval `dotnet shellenv`
In an ideal world, we could:
- Figure out where to move the global installation location to be outside of the HOME directory
- Or, coordinate with Apple such that HOME is expandable. Their bug report hasn't gotten any action. I wish it were public.
But this is a great workaround for now!
|
I did like the suggestion from jrdodds that we might be able to just add a line to replace ~ with $HOME in the .zprofile...it wasn't 100% clear to me that the other PR was rejected so much as that it wasn't in the preferred form and ended up not being finished. This PR looks very similar to that one, to be honest. Good call, nagilson. And yes, I'd target main for something like this. |
Yes! Great work on this PR. But it would be helpful to include a more in-depth PR description for these sorts of nuanced changes, so it's easier to review. The most important thing is the motivation and a summary of what your change does. That makes it much faster to figure out whether the code is doing what you intended for it to do, because I'm not sure what change we wanted to implement; I can infer it from the code, but that has the assumption that the code does whatever you intended for it to do. I think we're missing some context; did @baronfel suggest we have users run: |
|
Note: not passing all tests and needs to coordinate with baronfel on the path forward |
Fixes #23165
Targeting 9.0.1xx as an early version for exploration