Skip to content
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

Update System.CommandLine #6236

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

adamsitnik
Copy link

@adamsitnik adamsitnik commented Jan 23, 2025

Context: we are working on releasing a new version of System.CommandLine. The only change we decided to revert was the adding of "Cli" prefix to all symbol types (because it would break every single user of the library).

To unblock the dependencies flow to dotnet/sdk (dotnet/sdk#46241 and dotnet/sdk#46224), we are going to need a new version of NuGet.CommandLine.XPlat that consumes the renamed APIs.

I was able to build it locally, the only thing I don't understand is why there were two different versions of System.CommandLine in the project:

<SystemCommandLineVersion Condition="'$(SystemCommandLineVersion)' == ''">2.0.0-beta4.23307.1</SystemCommandLineVersion>

<Dependency Name="System.CommandLine" Version="2.0.0-beta4.24068.1">

@adamsitnik adamsitnik requested a review from a team as a code owner January 23, 2025 14:04
@microsoft-github-policy-service microsoft-github-policy-service bot added the Community PRs created by someone not in the NuGet team label Jan 23, 2025
@adamsitnik
Copy link
Author

@adamsitnik
Copy link
Author

The CI failure indicates some kind of chicken-and-egg problem: the repo uses an SDK version that uses the older version of S.CL and hence runs into issue at runtime. What is the recommended approach for such cases?

  [xUnit.net 00:00:08.33]     NuGet.Signing.CrossFramework.Test.CrossFrameworkVerificationTest.Verify_AuthorSignedTimestampedRepositoryCounterSignedTimestampedPackage_SuccessAsync [FAIL]
    Failed NuGet.Signing.CrossFramework.Test.CrossFrameworkVerificationTest.Verify_AuthorSignedTimestampedRepositoryCounterSignedTimestampedPackage_SuccessAsync [3 s]
    Error Message:
     Expected result.Success to be true
  because System.TypeLoadException: Could not load type 'System.CommandLine.CliConfiguration' from assembly 'System.CommandLine, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'.
     at Microsoft.DotNet.Cli.Parser..cctor()
     at Microsoft.DotNet.Cli.Program.ProcessArgs(String[] args, TimeSpan startupTime, ITelemetry telemetryClient)
     at Microsoft.DotNet.Cli.Program.Main(String[] args)
  , but found False.

@nkolev92
Copy link
Member

Those tests patch the NuGet SDK onto the existing SDK and run tests.

Maybe we can try copying the System.Commandline.dll in the NuGet test and see what happens?
Do typeforwarders exist for these types or will the SDK commands start failing in this case?

@zivkan
Copy link
Member

zivkan commented Jan 23, 2025

rather than a hard breaking change, can you create a version of System.CommandLine that has both the old and new types, so there is no breaking change (for example, public class CliArgument : Argument, and add any constructors needed to maintain API compatibility). Then in a future version of System.CommandLine (say, after checking in VMR to see if all components have migrated) you can remove the Cli* APIs?

@adamsitnik
Copy link
Author

rather than a hard breaking change, can you create a version of System.CommandLine that has both the old and new types, so there is no breaking change (for example, public class CliArgument : Argument, and add any constructors needed to maintain API compatibility). Then in a future version of System.CommandLine (say, after checking in VMR to see if all components have migrated) you can remove the Cli* APIs?

It would be tricky and dirty, but possible.

@zivkan How about releasing a new version of NuGet.CommandLine.XPlat based on this branch just to the internal NuGet feed used by the SDK? So then I could just bump the version number in the SDK and then get the changes to flow and just merge this PR once it becomes green? How much work would it take?

@jeffkl jeffkl self-assigned this Jan 28, 2025
@adamsitnik
Copy link
Author

adamsitnik commented Jan 29, 2025

I've taken a closer look at the proposal from @zivkan and got to the conclusion that I am not sure if this would have worked:

  • For CliConfiguration, we would just need to create rename CommandLineConfiguration to CliConfiguration and create a new type that derives from CliConfiguration and is called CommandLineConfiguration and exposes all the ctors.
  • For other types, especially symbols, it would be more tricky, as the inheritance hierarchy is already very deep and we would need to double it:
    • Symbol
      • Command
        • RootCommand
      • Option
        • Option<T>
      • Argument
        • Argument<T>
  • There are also enums like TokenType for which we can't handle it by inheritance. This is a blocker

I've briefly studied both repos and found one more solution:

  1. temporarily revert Invoke dotnet nuget why directly through API dotnet/sdk#44614 to unblock the SDK update in adjust to System.CommandLine renaming dotnet/sdk#46241
  2. merge adjust to System.CommandLine renaming dotnet/sdk#46241, get new SDK published to daily
  3. bump SDK version number in
    -Channel 9.0.2xx -Quality daily
    (I took the idea from Use .NET SDK 9.0.2xx for integration tests #6216)
  4. get the NuGet.CommandLine.XPlat update flow to SDK and revert the revert in the same PR

And the first one I had:

  1. Somehow release a new version of NuGet.CommandLine.XPlat to the internal nuget feed
  2. Extend adjust to System.CommandLine renaming dotnet/sdk#46241 with NuGet.CommandLine.XPlat version bump
  3. Bump SDK version number in https://github.com/NuGet/NuGet.Client/blob/c9471b4ad12e1fb75a5d929766f9327ad0017c56/buil
    d/DotNetSdkTestVersions.txt#L2

Of course all of the mentioned options are just workarounds. There might be more System.CommandLine breaking changes in the future (not many, but we need to have a solution ready).

@jeffkl @baronfel Please let me know what do you think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants