Add warnings for newer tool versions during dotnet tool restore#51011
Add warnings for newer tool versions during dotnet tool restore#51011marcpopMSFT merged 4 commits intomainfrom
dotnet tool restore#51011Conversation
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
dotnet tool restore should provide warnings if new versions of tool is availabledotnet tool restore
|
@baronfel I tested and this new code only runs if the current version of the tool is not found in the cache. That seems like an ok compromise to me as otherwise, we're hitting the network when theoretically we don't need to. However, it does mean that if you have the tool installed locally, you'll never get the notice. |
|
|
That's an interesting compromise for sure. I could see a |
|
or a flag to dotnet tool restore which asks for the check. Either way, this change is reasonable and working as is so published for review. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to display warnings when newer versions of tools are available during dotnet tool restore operations. This helps users stay informed about tool updates and prevent compatibility issues with newer SDKs.
Key Changes:
- Enhanced restore functionality to check for newer tool versions and display warnings
- Added localized warning messages for the new version notification feature
- Implemented comprehensive test coverage for the warning functionality
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
ToolPackageRestorer.cs |
Added version checking logic and warning generation functionality |
ToolRestoreCommand.cs |
Enhanced result structure and display logic to show version warnings |
CliCommandStrings.resx |
Added the new localized message string for version warnings |
Multiple .xlf files |
Added localization entries for the new warning message |
ToolRestoreCommandTests.cs |
Added comprehensive test coverage for the warning functionality |
I saw this and immediately worried about such a warning getting triggered in an automated build, being parsed by Jenkins and causing the build to be considered UNSTABLE. It looks like the output is not actually formatted anything like "warning SDK0000: tool is not the latest available version" though, so it won't be parsed as a warning. Please call it a note, then. |
|
@KalleOlaviNiemitalo yeah, this in the bucket of CLI messages so shouldn't impact exit codes or builds. It can impact if someone is processing the output. |
| // Use wildcard version range to get the latest version | ||
| var latestVersionRange = VersionRange.Parse("*"); | ||
|
|
||
| var (latestVersion, _) = _toolPackageDownloader.GetNuGetVersion( |
There was a problem hiding this comment.
I'm pretty confident this won't add additional unexpected terminal output, so that's good.
| if (latestVersion != null && latestVersion > package.Version) | ||
| { | ||
| // If the current version is prerelease, show warning for any newer version | ||
| // If the current version is stable, only show warning for newer stable versions |
|
|
||
|
|
||
| // Display warnings for successful restorations even in partial failure case | ||
| var warnings = toolRestoreResults.Where(r => r.IsSuccess && !string.IsNullOrEmpty(r.Warning)).Select(r => r.Warning); |
There was a problem hiding this comment.
Interesting, so we're returning it as a regular, (but yellow) line instead of a warning to prevent breaking changes. Seems good.
| [Fact] | ||
| public void WhenNewerVersionIsAvailableItShowsWarning() | ||
| { | ||
| var newerPackageVersion = NuGetVersion.Parse("2.1.4"); |
There was a problem hiding this comment.
nit: we could probably add to the _packageVersionA to prevent updating that from breaking this test.
@copilot We also don't test the prerelease version behaviors, which seems significant enough that we should test to prevent regression.
There was a problem hiding this comment.
Added in commit 1f3148a. The test now uses explicit versions to avoid breaking if _packageVersionA changes. Also added two tests for prerelease behaviors:
- Prerelease to stable: Shows warning
- Stable to prerelease: Does NOT show warning (prevents prerelease noise)
nagilson
left a comment
There was a problem hiding this comment.
Looks good - let's validate it adds the test and if it doesn't change anything else we should be good to go
Co-authored-by: nagilson <23152278+nagilson@users.noreply.github.com>
nagilson
left a comment
There was a problem hiding this comment.
Nicely done on the new test.
|
/backport to release/10.0.3xx |
|
Started backporting to |
|
@marcpopMSFT backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128 |
|
Started backporting to |
|
@marcpopMSFT backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128 |
|
manual backport pr for 10.0.3xx is #52715 |
|
Does this show a warning if:
In this case, I think it should warn that the newer stable version 1.1.0 is available; but the implementation looks like it won't warn anything about versions, because the latest version 1.2.0-rc.1 is a prerelease but the specified version 1.0.0 is stable. |
|
@KalleOlaviNiemitalo see the code here: https://github.com/dotnet/sdk/blob/main/src/Cli/dotnet/Commands/Tool/Restore/ToolPackageRestorer.cs#L159 If the current version is a prerelease, it'll tell you if there's a newer prerelease. Otherwise, it won't tell you to update to a prerelease. Looking at the code, there is a slight gap in the scenario you mentioned where if there is a prerelease, we won't tell you about the latest stable version to upgrade to. Not sure how common that is but if you think it's worth improving the logic for, feel free to file an issue and we'll see if copilot can change the GetNuGetVersion call to account for that. Not something I'd prioritize as that seems more niche. |
|
Linking back to #50991 for reference. This link was originally in the PR description but Copilot edited it out on December 10. |
Changes in this commit:
warning:parameter in ToolPackageRestorer.cs (line 103)Test Coverage:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.