Skip to content

WinHttpHandler: apply [SupportedOSPlatform("windows10.0.19041")] on TcpKeepAlive properties - #45494

Merged
antonfirsov merged 2 commits into
dotnet:masterfrom
antonfirsov:winhttp/SupportedOSPlatform
Dec 8, 2020
Merged

WinHttpHandler: apply [SupportedOSPlatform("windows10.0.19041")] on TcpKeepAlive properties#45494
antonfirsov merged 2 commits into
dotnet:masterfrom
antonfirsov:winhttp/SupportedOSPlatform

Conversation

@antonfirsov

Copy link
Copy Markdown
Contributor

WINHTTP_OPTION_TCP_KEEPALIVE is only available since the May 2020 Update, so it's reasonable to mark the corresponding managed API-s with [SupportedOSPlatform("windows10.0.2004")].

As discussed, I'm applying the internal variant of SupportedOSPlatformAttribute in all build targets for simplicity. Note that with this solution the attribute does not contribute to the public API as it was originally suggested in the proposal #44025.

This change finishes and resolves #44025.

/cc @scalablecory @ericstj

@ghost

ghost commented Dec 2, 2020

Copy link
Copy Markdown

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

Issue Details

WINHTTP_OPTION_TCP_KEEPALIVE is only available since the May 2020 Update, so it's reasonable to mark the corresponding managed API-s with [SupportedOSPlatform("windows10.0.2004")].

As discussed, I'm applying the internal variant of SupportedOSPlatformAttribute in all build targets for simplicity. Note that with this solution the attribute does not contribute to the public API as it was originally suggested in the proposal #44025.

This change finishes and resolves #44025.

/cc @scalablecory @ericstj

Author: antonfirsov
Assignees: -
Labels:

area-System.Net

Milestone: -

public System.Func<System.Net.Http.HttpRequestMessage, System.Security.Cryptography.X509Certificates.X509Certificate2, System.Security.Cryptography.X509Certificates.X509Chain, System.Net.Security.SslPolicyErrors, bool>? ServerCertificateValidationCallback { get { throw null; } set { } }
public System.Net.ICredentials? ServerCredentials { get { throw null; } set { } }
public System.Security.Authentication.SslProtocols SslProtocols { get { throw null; } set { } }
[System.Runtime.Versioning.SupportedOSPlatformAttribute("windows10.0.2004")]

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.

Should not it be 10.0.19041 (ie. the real version number, not the marketing one)?

@antonfirsov antonfirsov Dec 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

AFAIK the $"{year:00}{month:00}" thing is the Version, 19041 is the Build. See also the windows samples for SupportedOSPlatform: https://github.com/dotnet/designs/blob/main/accepted/2020/platform-exclusion/platform-exclusion.md#attribute

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.

@antonfirsov antonfirsov Dec 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seems like a valid concern, although nothing we can do at this point I guess. @terrajobst any comments on this?

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.

The official documentation also uses the build numbers (https://docs.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer)

@antonfirsov antonfirsov Dec 4, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm pretty sure now we should use Build here, and there is a bug in the design doc. Opened dotnet/designs#170 to get it clarified. Thanks a lot for spotting this!

@ghost

ghost commented Dec 3, 2020

Copy link
Copy Markdown

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

Issue Details

WINHTTP_OPTION_TCP_KEEPALIVE is only available since the May 2020 Update, so it's reasonable to mark the corresponding managed API-s with [SupportedOSPlatform("windows10.0.2004")].

As discussed, I'm applying the internal variant of SupportedOSPlatformAttribute in all build targets for simplicity. Note that with this solution the attribute does not contribute to the public API as it was originally suggested in the proposal #44025.

This change finishes and resolves #44025.

/cc @scalablecory @ericstj

Author: antonfirsov
Assignees: -
Labels:

area-System.Net, area-System.Net.Http

Milestone: -

@antonfirsov
antonfirsov requested a review from a team December 3, 2020 17:52

@alnikola alnikola left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@scalablecory

Copy link
Copy Markdown
Contributor

Didn't we plan to define a SupportedOSPlatform attribute inside this, as we don't have a .NET 5 build?

@ericstj

ericstj commented Dec 3, 2020

Copy link
Copy Markdown
Member

Didn't we plan to define a SupportedOSPlatform attribute inside this, as we don't have a .NET 5 build?

That should be happening automatically:

<!-- Adds System.Runtime.Versioning*Platform* annotation attributes to < .NET 5 builds -->
<Choose>
<When Condition="('$(IncludePlatformAttributes)' == 'true' or '$(IsWindowsSpecific)' == 'true') and ('$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '5.0')))">
<ItemGroup>
<Compile Include="$(CoreLibSharedDir)System\Runtime\Versioning\PlatformAttributes.cs" Link="System\Runtime\Versioning\PlatformAttributes.cs" />
</ItemGroup>
</When>
</Choose>

@antonfirsov antonfirsov changed the title WinHttpHandler: apply [SupportedOSPlatform("windows10.0.2004")] on TcpKeepAlive properties WinHttpHandler: apply [SupportedOSPlatform("windows10.0.19041")] on TcpKeepAlive properties Dec 7, 2020
@antonfirsov

Copy link
Copy Markdown
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov

Copy link
Copy Markdown
Contributor Author

Test failures are unrelated: #44352 #41606 #41531 #43917 #40798 #45773 #45774

@antonfirsov

antonfirsov commented Dec 8, 2020

Copy link
Copy Markdown
Contributor Author

According to the feedback around #45494 (comment), the change of platformName should be fine, so I'm merging.

@antonfirsov
antonfirsov merged commit 18d1527 into dotnet:master Dec 8, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 7, 2021
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API Proposal: WinHttpHandler.TcpKeepAlive

7 participants