S.N.Quic private #54610

ManickaP · 2021-06-23T13:20:38Z

Based on mail with @ericstj set up S.N.Quic to be flown to ASP.

~~I still see the S.N.Quic in artifacts/bin/ref/net6.0. How do I actually make it "private", i.e. to not publish ref assembly? @Anipik could you please help?~~
Fixed with @ViktorHofer help. So I guess this is it for runtime.

There's still the ASP.NET part that needs to be solved afterwards.
Contributes to #54534

cc: @wtgodbe @JamesNK @ViktorHofer @wfurt @CarnaViire @geoffkizer

ghost · 2021-06-23T13:20:44Z

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

Issue Details

Based on mail with @ericstj set up S.N.Quic to be flown to ASP.

I still see the S.N.Quic in artifacts/bin/ref/net6.0. How do I actually make it "private", i.e. to not publish ref assembly? @Anipik could you please help?

There's still the ASP.NET part that needs to be solved afterwards.
Contributes to #54534

cc: @wtgodbe @JamesNK @ViktorHofer @wfurt @CarnaViire @geoffkizer

Author:	ManickaP
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

ghost · 2021-06-23T13:20:59Z

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

Issue Details

Based on mail with @ericstj set up S.N.Quic to be flown to ASP.

I still see the S.N.Quic in artifacts/bin/ref/net6.0. How do I actually make it "private", i.e. to not publish ref assembly? @Anipik could you please help?

There's still the ASP.NET part that needs to be solved afterwards.
Contributes to #54534

cc: @wtgodbe @JamesNK @ViktorHofer @wfurt @CarnaViire @geoffkizer

Author:	ManickaP
Assignees:	-
Labels:	`area-System.Net.Quic`
Milestone:	-

ViktorHofer · 2021-06-23T13:41:25Z

How do I actually make it "private", i.e. to not publish ref assembly? @Anipik could you please help?

Set <IsNETCoreAppRef>false</IsNETCoreAppRef> here

ManickaP · 2021-06-23T14:18:18Z

Set <IsNETCoreAppRef>false</IsNETCoreAppRef> here

I just tried that and I got compilations errors:

System.Text.Json -> /home/manicka/repositories/runtime/artifacts/bin/System.Text.Json/net6.0-Debug/System.Text.Json.dll
/usr/share/dotnet/sdk/6.0.100-preview.4.21255.9/Microsoft.Common.CurrentVersion.targets(2202,5): error MSB3245: Could not resolve this reference. Could not locate the assembly "System.Net.Quic". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [/home/manicka/repositories/runtime/src/libraries/System.Net.Http/src/System.Net.Http.csproj]        
  Microsoft.VisualBasic.Core -> /home/manicka/repositories/runtime/artifacts/bin/Microsoft.VisualBasic.Core/net6.0-Debug/Microsoft.VisualBasic.Core.dll
/home/manicka/repositories/runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs(6,18): error CS0234: The type or namespace name 'Quic' does not exist in the namespace 'System.Net' (are you missing an assembly reference?) [/home/manicka/repositories/runtime/src/libraries/System.Net.Http/src/System.Net.Http.csproj]     
/home/manicka/repositories/runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs(7,18): error CS0234: The type or namespace name 'Quic' does not exist in the namespace 'System.Net' (are you missing an assembly reference?) [/home/manicka/repositories/runtime/src/libraries/System.Net.Http/src/System.Net.Http.csproj] 
...

I assume that the ref is now not produced, but that means that even our own libraries cannot see it?

ViktorHofer · 2021-06-23T14:28:51Z

I assume that the ref is now not produced, but that means that even our own libraries cannot see it?

The ref is produced but it now isn't binplaced into the directory that marks the Microsoft.NETCore.App.Ref targeting pack anymore, as expected.

The failure is because now that System.Net.Quic isn't part of the targeting pack anymore, a simple <Reference Include="System.Net.Quic" doesn't work anymore (because named references only look into the targeting pack, as expected). Instead you need to switch all references to it to <ProjectReference> items.

ManickaP · 2021-06-23T14:50:43Z

@ViktorHofer thanks! That worked!

ViktorHofer · 2021-06-23T14:56:55Z

src/libraries/System.Net.Http/src/System.Net.Http.csproj

@@ -629,7 +629,7 @@
    <Reference Include="System.Net.NameResolution" />
    <Reference Include="System.Net.NetworkInformation" />
    <Reference Include="System.Net.Primitives" />
-    <Reference Include="System.Net.Quic" />
+    <ProjectReference Include="..\..\System.Net.Quic\ref\System.Net.Quic.csproj" />


nit: please move this up to the other ProjectReference as we usually sort alphabetically. Also use the $(LibrariesProjectRoot) property instead of ..\..\.

CarnaViire

LGTM

stephentoub · 2021-06-23T16:00:32Z

So I guess this is it for runtime.

This needs to be removed:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs

Line 400 in c0ed319

public QuicImplementationProvider? QuicImplementationProvider

wtgodbe · 2021-06-23T16:25:19Z

src/libraries/System.Net.Quic/Directory.Build.props

@@ -3,6 +3,7 @@
  <PropertyGroup>
    <StrongNameKeyId>Microsoft</StrongNameKeyId>
    <IncludePlatformAttributes>true</IncludePlatformAttributes>
+    <IsAspNetCoreApp>true</IsAspNetCoreApp>


What exactly does this control? We don't want this .dll to be part of our SharedFx/Targeting Pack

Quoting @ericstj from the mail since I do not understand the process fully:

Today this is used to bundle public API which ships in ASP.NET but builds in dotnet/runtime. This allows us to avoid putting those builds in our public packages while still giving them reference assemblies and 6.0 targeted assemblies.

@wtgodbe if putting it into the transport package the same way as the other libraries that form the ref/runtime packs, you will likely need to exclude this one from automatically flowing in your infra.

ManickaP · 2021-06-23T17:18:39Z

This needs to be removed:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs

Line 400 in c0ed319

public QuicImplementationProvider? QuicImplementationProvider

Good catch! The easiest way ATM would be to just make it internal and set it via reflection in our tests here:

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.SocketsHttpHandler.cs

Line 52 in 0416c34

socketsHttpHandler.QuicImplementationProvider = quicImplementationProvider;

I know that @geoffkizer wanted to keep the mock implementation for testing purposes, which uses this settings. So I guess I cannot get rid of it completely. Any thoughts @stephentoub @geoffkizer?

stephentoub · 2021-06-24T04:05:38Z

I know that @geoffkizer wanted to keep the mock implementation for testing purposes

At this point, I'd have hoped the mock implementation would have outlived its usefulness and we could just nuke all of this. But I'll go with whatever Geoff prefers here.

ManickaP · 2021-06-24T15:17:59Z

Failures are unrelated.

S.N.Quic ref for ASP.NET

b557ee4

dotnet-issue-labeler bot added the area-System.Net label Jun 23, 2021

ManickaP added area-System.Net.Quic and removed area-System.Net labels Jun 23, 2021

Made Quic really private and properly consume it.

3be352f

ManickaP requested review from Anipik, wfurt, CarnaViire and geoffkizer June 23, 2021 14:50

ViktorHofer reviewed Jun 23, 2021

View reviewed changes

Feedback

2c4db42

CarnaViire approved these changes Jun 23, 2021

View reviewed changes

wtgodbe reviewed Jun 23, 2021

View reviewed changes

Fixed test build, hidden QuicImplementationProvider

146ea10

Fixed S.N.Quic tests build.

a49a257

karelz assigned ManickaP Jun 24, 2021

ManickaP merged commit 7ed5a67 into dotnet:main Jun 25, 2021

ManickaP deleted the mapichov/54534_private_quic branch June 25, 2021 04:37

JamesNK mentioned this pull request Jun 25, 2021

React to System.Net.Quic being made internal dotnet/aspnetcore#33834

Closed

ManickaP mentioned this pull request Jun 25, 2021

QUIC: Rename S.N.Quic --> S.N.Private.Quic #54720

Closed

karelz mentioned this pull request Jun 30, 2021

System.Net tests failing with System.TimeoutException #54778

Closed

karelz added this to the 6.0.0 milestone Jul 15, 2021

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S.N.Quic private #54610

S.N.Quic private #54610

ManickaP commented Jun 23, 2021 •

edited

Loading

ghost commented Jun 23, 2021

ghost commented Jun 23, 2021

ViktorHofer commented Jun 23, 2021

ManickaP commented Jun 23, 2021

ViktorHofer commented Jun 23, 2021 •

edited

Loading

ManickaP commented Jun 23, 2021

ViktorHofer Jun 23, 2021

ManickaP Jun 23, 2021

CarnaViire left a comment

stephentoub commented Jun 23, 2021

wtgodbe Jun 23, 2021

ManickaP Jun 23, 2021

ViktorHofer Jun 23, 2021

ManickaP commented Jun 23, 2021

stephentoub commented Jun 24, 2021

ManickaP commented Jun 24, 2021

S.N.Quic private #54610

S.N.Quic private #54610

Conversation

ManickaP commented Jun 23, 2021 • edited Loading

ghost commented Jun 23, 2021

ghost commented Jun 23, 2021

ViktorHofer commented Jun 23, 2021

ManickaP commented Jun 23, 2021

ViktorHofer commented Jun 23, 2021 • edited Loading

ManickaP commented Jun 23, 2021

ViktorHofer Jun 23, 2021

Choose a reason for hiding this comment

ManickaP Jun 23, 2021

Choose a reason for hiding this comment

CarnaViire left a comment

Choose a reason for hiding this comment

stephentoub commented Jun 23, 2021

wtgodbe Jun 23, 2021

Choose a reason for hiding this comment

ManickaP Jun 23, 2021

Choose a reason for hiding this comment

ViktorHofer Jun 23, 2021

Choose a reason for hiding this comment

ManickaP commented Jun 23, 2021

stephentoub commented Jun 24, 2021

ManickaP commented Jun 24, 2021

ManickaP commented Jun 23, 2021 •

edited

Loading

ViktorHofer commented Jun 23, 2021 •

edited

Loading