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

[Feature]: Add option to allow versions of transitive dependencies to be overridden #10389

Closed
marcin-krystianc opened this issue Dec 17, 2020 · 26 comments · Fixed by NuGet/NuGet.Client#4025
Assignees
Labels
Area:RestoreCPM Central package management Functionality:Restore Priority:1 High priority issues that must be resolved in the current sprint. Type:Feature

Comments

@marcin-krystianc
Copy link

Recently we've noticed a PR that disables the transitive pinning behaviour for CPVM.
It worries us because we started to rely on that feature (or rather on the unintentional side effect of that feature).

So the problem we are struggling with is the slowness of the restore operation. For large solutions that contain many projects and reference many packages the time needed for restore operation becomes unacceptable (see #10030 for details). As it is explained in the linked issue, the slowness is caused by the exponential nature of the graph walking algorithm.

We've discovered that enabling the CPVM for such solutions makes it to restore much quicker. It turns out that transitive pinning helps to reduce the impact of exponential nature of the graph walking algorithm. Because of the transitive pinning the graph walking needs to traverse the dependency graph of each centrally managed package only once. It happens only when a package node is visited from the top level node and it is skipped when visited from all other nodes (due to the "Nearest wins" rule).

I know that transitive pinning is a controversial feature. Some users want it but others really don't. But do you consider bringing it back and making it an opt-in feature as it was suggested in the comments of #10115 ? It is really important for us, so I'm happy to work on making this change if you are willing to accept it.

@rconard
Copy link

rconard commented Dec 17, 2020

@marcin-krystianc, this is our current plan. The actual implementation may be a little rough as we get the pieces in place, but we're working on it. 👍

@zkat zkat added the Priority:2 Issues for the current backlog. label Jan 7, 2021
@jimmylewis
Copy link

Throwing my vote on this too, hoping to see the priority bumped up. Honestly, I don't know where else to file it, but PackageReference has been completely unusable in my team's repo for the last several years due to the complex (and disorganized) nature of our dependencies. CPVM with transitive dependencies was our hope to finally make the move back to standardized tooling for our dependency management in a way that would not be grossly inconvenient for our developers.

For context, my team depends on several components coming from several other teams each in their own repos which all publish to a variety of feeds (our upstream list is impressive). This results in a lot of complex dependency conflicts. The options to resolve those are to:
(a) add any conflicting dependencies as primary to each project in order to force resolution of the package version (which really sucks, because it comes up all the time in new ways, and really devaules PackageReference in general),
(b) try to brute-force the package versions to get to a coherent graph (which really sucks, because it's basically a trial and error process, we have literally had developers spend multiple days trying to update a single package and resolve the ensuing conflicts, sometimes without ever finding a successful combination), or
(c) build a custom mechanism other than PackageReference to manage our binary dependencies (which really sucks, because it doesn't handle any other asset types, unless we basically reproduce a system to replace all of NuGet's functionality).

We've been living with (c) for a very long time, but it's a recurring tax to maintain and doesn't work with any .NET tooling features. I've already spent over a week of time trying to move to PackageReference with CPVM on the basis that it would solve our transitive version problems (because that's how the CPVM spec reads). Having transitive pinning removed explains a lot of the difficulties I've been encountering.

@tompazourek
Copy link

tompazourek commented Mar 5, 2021

I have updated from Visual Studio 2019 16.8 to 16.9 (released this week) and transitive pinning doesn't seem to work anymore and lot of stuff broke just by updating VS on developers' machines (because the version of the MSBuild, which contains NuGet changes).

@cristinamanum Was the transitive pinning disable rolled out as part of Visual Studio 16.9?

The main issue for us is that the projects in solution that reference a package directly get one version, and projects that reference a package indirectly get different version, which results in package downgrades and we're back in the dependency hell... And that is even though the version is fixed in Directory.Packages.props.

Also note that the documentation still doesn't reflect the changes, the docs still say that the transitive dependencies are pinned: https://github.com/NuGet/Home/wiki/Centrally-managing-NuGet-package-versions If you have some, thanks for any information or links on the topic, it's a bit difficult to find up-to-date info...

Anyway, transitive dependency pinning is a great feature, and one of the main reasons why we adopted CPVM. So it would be awesome to have it opt-in.

@marcin-krystianc
Copy link
Author

Hi @rconard,

are you actively working on bringing back the transitive pinning and making it opt-in feature ? If not, I'll start working on it next week. Any guidance on the implementation details will be very appreciated.

@rconard
Copy link

rconard commented Mar 25, 2021

I will defer to @nkolev92 and @zivkan for implementation details.

I believe that we laid the groundwork with #3719 to bring this functionality back as we implemented a feature flag that could evolve into the opt-in feature that you proposed.

@rconard rconard assigned nkolev92 and zivkan and unassigned rconard and cristinamanum Mar 25, 2021
@zivkan
Copy link
Member

zivkan commented Mar 29, 2021

@marcin-krystianc The NuGet team currently has other work as a higher priority, so we're not actively working on CPVM at this time.

As for guidance, I assume that the opt-in will be at a per-project level. Our msbuild property gathering and reading is spread across NuGet.targets and MSBuildStaticGraphRestore. Once the work here is done, we'll also need to create a PR on the dotnet/project-system to tell it to pass through any new properties in Visual Studio. But only do that when the PR for NuGet is done, or very, very close to done and we're certain we're not going to rename the msbuild property.

The PR you linked shows where the transitive dependencies code was #if def-ed out. Then use the debugger to see the call-stack/program flow to get from nuget.exe restore, RestoreTask.cs (for msbuild/dotnet restore), MSBuildStaticGraphRestore.cs, and VsSolutionRestoreService.cs, through to the dependency resolver code. Avoid public API changes, where you can, and introduce overloads to avoid breaking changes where you can't. As you see, additions to restore are a pain, because there are at least 4 entry-points where new msbuild properties need to be read and flowed through to RestoreCommand. We can provide assistance as the implementation progresses, but ultimately these 4 entry points will need to be tested.

Otherwise, I don't have any special insights/guidance.

If anyone thinks that maybe there should be a nuget.config setting, rather than a per-project setting, then we probably need a design spec to discuss the various options and decide before wasting too much time on an implementation that gets partially thrown away if we decide to go down another route. A nuget.config setting would make the implementaton significantly easier (and avoid needing a PR on project-system). But I'm not sure if transitive pinning makes sense as a per-project setting, or a nuget.config setting, or both. Maybe this needs customer development and therefore PM work, and therefore shouldn't have any implementation started until we've decided how the opt-in should work. Maybe we do need a design spec first.

@batzen
Copy link

batzen commented Apr 6, 2021

@zivkan Why were the changes made in the first place?
Whats the purpose of CPVM when we can't manage versions centrally anymore?
If some "internal customers" don't want transitive version pinning, why are they using CPVM in the first place?

@zivkan
Copy link
Member

zivkan commented Apr 6, 2021

The discussion is in #10115. As you can see, the people who dislike the functionality are not Microsoft employees.

@tompazourek
Copy link

tompazourek commented Apr 7, 2021

@batzen I think that without transitive dependency pinning, CPVM essentially becomes just a very simple cosmetic feature, just like syntactic sugar. It's just to save you repeating the version number in many places. It's still a little bit useful, but not very much.

It's the transitive dependency pinning that actually solved a difficult problem, and that is to get the versions of the dependencies under control and get out of the dependency hell in complex many-project solutions. Without transitive dependency pinning, you do actually use different versions of packages in different projects. For example, your test projects might end up using a different version of a library, because some direct dependency is missing somewhere in the chain (basically you can be testing something different than what you're running)... Transitive dependency pinning simplifies dependency management greatly for large complex solutions. It makes it much less fragile. With it, when someone adds a single <PackageReference> to a single project, you have more confidence that it doesn't change the dependency trees and resolved versions as much in unexpected places.

But some library authors didn't like that it changed the dependency surface of their libraries, and so it was disabled entirely without a way to opt-in, and now no one can have the nice things. 😞 I believe CPVM is the most useful for really large solutions with many projects, and that it's much more useful for applications than for libraries. And that's because applications need to have the runtime binding redirects set up correctly, and you actually have full control over the dependencies. For libraries, it's still the consumer of the library who has the full control, so the transitive dependency pinning isn't that useful. You can set it up in some way, but depending on the consumer of the library, the actual dependency graphs and assemblies that end up in runtime might look completely different.

I believe the decision to disable transitive dependency pinning entirely without providing an opt-in was a rushed decision and also a wrong decision to make. Just because transitive dependency pinning isn't a perfect fit for library authors, doesn't mean that it's not useful anywhere. That's why I'd love to see a comeback of that feature. And from what I read in these threads about CPVM, many other people actually do find it useful as well.

@jimmylewis
Copy link

jimmylewis commented Apr 7, 2021

Whole-heartedly agree with @tompazourek. Transitive pinning is the hugely appealing for managing dependencies; without it, trying to micro-manage dependency versions eventually leads to adding layers of transitive dependencies as primary references, which is little better than packages.config.

If it's just pinning primary reference versions, that problem was already solved a number of ways (maybe less aesthetically pleasing than the Directory.Packages.props file, but solved nonetheless).

@marcin-krystianc
Copy link
Author

I've finally made the change to bring the transitive dependency feature back, go to NuGet/NuGet.Client#4025 for more details

@zivkan zivkan assigned aortiz-msft and unassigned nkolev92 and zivkan May 24, 2021
@zivkan zivkan added this to the Sprint 2021-05 milestone May 24, 2021
@tebeco
Copy link

tebeco commented Jan 2, 2022

As long as it's optional that's fine.
A platform can only grow better when customer can choose what they want to opt-in / opt-out

@jeffkl jeffkl modified the milestones: Sprint 2021-11, Sprint 2022-01 Jan 3, 2022
@jeffkl jeffkl changed the title [CPVM] Can the transitive pinning be made an opt-in feature ? [CPM] Add option to allow versions of transitive dependencies to be overridden Jan 12, 2022
@jeffkl jeffkl changed the title [CPM] Add option to allow versions of transitive dependencies to be overridden [Feature]: Add option to allow versions of transitive dependencies to be overridden Jan 12, 2022
@jeffkl jeffkl added Type:Feature Area:RestoreCPM Central package management Pipeline:Backlog Functionality:Restore Priority:1 High priority issues that must be resolved in the current sprint. and removed Area:RestoreCPMTransitiveDependencies Triage:Discussions Priority:2 Issues for the current backlog. labels Jan 12, 2022
@JonDouglas
Copy link
Contributor

Hi friends,

We're getting closer to finalized PRs for both version overrides and optional transitive pinning. We'd really appreciate any last comments on the following proposal before we merge it as it has some limitations you all may be aware of for transitive pinning scenarios.

#11569

@batzen
Copy link

batzen commented Feb 18, 2022

optional transitive pinning

Does that wording mean transitive pinning is off by default?
That would be very surprising as the community voted for on by default.

@tebeco
Copy link

tebeco commented Feb 19, 2022

it means that people that voted against as part of the community will be able to have a choice.

This is a win/win.
You want it ? it's on by default
you don't want it ? there a flag to disable it

What would be the concern that it serves more people ?

@forki
Copy link

forki commented Feb 19, 2022

2022 and pinning deps is still controversial in .NET. 😂

@lazy8
Copy link

lazy8 commented Feb 20, 2022

@batzen

optional transitive pinning

Does that wording mean transitive pinning is off by default?

That was also my first knee-jerk reaction and had me scratching my head a bit ... but if you read the proposal, it talks about an opt-in to override the central version (via VersionOverride). I read that as: Transitive pinning will actually be the default, but you can opt-in to override that at project-level if you need/want. And, in addition, you can centrally disallow the use of that option. This would be perfectly in line with the results of @JonDouglas's poll taken above, that you were referring to. But please correct me if I misunderstood it.

@Smartisa
Copy link

Smartisa commented Mar 7, 2022

There's another question that bothers me for a long time. Is there a way to know the top-level dependencies from "packages. config"?
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:RestoreCPM Central package management Functionality:Restore Priority:1 High priority issues that must be resolved in the current sprint. Type:Feature
Projects
None yet