Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Target .NET Standard 2.0 #70

Closed
wants to merge 1 commit into from
Closed

Target .NET Standard 2.0 #70

wants to merge 1 commit into from

Conversation

natemcmaster
Copy link
Contributor

@natemcmaster natemcmaster commented Jun 7, 2017

Part of dotnet/aspnetcore#2045

Notable changes:

  • Target netstandard2.0, netcoreapp2.0, and net461
  • The netstandard2.0 throws PlatformNotSupportedException for the following public API
    • ProxyDiagnosticSourceMethodAdapter.Adapt.
  • A few .Internal types will throw PlatformNotSupportedExcpetion or will not be available at all in the ns2.0 assembly when their public surface depended on System.Reflection.Emit types

Background: System.Reflection.Emit was (perhaps mistakenly) part of .NET Standard 1.x, but was removed from 2.0. These APIs are only available when targeting .NET Core 2.0 and .NET Framework 4.6.1 directly.

@natemcmaster
Copy link
Contributor Author

After further discussion and internal conversation, we decided not to target .NET Standard 2.0. Look for a followup that will change this package to target only .NET Core 2.0 and .NET Framework 4.6.1. The decision comes primarily from this package's heavy dependency on System.Reflection.Emit. Without it, there isn't much useful API. Therefore, we'll only target frameworks that support refemit.

@terrajobst
Copy link
Member

terrajobst commented Aug 14, 2017

The decision comes primarily from this package's heavy dependency on System.Reflection.Emit. Without it, there isn't much useful API. Therefore, we'll only target frameworks that support refemit.

That makes no sense to me. You can target .NET Standard and reference System.Reflection.Emit. It just means you only run on .NET implementations that support runtime code gen.

Yes, this means folks can install the package into UWP without a build error which will blow up at runtime. However:

  1. You can work this around with a custom .targets file that fails in UWP
  2. We should address this behavior in NuGet

Dropping .NET Standard support entirely makes this package unusable* for the vast majority of libraries.

*Yes, folks will be able to consume this from .NET Standard 2.0 thanks to the compat shim, but this package violates what we tell customers to do: if you want to share code between .NET Core and .NET Framework, you should target .NET Standard.

@natemcmaster
Copy link
Contributor Author

You can target .NET Standard and reference System.Reflection.Emit.

@terrajobst at the time this was discussed, we were told mixing the 4.3.0 System.* packages with NETStandard.Library 2.0.0 was not okay. There is no System.Reflection.Emit package 4.4.0 and the APIs in this namespace are not in ns2.0.

See also this discussion: https://github.com/dotnet/corefx/issues/17746

@terrajobst
Copy link
Member

Odd. I tried this other day it worked as expected. System.Reflection.Emit 4.3.0 targets .NET Standard 1.1, so you can consume it from .NET Standard 2.0 -- and that version is from November 2016, so older than the linked issue.

@weshaggard?

@bording
Copy link

bording commented Aug 14, 2017

@terrajobst After my conversation with you on twitter, we did end up moving to .NET Standard 2.0 + Reflection.Emit 4.30 packages, so if there's actually a problem with that combination, I think it needs to be announced very publicly.

@terrajobst
Copy link
Member

terrajobst commented Aug 14, 2017

In other words, "if it works, we keep quiet, otherwise I expect Immo to publicly apologize for his bad guidance". Seems fair 😄

@bording
Copy link

bording commented Aug 14, 2017

Well, didn't mean it quite like that of course 😄 but something needs to be done to actually block the combination if it's not meant to be used together.

Right now there is zero indication that it could even be a problem. The packages happily install and so far seem to be working correctly.

@terrajobst
Copy link
Member

terrajobst commented Aug 15, 2017

Synced with @weshaggard offline. The problem is that we can't build the System.Reflection.Emit reference assembly cleanly in CoreFX because it effectively needs to gain access to internal constructors in order to derive from TypeInfo. The current 4.3 version basically hacked it, but should work everywhere.

The real downer is the experience I mentioned earlier: no build errors. We need to fix that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants