Skip to content

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