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

Clean up some package dependencies #111667

Closed
wants to merge 1 commit into from
Closed

Conversation

pentp
Copy link
Contributor

@pentp pentp commented Jan 21, 2025

Clean up unnecessary transitive dependencies as nuget packages are always shipped with a single version value now.
Remove netstandard2.1 targets for projects where it doesn't provide any practical benefit.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 21, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

@huoyaoyuan
Copy link
Member

This shouldn't be done by non-infra team member. There can be more implications in package dependencies, which need to be verified for many other scenarios, including source build.

<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Bcl.AsyncInterfaces\src\Microsoft.Bcl.AsyncInterfaces.csproj" />
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" />
Copy link
Member

@ViktorHofer ViktorHofer Jan 21, 2025

Choose a reason for hiding this comment

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

These are direct dependencies which are intentionally explicitly referenced, even if those would come in transitively via other dependencies.

@ViktorHofer
Copy link
Member

This shouldn't be done by non-infra team member. There can be more implications in package dependencies, which need to be verified for many other scenarios, including source build.

Yes and no. I'm fine with some of these changes (i.e. the netstandard2.1 TFM drops might be OK) but the dependencies are explicitly more verbose as we prefer referencing direct dependencies explicitly.

@ViktorHofer ViktorHofer requested a review from a team January 21, 2025 16:21
@ViktorHofer
Copy link
Member

@dotnet/area-infrastructure-libraries PTAL (I would like a second opinion on this one)

@pentp
Copy link
Contributor Author

pentp commented Jan 21, 2025

This shouldn't be done by non-infra team member. There can be more implications in package dependencies, which need to be verified for many other scenarios, including source build.

Yes and no. I'm fine with some of these changes (i.e. the netstandard2.1 TFM drops might be OK) but the dependencies are explicitly more verbose as we prefer referencing direct dependencies explicitly.

But what are the benefits of such explicit references? They are incomplete currently anyway.

@ViktorHofer
Copy link
Member

But what are the benefits of such explicit references? They are incomplete currently anyway.

Fair point about completeness. We had discussions about adding an analyzer that would enforce that direct assembly references are referenced.

The general advantages of explicitly referencing direct dependencies are:

  • Clear view of a project's direct dependencies (on top of the targeting pack).
  • When a transitive dependency changes, the project in question isn't impacted.

@pentp
Copy link
Contributor Author

pentp commented Jan 21, 2025

Clear view of a project's direct dependencies (on top of the targeting pack).

In some context a complete (flat) view of dependencies could be useful, but so could a more distilled view or even just the minimum required. This also seems partially a tooling/UI limitation.

When a transitive dependency changes, the project in question isn't impacted.

Any transitive dependency removals would have to be considered a breaking change anyway - any number of packages/projects out there could depend on it transitively. And since all package versions seem to be updated in-sync since .NET 9, there can't be any version mismatches either.

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 21, 2025

Any transitive dependency removals would have to be considered a breaking change anyway - any number of packages/projects out there could depend on it transitively. And since all package versions seem to be updated in-sync since .NET 9, there can't be any version mismatches either.

I wasn't exclusively talking about public dependencies here. Yes, for shipping packages, changing the dependency graph is considered a breaking change. We still have a lot of projects in this repository that don't ship as NuGet packages: All .Private. assemblies and the shared framework assemblies. Those projects can and should change their references based on demand.

In some context a complete (flat) view of dependencies could be useful, but so could a more distilled view or even just the minimum required. This also seems partially a tooling/UI limitation.

Correct. As mentioned, we prefer a view of direct dependencies in the project file which end up as the actual assembly's
references. We should document that and enforce via an analyzer.

@Varorbc
Copy link
Contributor

Varorbc commented Jan 21, 2025

I've mentioned this before, but I really prefer transitive references around. It makes dependencies much cleaner and easier to manage. For instance, by doing this, we could cut down the explicit references to something like the Microsoft.Extensions.Hosting package by about 50%.

explicit references

image

transitive references

image

@ericstj
Copy link
Member

ericstj commented Jan 22, 2025

I'm not sold on the value of this PR. Can you describe what issue this is fixing and how that impacts the end customer? What is the benefit the customer of these libraries will see by removing these?

As a customer of these packages - you don't need to manage the dependencies at all. The package manager does that for you.

It does have some impact on the maintainers of the libraries (EG the owners of Microsoft.Extensions.* and others). I can see how some folks might actually prefer to list their direct dependencies explicitly so they understand all the dependencies they have. Ideally we'd also have validation to let them know when there are unused dependencies - I could see value in a PR that does that.

I can see value in a change that is removing unused dependencies completely - but that would be a breaking change and require documentation.

I've mentioned before that there are actually good reasons to promote dependencies higher in the package graph - it reduces the number of round-trips NuGet needs to make on restore and it insulates the consumer from breaking changes, should any of its dependencies drop a package in a future version. Both of these are reasons for listing all dependencies.

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.

As I've mentioned I'm not sold on the value of this PR.

@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.1;netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Looks to me like this was added to make it possible to drop the BCL.AsyncInterfaces dependency. You'll need the owners of @dotnet/area-extensions-hosting to approve.

@ericstj ericstj requested a review from a team January 22, 2025 18:07
@pentp pentp closed this Jan 23, 2025
@pentp pentp deleted the ext-deps-cleanup branch January 23, 2025 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants