dotnet tool install --local works by default, by creating a manifest by default, utilizing --create-manifest-if-needed
#48906
Conversation
This should enable the findFirst function to create a tool manifest by default. Will break some tests. On initial glance,, the arg behavior only seems to be inherited by tool install local, which is all I want to impact right now. Although it also gets inherited by the construct command in command parser.
- We dont need two tests showing it should throw, and then it should throw with a message. Kept only the 2nd test. - Modified the we should throw test to only expect throw if create manifest if needed is false since the default is now true.
nagilson
added
Review for breaking changes
breaking-change
|
Added When you commit this breaking change:
You can refer to the .NET SDK breaking change guidelines |
nagilson
changed the title
dotnet tool install --local works by default, by creating a manifest by default, utilizing --create-manifest-if-needed
There was a problem hiding this comment.
Pull Request Overview
This PR changes the default behavior of the dotnet tool install local command to automatically create a tool manifest when one is not present, setting the --create-manifest-if-needed flag to true by default.
- Updated the ToolInstallCommandParser to use an arity of ZeroOrOne and a default value of true for the create manifest option.
- Modified and added tests to validate both the default behavior and the behavior when the flag is explicitly set to false.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs | Updated tests to assert that manifest creation is enabled by default and disabled when false is explicitly set. |
| src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs | Adjusted option configuration for --create-manifest-if-needed to allow an optional argument and default to true. |
Comments suppressed due to low confidence (2)
test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs:131
- [nitpick] The test name 'GivenCreateManifestIfNeededWithoutArgumentTheDefaultIsTrueForLegacyBehavior' is quite lengthy and similar to 'GivenNoManifestFileItUsesCreateManifestIfNeededByDefault'. Consider renaming or consolidating tests for improved clarity and maintainability.
public void GivenCreateManifestIfNeededWithoutArgumentTheDefaultIsTrueForLegacyBehavior()
src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs:51
- [nitpick] Consider adding a comment here that explains the use of ZeroOrOne arity and the default value of true, to clarify the intended behavior when no argument is provided.
public static readonly Option<bool> CreateManifestIfNeededOption = new("--create-manifest-if-needed")
|
@baronfel Is there a quick way to update the completions tests for each shell to be the current behavior? I think the Arity change here caused the snapshot tests to break because now it can accept false. I was able to update the snapshot by manually running it and copying over the new snapshot for bash, though I'm hoping there's an automated way to avoid having to do that for Fish, Zsh, and others |
1. Run Test 2. dotnet restore .\dotnet.Tests\ /t:CompareCliSnapshots in test folder on test prj 3. dotnet restore .\dotnet.Tests\ /t:UpdateCliSnapshots
…thub.com/nagilson/sdk into nagilson-create-tool-manifest-by-default
|
Oh awesome, thank you 🍰 🐙 |
Resolves #15254 (comment)
Resolves part of #36830 by fixing the default, but does not fix
-d. That should be done in another PR.What's this do
Makes
createManifestIfNeededbe enabled by default. This is a breaking change, since users may have relied on the failure behavior before to check if they needed to create a manifest, but now we will create one according to the rules followed by the flag, documented at: dotnet/docs#39600 regardless of if they set the flag or not.They can turn off the behavior by setting
--create-manifest-if-needed=false...Note: We should have someone who knows dotnet tools better review this PR, since I don't really know what side effects there are to making the tool manifest, besides that it puts a file on disk that allows you to run the tool locally. But I think that's what the user wants to do anyway.
Why / Should We
See the comments in the above PR...
The original maintainers were concerned this would cause people to start making tool manifests everywhere and get confused why did the tool not work from everywhere... However, we resolve this concern by only putting the tool manifest at the repo root when possible, and only in the cwd when we can't find a root. It is a strong ux improvement as before tool install local for the first time will always fail.
Explanation
I reviewed the previous PR #31231 and made sure I only impacted the related code added there, and that the code added there worked.
It basically looks for every manifest file, and returns if the manifest logic that already existed found a manifest.
If not:
It looks over the directory until it's the root directory, and searches correctly for .git, then .sln/.slnx (good change), to find a 'repo root'. If it finds one it uses that to make a manifest, otherwise it uses the CWD/probeStart which was existing logic.
It's a bit confusing due to the use of .Value.Value, but apparently the value of the directory path, probePath, is not a string but {""}, so you need to call value twice.
Code changes
The change simply makes the default factory constructor for the manifest if needed to set it to true. Added tests and modified existing tests. The manifest flag is only utilized by the local tool install command and then the default tool command parser. This does make all tool commands have --create-manifest-if-needed as true, but only the tool install local command does anything with it, so this shouldnt change the outcome of the other commands to my knowledge.