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

[release/6.0] Make sure WinHttpHandler 6.0.1 is released #65523

Merged

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Feb 17, 2022

This addresses an overlook from #63346. We forgot to follow the nuget servicing guideline so the bugfix was not released.

Fixes #60291

/cc @safern

Customer Impact

This is follow up to PR #63346 which was approved and merged for 6.0.2, but missed nuget authoring -- as a result, the fix didn't really ship as part of 6.0.2.

#60291 is a bug in WinHttpHandler that prevents running/debugging gRPC code on .NET Framework using Visual Studio + IIS Express which is an important customer scenario for dotnet/core#5713. It can probably also manifest in production environments with servers other than Kestrel.

Testing

N/A

Risk

Low.

@antonfirsov antonfirsov added Servicing-consider Issue for next servicing release review area-System.Net.Http labels Feb 17, 2022
@antonfirsov antonfirsov added this to the 6.0.x milestone Feb 17, 2022
@antonfirsov antonfirsov requested a review from a team February 17, 2022 18:25
@ghost ghost assigned antonfirsov Feb 17, 2022
@ghost
Copy link

ghost commented Feb 17, 2022

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

Issue Details

This addresses an overlook from #63346. We forgot to follow the nuget servicing guideline so the bugfix was not released.

Fixes #60291
/cc @safern

Customer Impact

#60291 is a bug in WinHttpHandler that prevents running/debugging gRPC code on .NET Framework using Visual Studio + IIS Express which is an important customer scenario for dotnet/core#5713. It can probably also manifest in production environments with servers other than Kestrel.

Testing

N/A

Risk

Low.

Author: antonfirsov
Assignees: -
Labels:

Servicing-consider, area-System.Net.Http

Milestone: 6.0.x

@antonfirsov
Copy link
Member Author

@danmoseley does this need to go through servicing process?

@danmoseley
Copy link
Member

No approval needed. This is part of the original approved fix

@antonfirsov antonfirsov removed the Servicing-consider Issue for next servicing release review label Feb 18, 2022
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@karelz
Copy link
Member

karelz commented Feb 22, 2022

@carlossanlop it does not need Tactics approval -- see above. Not sure if you want to put servicing-approved on it, or if it is ok this way.

@carlossanlop carlossanlop added the Servicing-approved Approved for servicing release label Feb 22, 2022
@carlossanlop
Copy link
Member

carlossanlop commented Feb 22, 2022

Added the label, per Dan's comment that it does not need approval.
I noticed one CI failure in wasm related to System.Text.Json, so I assume it's unrelated. I'll run it once more just to be 100% safe.
Looks good to merge (after we do the branding).

Question: @ericstj @bartonjs This kind of change is only made in a servicing branch, correct? It's not expected to see such change in main? I ask because this PR was not created as a backport, and I wonder if nuget packages for main get published similarly or differently.

@bartonjs
Copy link
Member

This kind of change is only made in a servicing branch, correct? It's not expected to see such change in main? I ask because this PR was not created as a backport, and I wonder if nuget packages for main get published similarly or differently.

In main all of the libraries that are distributed on NuGet already have <IsPackable>true</IsPackable> (https://github.com/dotnet/runtime/search?q=IsPackable).

Somewhere in the process of setting up servicing branches, all of the IsPackables are set to false (or deleted, or something). During servicing it needs to be manually asserted.

ServicingVersion is always 0 for main builds, so it's never present in csproj files in main. It's just the count of times that a package has been serviced during that major version. (The package version is BranchMajor.BranchMinor.ServicingVersion, so for here, 6.0.1. For main, it's the more complicated BranchMajor.BranchMinor.(ServicingVersion=0)-previewN(-maybe more stuff?).

@ericstj
Copy link
Member

ericstj commented Mar 1, 2022

Somewhere in the process of setting up servicing branches, all of the IsPackables are set to false (or deleted, or something). During servicing it needs to be manually asserted.

IIRC It's statically defined to give different behavior when PreReleaseVersionLabel is servicing. Once that's set we default to not build the package, unless individual projects opt-in.

@safern
Copy link
Member

safern commented Mar 1, 2022

Yup, this is where it's done:

<GeneratePackageOnBuild Condition="('$(BuildAllConfigurations)' == 'true' or
'$(IsRIDSpecificProject)' == 'true') and
'$(PreReleaseVersionLabel)' != 'servicing' and
'$(GitHubRepositoryName)' != 'runtimelab'">true</GeneratePackageOnBuild>
<GeneratePackageOnBuild Condition="'$(GeneratePackageOnBuild)' != 'true'">false</GeneratePackageOnBuild>
<!-- When in source-build we need to generate all packages when building for all configurations even in servicing. -->
<GeneratePackageOnBuild Condition="!$(GeneratePackageOnBuild) and
'$(BuildAllConfigurations)' == 'true' and
'$(DotNetBuildFromSource)' == 'true'">true</GeneratePackageOnBuild>

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants