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

Expose missing references which are present in the runtime and in packages in the targeting pack #54147

Merged

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jun 14, 2021

The platform analyzers warns for usages of APIs which aren't available
on specific platforms. Exposing the missing reference assemblies which
are already part of the runtime pack in the targeting pack.

The big benefit of doing so is that customers don't need to reference
the corresponding packages to target these APIs and that these five
packages can be dead-ended.

Affected packages:

  • Microsoft.Win32.Registry
  • System.IO.FileSystem.AccessControl
  • System.IO.Pipes.AccessControl
  • System.Security.AccessControl
  • System.Security.Principal.Windows

For more in-depth reasoning, see the detailed summary:
#53892 (comment)

Update depents by referencing the latest available package. Making sure
that packages are never used on NetCoreAppCurrent by applying
.NETCoreApp version conditions.

Fixes #53892

I will send out a notice to the affected upstream repos windowsdesktop and aspnetcore after this is merged so that they can remove the references from their targeting packs. cc @dougbu @RussKie

@ghost
Copy link

ghost commented Jun 14, 2021

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

Issue Details

The platform analyzers warns for usages of APIs which aren't available
on specific platforms. Exposing the missing reference assemblies which
are already part of the runtime pack in the targeting pack.

The big benefit of doing so is that customers don't need to reference
the corresponding packages to target these APIs and that these five
packages can be dead-ended.

For more in-depth reasoning, see the detailed summary:
#53892 (comment)

Update depents by referencing the latest available package. Making sure
that packages are never used on NetCoreAppCurrent by applying
.NETCoreApp version conditions.

Fixes #53892

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: 6.0.0

@ViktorHofer ViktorHofer changed the title Add platform specifics to targeting pack Expose missing references which are present in the runtime and in packages in the targeting pack Jun 14, 2021
@ViktorHofer ViktorHofer requested a review from safern June 14, 2021 15:08
@@ -22,7 +22,9 @@ name switch
"urn:mpeg:mpeg21:2003:01-REL-R-NS:licenseTransform" => new XmlLicenseTransform(),
"http://www.w3.org/2000/09/xmldsig# X509Data" => new KeyInfoX509Data(),
"http://www.w3.org/2000/09/xmldsig# KeyName" => new KeyInfoName(),
#pragma warning disable CA1416 // This call site is reachable on all platforms. 'DSAKeyValue' is unsupported on: 'ios', 'maccatalyst', 'tvos'
Copy link
Member Author

@ViktorHofer ViktorHofer Jun 14, 2021

Choose a reason for hiding this comment

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

Started an offline thread with @buyaa-n @GrabYourPitchforks and @bartonjs to figure out what to do about this and the other exclusion in SignedXmlDebugLog.cs (change below).

@dougbu
Copy link
Member

dougbu commented Jun 14, 2021

Affected packages:

  • Microsoft.Win32.Registry
  • System.IO.FileSystem.AccessControl;
  • System.IO.Pipes.AccessControl;
  • System.Security.AccessControl;
  • System.Security.Principal.Windows;

@ViktorHofer the Microsoft.AspNetCore.App.Ref package and our targeting packs contain Microsoft.Win32.Registry, System.Security.AccessControl, and System.Security.Principal.Windows. We also reference all but the System.Security.AccessControl package in a few projects.

To confirm: Once this is in and Maestro++ updates us to a build containing it,

  1. We won't need the above three assemblies in our targeting packs❔
  2. We shouldn't reference the same-named packages when targeting net6.0 or later❔

/cc @dotnet/aspnet-build

eng/Versions.props Outdated Show resolved Hide resolved
@@ -158,6 +160,7 @@
<Reference Include="System.ComponentModel.TypeConverter" />
<Reference Include="System.Diagnostics.Debug" />
<Reference Include="System.IO.FileSystem" />
<Reference Include="System.IO.FileSystem.AccessControl" Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))" />
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding this verbose condition to each item, can we group them in a shared itemgroup where the condition is set to the whole itemgroup?

Copy link
Member Author

Choose a reason for hiding this comment

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

These will go away in 4-5 months when we target NET7 so I don't care much about the added verbosity. Let's keep it as is meanwhile.

@ViktorHofer ViktorHofer force-pushed the AddPlatformSpecificsToTargetingPack branch from e23b794 to 256ead6 Compare June 15, 2021 11:59
@ViktorHofer
Copy link
Member Author

We won't need the above three assemblies in our targeting packs❔

Yes, the assemblies which are now part of the Microsoft.NETCore.App.Ref targeting pack shouldn't be exposed in aspnetcore's and windowsdesktop's targeting pack anymore.

