Add System.SystemException derivations by sepidehkh · Pull Request #12351 · dotnet/corefx

sepidehkh · 2016-10-04T20:21:05Z

Addresses issue https://github.com/dotnet/corefx/issues/12198

Since SystemException is only supported in netstandard1.7, I upgraded those contracts being touched here to netstandard1.7 as well.

cc: @danmosemsft @weshaggard @ericstj @joperezr

sepidehkh · 2016-10-04T20:24:53Z

    </Compile>
  </ItemGroup>
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <Target Name="RemoveProductionReference" BeforeTargets="CoreCompile">


This is a hack to remove System.Net.Security.dll dependency brought over all the way from NetStandardLibrary.
Here's the problem: This test project is doing unit tests by compiling some of the actual source files from System.Net.Security and the assumption is there will be no reference to System.Net.Security contract. However, in netstandard1.7, NetStandardLibrary causes a dependency to that. The hack enables us to work around the conflict errors at compile time.

This should be explained in a comment here

In reply to: 81845354 [](ancestors = 81845354)

/cc @davidsh

@sepidehms, @weshaggard Most System.Net.* contracts have UnitTests. Is this something that we have to do for all of them?
Is there any other workaround that we could do? Within the UTs, we usually mock enough of the implementation to get the unit under test to compile. That means that we should have virtually no external dependency except everything that XUnit needs to run.

