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

[release/6.0] Avoid not live built packages dependencies that are satisifed by the shared framework in M.E.Logging.Abstractions #59162

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Sep 15, 2021

Fixes #59161
Reported by customer via #59158
Regressed with 3897ee5

Customer Impact

Customers who reference the Microsoft.Extensions.Logging.Abstractions package transitively reference the System.Buffers and System.Memory packages when targeting .NETCoreApp even though those libraries are already satisfied by the shared framework.

It's undesirable to have the System.Buffers and System.Memory dependencies in the graph
as these aren't live built anymore and are only serviced on demand (for security issues) from the release/2.1 branch.

Testing

The package built locally doesn't contain the dependencies anymore for net6.0. Also, the runtime local package testing infrastructure restores the produced package and makes sure that all dependencies are satisfied.

Risk

Low. The dependencies are present in the RC1 package and this change just removes them from the net6.0 asset.

Fixes #59161

The System.Buffers and System.Memory packages are
dependencies of the net6.0 M.E.Logging.Abstractions
library even though these libraries are part of the
.NETCoreApp shared framework.

It's undesirable to have the dependencies in the graph
as these aren't built live anymore and are only serviced
on demand.
@ViktorHofer ViktorHofer added Servicing-consider Issue for next servicing release review area-Extensions-Logging labels Sep 15, 2021
@ViktorHofer ViktorHofer added this to the 6.0.0 milestone Sep 15, 2021
@ViktorHofer ViktorHofer self-assigned this Sep 15, 2021
@ghost
Copy link

ghost commented Sep 15, 2021

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #59161

The System.Buffers and System.Memory packages are
dependencies of the net6.0 M.E.Logging.Abstractions
library even though these libraries are part of the
.NETCoreApp shared framework.

It's undesirable to have the dependencies in the graph
as these aren't built live anymore and are only serviced
on demand.

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

Servicing-consider, area-Extensions-Logging

Milestone: 6.0.0

@ViktorHofer
Copy link
Member Author

@danmoseley I would like to get this into release/6.0 before we snap today.

@ViktorHofer ViktorHofer changed the title Avoid unnecessary deps in M.E.Logging.Abstractions [release/6.0] Avoid not live built packages dependencies that are satisifed by the shared framework in M.E.Logging.Abstractions Sep 15, 2021
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Seems reasonable. This is a "regression" from previous release due to 69d6127.

One alternative might be to if-def that change so that we only do it on .NETCore and not downlevel where we take a dependency, that said I'm OK with this change as is.

It is permissible to drop packages from a compatible framework iff:

  1. The assembly is already provided by that framework.
  2. The assembly provided by that framework will always have a higher version than the package.
  3. The assembly provided by that framework will always have all API provided by the package.
  4. The assembly provided by the framework provides equivalent functionality.

I believe we can check all these boxes here for both packages.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Sep 15, 2021

Seems reasonable. This is a "regression" from previous release due to 69d6127.

I would prefer to not call that change out as the one that caused the regression. This issue tracks unnecessary added dependencies to the net6.0 dependency group of the package which didn't regress with 69d6127 but with 3897ee5 as at that point the pkgproj infrastructure which stripped these unnecessary dependencies was replaced with NuGet's pack task.

@danmoseley
Copy link
Member

Approved. Reduces surface area, which has value when patching, etc. This one might not meet the higher bar tomorrow.

@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 15, 2021
@ViktorHofer ViktorHofer merged commit 6e67f83 into release/6.0 Sep 15, 2021
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-1 branch September 15, 2021 17:26
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Logging Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants