Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This change set demonstrates unifying the managed dotnet CLI and the NativeAOT “bridge” parser by sharing a single, full command-tree definition (via DotNetCommandDefinition) and switching AOT from a minimal parser to the shared parser/option-action infrastructure. It also expands AOT-focused tests and adjusts forwarding paths (notably MSBuild and NuGet) to support help/schema output without requiring a managed-process fallback.
Changes:
- Replace the AOT-only minimal parser with a shared full command tree and mode-specific action wiring (managed vs. CLI_AOT fallback/implementations).
- Introduce shared option actions (
--help,--version,--info,--cli-schema) and update AOT source inclusion to compile the shared implementation. - Adjust MSBuild/NuGet forwarding behavior for CLI_AOT, add/extend AOT parser and entrypoint tests, and update design documentation to reflect the new approach.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet-aot.Tests/dotnet-aot.Tests.csproj | Updates comment describing shared AOT sources imported by the test project. |
| test/dotnet-aot.Tests/AotParserTests.cs | Expands AOT parser/invocation/help/schema tests for the unified command tree. |
| src/Common/EnvironmentVariableNames.cs | Adds XML documentation for DOTNET_CLI_TELEMETRY_OPTOUT. |
| src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs | Adds forceOutOfProc support and extensive XML docs around MSBuild forwarding behavior. |
| src/Cli/Microsoft.DotNet.Cli.Definitions/Commands/DotNetCommandDefinition.cs | Switches root definition to Command, adds explicit --help, and defines shared root options/subcommands. |
| src/Cli/dotnet/ParserOptionActions.cs | Adds shared option-action implementations and CLI_AOT-aware behavior for info/schema/telemetry usage. |
| src/Cli/dotnet/Parser.cs | Builds the unified root command and wires managed vs. AOT actions; adds shared exception handler usage and help tweaks. |
| src/Cli/dotnet/NuGetSignatureVerificationEnabler.cs | Excludes MSBuild-specific signature verification enabling path from CLI_AOT. |
| src/Cli/dotnet/Extensions/ParseResultExtensions.cs | Moves common parsing helpers to be available in both managed and CLI_AOT shared sources. |
| src/Cli/dotnet/Commands/Run/CommonRunHelpers.cs | Guards in-proc MSBuild logger creation behind #if !CLI_AOT. |
| src/Cli/dotnet/Commands/NuGet/NuGetCommand.cs | Ensures CLI_AOT always uses out-of-proc NuGet runner; guards in-proc runner. |
| src/Cli/dotnet/Commands/MSBuild/MSBuildForwardingApp.cs | Forces out-of-proc MSBuild invocation under CLI_AOT and refactors execution path accordingly. |
| src/Cli/dotnet/CommandLineInfo.cs | Removes legacy info/version printing helper in favor of shared option actions. |
| src/Cli/dotnet/CliSchema.cs | Updates schema generation to use source-gen type metadata and improves enum handling for AOT. |
| src/Cli/dotnet-aot/NativeEntryPoint.cs | Enhances AOT parse-time robustness and routes invocation exceptions through shared exception rendering. |
| src/Cli/dotnet-aot/DESIGN.md | Updates AOT design documentation to match the “full command tree + fallback” architecture. |
| src/Cli/dotnet-aot/AotSourceFiles.props | Updates the set of shared sources compiled into the AOT bridge, including help/forwarding/schema support. |
| src/Cli/dn/dn-native-debug.vcxproj | Removes the obsolete AOT-linked CommandLineInfo.cs debug entry. |
Comment on lines
+91
to
+96
| else | ||
| { | ||
| // When user does not specify any args (just "dotnet"), a usage needs to be printed. | ||
| parseResult.InvocationConfiguration.Output.WriteLine(CliUsage.HelpText); | ||
| return 0; | ||
| } |
Comment on lines
+117
to
121
| else if (option is Option<bool> helpOption && helpOption.Name == "--help") | ||
| { | ||
| helpOption.Action = new PrintHelpAction(helpOption, DotnetHelpBuilder.Instance.Value); | ||
| option.Description = CliStrings.ShowHelpDescription; | ||
| helpOption.Description = CliStrings.ShowHelpDescription; | ||
| } |
Comment on lines
+102
to
+106
| /// <summary> | ||
| /// Applies tweaks to the options that <see cref="DotNetCommandDefinition"/> inherits from | ||
| /// <see cref="RootCommand"/>: the SDK defines its own <c>--version</c> option, so the built-in | ||
| /// one is removed, and the help option is re-pointed at <see cref="DotnetHelpBuilder"/>. | ||
| /// </summary> |
Comment on lines
+60
to
+68
| [Fact] | ||
| public void ParseUnknownToken_IsToleratedForExternalCommandForwarding() | ||
| { | ||
| // The dotnet root command is intentionally tolerant of unknown tokens so that | ||
| // `dotnet foo` can be forwarded to an external `dotnet-foo` command. Unknown tokens | ||
| // therefore do not produce parse errors; they are resolved by the managed CLI on fallback. | ||
| var result = Parser.Parse(["--this-option-does-not-exist"]); | ||
| Assert.Empty(result.Errors); | ||
| } |
Comment on lines
4
to
+6
| using System.Diagnostics; | ||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
Comment on lines
+75
to
78
| /// <summary> | ||
| /// | ||
| /// </summary> | ||
| private readonly Dictionary<string, string?> _msbuildRequiredEnvironmentVariables = GetMSBuildRequiredEnvironmentVariables(); |
Comment on lines
+187
to
+191
| /// <summary> | ||
| /// Directly execute's MSBuild's <see cref="Build.CommandLine.MSBuildApp.Main"/> method in the current process. | ||
| /// Sets up the local environment with required MSBuild environment variables before handing off execution entirely to MSBuild. | ||
| /// After execution, the original environment variables are restored for any remaining cleanup work the dotnet CLI needs to perform. | ||
| /// </summary> |
Owner
Author
|
Code review skill — before/after comparison This PR reproduces the exact diff Copilot reviewed on dotnet/sdk#54653 at commit
Net: default CCR surfaced 6 comments (2 substantive); the skill surfaced 7 (3 substantive). They share the AOT-forwarding bug. The skill additionally caught a real |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Demo reproduction of dotnet/sdk#54653 ("Unify the managed and NativeAOT CLI parsers into one shared implementation") at the exact commit Copilot originally reviewed (
119baec).The base branch contains
.github/skills/code-review/SKILL.mdso Copilot Code Review loads the skill. The diff is identical to what default CCR reviewed on the original PR (which produced zero inline findings). This PR is for an after-skill comparison.