Move System.Net.Sockets to netstandard 1.7 by ericeil · Pull Request #12249 · dotnet/corefx

ericeil · 2016-09-30T19:26:17Z

Moves System.Net.Sockets to netstandard 1.7. Thanks to @ericstj for the help with this!

Fixes #11812.

cc: @stephentoub, @CIPop, @davidsh, @karelz, @weshaggard

ericeil · 2016-09-30T20:59:30Z

@dotnet-bot test Innerloop CentOS7.1 Debug Build and Test please Another CentOS segfault

ericeil · 2016-09-30T21:00:48Z

@dotnet-bot Test Outerloop Windows_NT Debug please
@dotnet-bot Test Outerloop Ubuntu14.04 Debug please
@dotnet-bot Test Outerloop OSX Debug please

davidsh · 2016-10-01T01:36:51Z

+  <ItemGroup Condition="'$(TargetGroup)'=='netcore50'">
+    <Compile Include="$(CommonPath)\Microsoft\Win32\SafeHandles\SafeHandleZeroOrMinusOneIsInvalid.cs">
+      <Link>Common\Microsoft\Win32\SafeHandles\SafeHandleZeroOrMinusOneIsInvalid.cs</Link>
+    </Compile>


Should we even this Common code anymore? I thought SafeHandleZeroOrMinusOneIsInvalid and friends was being added back to the public API surface (i.e. part of .NET Standard 2.0). So, shouldn't these references just be coming from Microsoft.Win32.SafeHandles contract via a project.json reference?

They have already been added back, but netcore50 doesn't support netstandard1.7(aka 2.0), so the common source is still needed for that build.

When will netcore50 support .NET Standard 2.0? Or perhaps the real question is when will the "UWP" version of this support these APIs? Is there going to be a 'netcore51' for example? I did see uap10.1 vs. uap10.0 so I was wondering if there was a net platform definition for a revised version for UWP, similar to what is happening for 'netcoreapp10' vs. 'netcoreapp11'.

netcore50 will never support .NET Standard 2.0. We do not plan to version the netcore tfm for this as netcore50 has special meaning to various systems and so it is essentially a dead end. However UWP with uap10.1 tfm is currently the target to support .NET Standard 2.0.

So, are we changing our source code where we use "if netcore50". And we use that term in our folder names and partial class names. The documentation needs to be updated if we don't use "netcore50" TFM anymore. We have separate implementations for some of the networking libraries, so need good guidelines to keep this organized and building correctly.

davidsh · 2016-10-01T01:37:32Z

        //     An IASyncResult, representing the read.
+#if !netcore50
+        override
+#endif


Why are we doing this special if block for !netcore50?

When we build for netcore50, we're building against the older contract where these don't exist on Stream, so there's nothing to override.

Instead of doing this #ifdef'ing can we just remove the netcore50 build now? If you remove the build configuration we will essentially pick-up and reship the version for the last stable package.

We we do go that route and remove the netcore50 configuration from the live build then we will have the accept that we aren't going to be making in bug fixes in that configuration any longer and if we do need to fix something for it we will have to do it from the release/servicing branch or add back the configuration at that time.

For that reason I will leave it up to you guys to decide whether or not to remove the live build or not.

Do you think that's wise given it'll just need to come back for UAP10.1?

Scratch that, I see what you're saying. This won't need to come back for UAP10.1 since UAP10.1 will have this version of stream. I had recommended that @ericeil keep the netcore50 config around and building so that it would be eaiser to add the UAP10.1 config when that work is done, however here it's making the current work harder. Another option would be to bring up a UAP10.1 config now, but I suspect that'd require some more hacks.

I don't understand why we would drop 'netcore50' suport. I don't really care what moniker we call it. But we need to have UWP working at all times. Will I still be able to build a UWP app and use System.Net.Sockets (or any other networking library). And when/how will UWP be updated in order to use the newly added contract API surface. I've heard it is when "uap10.1" gets working. But I don't understand exactly when that is and how it works.

I've logged issue #12331 to track this potential extra work. I don't think we should hold this PR for that decision.

@davidsh removing the live netcore50 build doesn't break UWP. It just implies that we are going to pull the netcore50 version of the library from the last stable shipped package and reship that in the current package. It just allows us to eliminate a bunch of the extra live build configurations.

@weshaggard Ok. So, then when can be start to have UWP working with this expanded API set? Is this "uap10.1"? If so, does that live build work? Or is there work to do in this library to make that happen. I need to get this working in UWP for the expanded (a.k.a netstandard17.....a.k.a NET Standard 2.0).

Yes most of the .NET Standard 2.0 work is targeting "uap10.1", however that ties to a new tool chain that is also under development in TFS.

For some things we could add them to the netcore50 version of the library assuming it isn't part of the SharedLibrary and doesn't require any new dependent APIs that are in contracts that are part of the SharedLibrary. My expectation is that most of the .NET Standard 2.0 will require newer versions of things like System.Runtime which means we won't really be able to support them in netcore50 and we will need to use uap10.1 instead.

weshaggard · 2016-10-01T03:22:43Z

+    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'net463'">true</IsPartialFacadeAssembly>
+    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.7</NuGetTargetMoniker>
+    <AssemblyVersion Condition="'$(TargetGroup)' == 'netcore50'">4.1.1.0</AssemblyVersion>
+    <DefineConstants Condition="'$(TargetGroup)'=='netcore50'">$(DefineConstants);netcore50</DefineConstants>


nit: can you make the spacing around the == consistent in these conditions.

justinvp · 2016-10-02T15:58:02Z

        [OuterLoop] // TODO: Issue #11345
        [Fact]
-        [PlatformSpecific(PlatformID.Windows)]
+        [PlatformSpecific(Xunit.PlatformID.Windows)]


Now that #12284 has been merged, fully qualifying the enum is no longer necessary as it has been renamed PlatformID -> TestPlatforms. Sorry for the merge conflicts.

Resolved the merge conflicts. :)

ericeil · 2016-10-03T20:09:24Z

@dotnet-bot test Innerloop CentOS7.1 Debug Build and Test please
@dotnet-bot test Innerloop CentOS7.1 Release Build and Test please
(previous build hit dotnet/dotnet-ci#307)

ericeil · 2016-10-03T20:09:59Z

@dotnet-bot Test Outerloop Windows_NT Debug please
@dotnet-bot Test Outerloop Ubuntu14.04 Debug please
@dotnet-bot Test Outerloop OSX Debug please

ericeil · 2016-10-03T21:14:22Z

Ubuntu outerloop leg failed due to #5660. Not caused by this change.

* Make Sockets target ns1.7, maintaining old configs * Remove netstandard1.3 implementations. * Fix NetworkStream overrides * Fix serialization attributes * Move tests to netstandard1.7 Commit migrated from dotnet/corefx@3b71aec

ericeil added area-System.Net netstandard2.0 labels Sep 30, 2016

dnfclas added the cla-already-signed label Sep 30, 2016

ericeil force-pushed the socket_17_2 branch from bd06a7d to d069a35 Compare September 30, 2016 19:33

davidsh reviewed Oct 1, 2016

View reviewed changes

weshaggard reviewed Oct 1, 2016

View reviewed changes

justinvp reviewed Oct 2, 2016

View reviewed changes

justinvp mentioned this pull request Oct 2, 2016

Remove Xunit.PlatformID enum dotnet/buildtools#1084

Merged

ericeil force-pushed the socket_17_2 branch from d069a35 to 39e652a Compare October 3, 2016 16:12

ericstj and others added 10 commits October 3, 2016 10:35

Make Sockets target ns1.7, maintaining old configs

2f92999

Remove netstandard1.3 implementations.

13ccf5a

Upgrade refs in project.json

709d6cb

Fix NetworkStream overrides

8a3027d

Fix serialization attributes

096e2b4

Move tests to netstandard1.7

52b0129

Update versions in project.json

b5dcfd3

Fix project.json refs

5334302

Make considition formatting consistent

d991f1f

Update projects to account for recent changes elsewhere

943ab9f

ericeil force-pushed the socket_17_2 branch from 39e652a to 943ab9f Compare October 3, 2016 17:36

ericeil merged commit 3b71aec into dotnet:master Oct 3, 2016

karelz modified the milestone: 1.2.0 Dec 3, 2016

ericeil mentioned this pull request Jan 31, 2020

Consider removing netcore50 from the list of TFMs actively targeted by the System.Net.Sockets build dotnet/runtime#18843

Closed

Conversation

ericeil commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericeil commented Sep 30, 2016

Uh oh!

ericeil commented Sep 30, 2016

Uh oh!

davidsh Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericeil commented Oct 3, 2016

Uh oh!

ericeil commented Oct 3, 2016

Uh oh!

ericeil commented Oct 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ericeil commented Sep 30, 2016 •

edited

Loading

davidsh Oct 1, 2016 •

edited

Loading

weshaggard Oct 3, 2016 •

edited

Loading