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

Use Platform-Guard Assertion Annotations for some known cases #52689

Merged
merged 7 commits into from
May 17, 2021

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented May 13, 2021

As part of #44922 now we have Platform-Guard Assertion Annotation attributes added to libraries and the Platform Compatibility Analyzer now supports them. So its time to use SupportedOSPlatformGuard and UnsupportedOSPlatformGuard whenever needed

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@buyaa-n buyaa-n requested review from eerhardt and MaximLipnin May 13, 2021 05:50
@buyaa-n buyaa-n force-pushed the use-platform-guard branch from 14e5621 to ef24082 Compare May 13, 2021 06:16
@marek-safar
Copy link
Contributor

I think there are a few more cases that could be probably annotated. For example one for consideration is SocketsHttpHandler.IsSupported and there was something in crypto too

@MaximLipnin
Copy link
Contributor

@buyaa-n One question just for my information:

How could I use these guards to annotate the APIs which partly throw PNSE based on some condition, e.g. https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs#L32-L38 ? The methods using this one throw on ios/tvos (and a couple of related System.Net.NetworkInformation.Functional.Tests tests fail).

@buyaa-n
Copy link
Contributor Author

buyaa-n commented May 13, 2021

I think there are a few more cases that could be probably annotated. For example one for consideration is SocketsHttpHandler.IsSupported and there was something in crypto too

Thanks, i remember SocketsHttpHandler.IsSupported i will add that, but i don't think ever heard about crypto one

@buyaa-n
Copy link
Contributor Author

buyaa-n commented May 13, 2021

How could I use these guards to annotate the APIs which partly throw PNSE based on some condition, e.g. https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs#L32-L38 ? The methods using this one throw on ios/tvos (and a couple of related System.Net.NetworkInformation.Functional.Tests tests fail).

Not sure, from the code in the link it doesn't look like it does platform check, seems doing capabilities check

The methods using this one throw on ios/tvos (and a couple of related System.Net.NetworkInformation.Functional.Tests tests fail)

That sound like its need UnsupportedOSPlatform on ios/tvos attributes (not guard attributes)

@buyaa-n buyaa-n force-pushed the use-platform-guard branch from 9209a15 to e2e3218 Compare May 13, 2021 19:05
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Please do not reuse _http3Enabled for the guard. Add a new IsHttp3Supported as @marek-safar suggests.

@buyaa-n buyaa-n force-pushed the use-platform-guard branch from 95d2aec to f4644e0 Compare May 15, 2021 01:07
@buyaa-n buyaa-n force-pushed the use-platform-guard branch from ff13a9d to 8bdea14 Compare May 17, 2021 07:22
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Thanks!

@buyaa-n buyaa-n merged commit 3b8adb7 into dotnet:main May 17, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
@buyaa-n buyaa-n deleted the use-platform-guard branch July 13, 2021 16:35
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.

6 participants