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.

Sign up for GitHub

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

Jump to bottom

[BLOCKED] System.CommandLine update #111864

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

[BLOCKED] System.CommandLine update #111864

wants to merge 7 commits into from
+440 −423

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Jan 27, 2025 •
edited
Loading

Edit: this PR is blocked until NuGet/NuGet.Client#6236 gets merged (ETA is April).

This PR consists of three changes:

dotnet-maestro bot and others added 3 commits January 24, 2025 05:02
@dotnet-maestro
Update dependencies from https://github.com/dotnet/command-line-api b…
54174f4 
…uild 20250122.1

Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine
 From Version 0.1.552801 -> To Version 0.1.607201
@adamsitnik
adopt to breaking changes in System.CommandLine
f472311
@adamsitnik
stop using System.CommandLine.Experimental
af27504
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

@adamsitnik adamsitnik changed the base branch from darc-main-78d47b88-14c1-4273-ba59-3be62a433c52 to main January 27, 2025 15:07
@adamsitnik adamsitnik closed this Jan 27, 2025
@adamsitnik adamsitnik reopened this Jan 27, 2025
stephentoub
stephentoub reviewed Jan 27, 2025
View reviewed changes
src/libraries/System.Net.Http/tests/StressTests/HttpStress/Program.cs
if (!Version.TryParse(result.Tokens.Single().Value, out Version? parsed))
{
result.AddError($"'{result.Tokens[0].Value}' is not a valid Version");
return default;
Copy link
Member

@stephentoub stephentoub Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: presumably parsed here is already default / null, so this line could just be removed?

@adamsitnik
Copy link
Member Author

I am adding NO-MERGE to make sure it's not merged before Preview 1 snap (VMR could suffer as most of our repos have dependency to S.CL)

@adamsitnik adamsitnik added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 27, 2025
@adamsitnik
Copy link
Member Author

/azp list

@azure-pipelines Azure Pipelines
Copy link

CI/CD Pipelines for this repository:

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries stress-ssl

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Jan 27, 2025
Open
Open
Open
Open
Open
@adamsitnik adamsitnik mentioned this pull request Jan 29, 2025
Closed
@adamsitnik
fix the build by keeping the versions hardcoded, as stress test proje…
bf43a70 
…cts do not import the repo infrastructure intentionally
@adamsitnik
Merge remote-tracking branch 'upstream/main' into scl_prefix_update
c4a44c0 
# Conflicts:
#	src/coreclr/tools/InjectResource/Program.cs
@adamsitnik
Copy link
Member Author

/azp run runtime-libraries stress-ssl

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries stress-http

1 similar comment
@adamsitnik
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries stress-ssl

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Feb 4, 2025
Closed
Merged
@adamsitnik adamsitnik marked this pull request as draft February 6, 2025 11:25
@adamsitnik adamsitnik changed the title System.CommandLine update [BLOCKED] System.CommandLine update Feb 6, 2025
@adamsitnik adamsitnik added the blocked Issue/PR is blocked on something - see comments label Feb 6, 2025
am11
am11 reviewed Feb 6, 2025
View reviewed changes
src/libraries/System.Net.Security/tests/StressTests/SslStress/SslStress.csproj
@@ -3,12 +3,14 @@
<OutputType>Exe</OutputType>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
<Nullable>enable</Nullable>
<!-- the custom feed is required for System.CommandLine -->
<RestoreSources>https://api.nuget.org/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-libraries/nuget/v3/index.json</RestoreSources>
Copy link
Member

Choose a reason for hiding this comment

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

the custom feed is required

We already have:

runtime/NuGet.config

Line 18 in 4008f82

<add key="dotnet-libraries" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-libraries/nuget/v3/index.json" />

was it failing without this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The stress projects disable all settings that come from the root of the repo on purpose.

@am11 am11 mentioned this pull request Mar 6, 2025
Open
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@adamsitnik adamsitnik reopened this Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@am11 am11 am11 left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

@stephentoub stephentoub stephentoub approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@adamsitnik adamsitnik

Labels
area-System.Console blocked Issue/PR is blocked on something - see comments NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adamsitnik @jeffhandley @stephentoub @am11