Skip to content

dotnetup --untracked looks for existing versions + fix shimmer effect to use 'ed' verbs#54539

Open
nagilson wants to merge 14 commits into
dotnet:release/dnupfrom
nagilson:nagilson-dotnetup-respects-requested-versoin
Open

dotnetup --untracked looks for existing versions + fix shimmer effect to use 'ed' verbs#54539
nagilson wants to merge 14 commits into
dotnet:release/dnupfrom
nagilson:nagilson-dotnetup-respects-requested-versoin

Conversation

@nagilson
Copy link
Copy Markdown
Member

@nagilson nagilson commented Jun 1, 2026

Fix the shimmer effect to set 'installed' at the end and similar for other verb transformations.
Fix the --untracked arg to also see if the install already exists so it doesn't waste network/dl time.
Remove unnecessary root commands.

@nagilson nagilson changed the title bug fix: dotnetup respects requested version bug fix: dotnetup install respects requested version Jun 1, 2026
@nagilson nagilson marked this pull request as ready for review June 1, 2026 20:46
Copilot AI review requested due to automatic review settings June 1, 2026 20:46
@nagilson nagilson requested a review from a team June 1, 2026 20:47
@baronfel
Copy link
Copy Markdown
Member

baronfel commented Jun 1, 2026

(you can boo me)

boo hiss

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a dotnetup install/dotnetup sdk install parsing regression where the requested SDK version/channel token could be lost due to sharing System.CommandLine Argument instances across command trees, and adds a small optimization for --untracked installs. Also refactors the Spectre progress shimmer into a reusable ShimmerProgressTask with targeted tests.

Changes:

  • Stop sharing System.CommandLine Argument instances between root and subcommands; pass the specific Argument into the command so it reads the correct parse result.
  • Introduce ShimmerProgressTask and adapt Spectre progress tasks through a thin adapter; add tests around shimmer/installed description behavior.
  • For --untracked, add a filesystem-based “already installed” check (layout validation) to avoid unnecessary network/download work.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/dotnetup.Tests/ProgressDescriptionTests.cs New tests validating shimmer/description lifecycle and extraction “Installed …” final state.
test/dotnetup.Tests/ParserTests.cs New regression tests ensuring exact channel/spec tokens are preserved for root and subcommands.
src/Installer/dotnetup.Library/SpectreProgressTarget.cs Switches progress task implementation to use ShimmerProgressTask + adapter.
src/Installer/dotnetup.Library/ShimmerProgressTask.cs New reusable shimmer decorator for IProgressTask.
src/Installer/dotnetup.Library/Parser.cs Root action now constructs SdkInstallCommand with no channel argument to avoid conflicts.
src/Installer/dotnetup.Library/InstallerOrchestratorSingleton.cs Adds --untracked already-installed detection via ArchiveInstallationValidator.
src/Installer/dotnetup.Library/Commands/Sdk/Uninstall/SdkUninstallCommandParser.cs Splits channel arguments per command (sdk vs root) and passes argument into the command.
src/Installer/dotnetup.Library/Commands/Sdk/Uninstall/SdkUninstallCommand.cs Reads channel from the specific Argument passed by the parser to avoid symbol re-parenting issues.
src/Installer/dotnetup.Library/Commands/Sdk/Install/SdkInstallCommandParser.cs Splits install channel arguments per command and passes argument into the command.
src/Installer/dotnetup.Library/Commands/Sdk/Install/SdkInstallCommand.cs Reads channels from the provided Argument instance; supports root “bare dotnetup” path via null argument.
src/Installer/dotnetup.Library/Commands/Runtime/Update/RuntimeUpdateCommandParser.cs Removes unused root update command plumbing.
src/Installer/dotnetup.Library/Commands/Runtime/Uninstall/RuntimeUninstallCommandParser.cs Removes unused root uninstall command plumbing.

Comment thread src/Installer/dotnetup.Library/ShimmerProgressTask.cs
Comment thread src/Installer/dotnetup.Library/ShimmerProgressTask.cs
Comment thread test/dotnetup.Tests/ProgressDescriptionTests.cs
@baronfel
Copy link
Copy Markdown
Member

baronfel commented Jun 3, 2026

@nagilson I'm not sure I can reproduce the behavior you're seeing. Here's a test harness I made to try to isolate it:

#: property PublishAot=false
#: package System.CommandLine@2.0.8

using System.CommandLine;
using System.CommandLine.Parsing;

Command rootCommand = new("dotnetup");

Command sdkCommand = new("sdk");
rootCommand.Add(sdkCommand);

Command installCommand = new("install");
sdkCommand.Add(installCommand);
rootCommand.Add(installCommand);

Argument<string[]> channelArgument = new("channel") { Arity = ArgumentArity.ZeroOrMore };
rootCommand.Add(channelArgument);
installCommand.Add(channelArgument);

installCommand.SetAction(LogChannels);

rootCommand.Parse(args).Invoke();

void LogChannels(ParseResult parseResult)
{
    var channelResult = parseResult.GetResult(channelArgument);
    var resultTree = GetResultTree(channelResult);
    Console.WriteLine($"got result from {resultTree}");
    var channels = parseResult.GetValue(channelArgument);
    if (channels == null || channels.Length == 0)
    {
        Console.WriteLine("No channels specified.");
    }
    else
    {
        Console.WriteLine($"Channels: {string.Join(", ", channels)}");
    }
}

string GetResultTree(SymbolResult? result)
{
    if(result == null) return "<root>";
    string symbolName = GetSymbolName(result);
    return result.Parent switch
    {
        null => symbolName,
        var p => $"{GetResultTree(p)} -> {symbolName}"
    };
}

string GetSymbolName(SymbolResult result)
{
    return result switch
    {
        ArgumentResult arg => arg.Argument.Name,
        OptionResult opt => opt.Option.Name,
        CommandResult c => c.Command.Name,
        _ => throw new NotImplementedException()
    };
}
> dotnet run scl.cs --no-restore -- sdk install 8 9 latest
got result from dotnetup -> sdk -> install -> channel
Channels: 8, 9, latest
> dotnet run scl.cs --no-restore -- install 8 9 latest
got result from dotnetup -> install -> channel
Channels: 8, 9, latest

In S.CL, if you have a reference to the argument, results are looked up by name in the results dictionaries - so as long as the argument exists anywhere in the command heirarchy the lookup should resolve just fine.

@nagilson
Copy link
Copy Markdown
Member Author

nagilson commented Jun 3, 2026

@baronfel

Here's a test harness I made to try to isolate it...

You're right, I agree with you. Sorry, I was trying to fix this at the same time as too many other things.

I think the issue was the version parsing bug that was already fixed + that CI builds with the fix didn't get pushed due to the breaking change in az, breaking microbuild.

I will let this PR exist to clean up the unnecessary root commands while removing the copied arguments.
I regret including the other fixes here, but they are minimal enough I'm hoping it's ok.

@nagilson nagilson changed the title bug fix: dotnetup install respects requested version dotnetup --untracked looks for existing versions + fix shimmer effect Jun 3, 2026
@nagilson nagilson changed the title dotnetup --untracked looks for existing versions + fix shimmer effect dotnetup --untracked looks for existing versions + fix shimmer effect to use 'ed' verbs Jun 3, 2026
@nagilson nagilson requested a review from baronfel June 3, 2026 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants