Reference S.P.CoreLib from the facades by jkotas · Pull Request #12187 · dotnet/corefx

jkotas · 2016-09-29T07:47:58Z

Build facades for CoreCLR directly against S.P.CoreLib - similar to how they are built for CoreRT.

jkotas · 2016-09-29T07:49:11Z

@weshaggard PTLA. The changes build fine with mscorlib.dll removed from CoreCLR targeting pack.

jkotas · 2016-09-29T07:51:23Z

 MembersMustExist : Member 'System.Type.IsSecurityTransparent.get()' does not exist in the implementation but it does exist in the contract.
-Total Issues: 3
+MembersMustExist : Member 'System.Type.GetType(System.String, System.Func<System.Reflection.AssemblyName, System.Reflection.Assembly>, System.Func<System.Reflection.Assembly, System.String, System.Boolean, System.Type>)' does not exist in the implementation but it does exist in the contract.
+MembersMustExist : Member 'System.Type.GetType(System.String, System.Func<System.Reflection.AssemblyName, System.Reflection.Assembly>, System.Func<System.Reflection.Assembly, System.String, System.Boolean, System.Type>, System.Boolean)' does not exist in the implementation but it does exist in the contract.


Building against implementation uncovered several APIs missing.

Which implementation is missing these?

They are missing in CoreCLR System.Private.CoreLib, but they are present in the CoreCLR targeting pack mscorlib.dll.

Good that we caught this can we make sure there is an issue filed to get them exposed.

Opened https://github.com/dotnet/corefx/issues/12309

stephentoub · 2016-09-29T11:58:08Z

Thanks for doing this, Jan.

FYI, there are some clash errors:

Error : Multiple projects built twice with the same target path D:\j\workspace\windows_nt_de---4526f5ff\bin\Windows_NT.AnyCPU.Debug\System.IO.FileSystem\System.IO.FileSystem.dll.
D:\j\workspace\windows_nt_de---4526f5ff\build.proj -->
  Global Properties:
    OSGroup = Windows_NT
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
D:\j\workspace\windows_nt_de---4526f5ff\src\dirs.proj -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\src.builds -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.IO.FileSystem\src\System.IO.FileSystem.builds -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.IO.FileSystem\src\System.IO.FileSystem.csproj
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    OSGroup = Windows_NT

D:\j\workspace\windows_nt_de---4526f5ff\build.proj -->
  Global Properties:
    OSGroup = Windows_NT
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
D:\j\workspace\windows_nt_de---4526f5ff\src\dirs.proj -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\src.builds -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.Diagnostics.StackTrace\src\System.Diagnostics.StackTrace.builds -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.Diagnostics.StackTrace\src\System.Diagnostics.StackTrace.csproj -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    BuildAllProjects = true
    InputOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.IO.FileSystem\src\System.IO.FileSystem.csproj
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    BuildAllProjects = true
    InputOSGroup = Windows_NT


Error : Multiple projects built twice with the same target path D:\j\workspace\windows_nt_de---4526f5ff\bin\Windows_NT.AnyCPU.Debug\System.Runtime.Extensions\System.Runtime.Extensions.dll.
D:\j\workspace\windows_nt_de---4526f5ff\build.proj -->
  Global Properties:
    OSGroup = Windows_NT
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
D:\j\workspace\windows_nt_de---4526f5ff\src\dirs.proj -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\src.builds -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.Runtime.Extensions\src\System.Runtime.Extensions.builds -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.Runtime.Extensions\src\System.Runtime.Extensions.csproj
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    OSGroup = Windows_NT

D:\j\workspace\windows_nt_de---4526f5ff\build.proj -->
  Global Properties:
    OSGroup = Windows_NT
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
D:\j\workspace\windows_nt_de---4526f5ff\src\dirs.proj -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\src.builds -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.Diagnostics.StackTrace\src\System.Diagnostics.StackTrace.builds -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    DefaultBuildAllTarget = Build
    BuildAllProjects = true
    InputOSGroup = Windows_NT
    FilterToOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.Diagnostics.StackTrace\src\System.Diagnostics.StackTrace.csproj -->
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    BuildAllProjects = true
    InputOSGroup = Windows_NT
