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

Add NetCoreAppCurrent configuration to libs (Phase 1) #54544

Merged
merged 25 commits into from
Jun 29, 2021

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jun 22, 2021

Contributes to #54012
Fixes #54556
Fixes #54575

  • Commit 1: Infra change only to make it easier to bump the minimum supported version of .NETCoreApp
  • Commit 2: Add NetCoreAppCurrent to System.Numerics.Tensors (@tannergooding)
  • Commit 3: Add NetCoreAppCurrent to System.Threading.AccessControl (@mangod9)
  • Commit 4: Add NetCoreAppCurrent to System.Memory.Data (@adamsitnik @carlossanlop)
  • Commit 5: Add NetCoreAppCurrent to System.Composition.* projects (@buyaa-n)
  • Commit 6: Add NetCoreAppCurrent to System.IO.Packaging (@adamsitnik, @carlossanlop)
  • Commit 7: Add NetCoreAppCurrent to System.IO.Ports and use runtime checks instead of targeting FreeBSD, OSX and Linux to avoid an unexpected increase in package size and reduce build times (@adamsitnik @carlossanlop @jozkee)
  • Commit 8: Add NetCoreAppCurrent to System.Data.OleDb (@ajcvickers)
  • Commit 9: Add NetCoreAppCurrent to Microsoft.Win32.Registry.AccessControl (cc @Anipik and myself)
  • Commit 10: Add NetCoreAppCurrent to System.Reflection.Context (cc @buyaa-n)
  • Commit 11: Add NetCoreAppCurrent to System.ComponentModel.Composition.Registration (cc @buyaa-n)
  • Commit 12: Add NetCoreAppCurrent to System.Resources.Extensions (cc @buyaa-n)
  • Commit 13: Add NetCoreAppCurrent to System.Net.Http.WinHttpHandler (cc @dotnet/ncl)

@ghost
Copy link

ghost commented Jun 22, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #54012

Commit 1 is an infra change only to make it easier to bump the minimum supported version of .NETCoreApp
Commit 2 adds NetCoreAppCurrent to System.Numerics.Tensors (@tannergooding)
Commit 3 adds NetCoreAppCurrent to System.Threading.AccessControl (@mangod9)

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: -

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@azure-pipelines
Copy link

There was an error handling pipeline event 4f38cb9b-b770-4abb-b5c0-7bbb9145ef96.

@ViktorHofer ViktorHofer requested a review from buyaa-n June 22, 2021 15:07
@ViktorHofer ViktorHofer force-pushed the NetcoreAppCurrentLibsP1 branch 3 times, most recently from 08c07ea to 1983b22 Compare June 22, 2021 20:33
@ViktorHofer ViktorHofer force-pushed the NetcoreAppCurrentLibsP1 branch from 1983b22 to b5c575f Compare June 22, 2021 21:20
@@ -57,7 +57,7 @@
</ItemGroup>

<ItemGroup>
<NETStandardCompatError Include="netcoreapp2.0" Supported="netcoreapp3.1" />
<NETStandardCompatError Include="netcoreapp2.0" Supported="$(NetCoreAppMinimum)" />
Copy link
Member

@ericstj ericstj Jun 24, 2021

Choose a reason for hiding this comment

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

Would it make sense to default NETStandardCompatError/@Supported to $(NetCoreAppMinimum) so that we don't need to duplicate that in all the projects?

Choose a reason for hiding this comment

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

I do not know why I am here. But I love you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though we mostly care about .NETCoreApp, as .NETStandard has multiple compatible frameworks, I wouldn't want to default it to .NETCoreApp. As an example, for NET7 the min .NETFramework version will be net462 and we will make use of this infrastructure for .NETFramework as well.

Copy link
Member

Choose a reason for hiding this comment

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

Defaulting could happen in Targets after the project sets the Include value. You could handle both cases based on TFI of the include. It's just a suggestion, but could avoid boilerplate if we think the boilerplate isn't adding value to the project. I guess from that perspective we could automatically calculate these entirely since we see what the project is targeting.

Copy link
Member Author

@ViktorHofer ViktorHofer Jun 25, 2021

Choose a reason for hiding this comment

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

I guess from that perspective we could automatically calculate these entirely since we see what the project is targeting.

Thanks for the suggestion. Tbh that's what I would prefer. When I added this infrastructure in a previous PR I considered automation but didn't do it for different reasons. That said, I definitely wouldn't want to clean this up as part of this PR. For now I'm fine with having these items in the project file.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, also fine with it for now. Just had this gut-reaction when I saw this being treated like boilerplate. If we do end up keeping it in the project file then we should add it to the docs: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/libraries-packaging.md

@ViktorHofer
Copy link
Member Author

@stephentoub thanks for the review. Can you please take another look? Addressed your feedback.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than a few more tweaks, the source changes LGTM.

@@ -715,7 +729,7 @@ private void RaiseDataReceivedEof()

private unsafe int ProcessRead(SerialStreamIORequest r)
{
Span<byte> buff = r.Buffer.Span;
ReadOnlySpan<byte> buff = r.Buffer.Span;
Copy link
Member

Choose a reason for hiding this comment

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

The only reason this is working is because we pass this to native code... This should remain Span, same for Memory change below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Spoke with @krwq offline and I decided to revert the changes in this file as a suitable implementation for the overloads without compromising the existing functionality requires more changes than I would want to make as part of this PR. Will file an issue to add the overloads.

@ViktorHofer
Copy link
Member Author

Issues are:

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