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

msquic.dll is no longer included in the Windows shared runtime #81447

Closed
halter73 opened this issue Jan 31, 2023 · 13 comments · Fixed by #81490
Closed

msquic.dll is no longer included in the Windows shared runtime #81447

halter73 opened this issue Jan 31, 2023 · 13 comments · Fixed by #81490

Comments

@halter73
Copy link
Member

Description

Between build 8.0.0-alpha.1.23061.6 for https://github.com/dotnet/runtime/commits/3f814583fd942d252acd810170f4659d6642377d/ and build 8.0.0-alpha.1.23061.11 for https://github.com/dotnet/runtime/commits/2ca7cf7140ebfaa8e34732b529c194416b122e89/ the official build stopped producing msquic.dll as part of its PackageArtifacts effectively disabling System.Net.Quic on our latest .NET 8 builds. It looks like none of our tests caught this because we skip them if QuicListener.IsSupported or QuicConnection.IsSupported is false which is the case when msquic.dll is not on the path on Windows.

There are only a handful of commits between those two builds. #80164 looks like the most suspicious of those changes @ViktorHofer, but I'm not sure that it's the cause.

I noticed this because @amcasey was trying to add a test to Http3TlsTests in the aspnetcore repo and noticed that the tests were being skipped on Windows 11.

@JamesNK @dotnet/ncl @dotnet/dnceng

Reproduction Steps

Download version 8.0.0-alpha.1.23061.6 of the Microsoft.NETCore.App.Runtime.win-64/x86 nuget packages from https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json and notice that msquic.dll is inside the /runtimes/win-64/native/ directory of the nupkg. Then do the same for version 8.0.0-alpha.1.23061.11 (the next release in the feed) and notice that msuic.dll is no longer there.

Expected behavior

The windows shared runtime should include msquic.dll and QuicListener.IsSupported/QuicConnection.IsSupported should be true on Windows 11.

Actual behavior

The windows shared runtime does not include msquic.dll.

Regression?

Yes.

Known Workarounds

Copying msquic.dll from an older installation of the shared runtime.

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 31, 2023
@ghost
Copy link

ghost commented Jan 31, 2023

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

Issue Details

Description

Between build 8.0.0-alpha.1.23061.6 for https://github.com/dotnet/runtime/commits/3f814583fd942d252acd810170f4659d6642377d/ and build 8.0.0-alpha.1.23061.11 for https://github.com/dotnet/runtime/commits/2ca7cf7140ebfaa8e34732b529c194416b122e89/ the official build stopped producing msquic.dll as part of its PackageArtifacts effectively disabling System.Net.Quic on our latest .NET 8 builds. It looks like none of our tests caught this because we skip them if QuicListener.IsSupported or QuicConnection.IsSupported is false which is the case when msquic.dll is not on the path on Windows.

There are only a handful of commits between those two builds. #80164 looks like the most suspicious of those changes @ViktorHofer, but I'm not sure that it's the cause.

I noticed this because @amcasey was trying to add a test to Http3TlsTests in the aspnetcore repo and noticed that the tests were being skipped on Windows 11.

@JamesNK @dotnet/ncl @dotnet/dnceng

Reproduction Steps

Download version 8.0.0-alpha.1.23061.6 of the Microsoft.NETCore.App.Runtime.win-64/x86 nuget packages from https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json and notice that msquic.dll is inside the /runtimes/win-64/native/ directory of the nupkg. Then do the same for version 8.0.0-alpha.1.23061.11 (the next release in the feed) and notice that msuic.dll is no longer there.

Expected behavior

The windows shared runtime should include msquic.dll and QuicListener.IsSupported/QuicConnection.IsSupported should be true on Windows 11.

Actual behavior

The windows shared runtime does not include msquic.dll.

Regression?

Yes.

Known Workarounds

Copying msquic.dll from an older installation of the shared runtime.

Configuration

No response

Other information

No response

Author: halter73
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@halter73 halter73 changed the title msquic.dll is no longer included in Microsoft.NETCore.App.Runtime.win-64/x86 msquic.dll is no longer included in the Windows shared runtime Jan 31, 2023
@ghost
Copy link

ghost commented Feb 1, 2023

微软又在偷偷干坏事?

Bing translator:

Is Microsoft doing bad things secretly again?

@adityamandaleeka
Copy link
Member

adityamandaleeka commented Feb 1, 2023

Was this change to add the TargetOS condition in src/libraries/System.Net.Quic/src/System.Net.Quic.csproj intentional?

2ca7cf7#diff-1d4ce147edca0ce81f312b96fce76ac52eb02eb6d04e660817e3b47230edc794

cc @ViktorHofer

@samsp-msft
Copy link
Member

@karelz @wfurt

@ManickaP
Copy link
Member

ManickaP commented Feb 1, 2023

Based on this report, we should add test(s) that are not conditioned to IsSupported property but check if it is true on supported OSes.

@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Feb 1, 2023
@ManickaP ManickaP added this to the 8.0.0 milestone Feb 1, 2023
@karelz karelz added the bug label Feb 1, 2023
@karelz
Copy link
Member

