Add option to override transitive dependency versions when managing packages centrally #4025

marcin-krystianc · 2021-04-28T12:00:40Z

Bug

Fixes: NuGet/Home#10389
Fixes: NuGet/Home#10327

Regression? Last working version: 5.8 (feature removed in v5.9)

Description

This PR reverts the change that disabled the transitive dependency pinning feature. Additionally it is being now made optionable, so it will be possible to use CPVM (Central Package Version Management) with transitive dependency pinning or without it.

Project can opt-in for transitive dependency pinning by defining property:

    <CentralPackageTransitivePinningEnabled>true</CentralPackageTransitivePinningEnabled>

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
Documentation
- Documentation PR or issue filled: https://github.com/NuGet/docs.microsoft.com-nuget/issues/2674

marcin-krystianc · 2021-05-24T07:39:42Z

ping

stackedsax · 2021-06-17T15:38:28Z

@aortiz-msft any chance you'll be able to look at this one? I noticed @zivkan assigned this to you to look at some time back. Seems like the feature would be to the community's benefit.

marcin-krystianc · 2021-07-05T09:57:00Z

Recent comments on the NuGet/Home#6764 show, that there is a demand for this feature to be brought back.
It is important for us to get it in before .NET 6 release, otherwise we are going to be stuck with .NET 3.1 (which is the last version in which it was available).
What can we do to help reviewing it?

stackedsax · 2021-07-05T16:21:15Z

Yeah, maybe 20+ comments in the past week and lots more thumbs up to many of those comments. Seems like there's plenty of community interest in getting this feature back in. @aortiz-msft, @zivkan, anything more we can do to help push this one along?

jjmanton · 2021-07-25T18:59:02Z

@marcin-krystianc @nkolev92 Is this PR still under review? We would very much like to pin transitive dependencies

stackedsax · 2021-08-12T15:49:50Z

@jjmanton we'd certainly like it still be under review. @nkolev92, are you able to take a look and give us some feedback? We'll look for a different assignee if you are unavailable. Thanks!

stackedsax · 2021-08-12T15:53:59Z

@aortiz-msft , you were on the original #3719 issue -- do you have any thoughts here?

@zivkan, you've offered some good feedback for some of our other PR's in NuGet.Client -- is there anyone else we should direct this PR to?

@rconard, @cristinamanum, any thoughts here?

Any and all comments would be appreciated -- we'd just like to keep the conversation on this one moving!

avivanoff · 2021-08-25T16:27:43Z

There is quite some activity on the NuGet/Home#10389. This feature is definitely needed, and is quite critical for us.

lazy8 · 2021-09-08T08:10:37Z

Hi @nkolev92 is there still a chance for this PR to get accepted? Is there something anybody else could/should do? My company (and others too apparently) are pretty much stuck with nuget 5.8 (and respective dotnet version 5.0.1XX) until this gets re-enabled/fixed.

lazy8 · 2021-09-21T06:59:39Z

Hello @nkolev92, could you please review this merge request? Or assign it to somebody who can? Thanks.

stackedsax · 2021-09-23T15:43:11Z

@nkolev92 There a couple of us who would love some sort of signal on this one. We'd love this to be included .NET 6, and we're worried that it will miss that deadline.

@zivkan, any input on this one?

martincostello · 2021-09-23T15:46:17Z

As an external observer, with them already having shipped RC1 and the bar for inclusion being higher and higher for RC2 and GA, realistically I think the ship has already sailed on this being part of .NET 6.

stackedsax · 2021-09-23T15:48:33Z

As an external observer, with them already having shipped RC1 and the bar for inclusion being higher and higher for RC2 and GA, realistically I think the ship has already sailed on this being part of .NET 6.

Yup, but I must keep the sliver of hope alive!

More importantly, I'd just like to get some eyes on this so that it lands at some point. It looks like there are plenty of people who want this feature back.

batzen · 2021-09-23T17:17:48Z

@martincostello Transitive version pinning was removed in a minor patch version in .NET 5, so i don't think there is even a bar. The group which was invited and supposed to test and give feedback on CPVM wasn't even asked before it was suddenly removed.
The docs the expert panel group gave feedback on still state:

Transitive Dependencies
If any packages are imported as transitive dependencies from a <PackageReference> where the Version is controlled by a <PackageVersion> in Directory.Packages.props, then the version of the transitive dependency will be controlled by CPVM.

FYI: That's a quote from https://aka.ms/CentralNuGetLearningDocs

And https://github.com/NuGet/Home/wiki/Centrally-managing-NuGet-package-versions also says:

Transitive dependencies: The versions for the packages defined in the Directory.Packages.props will win for direct and transitive dependency resolution.

So currently CPVM does not behave as described in all docs available.

@rconard Why do we still have to wait for this PR to be merged? Not being able to define versions for transitive dependencies in CPVM causes issues all the time and we have to add direct dependencies to projects that don't require direct dependencies.

batzen · 2021-09-24T03:43:41Z

@aortiz-msft Russel said you might be able to help here as he moved to Azure IoT.

lazy8 · 2021-10-02T07:39:24Z

@aortiz-msft Russel said you might be able to help here as he moved to Azure IoT.

Hello @aortiz-msft, @nkolev92,
another week had passed without any kind of reaction (Plenty of other activities though).
Makes it now 157 days in total.
Really not sure what to make of this.

stackedsax · 2021-10-16T23:40:32Z

Just wanted to note that @JonDouglas did comment on the associated issue last week:

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

Hopefully, this is a good sign!

marcin-krystianc · 2021-11-16T11:49:54Z

Given that vast majority of votes is in favour of transitive dependency pinning being enabled by default, I'm going to update my branch to reflect that:

Are there any other concerns or requests for this features which are not reflected in my PR yet?

JonDouglas · 2021-11-16T14:28:02Z

Hey @marcin-krystianc,

We're finalizing a longer-term plan for this feature going into the new year(I owe the other thread an update). If you would like to take the time to reflect the recent consensus, please feel free but keep in mind that we're still planning to have a team member review the PR in-depth when they finish up with existing commitments. Thank you!

JonDouglas · 2021-11-16T19:13:39Z

Hi all,

We recently published a small document/proposal that includes a few thoughts around this behavior & some questions that would further help us move this feature forward. If would mean a lot if you can provide your feedback in the PR itself:

#11391

Thanks!

…PackageVersionOverrideDisabled

…ransitivepinning # Conflicts: # src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/LegacyPackageReferenceProject.cs # src/NuGet.Clients/NuGet.SolutionRestoreManager/VSNominationUtilities.cs # src/NuGet.Clients/NuGet.SolutionRestoreManager/VsSolutionRestoreService.cs # src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildStaticGraphRestore.cs # src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets # src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/MSBuildRestoreUtility.cs # src/NuGet.Core/NuGet.Common/PublicAPI/net45/PublicAPI.Unshipped.txt # src/NuGet.Core/NuGet.Common/PublicAPI/net472/PublicAPI.Unshipped.txt # src/NuGet.Core/NuGet.Common/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt # src/NuGet.Core/NuGet.ProjectModel/JsonPackageSpecReader.cs # src/NuGet.Core/NuGet.ProjectModel/PackageSpecWriter.cs # src/NuGet.Core/NuGet.ProjectModel/ProjectRestoreMetadata.cs # src/NuGet.Core/NuGet.ProjectModel/PublicAPI.Unshipped.txt # test/NuGet.Core.Tests/NuGet.Commands.Test/MSBuildRestoreUtilityTests.cs

jeffkl

Thank you for the contribution. Overall this looks great and has good test coverage, just minor comments. Let's get feedback on the property name and I think we're good.

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

jeffkl · 2022-03-01T19:50:47Z

src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs

+                if (packageSpec.RestoreMetadata?.CentralPackageVersionsEnabled == true &&
+                    packageSpec.RestoreMetadata?.TransitiveDependencyPinningEnabled == true)


Let's get rid of these old comparisons to true

Suggested change

if (packageSpec.RestoreMetadata?.CentralPackageVersionsEnabled == true &&

packageSpec.RestoreMetadata?.TransitiveDependencyPinningEnabled == true)

if (packageSpec.RestoreMetadata?.CentralPackageVersionsEnabled &&

packageSpec.RestoreMetadata?.TransitiveDependencyPinningEnabled)

Both terms are nullable so the comparison to true is rather necessary.

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreNETCoreTest.cs

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs

test/NuGet.Core.Tests/NuGet.Commands.Test/MSBuildRestoreUtilityTests.cs

src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildStaticGraphRestore.cs

test/NuGet.Core.Tests/NuGet.Build.Tasks.Console.Test/MSBuildStaticGraphRestoreTests.cs

jeffkl · 2022-03-01T20:09:47Z

@NuGet/nuget-client this PR is now ready for review. Please leave your feedback on the property name we'll use to enable the feature.

marcin-krystianc

@jeffkl Thanks for your review, I've responded to all suggested changes. so It is ready for another review.

marcin-krystianc · 2022-03-02T08:48:11Z

src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs

+                if (packageSpec.RestoreMetadata?.CentralPackageVersionsEnabled == true &&
+                    packageSpec.RestoreMetadata?.TransitiveDependencyPinningEnabled == true)


Both terms are nullable so the comparison to true is rather necessary.

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

test/NuGet.Core.Tests/NuGet.Commands.Test/MSBuildRestoreUtilityTests.cs

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

jeffkl

Thanks again for the contribution!

nkolev92

Thanks for the hard work @marcin-krystianc

marcin-krystianc requested a review from a team as a code owner April 28, 2021 12:00

marcin-krystianc mentioned this pull request Apr 28, 2021

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

Closed

donnie-msft added the Community PRs created by someone not in the NuGet team label Apr 28, 2021

zivkan assigned aortiz-msft May 24, 2021

swythan mentioned this pull request Jul 3, 2021

[PackageReference] Centrally manage NuGet package versions for a solution or a repo NuGet/Home#6764

Closed

aortiz-msft assigned nkolev92 and unassigned aortiz-msft Jul 8, 2021

CZEMacLeod mentioned this pull request Sep 1, 2021

Transitive dependency pinning microsoft/MSBuildSdks#283

Closed

nkolev92 assigned jeffkl and unassigned nkolev92 Jan 11, 2022

jeffkl added 4 commits February 25, 2022 10:18

Move new warning to correct range

a5515b1

Address comments

65a84e5

Address comments

76b5b4d

Implementing IsTransitiveDependencyPinningEnabled on top of IsCentral…

8d4cf6d

…PackageVersionOverrideDisabled

marcin-krystianc force-pushed the dev-marcink-20210428-transitivepinning branch from 6a88650 to 8d4cf6d Compare February 28, 2022 12:51

marcin-krystianc added 2 commits March 1, 2022 11:02

adding more tests

0d98d88

marcin-krystianc force-pushed the dev-marcink-20210428-transitivepinning branch from 05001df to 0d98d88 Compare March 1, 2022 16:02

jeffkl added Approved for CI and removed Approved for CI labels Mar 1, 2022

jeffkl changed the title ~~CPVM - Resurrect the "transitive dependency pinning" feature and make it optionable~~ Add option to override transitive dependency versions when managing packages centrally Mar 1, 2022

jeffkl marked this pull request as ready for review March 1, 2022 19:56

jeffkl reviewed Mar 1, 2022

View reviewed changes

jeffkl mentioned this pull request Mar 1, 2022

[Epic]: Central Package Management Improvements for 17.2 NuGet/Home#11504

Closed

15 tasks

marcin-krystianc added 2 commits March 2, 2022 12:08

PR remarks

5877383

diff noise

db209ed

marcin-krystianc commented Mar 2, 2022

View reviewed changes

Property name

5f5981c

jeffkl added Approved for CI and removed Approved for CI labels Mar 2, 2022

jeffkl approved these changes Mar 2, 2022

View reviewed changes

nkolev92 approved these changes Mar 2, 2022

View reviewed changes

marcin-krystianc mentioned this pull request Mar 4, 2022

[Bug]: CPVM - NuGet.exe doesn't include central transitive dependencies in the nuspec of the created package NuGet/Home#11647

Closed

jeffkl merged commit d059010 into NuGet:dev Mar 8, 2022

marcin-krystianc deleted the dev-marcink-20210428-transitivepinning branch March 9, 2022 07:37

