Skip to content

System.CommandLine update #48477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Apr 16, 2025
Merged

System.CommandLine update #48477

merged 16 commits into from
Apr 16, 2025

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Apr 15, 2025

dotnet-maestro bot and others added 7 commits April 15, 2025 08:33
…414.5

Microsoft.SourceBuild.Intermediate.roslyn , Microsoft.CodeAnalysis , Microsoft.CodeAnalysis.CSharp , Microsoft.CodeAnalysis.CSharp.CodeStyle , Microsoft.CodeAnalysis.CSharp.Features , Microsoft.CodeAnalysis.CSharp.Workspaces , Microsoft.CodeAnalysis.PublicApiAnalyzers , Microsoft.CodeAnalysis.Workspaces.Common , Microsoft.CodeAnalysis.Workspaces.MSBuild , Microsoft.Net.Compilers.Toolset , Microsoft.Net.Compilers.Toolset.Framework
 From Version 5.0.0-1.25213.1 -> To Version 5.0.0-1.25214.5
…414.8

Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Templating , Microsoft.DotNet.Build.Tasks.Workloads , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.XliffTasks , Microsoft.DotNet.XUnitExtensions
 From Version 10.0.0-beta.25212.1 -> To Version 10.0.0-beta.25214.8
…0250414.7

Microsoft.SourceBuild.Intermediate.templating , Microsoft.TemplateEngine.Abstractions , Microsoft.TemplateEngine.Authoring.TemplateVerifier , Microsoft.TemplateEngine.Edge , Microsoft.TemplateEngine.Mocks , Microsoft.TemplateEngine.Orchestrator.RunnableProjects , Microsoft.TemplateEngine.TestHelper , Microsoft.TemplateEngine.Utils , Microsoft.TemplateSearch.Common , Microsoft.TemplateSearch.TemplateDiscovery
 From Version 10.0.100-preview.4.25214.2 -> To Version 10.0.100-preview.4.25214.7
…0250414.8

Microsoft.SourceBuild.Intermediate.templating , Microsoft.TemplateEngine.Abstractions , Microsoft.TemplateEngine.Authoring.TemplateVerifier , Microsoft.TemplateEngine.Edge , Microsoft.TemplateEngine.Mocks , Microsoft.TemplateEngine.Orchestrator.RunnableProjects , Microsoft.TemplateEngine.TestHelper , Microsoft.TemplateEngine.Utils , Microsoft.TemplateSearch.Common , Microsoft.TemplateSearch.TemplateDiscovery
 From Version 10.0.100-preview.4.25214.2 -> To Version 10.0.100-preview.4.25214.8
…0250414.2

Microsoft.SourceBuild.Intermediate.sourcelink , Microsoft.Build.Tasks.Git , Microsoft.SourceLink.AzureRepos.Git , Microsoft.SourceLink.Bitbucket.Git , Microsoft.SourceLink.Common , Microsoft.SourceLink.GitHub , Microsoft.SourceLink.GitLab
 From Version 10.0.0-beta.25203.1 -> To Version 10.0.0-beta.25214.2
@Copilot Copilot AI review requested due to automatic review settings April 15, 2025 11:24
@adamsitnik adamsitnik requested review from a team as code owners April 15, 2025 11:24
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Apr 15, 2025
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

1 similar comment
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Contributor

@Copilot 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.

Copilot reviewed 175 out of 178 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • eng/Version.Details.xml: Language not supported
  • eng/Versions.props: Language not supported
  • global.json: Language not supported
Comments suppressed due to low confidence (3)

src/Cli/Microsoft.TemplateEngine.Cli/Commands/ICommandDocument.cs:9

  • Ensure that the XML documentation consistently reflects the use of the new Command type instead of the legacy CliCommand, and verify that this update is in line with the rest of the codebase.
/// If a <see cref="Command"/> implements this interface, it can open

src/Cli/Microsoft.TemplateEngine.Cli/Commands/BaseCommand.cs:225

  • Verify that replacing AsynchronousCliAction with AsynchronousCommandLineAction meets the expected asynchronous action contract, ensuring no behavioral regressions occur.