karelz commented Feb 1, 2023

This is high-pri blocker for .NET 8.0 Preview 1. We need to fix it ASAP @ManickaP @wfurt cc @ViktorHofer

@karelz
Copy link
Member

karelz commented Feb 1, 2023

@cnmade please post comments in English -- I translated yours.
I don't understand your question. Why would Microsoft try to do bad things? Bugs and mishaps happen. The important thing is how teams react. I don't see any signs of anything malicious here.

@ShreyasJejurkar
Copy link
Contributor

Don't we have tests to validate what the installer splits on the box!? Like the required set of files, their correct location, and version!?

@carlossanlop
Copy link
Member

Between build 8.0.0-alpha.1.23061.6 for https://github.com/dotnet/runtime/commits/3f814583fd942d252acd810170f4659d6642377d/ and build 8.0.0-alpha.1.23061.11 for https://github.com/dotnet/runtime/commits/2ca7cf7140ebfaa8e34732b529c194416b122e89/

Thanks for tracking down the range of commits, @halter73. I am very confident that the culprit was 2ca7cf7#diff-8fa5dbb3a7a00a71e725ad8798c2a6ef22fc3bf770c62651e2e74ca02655aaa8 since it modified many infra keywords, and changes many conditions in the Quic csproj. I'm trying to understand the changes.

none of our tests caught this because we skip them if QuicListener.IsSupported or QuicConnection.IsSupported is false which is the case when msquic.dll is not on the path on Windows.

Anyone knows if it is possible to make this check more resilient? If the DLL is expected, but not found, would it be possible to show a failure message?

@carlossanlop
Copy link
Member

carlossanlop commented Feb 1, 2023

The Quic csproj used to have these conditions:

- <ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'windows' and '$(UseQuicTransportPackage)' == 'true' and '$(DotNetBuildFromSource)' != 'true'">
+ <ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'windows' and
+                        '$(TargetOS)' == 'windows' and
+                        '$(UseQuicTransportPackage)' == 'true' and
+                        '$(DotNetBuildFromSource)' != 'true'">

and

-    <NativeBinPlaceItem Include="$(PkgSystem_Net_MsQuic_Transport)\runtimes\win10-$(TargetArchitecture)\native\*" />
-  </ItemGroup>
-  <ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'windows' and '$(UseQuicTransportPackage)' != 'true' and '$(DotNetBuildFromSource)' != 'true'">
-    <BinPlaceDir Include="$(MicrosoftNetCoreAppRuntimePackNativeDir)" ItemName="NativeBinPlaceItem" />
-    <BinPlaceDir Include="$(NetCoreAppCurrentTestHostSharedFrameworkPath)" ItemName="NativeBinPlaceItem" />
-    <BinPlaceDir Include="$(LibrariesAllBinArtifactsPath)" ItemName="NativeBinPlaceItem" />
-    <BinPlaceDir Include="$(LibrariesNativeArtifactsPath)" ItemName="NativeBinPlaceItem" />
-    <NativeBinPlaceItem Include="$(PkgMicrosoft_Native_Quic_MsQuic_Schannel)\build\native\bin\$(TargetArchitecture)\*" />

+    <NativeBinPlaceItem Include="$(PkgSystem_Net_MsQuic_Transport)\runtimes\win10-$(TargetArchitecture)\native\*"
+                        Condition="'$(UseQuicTransportPackage)' == 'true'" />
+    <NativeBinPlaceItem Include="$(PkgMicrosoft_Native_Quic_MsQuic_Schannel)\build\native\bin\$(TargetArchitecture)\*"
+                        Condition="'$(UseQuicTransportPackage)' != 'true'" />

So the DLL locations are not exactly the same anymore. Talking to @ManickaP and @wfurt about this.

@carlossanlop
Copy link
Member

From my chat with Tomas and Mana, the problem, summarized, was:

the UseQuicTransportPackage should control what sources we use, not the inclusion in general. That is decided by TargetOS

So removing the UseQuickTransportPackage condition from the ItemGroup at the top should fix that. Mana took care of the fix, along with a test, and sent the PRs to main and the preview branch.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 1, 2023
@ViktorHofer
Copy link
Member

Was this change to add the TargetOS condition in src/libraries/System.Net.Quic/src/System.Net.Quic.csproj intentional?

Yes that change was intentional as previously when building libraries for all configurations on non Windows, the msquic.dll was added to the shared framework, even though it shouldn't be there which caused the shared framework pack step to fail.

Big thanks to @ManickaP and @carlossanlop for finding the issue so quickly and fixing it. When we did clean-up the binplacing logic, I mistakenly forgot to change the condition from the ItemGroup and as builds passed, nothing indicated that msquic.dll wasn't binplaced anymore. It's great that we now have a test that asserts the desired behavior. Again sorry for the inconvenience. (I'm still out but wanted to share this message)

@karelz
Copy link
Member

karelz commented Feb 21, 2023

Fixed in 8.0 (main) in PR #81490 and in 8.0 Preview1 in PR #81492.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants