`dotnet tool install --local` works by default, by creating a manifest by default, utilizing `--create-manifest-if-needed` #48906

nagilson · 2025-05-12T17:29:51Z

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 createManifestIfNeeded be 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.

This points to MAIN, please lmk if we want this sooner.

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.

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.

dotnet-policy-service · 2025-05-12T18:32:28Z

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
Ask a committer to mail the .NET SDK Breaking Change Notification email list.

You can refer to the .NET SDK breaking change guidelines

Copilot

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")

nagilson · 2025-05-12T21:47:44Z

@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

baronfel · 2025-05-12T21:48:59Z

yeah, @edvilme recently added a few helper MSBuild targets to make this easier: #48396

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

nagilson · 2025-05-12T22:04:37Z

Oh awesome, thank you 🍰 🐙

dsplaisted

The code change looks good, but I'm wondering if we may want to tweak the design.

Do we actually want to create the manifest by default if you don't specify --local? I think it makes sense to create the manifest by default if someone specified dotnet tool install --local MyTool, but if they specify dotnet tool install MyTool maybe they wanted a global tool but forgot to specify.

What do you think @baronfel?

test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs

nagilson · 2025-05-20T20:33:55Z

Great point; I don't have a strong opinion on the --local part, though I would default to doing this always as it makes the 'common case' easier. Assumption: Local tools are the common case, maybe if we look at telemetry that's not the case?

nagilson · 2025-06-02T17:08:24Z

Based on our partner sync, we are going to go forward with this local option. Thanks for including that in the discussion. Do you have an opinion on changing the error message?

dsplaisted · 2025-06-02T21:05:02Z

Yes, I think we should change the message. It looks like first a ToolManifestCannotBeFoundException is thrown using CliStrings.CannotFindAManifestFile as the message. Then in ToolInstallLocalCommand.GetManifestFilePath, the exception is rethrown with the additional CliCommandStrings.ToolInstallNoManifestGuide message. I think we can get rid of the rethrow and the CliCommandStrings.ToolInstallNoManifestGuide string altogether.

The text for CliStrings.CannotFindAManifestFile is:

Cannot find a manifest file.
For a list of locations searched, specify the "-d" option before the tool name.

I think specifying the -d option doesn't actually work. So we should either make it work or always list the locations searched (and remove the mention of the -d option).

`CannotFindAManifestFile` now always provides the list of searched locations. Remove the string that is now longer needed now that --create-manifest-if-needed is added by default. Need to look into `ToolCommonNoManifestGuide` and if we still need that

Looks like we can keep the common one since its used in uninstall as well as install, but in cases where you set --create-manifest-if-needed to false, which is a case where you likely did want a global tool.

nagilson · 2025-06-02T22:58:52Z

Yes, I think we should change the message. It looks like first a ToolManifestCannotBeFoundException is thrown using CliStrings.CannotFindAManifestFile as the message. Then in ToolInstallLocalCommand.GetManifestFilePath, the exception is rethrown with the additional CliCommandStrings.ToolInstallNoManifestGuide message. I think we can get rid of the rethrow and the CliCommandStrings.ToolInstallNoManifestGuide string altogether.

The text for CliStrings.CannotFindAManifestFile is:

Cannot find a manifest file.
For a list of locations searched, specify the "-d" option before the tool name.

I think specifying the -d option doesn't actually work. So we should either make it work or always list the locations searched (and remove the mention of the -d option).

Thanks, your description was helpful and I appreciate the thoroughness. I followed this up by removing the -d flag and doing this by default, as well as removing the string. I also found ToolCommonNoManifestGuide but I think we can keep that and I provided more info in the commit details.

src/Cli/dotnet/ToolManifest/ToolManifestFinder.cs

Just search an arbitrary number of the substring Not ideal, trying this for now

The productBuild variable got renamed in dotnet#49080 which caused errors.

nagilson · 2025-06-06T18:14:22Z

/azp run

azure-pipelines · 2025-06-06T18:14:31Z

Azure Pipelines successfully started running 1 pipeline(s).

Forgind

side effects there are to making the tool manifest

The only thing that comes to mind is that if you specify that it's a root manifest, and we ever see that, we stop probing up. If we put this tools manifest at cwd and specify root: true, that might mean we'd fail to find a tool specified in a higher manifest if present...except that we'd just probed for a higher manifest and didn't find one, so we'd basically have to not have a manifest, call this, then create a manifest somewhere higher and install a local tool using it, and then we'd fail here. I don't think that's worth worrying about.

searches correctly for .git, then .sln/.slnx

Do we want to be able to support other source control systems? I do think we should permit .slnf as well even though you'd need a .sln somewhere, since you can technically have the .sln somewhere random.

That said, I don't see any of that in this PR, just the string tweaks and changing the default 🙂

Forgind · 2025-06-06T23:38:40Z

src/Cli/dotnet/Commands/Tool/Install/ToolInstallLocalCommand.cs

-                ? _toolManifestFinder.FindFirst(_createManifestIfNeeded)
-                : new FilePath(_explicitManifestFile);
-        }
-        catch (ToolManifestCannotBeFoundException e)


Why don't we want this to be graceful?

It throws ToolManifestCannotBeFoundException in findFirst, which is a child class of GracefulException