private sealed class CommandAction : AsynchronousCommandLineAction

src/BuiltInTools/dotnet-watch/CommandLineOptions.cs:67

  • Confirm that replacing CliRootCommand with RootCommand and updating to CommandLineConfiguration preserves the intended command-line behavior in dotnet-watch.
var rootCommand = new RootCommand(Resources.Help)

@@ -10,10 +10,11 @@
using Microsoft.TemplateEngine.Edge;
using Microsoft.TemplateEngine.Edge.Settings;
using Microsoft.TemplateEngine.Utils;
using Command = System.CommandLine.Command;
Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting the following error:

'Command' is an ambiguous reference between 'System.CommandLine.Command' and 'Microsoft.DotNet.Cli.Utils.Command'

and decided to make it explicit with a using rather than using fully qualified name everywhere

Copy link
Member

Choose a reason for hiding this comment

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

I'll clean this stuff up later so don't worry about it.

@@ -97,13 +97,13 @@ public static bool IsDotnetBuiltInCommand(this ParseResult parseResult)

public static bool IsTopLevelDotnetCommand(this ParseResult parseResult)
{
return parseResult.CommandResult.Command.Equals(RootCommand) && string.IsNullOrEmpty(parseResult.RootSubCommandResult());
return parseResult.CommandResult.Command.Equals(Microsoft.DotNet.Cli.Parser.RootCommand) && string.IsNullOrEmpty(parseResult.RootSubCommandResult());
Copy link
Member Author

Choose a reason for hiding this comment

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

CliRootCommand got renamed to RootCommand and it conflicted with RootCommand which was imported here with a static using:

using static Microsoft.DotNet.Cli.Parser;

adamsitnik and others added 2 commits April 15, 2025 21:27
…6.15.0.7

Microsoft.Build.NuGetSdkResolver , NuGet.Build.Tasks , NuGet.Build.Tasks.Console , NuGet.Build.Tasks.Pack , NuGet.CommandLine.XPlat , NuGet.Commands , NuGet.Common , NuGet.Configuration , NuGet.Credentials , NuGet.DependencyResolver.Core , NuGet.Frameworks , NuGet.LibraryModel , NuGet.Localization , NuGet.Packaging , NuGet.ProjectModel , NuGet.Protocol , NuGet.Versioning
 From Version 6.14.0-preview.1.102 -> To Version 6.15.0-preview.1.7
@nkolev92
Copy link
Contributor

I cherry-picked #48489 into this PR.

@@ -10,10 +10,11 @@
using Microsoft.TemplateEngine.Edge;
using Microsoft.TemplateEngine.Edge.Settings;
using Microsoft.TemplateEngine.Utils;
using Command = System.CommandLine.Command;
Copy link
Member

Choose a reason for hiding this comment

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

I'll clean this stuff up later so don't worry about it.

@ViktorHofer
Copy link
Member

ViktorHofer commented Apr 16, 2025

@premun we need to flow sourcelink into the VMR from sdk for this PR. Can we change the flow back temporarily to unblock the PR?

dotnet-maestro bot and others added 2 commits April 16, 2025 08:02
…0415.12

Microsoft.Bcl.AsyncInterfaces , Microsoft.Extensions.Configuration.Ini , Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.Logging , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging.Console , Microsoft.NET.HostModel , Microsoft.NET.ILLink.Tasks , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.Platforms , Microsoft.Win32.SystemEvents , System.CodeDom , System.Composition.AttributedModel , System.Composition.Convention , System.Composition.Hosting , System.Composition.Runtime , System.Composition.TypedParts , System.Configuration.ConfigurationManager , System.Formats.Asn1 , System.IO.Hashing , System.Reflection.MetadataLoadContext , System.Resources.Extensions , System.Security.Cryptography.Pkcs , System.Security.Cryptography.ProtectedData , System.Security.Cryptography.Xml , System.Security.Permissions , System.ServiceProcess.ServiceController , System.Text.Encoding.CodePages , System.Text.Json , System.Windows.Extensions , VS.Redist.Common.NetCore.SharedFramework.x64.10.0 , VS.Redist.Common.NetCore.TargetingPack.x64.10.0 , Microsoft.SourceBuild.Intermediate.runtime.linux-x64
 From Version 10.0.0-preview.4.25211.19 -> To Version 10.0.0-preview.4.25215.12
@premun
Copy link
Member

premun commented Apr 16, 2025

@ViktorHofer do you mean to enable to old kind of synchronization for sourcelink?
Why can't we make the change sourcelink directly?

@ViktorHofer
Copy link
Member

ViktorHofer commented Apr 16, 2025

The change was already made to sourcelink but everything needs to flow-in together via sdk in this case. Those are tightly coupled product dependencies.

Yes, old synchronization. See my commits in this PR.

@premun
Copy link
Member

premun commented Apr 16, 2025

@ViktorHofer then please change the disableSynchronization for sourcelink in this PR to see if it works.
(and update the V.D.xml record of sourcelink)

@premun
Copy link
Member

premun commented Apr 16, 2025

Ah, I see. Then yeah, feel free to merge this

@ViktorHofer
Copy link
Member

@adamsitnik I think you need to revert changes under src/SourceBuild.

@adamsitnik
Copy link
Member Author

I've re-run the failed sdk test leg as the failing test has 6% failure rate.

@ViktorHofer how can we overcome the source build failure?

@ViktorHofer
Copy link
Member

Ok that worked. The latest update should resolve the test failure in the source build job.

@ViktorHofer
Copy link
Member

We will need to temporarily break source-build stage2 for this. Merge the PR, wait for a new official build and then re-bootstrap.

@adamsitnik
Copy link
Member Author

@ViktorHofer the sdk-source-build has failed with:

/__w/1/vmr/eng/tools/BinaryToolKit/Program.cs(61,20): error CS0246: The type or namespace name 'CliCommand' could not be found (are you missing a using directive or an assembly reference?) [/__w/1/vmr/eng/tools/BinaryToolKit/BinaryToolKit.csproj]
/__w/1/vmr/eng/tools/BinaryToolKit/Program.cs(71,42): error CS0246: The type or namespace name 'CliCommand' could not be found (are you missing a using directive or an assembly reference?) [/__w/1/vmr/eng/tools/BinaryToolKit/BinaryToolKit.csproj]
/__w/1/vmr/eng/tools/BinaryToolKit/Program.cs(12,28): error CS0246: The type or namespace name 'CliArgument<>' could not be found (are you missing a using directive or an assembly reference?) [/__w/1/vmr/eng/tools/BinaryToolKit/BinaryToolKit.csproj]
/__w/1/vmr/eng/tools/BinaryToolKit/Program.cs(18,28): error CS0246: The type or namespace name 'CliOption<>' could not be found (are you missing a using directive or an assembly reference?) [/__w/1/vmr/eng/tools/BinaryToolKit/BinaryToolKit.csproj]
/__w/1/vmr/eng/tools/BinaryToolKit/Program.cs(25,28): error CS0246: The type or namespace name 'CliOption<>' could not be found (are you missing a using directive or an assembly reference?) [/__w/1/vmr/eng/tools/BinaryToolKit/BinaryToolKit.csproj]
/__w/1/vmr/eng/tools/BinaryToolKit/Program.cs(33,28): error CS0246: The type or namespace name 'CliOption<>' could not be found (are you missing a using directive or an assembly reference?) [/__w/1/vmr/eng/tools/BinaryToolKit/BinaryToolKit.csproj]

Does it mean I should actually port everything in https://github.com/dotnet/sdk/blob/main/src/SourceBuild/content/eng/tools/BinaryToolKit ? (I did that initially and then you told me to revert it).

@ViktorHofer
Copy link
Member

See my previous comment. We can't fix source-build stage2 here as then source-build stage1 would be broken. We will need to force-merge that PR and then unbreak it by doing some work. I will drive that today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants