Skip to content

Improve RootCommand executable name handling (fix #2812)#2820

Draft
jonsequitur wants to merge 2 commits into
dotnet:mainfrom
jonsequitur:fix-2812
Draft

Improve RootCommand executable name handling (fix #2812)#2820
jonsequitur wants to merge 2 commits into
dotnet:mainfrom
jonsequitur:fix-2812

Conversation

@jonsequitur

Copy link
Copy Markdown
Contributor

This pull request introduces several improvements and bug fixes to how RootCommand determines the executable name and path, especially in advanced hosting scenarios like NativeAOT native libraries. It adds comprehensive tests to ensure correct behavior, addresses edge cases with command-line parsing, and improves packaging for build targets.

Executable Name/Path Resolution Improvements:

  • Refactored RootCommand.ExecutableName and RootCommand.ExecutablePath to more robustly determine the executable name and path, including fallback to an AppContext value injected by build targets when no command line arguments are available (e.g., in NativeAOT shared library scenarios) (src/System.CommandLine/RootCommand.cs).
  • Added the System.CommandLine.targets file to the NuGet package to ensure the executable-name module initializer is available for all consumers, including test projects and native libraries (src/System.CommandLine/System.CommandLine.csproj).

NativeAOT and Advanced Hosting Test Coverage:

  • Added a new test project (NativeLibrary) that builds as a NativeAOT shared library, exporting a native symbol to validate that RootCommand.Name falls back to the correct value when Environment.GetCommandLineArgs() is empty (src/System.CommandLine.Tests/TestApps/NativeLibrary/NativeLibrary.csproj, src/System.CommandLine.Tests/TestApps/NativeLibrary/Program.cs). [1] [2]
  • Introduced an end-to-end test in CompilationTests.cs to verify the fallback logic for RootCommand.Name in native library hosting scenarios, including platform-specific handling and dynamic invocation of the exported native symbol (src/System.CommandLine.Tests/CompilationTests.cs).

Command-Line Parsing Bug Fixes and Tests:

  • Fixed a bug where option values resembling paths (e.g., /p:Key=something/myapp) could incorrectly be treated as root command names, and added targeted regression tests for these scenarios (src/System.CommandLine/Parsing/StringExtensions.cs, src/System.CommandLine.Tests/ParserTests.RootCommandAndArg0.cs). [1] [2]
  • Enhanced tests for root command and argument matching, including new assertions on error-free parsing and correct command resolution for various input patterns (src/System.CommandLine.Tests/ParserTests.RootCommandAndArg0.cs). [1] [2]

General Test Improvements:

  • Added basic tests to ensure RootCommand.ExecutablePath and RootCommand.ExecutableName are never null or empty (src/System.CommandLine.Tests/RootCommandTests.cs).

Build and Target Framework Updates:

  • Updated test and solution files to target .NET 10.0, ensuring compatibility with new features and NativeAOT scenarios (System.CommandLine.v3.ncrunchsolution).

These changes collectively improve the reliability and correctness of executable name detection across diverse hosting models, strengthen the test suite, and fix subtle parsing bugs.

<_SystemCommandLineGeneratedFile>$(IntermediateOutputPath)System.CommandLine.ExecutableName.g.cs</_SystemCommandLineGeneratedFile>
</PropertyGroup>

<Target Name="_GenerateSystemCommandLineExecutableName"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this needs to be conditioned on Language or else it'd break C#, VB, etc consumers.

@baronfel baronfel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Logic looks fine but the MSBuild targets need some work to not break incrementality


<Target Name="_GenerateSystemCommandLineExecutableName"
BeforeTargets="CoreCompile"
Inputs="$(MSBuildAllProjects)"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IIRC MSBuildAllProjects as an input is an antipattern - if you care about properties as inputs, the pattern today you should use is

  • have a target before your main target that writes the properties you care about into a file in the IntermeidateOutputPath with WriteOnlyWhenDifferent='true'
  • have that file be the input to your main target
  • make sure to add any files you generate to the FileWrites Item type in MSbuild so that dotnet clean clears them out

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.

2 participants