Configuration changes so that System.Reflection.Emit may compile against netstandard2.0 #30741

joperezr · 2018-06-28T21:10:50Z

Initial configuration work for System.Reflection.Emit in order to unblock the work of writing a netsandard 2.0 implementation of the library. @atsushikan will work on top of this changes in order to actually provide the implementation later.

netstandard2.0

ghost · 2018-06-28T21:15:21Z

I think we concluded we need to give System.Reflection.Emit.Lightweight.dll the same treatment?

joperezr · 2018-06-28T21:18:13Z

Right, I'll add that too.

ghost · 2018-06-28T21:55:08Z

src/System.Reflection.Emit/ref/System.Reflection.Emit.cs

        protected override bool IsPointerImpl() { throw null; }
        protected override bool IsPrimitiveImpl() { throw null; }
+#if !TargetsNetstandard
+        public override bool IsByRefLike { get { throw null; } }


Non-blocking comment: These could be declared in the s.r.e.netstandard.cs file rather than using #if?

Other than that, looks plausible to me though Wes will have do to review all the config/msbuild stuff.

Yes given the number of #ifdef's you should move them to another file.

ghost

lgtm but Wes or Eric will have to review the config/msbuild stuff.

ericstj · 2018-06-28T22:11:25Z

src/System.Reflection.Emit/ref/System.Reflection.Emit.csproj

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
    <ProjectGuid>{4FBD1556-7B96-414E-9EA4-85281956D60E}</ProjectGuid>
+    <DefineConstants Condition="'$(TargetGroup)' == 'netstandard'">$(DefineConstants);TargetsNetstandard</DefineConstants>


Either move these to a separate file as @atsushikan suggests or change this to follow the convention: https://github.com/dotnet/corefx/blob/6cb23ac20696ab314d2b28e95af40c8454bd7c0d/Documentation/coding-guidelines/project-guidelines.md#define-naming-convention.

I'll take care of this.

ericstj · 2018-06-28T22:14:18Z

src/System.Reflection.Emit.Lightweight/ref/Configurations.props

    <BuildConfigurations>
      netcoreapp;
      uap;
+      netstandard;


You'll need a netfx configuration for all of these that typeforwards so that you don't have duplicate types.

This assembly exists inbox on .NET Framework already so I think the placeholder configuration is correct but we should validate that the contract is valid which would happen in the shim apicompat run (which I think are currently manual).

May not be the same version here so you may need to dial the assembly version of the netstandard ref back.

For now the real intention was to unblock @atsushikan for his implementation changes, so I just added the placeholder one for netfx for now so that the build would pass. If we decide we need a runtime facade instead, then I can change that later.

ericstj · 2018-06-28T22:15:51Z

src/System.Reflection.Emit/pkg/System.Reflection.Emit.pkgproj

+<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <SkipValidatePackage>true</SkipValidatePackage>


Curious what validation errors you came up with. I would imagine that getting the shape of the package correct is the goal of this PR and that's what validation checks.

mm a bunch of errors where due to not harvesting the old package contents but validation seeing it so noticing that we dropped support of a bunch of TFMs. The goal of this PR is not really to have a working package, but instead to unblock NS2.0 development for these libraries. I was planning to deal with packaging later.

With my latest set of changes I have the pkgproj working and almost passing validation. The last piece failing validation is:

F:\git\corefx\Tools\Packaging.targets(1131,5): error : System.Reflection.Emit should be supported on .NETStandard,Version=v1.1 but has no runtime assets. [F:\git\corefx\src\System.Reflection.Emit\pkg\System.Reflection.Emit.pkgproj] F:\git\corefx\Tools\Packaging.targets(1131,5): error : System.Reflection.Emit should be supported on .NETStandard,Version=v1.2 but has no runtime assets. [F:\git\corefx\src\System.Reflection.Emit\pkg\System.Reflection.Emit.pkgproj]

And that makes sense, it is caused because in the latest package (4.3.0) we have a ref for netstandard1.1, but have an implementation for netstandard1.3. I guess that in order for these errors to go away, I would have to add a cross compilation of the assembly on netstandard1.1 which would be a turd assembly as well. I'm curious as to why did we ever shipped a package like that which seems pretty broken to me.

That requirement was a new one we started following in 2.0. Before that we relied on NuGet doing ref/lib matching to reject the package if it had a ref with no lib. NuGet ended up removing that check and relying on ref with no lib as a valid encoding for shared framework.

We should add that not-supported assembly for ns1.1. We should also double check if the ns1.3 impl should be moved to netcoreapp / uap (for instance if it is forwarding to our corelib).

weshaggard · 2018-06-28T23:20:15Z

src/System.Reflection.Emit.Lightweight/src/System.Reflection.Emit.Lightweight.csproj

    <ReferenceFromRuntime Include="System.Private.CoreLib" />
  </ItemGroup>
+  <ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true'">
+    <!-- TODO: Add NS2.0 implementation here -->


Isn't the plan to have this be a "PNSE" generated assembly?

weshaggard · 2018-06-28T23:21:21Z

src/System.Reflection.Emit/src/ApiCompatBaseline.txt

