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

Move Msquic over to Source-generated delegate interop. #65204

Merged
merged 2 commits into from
Feb 13, 2022

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Feb 11, 2022

I missed this when turning on DisableRuntimeMarshalling because we don't run any Quic tests in our regular CI that actually use MsQuic and we don't have the DisableRuntimeMarshalling analyzer yet.

cc: @dotnet/interop-contrib

@ghost
Copy link

ghost commented Feb 11, 2022

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

Issue Details

I missed this when turning on DisableRuntimeMarshalling because we don't run any Quic tests in our regular CI that actually use MsQuic and we don't have the DisableRuntimeMarshalling analyzer yet.

Author: jkoritzinsky
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@ghost ghost assigned jkoritzinsky Feb 11, 2022
@kasperk81
Copy link
Contributor

why there is so much more code? shouldn't "generator" have the opposite effect?

@jkoritzinsky
Copy link
Member Author

This code was generated from a generator that runs in a different mode and adapted to this use case. A source generator to capture this scenario is tracked in #63590. When that feature is implemented, we will move this code back to being source-generated.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Do we have a test to run or some other form of confirmation that the related issues in ASP.NET are fixed with this?

@jkoritzinsky
Copy link
Member Author

@BrennanConroy is there any validation we can do to make sure I caught all the cases that are causing problems?

@BrennanConroy
Copy link
Member

BrennanConroy commented Feb 11, 2022

All we're doing in ASP.NET Core is calling QuicImplementationProviders.Default.IsSupported, so if you can run that successfully (maybe check it failed before) then it should be good.

Edit: No guarantee that the actual usage of MSQuic has issues after this change of course, but that'll be verified once we consume the fix.

@jkoritzinsky
Copy link
Member Author

It looks like there might be some more non-blittable marshalling places in MsQuic. Marking this as no-merge for now.

@jkoritzinsky jkoritzinsky added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 11, 2022
@wfurt
Copy link
Member

wfurt commented Feb 11, 2022

just a note that msquic now produce low-level generated binding for c#. There is desire to abandon ours and use theirs in 7.0 time frame.

@jkoritzinsky jkoritzinsky removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 12, 2022
@jkoritzinsky
Copy link
Member Author

I've fixed the last few cases that were causing issues. Once this is green, it's ready to merge.

@kasperk81
Copy link
Contributor

just a note that msquic now produce low-level generated binding for c#. There is desire to abandon ours and use theirs in 7.0 time frame.

if eventually it will be replaced, what value this temporary excessive glue would bring (other than burdening the git history)?

@jkoritzinsky
Copy link
Member Author

The library is currently broken without this change, so it is necessary to enable ASP.NET to consume newer dotnet/runtime builds

@jkoritzinsky jkoritzinsky merged commit 6e05d78 into dotnet:main Feb 13, 2022
@jkoritzinsky jkoritzinsky deleted the msquic-srcgen-interop branch February 13, 2022 16:40
@ghost ghost locked as resolved and limited conversation to collaborators Mar 15, 2022
@karelz karelz added this to the 7.0.0 milestone Apr 8, 2022
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.

7 participants