D:\j\workspace\windows_nt_de---4526f5ff\src\System.Runtime.Extensions\src\System.Runtime.Extensions.csproj
  Global Properties:
    TargetOS = Windows_NT
    ConfigurationGroup = Debug
    WithoutCategories = IgnoreForCI
    BuildAllProjects = true
    InputOSGroup = Windows_NT

jkotas · 2016-09-29T14:07:03Z

I have fixed the binclash errors. @weshaggard also mentioned before that there may be packaging issues that will need fixing ... not sure what to look for.

weshaggard · 2016-09-30T06:19:09Z

  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'net463_Release|AnyCPU'" />
  <ItemGroup Condition="'$(IsPartialFacadeAssembly)'=='true'">
-    <TargetingPackReference Include="mscorlib" />
+    <TargetingPackReference Include="mscorlib" Condition="'$(TargetGroup)' == 'net463'" />


You can actually remove these TargetingPackReference's to the core assembly they should default to System.Private.CoreLib if that is present, See https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/packageresolve.targets#L148. You could also explicitly setTargetingPackCoreAssembly property if needed.

When I remove the TargetingPackReference for System.Private.CoreLib, it does not build anymore.

Removing TargetingPackReference for mscorlib works fine - I will do that.

Hmmm, the TargetingPackReference for mscorlib is set in like hundreds more files than what I am touching in this change. I will do it as a follow sweep PR to remove it from everywhere.

weshaggard · 2016-09-30T06:22:29Z

      <OSGroup>Unix</OSGroup>
    </Project>
    <Project Include="System.Diagnostics.Debug.Tests.csproj">
-      <TestTFMs>netcore50;netcoreapp1.0;net46</TestTFMs>


What prompted this split? TestTFM isn't factored into the global build options and so you are essentially building this twice in the same configuration. TestTFMs is only about which things to run.

weshaggard · 2016-09-30T06:24:48Z

    </ProjectReference>
    <ProjectReference Include="..\..\System.Runtime\src\System.Runtime.csproj" />
    <ProjectReference Include="..\..\System.Runtime.Extensions\src\System.Runtime.Extensions.csproj" />
+    <TargetingPackReference Include="System.Private.CoreLib" Condition="'$(TestTFM)' != 'net46'" />


We shouldn't condition things like this in the test project on TestTFM. If we need to make this test project have a TargetGroup net46 configuration. It is just pure accident that these ever worked when running on net46 (assuming they actually do work there).

The white-box part of the tests could not ever work on net46. I have excluded them for TargetGroup net46 configuration.

weshaggard · 2016-09-30T06:27:53Z

-    </ProjectReference>
-    <ProjectReference Include="..\..\System.Runtime\src\redist\System.Runtime.depproj">
-      <TargetGroup>netcore50</TargetGroup>
+    <ProjectReference Include="..\..\System.Collections\src\System.Collections.csproj" />


This is going to break the netcore50 builds, so we either need to fork these based on that configuration or we need to remove that build configuration completely. If we remove it means we will essentially harvest that configuration from the last stable package and reship that same binary for those configs.

The netcore50 build configuration did not exist in the .builds file, even before my change. Is there anything that needs fixing?

weshaggard · 2016-09-30T06:30:42Z

    <TargetingPackReference Include="System.Private.Interop" />
+    <ValidateIgnoreReference Include="@(TargetingPackReference)" />
+  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)' != 'net462' and '$(TargetGroup)' != 'net463' and '$(TargetGroup)' != 'netcore50aot' and '$(TargetGroup)' != 'netstandard13aot'">


For this project in particular I think I would just remove the TargetPackReference for System.Private.CoreLib which will eliminate the need for this crazy long condition here.

I have simplified the condition - to match the pattern used in other places. I have also removed the early CoreRT netstandard13aot config - it was unused and bit-rotten.

weshaggard · 2016-09-30T06:32:58Z

@@ -0,0 +1,4 @@
+Compat issues with assembly System.Reflection:


Are these CoreCLR or CoreRT issues? If they are for both then keep the file named like this but if it is just one or the other please update the name of the baseline file to add a ".uap101" or "netstandard1.3" at the end.

These are CoreCLR issues. If I rename the file to netstandard1.3 at the end, it does not get picked up and the build fails. I see the file without suffix to be used everywhere else to track CoreCLR issues - I do not see why it should be different here.

Looks like this doesn't have a netstandard1.3 build just a netstandard1.7 build. The reason we try to make these target specific is so that we don't end up masking work in other configurations. In this case I suspect that these are missing from CoreCLR and CoreRT so using the common name should be fine because we likely need to fix both.

Just for context here is the logic where we pick the ApiCompatBaseline file name https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/ApiCompat.targets#L6.

weshaggard · 2016-09-30T06:34:08Z

    <ProjectReference Include="..\..\System.ObjectModel\src\redist\System.ObjectModel.depproj">
      <TargetGroup>netcore50</TargetGroup>
    </ProjectReference>
-    <ProjectReference Include="..\..\System.Runtime\src\redist\System.Runtime.depproj">


Similar to stack trace I think this will break netcore50 builds.

There is no netcore50 build for this one...

There is a netcore50aot build, we should either remove that or change it to uap101 because otherwise the changes to this project may fail if they get picked up by .NET Native. We should probably make the System.ObjectModel consistent here as well to go with a straight csproj reference like System.Runtime.

Actually looking at the package this is one of the cases where we end up using the netstandard1.3 assets for netcore50 so this change will break that configuration. In which case we either need to bump this to netstandard1.7 or explicitly add a netcore50 build.

I have bumped the default build to netstandard1.7 and fixed System.ObjectModel to be consistent.

I am trying not to change anything about netcore50aot build in this change. There are number of other projects that have netcore50aot build that have risk of not mixing well with shipped .NET Native. @SedarG is cleaning it up one assembly at a time.

weshaggard · 2016-09-30T06:35:40Z

    <Compile Include="System\Threading\Tasks\TaskExtensions.CoreRT.cs" />
    <ProjectReference Include="..\..\System.Runtime\src\redist\System.Runtime.depproj" />
    <TargetingPackReference Include="System.Private.CoreLib" />
-    <TargetingPackReference Include="System.Private.CoreLib.Threading" />


Doesn't this break? As I would expect it to still need the System.Private.CoreLib.Threading for the netcore50aot build configuration.

Ah.. looks like that build configuration doesn't even exist in the .builds file so we can probably remove all these or convert them to uap101 configuration and add that configuration in the .builds file.

There are multiple things broken that would need to be fixed to get this to build again for uap101aot. @SedarG has been working through them one .dll at a time - I will leave to him to deal with and re-enable.

weshaggard · 2016-09-30T06:42:00Z

I have fixed the binclash errors. @weshaggard also mentioned before that there may be packaging issues that will need fixing ... not sure what to look for.

The packaging issue here is that we need to be sure that all the existing platforms don't break as our packages need to be backwards compatible. For example anything netcore50 build configuration still needs to be reference mscorlib otherwise when someone installs our latest packages in UWP they will break for UWP F5 scenarios because there only mscorlib exists. So we have 2 options to deal with that problem:

We remove the netcore50 build configurations which will cause the older versions from our last stable packages to be reshipped, and so they will still be reference mscorlib.
We add more conditions in these build files make the netcore50 builds still target mscorlib.

I'm leaning toward option (1) but the one downside of that option is that if we ever did need to fix something for those libraries we would either need to do it in a servicing branch or retroactively add that build configuration for the library in question. We are doing this in other scenarios so I still feel option (1) is the best way to go.

jkotas · 2016-09-30T16:14:30Z

I agree with option (1) as well. There is no way we can maintain the matrix working live given how many changes are being made for the netstandard20 work.

I went over all packages that I am touching and made sure that they do not build netcore50 TargetGroup. I assume that I do not need to add uap101 TargetGroup if the regular CoreCLR flavor works fine - it is what was done in other places. All other feedback is incorporated as well. @weshaggard Could you please take a look?