We shouldn't reference the same-named packages when targeting net6.0 or later❔

Exactly. Conflict resolution will trim the reference assembly that comes from the package away anyway but removing the now unnecessary PackageReferences from project files reduces complexity and restore time.

The platform analyzers warns for usages of APIs which aren't available
on specific platforms. Exposing the missing reference assemblies which
are already part of the runtime pack in the targeting pack.

The big benefit of doing so is that customers don't need to reference
the corresponding packages to target these APIs and that these five
packages can be dead-ended.

For more in-depth reasoning, see the detailed summary:
dotnet#53892 (comment)

Fixes dotnet#53892
Update depents by referencing the latest available package. Making sure
that packages are never used on NetCoreAppCurrent by applying
.NETCoreApp version conditions.
@ViktorHofer ViktorHofer force-pushed the AddPlatformSpecificsToTargetingPack branch from dd31634 to c549d96 Compare June 15, 2021 14:46
@ViktorHofer ViktorHofer merged commit 663c40d into dotnet:main Jun 15, 2021
@ViktorHofer ViktorHofer deleted the AddPlatformSpecificsToTargetingPack branch June 15, 2021 17:32
<Dependency Include="@(ExternalLibraryPackage)">
<TargetFramework>netstandard2.1</TargetFramework>
</Dependency>
<Dependency Include="@(ExternalLibraryPackage)"
Copy link
Member

@safern safern Jun 15, 2021

Choose a reason for hiding this comment

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

Do net6.0 need ExternalLibraryPackage in it's dependency group?

Copy link
Member

@safern safern 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 one question, LGTM. Thank you :)

@ericstj
Copy link
Member

ericstj commented Jun 16, 2021

Cool! @ryalanms @dougbu FYI you'll need to react to this in ASP.NET and WindowsDesktop shared frameworks.

@ViktorHofer
Copy link
Member Author

@ericstj I contacted both here in the PR and via mail. We are following up offline.

dougbu added a commit to dotnet/aspnetcore that referenced this pull request Jun 16, 2021
- remove newly-unnecessary package references
- do not expect affected assemblies in _our_ targeting packs
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Jun 16, 2021
- remove newly-unnecessary package references
- do not expect affected assemblies in _our_ targeting packs
RussKie added a commit to RussKie/winforms that referenced this pull request Jun 18, 2021
dotnet-maestro bot added a commit to dotnet/aspnetcore that referenced this pull request Jun 18, 2021
…/efcore (#33571)

[release/6.0-preview6] Update dependencies from dotnet/runtime dotnet/efcore


 - React to dotnet/runtime#54147
- remove newly-unnecessary package references
- do not expect affected assemblies in _our_ targeting packs

 - Add baseline suppression

 - !fixup! Suppress correct reference

 - Omit System.Security.Permissions.

dotnet/runtime#54341
System.Security.Permissions is brought in by System.Security.Cryptography.Xml
for netstandard2.0 compatibility but not required in net6.0 since the
types required exist in System.Security.AccessControl

 - Permit PackageReferences from ref/runtime-pack

 - Fix Permissions version reference and delete dead packages

 - More dead package cleanup

 - Try again at transitioning dead-end package to 5.0 version

 - Fix up version of added package

 - Fix PackageReference from DataProtection -> Registry

This was present in the previous release and shouldn't be removed since
this is part of the public contract of the netstandard2.0 build of DataProtection.

 - Apply suggestions from code review

Co-authored-by: Doug Bunting <[email protected]>

 - Get System.Security.Permissions refs working

 - Update src/DataProtection/DataProtection/src/Microsoft.AspNetCore.DataProtection.csproj

 - Suppress all of System.Security.Permissions dependency closure

 - Add missing dependencies
dotnet-maestro bot added a commit to dotnet/aspnetcore that referenced this pull request Jun 18, 2021
[main] Update dependencies from dotnet/runtime dotnet/efcore


 - React to dotnet/runtime#54147
- remove newly-unnecessary package references
- do not expect affected assemblies in _our_ targeting packs

 - Add baseline suppression

 - !fixup! Suppress correct reference

 - Manually align w/ #33571
- add excluded System.Security.Permissions references
- stop auto-updating dead-ended packages
  - switch to 5.0.0 version of Microsoft.Win32.Registry
  - switch to 5.0.0 version of System.Security.Principal.Windows

 - Suppress all of System.Security.Permissions dependency closure
- cherry-picked from #33571

 - Add missing dependencies
- cherry-picked from #33571
@ghost ghost locked as resolved and limited conversation to collaborators Jul 16, 2021
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.

Expose platform specific assemblies which are already exposed in the runtime pack in the targeting pack
5 participants