WIP: Adding new SslStream API by CIPop · Pull Request #11265 · dotnet/corefx

CIPop · 2016-08-30T19:39:11Z

@ericstj @weshaggard @davidsh @bartonjs PTAL

Creating a new, simplified PR only for the API addition based on master.

The current error is:

"S:\c1\src\System.Net.Security\ref\System.Net.Security.csproj" (rebuild target) (1) ->
(ValidatePackageTargetFramework target) ->
  S:\c1\Tools\PackageLibs.targets(213,5): error : Expected version 4.1.0.0 for referenced assembly S:\c1\packages\System.Security
.Cryptography.Algorithms\4.2.0\ref\netstandard1.6\System.Security.Cryptography.Algorithms.dll but found 4.2.0.0 [S:\c1\src\System
.Net.Security\ref\System.Net.Security.csproj]

The error is probably because of the switch to netstandard1.6 which includes UWP.
What are the strings (I couldn't find documentation) to make System.Net.Security 4.1.0.0 a net463 contract only (no netstandard) while keeping 4.0.0.0 part of netstandard1.3?

davidsh · 2016-08-30T19:47:16Z

What are the strings (I couldn't find documentation) to make System.Net.Security 4.1.0.0 a net463 contract only (no netstandard) while keeping 4.0.0.0 part of netstandard1.3?

If we not going to use 'netstandard16' (because it brings in UWP which we don't support), then we need to explicitly have both 'net463' (for Desktop) and 'netcoreapp10' (for CoreFx cross-plat).

davidsh · 2016-08-30T19:50:00Z

The folder names under the 'ref' folder for the "older" contract versions should only be major.minor and not the 3 digits you have:

src/System.Net.Security/ref/4.0.0/System.Net.Security.Manual.cs

should be named:

src/System.Net.Security/ref/4.0/System.Net.Security.Manual.cs

CIPop · 2016-08-30T20:00:28Z

@davidsh I took the 4.0.0 example from System.Net.Primitives.

With the following:

diff --git a/src/System.Net.Security/ref/project.json b/src/System.Net.Security/ref/project.json
index dda4cfa..f9ea9e2 100644
--- a/src/System.Net.Security/ref/project.json
+++ b/src/System.Net.Security/ref/project.json
@@ -8,11 +8,11 @@
     "System.Security.Principal": "4.0.0",
     "System.Threading.Tasks": "4.0.0"
   },
-  "frameworks": {
-    "netstandard1.6": {
-      "imports": [
-        "dotnet5.5"
-      ]
+ "frameworks": {
+    "net463": {
+      "dependencies": {
+        "Microsoft.TargetingPack.NETFramework.v4.6.2": "1.0.1"
+      }
     }
   }
 }

I'm getting

"S:\c1\src\System.Net.Security\ref\System.Net.Security.csproj" (rebuild;test target) (1) ->
(ValidatePackageTargetFramework target) ->
  S:\c1\Tools\PackageLibs.targets(213,5): error : Cannot resolve indirect dependency System, Version=4.0.0.0 [S:\c1\src\System.Ne
t.Security\ref\System.Net.Security.csproj]
  S:\c1\Tools\PackageLibs.targets(213,5): error : Could not determine generation for System, 4.0.0.0.  File did not exist and isn
't a known mapping. [S:\c1\src\System.Net.Security\ref\System.Net.Security.csproj]
  S:\c1\Tools\PackageLibs.targets(213,5): error : Cannot resolve indirect dependency System.Core, Version=4.0.0.0 [S:\c1\src\Syst
em.Net.Security\ref\System.Net.Security.csproj]
  S:\c1\Tools\PackageLibs.targets(213,5): error : Could not determine generation for System.Core, 4.0.0.0.  File did not exist an
d isn't a known mapping. [S:\c1\src\System.Net.Security\ref\System.Net.Security.csproj]

    4 Warning(s)
    4 Error(s)

davidsh · 2016-08-30T20:08:29Z

I took the 4.0.0 example from System.Net.Primitives.

The "4.0.0" is ok to use in the project.json. I was referring to the folder name as the subfolder in the 'ref' directory. That is usually only major.minor.

CIPop · 2016-08-30T20:12:58Z

@davidsh see https://github.com/dotnet/corefx/tree/master/src/System.Net.Primitives/ref/4.0.0 . If the format is incorrect, please open a bug.

davidsh · 2016-08-30T20:17:38Z

@davidsh see https://github.com/dotnet/corefx/tree/master/src/System.Net.Primitives/ref/4.0.0 . If the format is incorrect, please open a bug.

I think @ericstj can answer this. System.Net.Primitives was done a while ago before things settled down. The current best practice as I understood it when I did System.Net.Http 4.0 -> 4.1 is to use only 2 digits. I don't think, though, that we are doing PR's everywhere in the codebase to make everything consistent, though.

bartonjs · 2016-08-31T13:09:43Z

-    <PackageTargetFramework Condition="'$(TargetsUnix)' == 'true'">netstandard1.6</PackageTargetFramework>
-    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.3</NuGetTargetMoniker>
-    <NuGetTargetMoniker Condition="'$(TargetsUnix)' == 'true'">.NETStandard,Version=v1.6</NuGetTargetMoniker>
+    <PackageTargetFramework Condition="'$(PackageTargetFramework)' == ''">netstandard1.6</PackageTargetFramework>


After rebasing on master PackageTargetFramework shouldn't be specified anymore

CIPop · 2016-08-31T17:55:50Z

Thanks @bartonjs. Giving that a try.

CIPop · 2016-09-01T01:31:55Z

I'm now getting an apicompat error saying that the ShutdownAsync member is missing from net46 when building pkg.

"S:\c1\src\system.net.security\pkg\System.Net.Security.pkgproj" (rebuild target) (1) ->
"S:\c1\src\system.net.security\src\System.Net.Security.builds" (Build target) (4) ->
"S:\c1\src\system.net.security\src\System.Net.Security.csproj" (Build target) (5:3) ->
(ValidateApiCompatForSrc target) ->
  S:\c1\Tools\ApiCompat.targets(55,5): error : MembersMustExist : Member 'System.Net.Security.SslStream.ShutdownAsync()' does no
t exist in the implementation but it does exist in the contract. [S:\c1\src\system.net.security\src\System.Net.Security.csproj]
  S:\c1\Tools\ApiCompat.targets(69,5): error : ApiCompat failed for 'S:\c1\bin\Windows_NT.AnyCPU.Debug\System.Net.Security\net46
\System.Net.Security.dll' [S:\c1\src\system.net.security\src\System.Net.Security.csproj]

    0 Warning(s)
    2 Error(s)

davidsh · 2016-09-01T01:42:40Z

  },
  "frameworks": {
-    "netstandard1.3": {
+    "netstandard1.6": {


See #11321.

Looks like this should be netstandard1.7 instead of netstandard1.6. (Currently docs are wrong).

Correct you cannot add more apis to netstandard16. Given this API doesn't exist anywhere else you actually should add it to only .NET Core similar to what @ericstj is doing in #11272.

Correct you cannot add more apis to netstandard16. Given this API doesn't exist anywhere else you actually should add it to only .NET Core similar to what @ericstj is doing in #11272.

@weshaggard We just found out that the docs are wrong and that we should have listed it as netstandard1.7. Since that maps to .NET Framework 4.6.3 (vNext) and we plan to add this API there as well.

Currently all the netstandard17 work is happening in the dev/api branch and is only a placeholder for now. We are going to soon be shifting to exposing APIs in .NET Core specifically first and then later adding them to netstandard. This enables people to add APIs just for .NET Core much more agile and not be blocked by our restrictions on .NET Standard.

davidsh · 2016-09-01T01:44:40Z

I'm now getting an apicompat error saying that the ShutdownAsync member is missing from net46 when building pkg.

There shouldn't be 'net46' build with the new API (since it doesn't exist in .NET 4.6) There should only be a 'net463' build with the new API.

weshaggard · 2016-09-01T01:59:40Z

@@ -0,0 +1,162 @@
+// Licensed to the .NET Foundation under one or more agreements.


You don't need this ref/4.0.0 folder. We will get this asset from the last stable package.

@weshaggard Is that a new procedure? Because historically we always create the ref/"old contract" folders. See System.Net.Http library.

Yes that is fairly recently @ericstj did some tooling so we don't need to keep these older builds actually compiling in the repo and instead just grab them from our last stable package.

CIPop · 2016-09-02T01:06:59Z

Thanks @weshaggard . I can now build ref, src and pkg but the test/FunctionalTests folder doesn't build. I presume that I need to somehow change the test project.json/csproj to target netcoreapp1.1 but I'm not sure how.

I've tried different test/FunctionalTests monikers within the System.Net.Security.Tests.csproj without success:

    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETCoreApp,Version=v1.1</NuGetTargetMoniker>
    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.6</NuGetTargetMoniker>

The error is:

GetPkgProjPackageDependencies:
Skipping target "GetPkgProjPackageDependencies" because it has no inputs.
GetPkgProjPackageDependencies:
Skipping target "GetPkgProjPackageDependencies" because it has no inputs.
Done Building Project "S:\c1\src\system.net.security\pkg\System.Net.Security.pkgproj" (GetPackageAssets target(s)).

S:\c1\Tools\PackageLibs.targets(34,5): error : Could not locate compile assets for any of the frameworks .NETCoreApp,Version=v1.
1 [S:\c1\src\system.net.security\tests\FunctionalTests\System.Net.Security.Tests.csproj]
  Resolved runtime assets from .NETCoreApp,Version=v1.0: S:\c1\bin\Windows_NT.AnyCPU.Debug\System.Net.Security\System.Net.Securi
  ty.dll;S:\c1\bin\Windows_NT.AnyCPU.Debug\System.Net.Security\System.Net.Security.pdb
Done Building Project "S:\c1\src\system.net.security\tests\FunctionalTests\System.Net.Security.Tests.csproj" (rebuild target(s))
 -- FAILED.


Build FAILED.

"S:\c1\src\system.net.security\tests\FunctionalTests\System.Net.Security.Tests.csproj" (rebuild target) (1) ->
(ResolvePkgProjReferences target) ->
  S:\c1\Tools\PackageLibs.targets(34,5): error : Could not locate compile assets for any of the frameworks .NETCoreApp,Version=v
1.1 [S:\c1\src\system.net.security\tests\FunctionalTests\System.Net.Security.Tests.csproj]

    0 Warning(s)
    1 Error(s)

weshaggard · 2016-09-02T02:44:34Z

    <ProjectGuid>{A55A2B9A-830F-4330-A0E7-02A9FB30ABD2}</ProjectGuid>
    <OutputType>Library</OutputType>
-    <NugetTargetMoniker>.NETStandard,Version=v1.3</NugetTargetMoniker>
+    <NuGetTargetMoniker>.NETCoreApp,Version=v1.1</NuGetTargetMoniker>


If this is set to netcoreapp1.1 you need to have the matching section in your project.json file.

CIPop · 2016-10-06T22:35:06Z

This is being merged into #11489.

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

CIPop added 3 - Ready For Review area-System.Net and removed 3 - Ready For Review labels Aug 30, 2016

CIPop self-assigned this Aug 30, 2016

bartonjs reviewed Aug 31, 2016
View reviewed changes

CIPop force-pushed the alert_api_master branch from f6dba08 to 2f45b69 Compare August 31, 2016 17:53

davidsh reviewed Sep 1, 2016
View reviewed changes

weshaggard reviewed Sep 1, 2016
View reviewed changes

Cristian Pop added 5 commits September 1, 2016 18:00

Adding new API.

a381f7c

Fixing contract.

2712fc8

Switching to netcoreapp1.1. Removing 4.0.0 folder.

5336bfa

Changing test project.json

9aa6ed5

Removing net46 added during rebase.

3594a3d

CIPop force-pushed the alert_api_master branch from cab6a01 to 3594a3d Compare September 2, 2016 01:07

weshaggard reviewed Sep 2, 2016
View reviewed changes

CIPop mentioned this pull request Sep 7, 2016

Adding TLS Alerts #11489

Merged

CIPop closed this Oct 6, 2016

CIPop deleted the alert_api_master branch October 17, 2016 23:16

karelz modified the milestone: 1.2.0 Dec 3, 2016

CIPop mentioned this pull request Jan 31, 2020

System.Net.SslStream new APIs for TLS Alert support dotnet/runtime#18785

Closed

		@@ -0,0 +1,162 @@
		// Licensed to the .NET Foundation under one or more agreements.

Conversation

CIPop commented Aug 30, 2016

Uh oh!

davidsh commented Aug 30, 2016

Uh oh!

davidsh commented Aug 30, 2016

Uh oh!

CIPop commented Aug 30, 2016

Uh oh!

davidsh commented Aug 30, 2016

Uh oh!

CIPop commented Aug 30, 2016

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CIPop commented Aug 31, 2016

Uh oh!

CIPop commented Sep 1, 2016

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!

davidsh commented Sep 1, 2016

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!

CIPop commented Sep 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CIPop commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davidsh commented Aug 30, 2016 •

edited

Loading