-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 .NET Standard 2.0 target and set .NET Core 3.1 as the minimum #6984
Conversation
<NetCoreAppTargetFrameworks>netcoreapp3.1;net5.0</NetCoreAppTargetFrameworks> | ||
<StandardTargetFrameworks>netstandard2.0</StandardTargetFrameworks> | ||
<MultiTargetFrameworks>$(StandardTargetFrameworks);$(NetCoreAppTargetFrameworks)</MultiTargetFrameworks> | ||
<StandardTargetFrameworks>netcoreapp3.1;net5.0</StandardTargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using just netcoreapp3.1
could be more optimal here - packages that currently target only netstandard2.0
would then continue being compiled only once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured we may want to take advantage of .NET 5 features, but you are right. I wonder if there are any packages which we explicitly wouldn't want to multi-target.
@@ -1,7 +1,7 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
|
|||
<PropertyGroup> | |||
<TargetFrameworks>net5.0;netcoreapp3.1;net472</TargetFrameworks> | |||
<TargetFrameworks>net5.0;netcoreapp3.1</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use $(TestTargetFrameworks)
now.
@@ -7,5 +7,6 @@ | |||
<TargetFrameworks>$(StandardTargetFrameworks)</TargetFrameworks> | |||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | |||
<RootNamespace>Orleans</RootNamespace> | |||
<ProduceReferenceAssembly>false</ProduceReferenceAssembly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a workaround for some issue where the ref assemblies cause bad interactions with codegen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is something that will likely need deeper investigation. The Source Generator only has ref assemblies available to it, and they do not include private members, so when codegen tries to generate a serializer for a type whose base is in this assembly (StreamIdentity), it complains that it might be missing some fields.
There are a few approaches to fixing it:
- Delegate serialization of base type fields like Hagar does
- Migrate to a different serializer (eg, Hagar)
- Change these certain types so that they are interfaces instead
- Make exceptions for these known-empty abstract base types
I'd prefer an option which also works for end users, which therefore excludes the last two. There are other options, like requiring this csproj flag or the similar flag to still include private members
411c269
to
2433f92
Compare
2433f92
to
aac435c
Compare
This aligns with the move ASP.NET Core made in v3.0 and allows us to modernize a few things which are not supported on older frameworks.
This change does not affect 3.x