weshaggard · 2016-10-01T05:28:18Z

    <AssemblyName>System.Diagnostics.Tools</AssemblyName>
    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
-    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.3</NuGetTargetMoniker>
+    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.7</NuGetTargetMoniker>


Note this was just bumped in #11986. That PR will give you an indication of some of the things that change as part of the bump.

weshaggard · 2016-10-01T05:31:43Z

    <AssemblyName>System.Diagnostics.Contracts</AssemblyName>
    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
-    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.0</NuGetTargetMoniker>
+    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.7</NuGetTargetMoniker>


whenever you bump this you will need to minimally update the associated project.json version otherwise you will see a bunch of warnings about them not finding this version. I also expect that you will likely need to make some updates to the pkgproj. @ericstj if we don't change assembly version but just bump the standard version shouldn't that hit some package validation issues?

jkotas · 2016-10-02T00:52:53Z

@weshaggard This should be complete now. Could you please take a look one more time?

jkotas · 2016-10-04T00:17:50Z

@dotnet-bot test Innerloop CentOS7.1 Release Build and Test please

…ostics.Debug default to .NETStandard,Version=v1.7

Reference S.P.CoreLib from the facades Commit migrated from dotnet/corefx@c407bf9

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

jkotas commented Sep 29, 2016

View reviewed changes

weshaggard mentioned this pull request Sep 30, 2016

Move S.IO.FileAccess to corelib. #12206

Merged

weshaggard reviewed Sep 30, 2016

View reviewed changes

jkotas force-pushed the CoreLib branch 3 times, most recently from 4fdc17a to b826477 Compare September 30, 2016 15:57

jkotas force-pushed the CoreLib branch 2 times, most recently from 58dc63a to 92e4feb Compare September 30, 2016 19:16

This was referenced Oct 1, 2016

Cleanup #12268

Merged

Ensure that netcore50 won't be broken by directly referencing S.P.CoreLib in facades #12269

Merged

weshaggard reviewed Oct 1, 2016

View reviewed changes

jkotas force-pushed the CoreLib branch 5 times, most recently from 572d9a9 to ff226c1 Compare October 1, 2016 20:11

jkotas force-pushed the CoreLib branch from ff226c1 to d6422b1 Compare October 3, 2016 20:45

jkotas force-pushed the CoreLib branch 2 times, most recently from 33c4435 to 65702e3 Compare October 4, 2016 15:51

jkotas added 2 commits October 4, 2016 17:31

Reference S.P.CoreLib from the facades

b24644c

Bump System.Runtime.WindowsRuntime, System.Threading and System.Diagn…

3f907c1

…ostics.Debug default to .NETStandard,Version=v1.7

jkotas force-pushed the CoreLib branch from 65702e3 to 3f907c1 Compare October 5, 2016 00:34

jkotas merged commit c407bf9 into dotnet:master Oct 5, 2016

jkotas deleted the CoreLib branch October 5, 2016 07:07

jkotas mentioned this pull request Oct 5, 2016

Delete mscorlib from CoreCLR targeting pack dotnet/coreclr#7494

Merged

karelz modified the milestone: 1.2.0 Dec 3, 2016

jkotas mentioned this pull request Jan 31, 2020

Reflection APIs missing in the implemementation dotnet/runtime#18833

Closed

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

Merge pull request dotnet/corefx#12187 from jkotas/CoreLib

465f707

Reference S.P.CoreLib from the facades Commit migrated from dotnet/corefx@c407bf9

		@@ -0,0 +1,4 @@
		Compat issues with assembly System.Reflection:

Conversation

jkotas commented Sep 29, 2016

Uh oh!

jkotas commented Sep 29, 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!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Sep 29, 2016

Uh oh!

jkotas commented Sep 29, 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!

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!

weshaggard commented Sep 30, 2016

Uh oh!

jkotas commented Sep 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!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Oct 2, 2016

Uh oh!

jkotas commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jkotas commented Sep 29, 2016 •

edited

Loading

jkotas commented Sep 30, 2016 •

edited

Loading