Skip to content

Conversation

@dotnet-maestro
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented Jun 8, 2023

This pull request updates the following dependencies

From https://github.com/dotnet/command-line-api

  • Subscription: a08a5052-c9e7-44a2-9fba-08d94728d7f2
  • Build: 20230807.1
  • Date Produced: August 7, 2023 4:34:26 PM UTC
  • Commit: a045dd54a4c44723c215d992288160eb1401bb7f
  • Branch: refs/heads/main

…uild 20230608.1

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.430701 -> To Version 0.1.430801
@ghost ghost added Area-CodeFlow untriaged Request triage from a team member labels Jun 8, 2023
@v-wuzhai
Copy link
Contributor

v-wuzhai commented Jun 9, 2023

@jonsequitur Looks like dotnet/command-line-api#2205 changed the namespace name "CliAction" caused an error, could you take a look?

@v-wuzhai v-wuzhai requested a review from jonsequitur June 12, 2023 07:17
Jason Zhai and others added 3 commits June 12, 2023 00:21
…uild 20230612.1

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.356401 -> To Version 0.1.431201
…uild 20230612.2

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.356401 -> To Version 0.1.431202
@JL03-Yue
Copy link
Contributor

@jonsequitur Could you take a look at the error here? The build is failing because of missing assembly reference.

dotnet-maestro bot and others added 5 commits June 15, 2023 17:39
…uild 20230615.1

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.356401 -> To Version 0.1.431501
…uild 20230619.1

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.356401 -> To Version 0.1.431901
…uild 20230626.1

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.356401 -> To Version 0.1.432601
@marcpopMSFT marcpopMSFT requested a review from a team as a code owner July 3, 2023 18:18
@marcpopMSFT
Copy link
Member

@adamsitnik @baronfel can you take a look as it looks like the underlying code for CliAction changed here and I'm not sure what the update should be.

…ynchronous Invoke was not called. Removed uses of EnableParseErrorReporting and EnableTypoCorrecti since these were removed from CliConfiguration. Set the Directives on CliRootCommand since CliConfiguration no longer has a local list for Directives.
@MiYanni
Copy link
Member

MiYanni commented Jul 17, 2023

@jonsequitur Can you review the changes I made in my commit above? I based the adjustments from the breaking change PR that you did for S.CL.

@marcpopMSFT
Copy link
Member

Looks like there's a test failure where missing a required parameter prints out the usage output but it missing some pieces. Not sure why some of the usage is missing and the arguments are fully missing. Odd change in behavior.

Usage:
dotnet new [ [...]] [options]
dotnet new [command] [options]

Arguments:
A short name of the template to create.
Template specific options to use.

@MiYanni
Copy link
Member

MiYanni commented Jul 19, 2023

@jonsequitur @adamsitnik Did some debugging. Looks like ParseErrorAction has changed how the help writing gets activated, which changed the output of our help when testing an error situation. The test Marc mentioned is calling dotnet new -v which normally requires you to provide a value for the -v argument. When this errors, the help is not hitting our argument handlers and not using our custom help logic.

Before

image

The CliConfiguration is created with the root command being the result of this ConfigureCommandLine method. This method always sets the HelpAction builder to use our custom builder, which eventually calls the custom help rendering.

image

You can see the Builder is set properly in HelpAction while debugging.

image

The weird part for me is when I try to add a breakpoint to the setter for the Builder property, it is never hit... Same thing for ConfigureCommandLine. There was likely some voodoo magic that was happening in the previous version of S.CL.

After

image

This new implementation doesn't seem to set the Builder properly in this scenario. The code on our side hasn't changed for setting this builder.

image

@MiYanni
Copy link
Member

MiYanni commented Jul 24, 2023

The issue was identified to be the one reported here: dotnet/command-line-api#2226

@MiYanni
Copy link
Member

MiYanni commented Jul 27, 2023

There was a fix merged for this issue. We'll have to wait until a new version is published. dotnet/command-line-api#2249

dotnet-maestro bot and others added 4 commits July 31, 2023 20:44
…uild 20230731.1

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.430701 -> To Version 0.1.438101
…uild 20230807.1

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.430701 -> To Version 0.1.440701
@MiYanni
Copy link
Member

