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

more fixes for msquic packaging #55607

Merged
merged 2 commits into from
Jul 15, 2021
Merged

more fixes for msquic packaging #55607

merged 2 commits into from
Jul 15, 2021

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 13, 2021

It seems like previous change was good enough get it to runtime and run tests but it was not good enough to make it the final product.
With this, I verified that msquic.dll is part of created dotnet-runtime-6.0.0-dev-win-x64.zip and I also run the msi installer and I check install shared directory.

We also crate aspnet transport package and I'm not sure if the native dll needs to be there since it is now part of the runtime. We can possibly re-visit this later if need to.

@wfurt wfurt self-assigned this Jul 13, 2021
@ghost
Copy link

ghost commented Jul 13, 2021

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

Issue Details

It seems like previous change was good enough get it to runtime and run tests but it was not good enough to make it the final product.
With this, I verified that msquic.dll is part of created dotnet-runtime-6.0.0-dev-win-x64.zip and I also run the msi installer and I check install shared directory.

We also crate aspnet transport package and I'm not sure if the native dll needs to be there since it is now part of the runtime. We can possibly re-visit this later if need to.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Quic

Milestone: -

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM

@ericstj
Copy link
Member

ericstj commented Jul 14, 2021

We also crate aspnet transport package and I'm not sure if the native dll needs to be there since it is now part of the runtime. We can possibly re-visit this later if need to.

It doesn’t need to be there. You don’t need ASP.NET to know about this file in any way right? It’s an implementation detail of the assembly in the runtime.

@wfurt
Copy link
Member Author

wfurt commented Jul 14, 2021

We also crate aspnet transport package and I'm not sure if the native dll needs to be there since it is now part of the runtime. We can possibly re-visit this later if need to.

It doesn’t need to be there. You don’t need ASP.NET to know about this file in any way right? It’s an implementation detail of the assembly in the runtime.

Currently the package has ref assembly but also System.Net.Quic.dll itself. I don't know how somebody would consume it without the native bits. But that may be just artifact of use trying to pass in the refs.

@ericstj
Copy link
Member

ericstj commented Jul 14, 2021

Currently the package has ref assembly but also System.Net.Quic.dll itself.

That’s not right but they don’t consume it so it’s not a major issue, just cleanup.

@ManickaP
Copy link
Member

Is this closing #52085 once again? Should we adjust the description then?

@jkoritzinsky
Copy link
Member

I think we might have a problem with single-file host + msquic. Since msquic.dll isn't linked into the single file host statically, it needs to be included in the SingleFileHostIncludeFilename item group. However, that would mean that msquic.dll is outputted alongside the single file host for everyone who uses single file, which is a subpar experience since QUIC support is in preview.

@ericstj
Copy link
Member

ericstj commented Jul 14, 2021

@jkoritzinsky what happens for System.IO.Compression.Native?

@jkoritzinsky
Copy link
Member

System.IO.Compression.Native is statically linked into the single file host, so it doesn't need to ship alongside it.

In the CMake build script:

System.IO.Compression.Native-Static

@ericstj
Copy link
Member

ericstj commented Jul 14, 2021

Interesting. Perhaps that's a separate issue for the networking team to peel off and address since this shouldn't make it any worse, right? Today msquic fails to load for all apps, after this change it will fail to load for single-file apps.

Some options to consider are:

  1. Provide an msquic static lib and link that to the host.
  2. Treat msquic as a SingleFileHostIncludeFilename and always make self contained apps bundle it.
  3. Create some way (using build functionality, linker, etc) for it to be optionally included either by opt-in or detection of API usage.

@ManickaP
Copy link
Member

Create some way (using build functionality, linker, etc) for it to be optionally included either by opt-in or detection of API usage.

We have that. If the msquic.dll is not present on the target machine, we just disable QUIC and by proxy HTTP/3.

@jkoritzinsky
Copy link
Member

Yes, this can be future work. I just wanted to raise it before I forgot.

@ericstj
Copy link
Member

ericstj commented Jul 14, 2021

We have that. If the msquic.dll is not present on the target machine, we just disable QUIC and by proxy HTTP/3.

I was talking about the reverse, where the build could decide when to include the dll in the customer's single file app. If there was a way for the customer (or the customer's app/project) to express "I want my single file app to have QUIC" rather than us say in our framework metadata that it must always be present.

@ManickaP ManickaP mentioned this pull request Jul 14, 2021
12 tasks
@ManickaP
Copy link
Member

ManickaP commented Jul 14, 2021

We have an AppContext switch: System.Net.SocketsHttpHandler.Http3Support would that be enough?

This PR: #55332

@wfurt
Copy link
Member Author

wfurt commented Jul 14, 2021

I'm glad you brought it up @jkoritzinsky and I think this should be future work. Quic/H3 is preview feature for 6.0 so I'm not sure if we should pollute the single file with something that is unlikely to be used. As @ericstj mentioned it would be great if this is something the project can control.
Also this is specific to Windows. For Linux/macOS we expect user to provide msquic as system dependency.

@ManickaP
Copy link
Member

Yes, this can be future work. I just wanted to raise it before I forgot.

Tracking in #55639

@wfurt
Copy link
Member Author

wfurt commented Jul 15, 2021

Mono build failure looks irrelevant.

@wfurt wfurt merged commit 7aaffcb into dotnet:main Jul 15, 2021
@wfurt wfurt deleted the packaging_52085 branch July 15, 2021 15:14
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