@@ -0,0 +1,13 @@
+Compat issues with assembly System.Reflection.Emit:
+TypesMustExist : Type 'System.Reflection.Emit.AssemblyBuilder' does not exist in the implementation but it does exist in the contract.


What configuration is this baseline file needed for?

it was for netstandard, sorry I didn't add it to the file name, I'll fix that.

weshaggard · 2018-06-28T23:21:43Z

src/System.Reflection.Emit/src/System.Reflection.Emit.csproj

+  <ItemGroup Condition="'$(IsPartialFacadeAssembly)' == 'true'">
    <ReferenceFromRuntime Include="System.Private.CoreLib" />
  </ItemGroup>
+  <ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true'">


Generate a PNSE assembly

But the idea here is that @atsushikan will provide the implementation, If I understood correctly these won't be turd assemblies but ones with actual implementation.

ericstj

Why placeholders for Netfx? Are you sure you don’t need a newer facade? Please doublecheck package content and targeting pack.

MichalStrehovsky · 2018-06-29T09:09:42Z

src/System.Reflection.Emit.Lightweight/src/System.Reflection.Emit.Lightweight.csproj

-    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
-    <GenFacadesIgnoreMissingTypes Condition="'$(TargetGroup)'=='uapaot'">true</GenFacadesIgnoreMissingTypes>
+    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' != 'netstandard'">true</IsPartialFacadeAssembly>
+    <GenFacadesIgnoreMissingTypes Condition="'$(TargetGroup)'=='uapaot' And '$(IsPartialFacadeAssembly)' != 'true'">true</GenFacadesIgnoreMissingTypes>


Do we need to keep the $(TargetGroup)'=='uapaot' condition here and in other files? Should I have deleted this in #30472?

conflicts

joperezr · 2018-06-30T18:26:57Z

The failing test on Windows Nano is not related to my PR.

ericstj · 2018-07-02T15:36:51Z

src/System.Reflection.Emit.ILGeneration/src/System.Reflection.Emit.ILGeneration.csproj

+    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' != 'netstandard'">true</IsPartialFacadeAssembly>
+    <GenFacadesIgnoreMissingTypes Condition="'$(TargetGroup)'=='uapaot' And '$(IsPartialFacadeAssembly)' == 'true'">true</GenFacadesIgnoreMissingTypes>
+    <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetGroup)' == 'netstandard'">SR.PlatformNotSupported_RefEmitILGeneration</GeneratePlatformNotSupportedAssemblyMessage>
    <ProjectGuid>{8F05FFD6-6697-41CA-B733-709F5A6D3BF1}</ProjectGuid>


The resx should be conditioned so that we don't add resources to the partial facade.

ericstj · 2018-07-02T22:26:44Z

This is still missing PKGProjs for Reflection.Emit.ILGeneration and Reflection.Emit.Lightweight.

These packages all needed the following workarounds: 1. Matching not-supported implementation assemblies for netstandard1.x 2. Not support implementation for AOT frameworks 3. Ensure we don't add API to old frameworks (eg: UAP10.0.16299, desktop) or frameworks compatible with those (netstandard2).

ericstj · 2018-07-03T12:42:46Z

Test Windows x64 Debug Build

ericstj · 2018-07-03T13:30:34Z

@weshaggard please have a look

weshaggard · 2018-07-03T16:58:07Z

src/System.Reflection.Emit.ILGeneration/ref/System.Reflection.Emit.ILGeneration.cs

        public virtual void Emit(System.Reflection.Emit.OpCode opcode, System.Type cls) { }
        public virtual void EmitCall(System.Reflection.Emit.OpCode opcode, System.Reflection.MethodInfo methodInfo, System.Type[] optionalParameterTypes) { }
        public virtual void EmitCalli(System.Reflection.Emit.OpCode opcode, System.Reflection.CallingConventions callingConvention, System.Type returnType, System.Type[] parameterTypes, System.Type[] optionalParameterTypes) { }
-        public virtual void EmitCalli(System.Reflection.Emit.OpCode opcode, System.Runtime.InteropServices.CallingConvention unmanagedCallConv, Type returnType, Type[] parameterTypes) { }


Why was this API removed?

It was moved into a netcore-specific file since it wasn't present in all frameworks that support netstandard2.0 (eg: nca2.0, uwp)
https://github.com/dotnet/corefx/pull/30741/files/7eb9a5f89446dbb4aa97fa2d3575cca8fe5bae87#diff-17fa736219e2458643a24ea213deb9e4

weshaggard · 2018-07-03T16:59:24Z

src/System.Reflection.Emit.ILGeneration/src/Configurations.props

  <PropertyGroup>
+    <PackageConfigurations>
+      netstandard;
+      netstandard1.0;


Aren't you harvesting netstandard1.0?

Not for source, we need a not-supported assembly.

weshaggard · 2018-07-03T16:59:53Z

src/System.Reflection.Emit.Lightweight/dir.props

  <Import Project="..\dir.props" />
  <PropertyGroup>
    <AssemblyVersion>4.0.3.0</AssemblyVersion>
