-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Don't throw PNSE on QuicImplementationProvider.IsSupported #67036
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis PR adds static Fixes #65802.
|
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is not to put IsSupported
on connection but to replace generated PNSE on "other" platforms with false returning, already existing property on MsQuicApi.
This comment from the issue has info about how to exclude the auto-generated PNSE: #65802 (comment) |
Should be better now. I also tested it by manually copying the "unsupported" |
CI Failures are unrelated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
</PropertyGroup> | ||
<!-- Source files --> | ||
<ItemGroup Condition="'$(TargetPlatformIdentifier)' != ''"> | ||
<Compile Include="System\Net\Quic\*.cs" /> | ||
<Compile Include="System\Net\Quic\NetEventSource.Quic.cs" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use something like this:
<Compile Include="System\Net\Quic\*.cs" Exclude="QuicImplementationProviders.Unsupported.cs">
Instead of naming out all the files.
Up to you, I don't mind either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should enumerate all files here to be consistent with other project files. I kept the wildcards in the rest of the file since that part can still change quite often.
{ | ||
public abstract partial class QuicImplementationProvider | ||
{ | ||
// alternative constructor because currently it is not possible to exlude ctors from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue for this? If so, we should link it here.
{ | ||
// alternative constructor because currently it is not possible to exlude ctors from | ||
// PNSE autogeneration | ||
internal QuicImplementationProvider(bool dummy) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use _
as a discard. No preference on my side, up to you.
@@ -285,6 +285,7 @@ public async Task Dispose_WithOpenLocalStream_LocalStreamFailsWithQuicOperationA | |||
} | |||
} | |||
|
|||
[ConditionalClass(typeof(QuicTestBase<MockProviderFactory>), nameof(QuicTestBase<MockProviderFactory>.IsSupported))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand it, we build the S.N.Quic tests only for Linux and Windowsm so they are never run on any of the platforms where we throw PNSE from everything. Thus, the condition was never needed.
But I do agree it doesn't hurt here, and at least it won't break if/when we expand the target OS list.
Also you can look with ILSpy, ildasm, etc. into the content of S.N.Quic.dll for other platforms to check it does have |
there is a single dll for all unsupported platforms, it looks ok |
This PR adds static
QuicConnection.IsSupported
member which can be used to check if the platform is capable of QUIC. The implementation is routed toMsQuicApi.IsQuicSupported
.Fixes #65802.