opheoHeidrich mentioned this pull request Mar 11, 2022

[Feature]: CentralPackageTransitivePinningEnabled should work with the newest Version of the SDK NuGet/Home#11668

Closed

jeffkl mentioned this pull request Mar 31, 2022

Pass CentralPackageTransitivePinningEnabled and CentralPackageVersionOverrideEnabled to NuGet restore dotnet/project-system#8028

Merged

marcin-krystianc mentioned this pull request Apr 6, 2022

[Bug]: Metadata like PrivateAssets does not flow from parent to transitively pinned dependency in CPM NuGet/Home#10311

Closed

jeffkl mentioned this pull request Nov 15, 2022

[Epic]: Central Package Management improvements for 17.4 NuGet/Home#11952

Closed

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to override transitive dependency versions when managing packages centrally #4025

Add option to override transitive dependency versions when managing packages centrally #4025

marcin-krystianc commented Apr 28, 2021 •

edited by jeffkl

Loading

marcin-krystianc commented May 24, 2021

stackedsax commented Jun 17, 2021

marcin-krystianc commented Jul 5, 2021

stackedsax commented Jul 5, 2021 •

edited

Loading

jjmanton commented Jul 25, 2021

stackedsax commented Aug 12, 2021

stackedsax commented Aug 12, 2021

avivanoff commented Aug 25, 2021

lazy8 commented Sep 8, 2021

lazy8 commented Sep 21, 2021

stackedsax commented Sep 23, 2021

martincostello commented Sep 23, 2021

stackedsax commented Sep 23, 2021

batzen commented Sep 23, 2021

batzen commented Sep 24, 2021

lazy8 commented Oct 2, 2021

stackedsax commented Oct 16, 2021

marcin-krystianc commented Nov 16, 2021

JonDouglas commented Nov 16, 2021

JonDouglas commented Nov 16, 2021 •

edited

Loading

jeffkl left a comment

jeffkl Mar 1, 2022

marcin-krystianc Mar 2, 2022

jeffkl commented Mar 1, 2022

marcin-krystianc left a comment

marcin-krystianc Mar 2, 2022

jeffkl left a comment

nkolev92 left a comment

		if (packageSpec.RestoreMetadata?.CentralPackageVersionsEnabled == true &&
		packageSpec.RestoreMetadata?.TransitiveDependencyPinningEnabled == true)

Add option to override transitive dependency versions when managing packages centrally #4025

Add option to override transitive dependency versions when managing packages centrally #4025

Conversation

marcin-krystianc commented Apr 28, 2021 • edited by jeffkl Loading

Bug

Description

PR Checklist

marcin-krystianc commented May 24, 2021

stackedsax commented Jun 17, 2021

marcin-krystianc commented Jul 5, 2021

stackedsax commented Jul 5, 2021 • edited Loading

jjmanton commented Jul 25, 2021

stackedsax commented Aug 12, 2021

stackedsax commented Aug 12, 2021

avivanoff commented Aug 25, 2021

lazy8 commented Sep 8, 2021

lazy8 commented Sep 21, 2021

stackedsax commented Sep 23, 2021

martincostello commented Sep 23, 2021

stackedsax commented Sep 23, 2021

batzen commented Sep 23, 2021

batzen commented Sep 24, 2021

lazy8 commented Oct 2, 2021

stackedsax commented Oct 16, 2021

marcin-krystianc commented Nov 16, 2021

JonDouglas commented Nov 16, 2021

JonDouglas commented Nov 16, 2021 • edited Loading

jeffkl left a comment

Choose a reason for hiding this comment

jeffkl Mar 1, 2022

Choose a reason for hiding this comment

marcin-krystianc Mar 2, 2022

Choose a reason for hiding this comment

jeffkl commented Mar 1, 2022

marcin-krystianc left a comment

Choose a reason for hiding this comment

marcin-krystianc Mar 2, 2022

Choose a reason for hiding this comment

jeffkl left a comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

marcin-krystianc commented Apr 28, 2021 •

edited by jeffkl

Loading

stackedsax commented Jul 5, 2021 •

edited

Loading

JonDouglas commented Nov 16, 2021 •

edited

Loading