+    <AssemblyVersion Condition="'$(TargetsNetStandard)' == 'true'">4.0.0.0</AssemblyVersion>


Might be worth a comment saying this needs to match .NET Framework.

weshaggard · 2018-07-03T17:03:32Z

src/System.Reflection.Emit/src/System.Reflection.Emit.csproj

+    <PackageDestination Include="lib/netstandard1.1">
+      <TargetFramework>netstandard1.1</TargetFramework>
+    </PackageDestination>
+    <PackageDestination Include="runtimes/aot/lib/netcore50">


Isn't this getting harvested?

No, we need a not-supported assembly. We didn't have one in the package previously, we used a placeholder (relying on the old NuGet gaurdrails feature).

weshaggard

LGTM

ericstj · 2018-07-03T21:43:05Z

Thanks for merging @atsushikan.

You may still want to tweak this a bit in your work.

For instance the old netcoreapp1.x impl is still in the nestandard1.3 folder in the package: we may want to move that to netcoreapp1.0.

Also the UWP-non-aot implementations are inbetween the core surface area and the netstandard surface area. In the past we never shipped a reference for this dll to UAP, but did have the implementation. In the latest UWP that even had the netcore-version (4.1.x) so I kept the core versions. Since then, some API has been exposed from CoreLib. This wasn't present in UWP-version-last, so I left it out of that reference assembly, but it is present in the latest UWP reference. Ideally we might want to stop shipping the UWP dlls in this package so that we don't have to keep bloating the package with version-specific references and version-specific implementations which forward to private assemblies. That's what we do for netcoreapp: we just have a placeholder and let the framework package provide it. The downside of that is we expose Ref.Emit to UWP customers even though it won't work for AOT.

…nst netstandard2.0 (dotnet/corefx#30741) * Configuration changes so that System.Reflection.Emit may compile against netstandard2.0 * Adding NS2.0 config for System.Reflection.Emit.Lightweight * Fixing netfx vertical * Addressing PR Feedback * Fixing mscorlib facade generation now that there are a few type conflicts * Add back Ref.Emit packages These packages all needed the following workarounds: 1. Matching not-supported implementation assemblies for netstandard1.x 2. Not support implementation for AOT frameworks 3. Ensure we don't add API to old frameworks (eg: UAP10.0.16299, desktop) or frameworks compatible with those (netstandard2). Commit migrated from dotnet/corefx@1e72f59

Configuration changes so that System.Reflection.Emit may compile against

9a13302

netstandard2.0

joperezr requested review from a user, ericstj and weshaggard June 28, 2018 21:10

Adding NS2.0 config for System.Reflection.Emit.Lightweight

1a723a9

ghost reviewed Jun 28, 2018

View reviewed changes

ghost approved these changes Jun 28, 2018

View reviewed changes

ericstj reviewed Jun 28, 2018

View reviewed changes

Fixing netfx vertical

5cca276

weshaggard reviewed Jun 28, 2018

View reviewed changes

ericstj reviewed Jun 28, 2018

View reviewed changes

MichalStrehovsky reviewed Jun 29, 2018

View reviewed changes

Addressing PR Feedback

ea678d0

joperezr force-pushed the RefEmitNS20 branch from 340578e to ea678d0 Compare June 30, 2018 13:19

Fixing mscorlib facade generation now that there are a few type

8d4a7e8

conflicts

ericstj reviewed Jul 2, 2018

View reviewed changes

ericstj force-pushed the RefEmitNS20 branch from e1b0cab to 7eb9a5f Compare July 3, 2018 06:05

weshaggard reviewed Jul 3, 2018

View reviewed changes

weshaggard approved these changes Jul 3, 2018

View reviewed changes

ghost merged commit 1e72f59 into dotnet:master Jul 3, 2018

karelz added this to the 3.0 milestone Jul 8, 2018

joperezr deleted the RefEmitNS20 branch November 2, 2018 20:09

YairHalberstadt mentioned this pull request Feb 19, 2019

A number of APIs still exist in both .NetCore and .NetFramework, but are missing in the .NetStandard 2.0 version of System.Reflection.Emit dotnet/standard#1075

Closed

YairHalberstadt mentioned this pull request Feb 1, 2020

System.Reflection.Emit Nuget package still missing APIs on .Net Standard 2.0 dotnet/runtime#28724

Closed

This pull request was closed.

		@@ -0,0 +1,13 @@
		Compat issues with assembly System.Reflection.Emit:
		TypesMustExist : Type 'System.Reflection.Emit.AssemblyBuilder' does not exist in the implementation but it does exist in the contract.

Configuration changes so that System.Reflection.Emit may compile against netstandard2.0 #30741

Configuration changes so that System.Reflection.Emit may compile against netstandard2.0 #30741

Uh oh!

Conversation

joperezr commented Jun 28, 2018

Uh oh!

ghost commented Jun 28, 2018

Uh oh!

joperezr commented Jun 28, 2018

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!

ghost left a comment

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!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Jun 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj commented Jul 2, 2018

Uh oh!

ericstj commented Jul 3, 2018

Uh oh!

ericstj commented Jul 3, 2018

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 left a comment

Choose a reason for hiding this comment