MiYanni commented Nov 7, 2023

Current Progress: Waiting for this change to fix the current test failures for the OptionDuplicates_NotAllowed test.

Edit: That change was merged but a new S.CL package hasn't been published since Aug 7th. Disabling that check in the test for the time being.

@MiYanni MiYanni requested review from a team, arunchndr and tmat as code owners November 7, 2023 23:02
…es. But contains commented code of other variants.
…ges. The logic around OptionResult was required or else it would both show the warning and try to use -p as a property (when it should be a project) for the particular failing tests.
@marcpopMSFT marcpopMSFT requested a review from nagilson November 10, 2023 00:14
@marcpopMSFT
Copy link
Member

Passing checks, is this actually ready finally? Not sure how to review the changes given they are in response to S.CL changes.

@MiYanni
Copy link
Member

MiYanni commented Nov 10, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MiYanni
Copy link
Member

MiYanni commented Nov 10, 2023

@marcpopMSFT

Passing checks, is this actually ready finally? Not sure how to review the changes given they are in response to S.CL changes.

This is ready. Looking in DARC, here are the repos that dotnet/command-line-api flows to:

  • dotnet/dotnet-monitor
  • dotnet/format
  • dotnet/roslyn
  • dotnet/runtime
  • dotnet/templating
  • dotnet/sdk

We'll need to coordinate so that there is only 1 version of S.CL for source build to use. I'm not sure how to do that. Any suggestions?

@marcpopMSFT
Copy link
Member

Hmm, I think we need some guidance from @MichaelSimons as I think the coordination was specifically driven by source build. The runtime makes this hard to do outside of main (ie we may have to leave 8.0.2xx untouched unless we want to take a servicing change to 8.0 and 8.0.1xx)

Can we get PRs triggered for each of those repos in main? Roslyn main will flow to 8.0.2xx so I'm not sure how to make this work.

HelpBuilder.Default.CommandArgumentsSection()(context);
context.Output.WriteLine();
HelpBuilder.Default.OptionsSection()(context);
context.Output.WriteLine();
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity why was this newline needed? Did formatting change upstream or something?

Copy link
Member

@MiYanni MiYanni Nov 10, 2023

Choose a reason for hiding this comment

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

It did. The help output logic was changed and normalized. As you can see, previously, every other section here had an added WriteLine added to it except this one. It was likely being added (by accident) in the previous versions of S.CL.

I had to add it here so that the tests for checking help output passed and remained the same as expected.

public static CliConfiguration Instance { get; } = new(ConfigureCommandLine(RootCommand))
{
EnableDefaultExceptionHandler = false,
EnableParseErrorReporting = true,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we remove the error reporting here and in the other place? S.CL uses this for reporting parse errors in a nice way - I'm actually pretty confused why our tests pass with such a large change!

Copy link
Member

Choose a reason for hiding this comment

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

This feature was changed and this property no longer exists on this class. There is a discussion here that you responded to. This concept is bound to the ParseErrorAction now. From my understanding, it isn't a thing you turn on/off, but the type of action returned when a parsing error occurs. It is actually one of the things I've made a PR for, as there was a problem with a stray newline in its output: dotnet/command-line-api#2289

Copy link
Member

Choose a reason for hiding this comment

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

Wow I completely forgot about both of those things 😅

Thank you for the reminder, and the explanation! All good from my end now.

@MiYanni
Copy link
Member

MiYanni commented Nov 15, 2023

Created PRs for the other repos to update the S.CL version:

SDK PR

New PRs

Already has latest S.CL version

@lewing
Copy link
Member

lewing commented Nov 16, 2023

The changes have hit dotnet/installer#17818 so we won't get a new installer until all the prs have merged and completed flow into sdk

@nagilson nagilson self-requested a review November 22, 2023 18:09
@dotnet-maestro dotnet-maestro bot merged commit 1a64a74 into main Nov 23, 2023
@dotnet-maestro dotnet-maestro bot deleted the darc-main-a7233ddc-4b19-4004-b583-4a3b720def80 branch November 23, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeFlow untriaged Request triage from a team member waiting-on-feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.