sepidehkh · 2016-10-04T20:28:21Z

        {
-            _serverCertificateCollection = Configuration.Certificates.GetServerCertificateCollection();
-            _serverCertificate = Configuration.Certificates.GetServerCertificate();
+            _serverCertificateCollection = System.Net.Test.Common.Configuration.Certificates.GetServerCertificateCollection();


This is definitely not nice, but for some reason after upgrading this test project to netstandard1.7, this usage of Configuration is not recognised by the compiler and using System.Net.Test.Common didn't seem to suffice. I'm not sure if we care about this being there in a test project. Any suggestions/ideas for not having to do this?

Did you try building outside VS? I don't really understand why this would fail. In any case, if the using doesn't work, probably a better thing would be to add an alias at the top so that you don't have to type the fully qualified name everytime.

In reply to: 81846153 [](ancestors = 81846153)

+1 on the alias idea.
Also if something worked before but doesn't now, it should be investigated. Please add a TODO/issue.

justinvp · 2016-10-04T22:01:52Z

 namespace System.Net.Security.Tests
 {
-    [PlatformSpecific(TestPlatforms.Windows)] // NegotiateStream only supports client-side functionality on Unix
+    [PlatformSpecific(Xunit.PlatformID.Windows)] // NegotiateStream only supports client-side functionality on Unix


Why are these changing back to Xunit.PlatformID? We renamed PlatformID to TestPlatforms and PlatformID is being deleted. See #12284.

@justinvp I made this change because for some reason after upgrading the project to netstandard1.7 TestPlatformswas not recognized at compile time. However, I tried it again now and looks like after rebasing my changes on top of latest commits and after updating the dependencies, that error is gone. I will change this back to TestPlatforms. Sorry for that, I didn't know PlatformID is going to be gone.

joperezr · 2016-10-04T23:23:04Z

        "4.0.0.0": "4.3.0",
-        "4.0.2.0": "4.3.0"
+        "4.0.2.0": "4.3.0",
+        "4.0.1.0": "4.3.0",


This looks wierd to me, why do we have 4.0.2 and 4.0.1 pointing to the same package?

joperezr · 2016-10-05T17:09:26Z

        {
            Debug.Assert(protocols != SslProtocols.None, "All protocols are disabled");
-
+#pragma warning disable 0618


is 618 for obsolete? if so perhaps just add a small comment to note why are we adding this supression.

joperezr · 2016-10-05T17:10:36Z

 #Compat issues with assembly System.Globalization (https://github.com/dotnet/corefx/issues/11035) :
-CannotRemoveBaseTypeOrInterface : Type 'System.Globalization.CultureNotFoundException' does not implement interface 'System.Runtime.Serialization.ISerializable' in the implementation but it does in the contract.
+CannotRemoveBaseTypeOrInterface : Type 'System.Globalization.CultureNotFoundException' does not implement interface 'System.Runtime.Serialization.ISerializable' in the implementation but it does in the contract.
+CannotRemoveBaseTypeOrInterface : Type 'System.Globalization.CultureNotFoundException' does not inherit from base type 'System.SystemException' in the implementation but it does in the contract.


I don't think this is right, why would netcore50 contract would inherit something from SystemException? netcore50 should have the old contract still.

correct we should just remove the netcore50 build of System.Globalization.

joperezr · 2016-10-05T17:11:09Z

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <ItemGroup>
    <ProjectReference Include="..\ref\System.IO.csproj">
-      <SupportedFramework>net463;netcoreapp1.1;uap10.1;$(AllXamarinFrameworks)</SupportedFramework>


I don't understand why are we doing this change

oh is it because SystemException is not available in UWP? in that case should we just have a stub or something for now so that we don't drop support for a framework? cc: @weshaggard

In reply to: 82022610 [](ancestors = 82022610)

I agree, and then you can keep the P2P as well.

In reply to: 82022847 [](ancestors = 82022847,82022610)

Will add this back.

joperezr · 2016-10-05T17:12:45Z

    <OutputType>Library</OutputType>
    <NuGetTargetMoniker>.NETStandard,Version=v1.7</NuGetTargetMoniker>
-    <PackageTargetFramework>netstandard1.7;uap10.1</PackageTargetFramework>
+    <PackageTargetFramework>netstandard1.7</PackageTargetFramework>


If we do decide to remove uap10.1, then you won't need this property at all anymore.

We do need to keep this uap10.1 as well as the build.

Will add this back.

joperezr · 2016-10-05T17:12:58Z

    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.7</NuGetTargetMoniker>
-    <PackageTargetFramework Condition="'$(TargetGroup)' == ''">netstandard1.7;uap10.1</PackageTargetFramework>
+    <PackageTargetFramework Condition="'$(TargetGroup)' == ''">netstandard1.7</PackageTargetFramework>


Will add back.

joperezr · 2016-10-05T17:28:29Z

@@ -1,31 +1,31 @@
 {


I don't see any platform specific dependencies here. Does it make sense to join both unix and win project.json files into one?

joperezr · 2016-10-05T17:30:20Z

  <ItemGroup>
    <Project Include="FunctionalTests\System.Net.Security.Tests.csproj">
-      <TestTFMs>netcoreapp1.0;net46</TestTFMs>
+      <TestTFMs>netcoreapp1.1;net463</TestTFMs>


net463 [](start = 30, length = 6)

Did you actually try running the tests for net463? I ask because people have been adding this but not really test if the tests passes. in order to test it, try running

msbuild System.Net.Security.Tests.builds /p:FilterToTestTFM=net463

and see if that works.

@joperezr @weshaggard Isn't this automatically tested somewhere? If not we need to make sure we have a tracking test issue.

We run net46 tests as part of our official builds in helix.

Yes, I ran them for net463 and tests passed.

joperezr · 2016-10-05T17:32:25Z

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


NugetTargetMoniker [](start = 5, length = 18)

add a Condition="'$(TargetGroup)'==''" here

doesn't look like you added the condition here.

joperezr · 2016-10-05T17:33:02Z

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


you'll need to add "net463": {} as well if you want the tests to work with net463

joperezr · 2016-10-05T17:34:02Z

  </ItemGroup>
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <Target Name="RemoveProductionReference" BeforeTargets="CoreCompile">
+    <Message Text="Removing System.Net.Security reference before compiling" Importance="High" />


You can probably remove the Message here, when we wrote this target this was only meant to prove that we were actually running the target before csc.exe 😄

joperezr · 2016-10-05T17:35:12Z

  <PropertyGroup Condition="'$(TargetGroup)' == 'uap101aot'">
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'net463_Debug|AnyCPU'" />


Nit: Can you keep this together with the rest of the configurations?

joperezr · 2016-10-05T17:36:46Z

  <ItemGroup>
    <None Include="project.json" />
  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)' == '' Or '$(TargetGroup)' == 'netstandard1.7'">


'$(TargetGroup)' == '' Or '$(TargetGroup)' == 'netstandard1.7' [](start = 24, length = 62)

looks like the default build here is netstandard1.7, so you don't really need the second part of your condition.

joperezr · 2016-10-05T17:38:47Z

-    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.3</NuGetTargetMoniker>
-    <PackageTargetFramework Condition="'$(TargetGroup)' == ''">netstandard1.3;netcore50</PackageTargetFramework>
+    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.7</NuGetTargetMoniker>
+    <PackageTargetFramework Condition="'$(TargetGroup)' == ''">netstandard1.7</PackageTargetFramework>


You won't need this. basically when NuGetTargetMoniker is set, and you are only packaging this build for one framework, you shouldn't need to have this property.

danmoseley · 2016-10-05T19:04:16Z

-      <Link>Common\Microsoft\Win32\SafeHandlres\SafeHandleZeroOrMinusOneIsInvalid.cs</Link>
-    </Compile>
    <Compile Include="$(CommonPath)\Microsoft\Win32\SafeHandles\SafeLocalAllocHandle.cs">
      <Link>Common\Microsoft\Win32\SafeHandlres\SafeLocalAllocHandle.cs</Link>


SafeHandlres [](start = 35, length = 12)

someone should rename this folder...

danmoseley · 2016-10-05T19:10:48Z

weshaggard · 2016-10-06T22:21:42Z

    <PreventImplementationReference>true</PreventImplementationReference>
  </PropertyGroup>
  <ItemGroup>
+    <!-- TODO: re-enable these when aot support is added


We shouldn't disable this uap101aot build as it is needed.

cc @SedarG

Will add back.

weshaggard · 2016-10-06T22:22:40Z

    <Project Include="System.IO.csproj">
      <TargetGroup>net463</TargetGroup>
    </Project>
-    <Project Include="System.IO.csproj">


ditto we cannot remove this build.

Will add this back.

weshaggard · 2016-10-06T22:23:47Z

@@ -9,10 +9,9 @@
    <ProjectGuid>{89F37791-6254-4D60-AB96-ACD3CCA0E771}</ProjectGuid>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <DefineConstants>$(DefineConstants);FEATURE_CORECLR</DefineConstants>


/cc @davidsh
Thanks @weshaggard . I think i'll wait for this before I complete #11489.

weshaggard · 2016-10-06T22:25:30Z

  <ItemGroup>
    <ProjectReference Include="..\ref\System.Resources.ResourceManager.csproj">
-      <SupportedFramework>net45;netcore45;netcoreapp1.0;wp8;wpa81;$(AllXamarinFrameworks)</SupportedFramework>
+      <SupportedFramework>net463;netcoreapp1.1;$(AllXamarinFrameworks)</SupportedFramework>


cc @alexperovich

weshaggard · 2016-10-06T22:27:45Z

  <ItemGroup>
    <None Include="project.json" />
  </ItemGroup>
-  <ItemGroup>


This should remain an pkgproj reference to the library under test. Whey did you need to remove it?

weshaggard · 2016-10-06T22:28:48Z

 MembersMustExist : Member 'System.Attribute.IsDefaultAttribute()' does not exist in the implementation but it does exist in the contract.
 MembersMustExist : Member 'System.Attribute.Match(System.Object)' does not exist in the implementation but it does exist in the contract.
-MembersMustExist : Member 'System.Attribute.TypeId.get()' does not exist in the implementation but it does exist in the contract.
+MembersMustExist : Member 'System.Attribute.TypeId.get()' does not exist in the implementation but it does exist in the contract.


We can fix these by adding SystemException into the System.Runtime uap101 build.

@weshaggard By adding SystemException in System.Runtime uap10.1 library, do you mean adding a stub for it? I'm asking because adding that still doesn't get rid of ApiCompat errors like:

CannotRemoveBaseTypeOrInterface : Type 'System.ArgumentException' does not inherit from base type 'System.SystemException' in the implementation but it does in the contract.

Your correct as those are implemented in System.Private.CoreLib. I think those will need to remain in the baseline file for now.

AlexGhiondea · 2016-10-11T05:18:36Z

-    "net46": {
+    "net463": {
      "dependencies": {
        "Microsoft.TargetingPack.NETFramework.v4.6": "1.0.1"


4.6 [](start = 47, length = 3)

4.6.2 -- I will stop commenting about these. Please take a look and update wherever needed.

AlexGhiondea · 2016-10-11T05:20:44Z

sepidehkh · 2016-10-11T19:04:34Z

Any additional comments or is this good to merge?

weshaggard · 2016-10-12T04:13:48Z

    </Compile>
  </ItemGroup>
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <!-- Hack to remove System.Net.Security dependency brought over by NetStandardLibrary before compile time to prevent conflicts, as this test project builds source files from that contract directly -->


Do we need this hack? I thought we were doing "exclude" compile for this issue in other places.

I didn't think of doing it with "exclude". Replaced this in a later commit.

weshaggard · 2016-10-12T04:17:06Z

    <OutputType>Library</OutputType>
    <StringResourcesPath>../../src/Resources/Strings.resx</StringResourcesPath>
-    <NugetTargetMoniker>.NETStandard,Version=v1.3</NugetTargetMoniker>
+    <NugetTargetMoniker>.NETStandard,Version=v1.7</NugetTargetMoniker>


Looks like you are removing the netstandard1.3 test build for this project, as I don't see you adding that configuration in the test.builds file. Also having a quick look at https://github.com/dotnet/corefx/blob/master/src/System.Net.Security/tests/System.Net.Security.Tests.builds I suspect this is going to break some of those test run configurations. In particular the netcoreapp1.0/net46 runs which don't support netstandard1.7.

Made changes to the configuration of these tests. However, since the UnitTests were configured to tests internal methods and are testing newly exposed APIs in NetStandard1.7, it didn't make sense to me to test them against netstandard1.3 or net46 anymore. @stephentoub any thoughts on this?

CIPop · 2016-10-14T23:20:12Z

    "System.Security.Cryptography.X509Certificates": "4.4.0-beta-24613-01",
    "System.Text.RegularExpressions": "4.4.0-beta-24613-01",
    "System.Resources.ResourceManager": "4.4.0-beta-24613-01",
+    "System.Net.Security": {


UnitTests should not depend on the tested binary version of the lib. Instead they need to include the files.

That's exactly what this is doing. Even though System.Net.Security wasn't directly referenced before, it was getting pulled in as a nested dependency, but with explicitly adding it here and having the exclude: compile line, we are removing it even when it get's pulled in by other dependencies.

sepidehkh · 2016-10-17T21:33:22Z

I removed all my System.Net.Security changes except for the SystemException base change as System.Net.Security was upgraded to netstandard1.7 in #11489

cc: @weshaggard @joperezr @CIPop

SedarG · 2016-10-17T22:31:57Z

 MembersMustExist : Member 'System.Globalization.TextInfo.ReadOnly(System.Globalization.TextInfo)' does not exist in the implementation but it does exist in the contract.
-MembersMustExist : Member 'System.Globalization.TextInfo.ToTitleCase(System.String)' does not exist in the implementation but it does exist in the contract.
+MembersMustExist : Member 'System.Globalization.TextInfo.ToTitleCase(System.String)' does not exist in the implementation but it does exist in the contract.
+CannotRemoveBaseTypeOrInterface : Type 'System.Globalization.CultureNotFoundException' does not inherit from base type 'System.SystemException' in the implementation but it does in the contract.


Is there an issue on CoreRT to follow up on this one and similar changes?

Created dotnet/corert#2040

SedarG · 2016-10-17T22:35:37Z


  <!-- Common -->
  <ItemGroup Condition="'$(TargetGroup)' == 'uap101aot'">
+    <Compile Include="$(CommonPath)\System\SystemException.cs" />


Doesn't System.SystemException need to be in S.P.CorLib and exposed through a contract, say S.Runtime??

From my understanding, that's what's supposed to happen. This, however, is just a temporary stub for System.SystemException for UWP. Issue https://github.com/dotnet/corefx/issues/12550 is tracking this.

SedarG · 2016-10-17T22:36:50Z

    <OutputType>Library</OutputType>
    <NuGetTargetMoniker>.NETStandard,Version=v1.7</NuGetTargetMoniker>
-    <PackageTargetFramework>netstandard1.7;uap10.1</PackageTargetFramework>
+    <PackageTargetFramework Condition="'$(TargetGroup)' == ''">netstandard1.7;uap10.1</PackageTargetFramework>


Thanks for fixing this!

SedarG · 2016-10-17T22:39:24Z

    <Project Include="System.Resources.ResourceManager.csproj">
      <TargetGroup>net463</TargetGroup>
    </Project>
+    <Project Include="System.Resources.ResourceManager.csproj" >


TargetGroup=net463 already exists. you can undo this change

That must have happened when hitting merge conflicts. Will remove it.

weshaggard

Thanks for pushing this through. I know it was a challenge with all the other conflicting PRs.

sepidehkh · 2016-10-18T18:12:05Z

Thank you everyone for all the feedback.

* Add missing SystemException derivations * Address feedback * Update dependencies Commit migrated from dotnet/corefx@51663da

dnfclas added the cla-already-signed label Oct 4, 2016

sepidehkh commented Oct 4, 2016

View reviewed changes

justinvp reviewed Oct 4, 2016

View reviewed changes

sepidehkh force-pushed the AddSystemExceptionDerivations branch 3 times, most recently from 6b82756 to 6c89ce2 Compare October 4, 2016 23:00

joperezr reviewed Oct 4, 2016

View reviewed changes

sepidehkh force-pushed the AddSystemExceptionDerivations branch from 6c89ce2 to 3a043e6 Compare October 4, 2016 23:40

joperezr reviewed Oct 5, 2016

View reviewed changes

danmoseley reviewed Oct 5, 2016

View reviewed changes

weshaggard reviewed Oct 6, 2016

View reviewed changes

AlexGhiondea reviewed Oct 11, 2016

View reviewed changes

sepidehkh force-pushed the AddSystemExceptionDerivations branch from 6012ffa to 5cf8239 Compare October 11, 2016 17:53

sepidehkh force-pushed the AddSystemExceptionDerivations branch from 5cf8239 to a439e28 Compare October 11, 2016 23:25

weshaggard reviewed Oct 12, 2016

View reviewed changes

sepidehkh force-pushed the AddSystemExceptionDerivations branch 4 times, most recently from 182564e to ac3fc8b Compare October 14, 2016 19:03

CIPop reviewed Oct 14, 2016

View reviewed changes

sepidehkh force-pushed the AddSystemExceptionDerivations branch from ac3fc8b to 175557f Compare October 17, 2016 20:41

SedarG reviewed Oct 17, 2016

View reviewed changes

sepidehkh force-pushed the AddSystemExceptionDerivations branch from 175557f to cd95334 Compare October 18, 2016 00:57

weshaggard approved these changes Oct 18, 2016

View reviewed changes

sepidehkh added 3 commits October 18, 2016 10:03

Add missing SystemException derivations

378cd88

Address feedback

887707f

Update dependencies

d7eccf3

sepidehkh force-pushed the AddSystemExceptionDerivations branch 2 times, most recently from bbb1907 to d7eccf3 Compare October 18, 2016 17:11

sepidehkh merged commit 51663da into dotnet:master Oct 18, 2016

karelz added area-System.Runtime and removed area-System labels Nov 7, 2016

karelz modified the milestone: 1.2.0 Dec 3, 2016

EgorBo mentioned this pull request Feb 23, 2017

Change base class of OperationAbortedException to SystemException #16391

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Add System.SystemException derivations (dotnet/corefx#12351)

262588c

* Add missing SystemException derivations * Address feedback * Update dependencies Commit migrated from dotnet/corefx@51663da

Conversation

sepidehkh commented Oct 4, 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!

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!

sepidehkh Oct 4, 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!

sepidehkh Oct 4, 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!

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!

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!

sepidehkh commented Oct 4, 2016 •

edited

Loading

sepidehkh Oct 4, 2016 •

edited

Loading

sepidehkh Oct 4, 2016 •

edited

Loading