We don't need a verbose message that's a copy of the non verbose message each time....

nagilson · 2025-06-11T17:01:23Z

@marcpopMSFT This is blocked by only dotnet watch tests now, may you please merge on red

nagilson added 3 commits May 12, 2025 10:29

Add test to show it uses manifest if needed by default

6c059f9

Update Tests

9973ff0

- 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 Using this label will notify dotnet/compat and trigger a request to file a compat bug needs-breaking-change-doc-created Bug tool labels May 12, 2025

Fix Test

ac1eae1

nagilson changed the title ~~Set the default value of Create Manifest If Needed to True~~ dotnet tool install --local works by default, by creating a manifest by default, utilizing --create-manifest-if-needed May 12, 2025

Accept 'false' instead of just empty arity and add 0 arity test

11d59e6

nagilson marked this pull request as ready for review May 12, 2025 20:40

Copilot AI review requested due to automatic review settings May 12, 2025 20:40

Copilot AI reviewed May 12, 2025

View reviewed changes

nagilson requested a review from a team May 12, 2025 20:50

Update completions test bash

ff21731

nagilson added the Tab Completion label May 12, 2025

Merge branch 'main' into nagilson-create-tool-manifest-by-default

fbc755f

nagilson added 2 commits May 12, 2025 15:02

Update snapshots

99b0d0e

1. Run Test 2. dotnet restore .\dotnet.Tests\ /t:CompareCliSnapshots in test folder on test prj 3. dotnet restore .\dotnet.Tests\ /t:UpdateCliSnapshots

Merge branch 'nagilson-create-tool-manifest-by-default' of https://gi…

a3124b9

…thub.com/nagilson/sdk into nagilson-create-tool-manifest-by-default

marcpopMSFT requested a review from dsplaisted May 20, 2025 20:15

dsplaisted reviewed May 20, 2025

View reviewed changes

test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs Show resolved Hide resolved

nagilson added 2 commits June 2, 2025 15:56

Update xlf

878dd64

Looks like we can keep the common one since its used in uninstall as well as install, but in cases where you set --create-manifest-if-needed to false, which is a case where you likely did want a global tool.

Merge branch 'main' into nagilson-create-tool-manifest-by-default

f99aa7d

dsplaisted approved these changes Jun 3, 2025

View reviewed changes

src/Cli/dotnet/ToolManifest/ToolManifestFinder.cs Outdated Show resolved Hide resolved

nagilson added 5 commits June 3, 2025 11:56

update xlf again

dff877f

Remove ListOfSearched

ea6a8c1

Update XLF

fb759dc

Fix Test

7c86837

Just search an arbitrary number of the substring Not ideal, trying this for now

Fix test string format

e9d6d70

nagilson enabled auto-merge June 6, 2025 16:59

nagilson and others added 2 commits June 6, 2025 10:33

Merge branch 'main' into nagilson-create-tool-manifest-by-default

e5823e9

Fix variables in restore-toolset.sh/.ps1

80cf895

The productBuild variable got renamed in dotnet#49080 which caused errors.

This was referenced Jun 6, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

dotnet-watch tests failing #40006

Open

Forgind reviewed Jun 6, 2025

View reviewed changes

nagilson added 2 commits June 10, 2025 11:13

Merge branch 'main' into nagilson-create-tool-manifest-by-default

3979190

Fix all of the tests

76fe22d

We don't need a verbose message that's a copy of the non verbose message each time....

build-analysis bot mentioned this pull request Jun 11, 2025

[dotnet watch] Cannot open project Lib.fsproj because extension fsproj is not associated with a language #46378

Open

Merge branch 'main' into nagilson-create-tool-manifest-by-default

2543536

marcpopMSFT disabled auto-merge June 11, 2025 17:11

marcpopMSFT merged commit 00768c6 into dotnet:main Jun 11, 2025
9 of 30 checks passed

This was referenced Jun 11, 2025

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

Unable to checkout dotnet/dnceng#4115

Open

dotnet tool install --local works by default, by creating a manifest by default, utilizing --create-manifest-if-needed #48906

dotnet tool install --local works by default, by creating a manifest by default, utilizing --create-manifest-if-needed #48906

Uh oh!

Conversation

nagilson commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's this do

Why / Should We

Explanation

Code changes

Uh oh!

dotnet-policy-service bot commented May 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

nagilson commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baronfel commented May 12, 2025

Uh oh!

nagilson commented May 12, 2025

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nagilson commented May 20, 2025

Uh oh!

nagilson commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsplaisted commented Jun 2, 2025

Uh oh!

nagilson commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nagilson commented Jun 6, 2025

Uh oh!

azure-pipelines bot commented Jun 6, 2025

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Forgind Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

nagilson Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

nagilson commented Jun 11, 2025

Uh oh!

Uh oh!

Uh oh!

`dotnet tool install --local` works by default, by creating a manifest by default, utilizing `--create-manifest-if-needed` #48906

`dotnet tool install --local` works by default, by creating a manifest by default, utilizing `--create-manifest-if-needed` #48906

nagilson commented May 12, 2025 •

edited

Loading

nagilson commented May 12, 2025 •

edited

Loading

nagilson commented Jun 2, 2025 •

edited

Loading

nagilson commented Jun 2, 2025 •

edited

Loading