Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove framework targets net40, netstandard1.3, netstandard1.5 #495

Merged
merged 16 commits into from
May 16, 2020

Conversation

stakx
Copy link
Member

@stakx stakx commented May 1, 2020

This is a follow-up to #407 (comment).

My suggestion is to get #485 done first (Update: ✔️), then I'll rebase this PR and complete the removal of all obsolete targets.

@stakx stakx changed the title Remove framework targets net35, net40, netstandard1.3, netstandard1.5 Remove framework targets net40, netstandard1.3, netstandard1.5 May 6, 2020
@stakx stakx added this to the v5.0.0 milestone May 6, 2020
@stakx stakx force-pushed the remove-targets branch 4 times, most recently from 66a0f5e to c328a6b Compare May 13, 2020 21:35
@@ -3,7 +3,7 @@
<Import Project="..\..\buildscripts\common.props"></Import>

<PropertyGroup>
<TargetFrameworks>net45;netstandard1.5</TargetFrameworks>
<TargetFrameworks>net45;netstandard1.5;netstandard2.0;netstandard2.1</TargetFrameworks>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose adding just netstandard2.0 would be sufficient. I've included netstandard2.1 mainly for consistency with the main library—I expect it'll be less confusing in the future to see the same list of target frameworks everywhere. (Same goes for the other logging integration projects.)

@stakx stakx marked this pull request as ready for review May 13, 2020 21:45
@stakx
Copy link
Member Author

stakx commented May 13, 2020

@jonorossi, this should be good to go.

I've made these changes one target at a time, which is why the commit history may seem a little repetitive. It should make reviewing a little easier, though.

There are some more things that could be done:

  • Checking whether we can simplify the Type vs. TypeInfo API mess. IIRC, .NET Standard 2.x has gravitated back towards the traditional Reflection APIs, so it might be possible to remove a few .GetTypeInfo()s here and there. This didn't seem too urgent to me, so I might do this in a separate PR. (Update: ✔️)

  • It might be good to shift PublicApiGenerator execution to the .NET Core build, so that we get as much work done on the cross-platform build as possible. This is more of a CI concern, it might be more appropriate to do this once we rewrite the CI scripts (for Move from AppVeyor to GitHub Actions #497).

What do you think?

@stakx stakx requested a review from jonorossi May 13, 2020 21:52
Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Great work, that is a good clean up. Luckily things have moved on a lot since .NET Standard 1.x.

Comment on lines -22 to 24
#if FEATURE_SECURITY_PERMISSIONS && DOTNET40
#if FEATURE_SECURITY_PERMISSIONS
[assembly: SecurityRules(SecurityRuleSet.Level2)]
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it isn't already, removing all the security transparency junk should be on the list for this major release.

But leave it for another pull request, this one is already big 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that next.

@@ -29,42 +29,42 @@ public class BasicClassProxyTestCase : BasePEVerifyTestCase
public void ProxyForBaseTypeFromUnsignedAssembly()
{
Type t = typeof(Class);
Assert.False(StrongNameUtil.IsAssemblySigned(t.GetTypeInfo().Assembly));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do always wonder what .NET would look like today if Microsoft had iterated on the .NET Framework rather than starting from scratch (i.e. ProjectK, DNX, .NET Core). .NET Core 3.1 BCL and tooling is very similar to .NET Framework and the migration wouldn't have been so painful for so long. Could have just had a self-contained .NET Framework 5, 6 or 7 with breaking changes between releases.

Glad to see GetTypeInfo calls gone, from a purist viewpoint they made sense, but they definitely get in the way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, now that they have announced .NET 5 one cannot help but wonder. Feels like we've come full circle, but I suppose it's been a learning experience for the people behind the runtime, too. And had we followed a more direct path, me might have never gotten the runtime open-sourced, nor .NET Standard; both are great achievements.

The NuGet story, OTOH, has been one huge disaster. 😄

Comment on lines 77 to -114
* `FEATURE_APPDOMAIN` - enables support for features that make use of an AppDomain in the host.
* `FEATURE_ASSEMBLYBUILDER_SAVE` - enabled support for saving the dynamically generated proxy assembly.
* `FEATURE_BINDINGLIST` - enables support features that make use of System.ComponentModel.BindingList.
* `FEATURE_DICTIONARYADAPTER_XML` - enable DictionaryAdapter Xml features.
* `FEATURE_CUSTOMMODIFIERS` - enables reading and emitting optional and required custom modifiers defined on parameters including return parameters. It seems like a defect in corefx not to expose these methods because they are still implemented.
* `FEATURE_EVENTLOG` - provides a diagnostics logger using the Windows Event Log.
* `FEATURE_GAC` - enables support for obtaining assemblies using an assembly long form name.
* `FEATURE_GET_REFERENCED_ASSEMBLIES` - enables code that takes advantage of System.Reflection.Assembly.GetReferencedAssemblies().
* `FEATURE_IDATAERRORINFO` - enables code that depends on System.ComponentModel.IDataErrorInfo.
* `FEATURE_ISUPPORTINITIALIZE` - enables support for features that make use of System.ComponentModel.ISupportInitialize.
* `FEATURE_LEGACY_REFLECTION_API` - provides a shim for .NET 4.0 that emulates the `TypeInfo` API available in .NET 4.5+ and .NET Core.
* `FEATURE_LISTSORT` - enables support for features that make use of System.ComponentModel.ListSortDescription.
* `FEATURE_NETCORE_REFLECTION_API` - provides shims to implement missing functionality in .NET Core that has no alternatives.
* `FEATURE_REMOTING` - supports remoting on various types including inheriting from MarshalByRefObject.
* `FEATURE_SECURITY_PERMISSIONS` - enables the use of CAS and Security[Critical|SafeCritical|Transparent].
* `FEATURE_SERIALIZATION` - enables support for serialization of dynamic proxies and other types.
* `FEATURE_SMTP` - provides the email sender abstraction and implementation.
* `FEATURE_SYSTEM_CONFIGURATION` - enables features that use System.Configuration and the ConfigurationManager.
* `FEATURE_TARGETEXCEPTION` - enabled catching a `TargetException`. `System.Reflection.TargetException` is implemented by .NET Core but not exposed by corefx.
* `FEATURE_TEST_COM` - enables COM Interop tests.
* `FEATURE_TEST_DATASET` - enables tests that involve `System.Data.DataSet`s.
* `FEATURE_TEST_PEVERIFY` - enables verification of dynamic assemblies using PEVerify during tests. (Only defined on Windows builds since Windows is currently the only platform where PEVerify is available.)
* `FEATURE_TEST_SERILOGINTEGRATION` - enables Serilog integration tests.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this is merged we can talk through these, more definitely need to go.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'll open an issue for that.

@jonorossi
Copy link
Member

It might be good to shift PublicApiGenerator execution to the .NET Core build, so that we get as much work done on the cross-platform build as possible. This is more of a CI concern, it might be more appropriate to do this once we rewrite the CI scripts

Makes sense, definitely think it'll be easier when the build scripts are cleaned up.

stakx added 13 commits May 16, 2020 10:50
Otherwise we cannot remove the last remaining `netstandard1.x` target
without losing .NET Core support.
Those are now defined for all target frameworks and therefore no
longer needed:

 * `FEATURE_BINDINGLIST`
 * `FEATURE_DICTIONARYADAPTER_XML`
 * `FEATURE_GAC`
 * `FEATURE_GET_REFERENCED_ASSEMBLIES`
 * `FEATURE_IDATAERRORINFO`
 * `FEATURE_ISUPPORTINITIALIZE`
 * `FEATURE_LISTSORT`
 * `FEATURE_NETCORE_REFLECTION_API`
 * `FEATURE_NETSTANDARD2_COMPATIBILITY` along w/ `.AsType()` calls
 * `FEATURE_SMTP`
 * `FEATURE_TARGETEXCEPTION`
 * `FEATURE_TEST_DATASET`
 * `FEATURE_TEST_SERILOGINTEGRATION`

And further:

 * some usages of `DOTNET45` where it guards `XmlConvert` (which is
   now also available on all targets).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants