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

Disable cpvm transitive pinning. #3719

Merged
merged 4 commits into from
Oct 16, 2020
Merged

Conversation

cristinamanum
Copy link
Contributor

@cristinamanum cristinamanum commented Oct 14, 2020

Bug

Fixes: This change disable the enforcing/pinning of package versions for transitive dependencies defined in the CentralManagement file.
NuGet/Home#10132

Regression: No

Fix

Disable transitive dependency pinning.

Testing/Validation

Tests Added: Yes
Validation: Automated and manual tests.

@cristinamanum cristinamanum marked this pull request as ready for review October 15, 2020 21:19
@cristinamanum cristinamanum requested a review from a team as a code owner October 15, 2020 21:19
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

minor things, nitpicks really. Keep it in mind for furture changes, but I'm fine with this being merged as-is.

@@ -9795,7 +9795,7 @@ public async Task RestoreNetCore_CPVMProject_DowngradedByCentralDirectDependency
/// P will be accepted (because its parent B is Accepted)
/// S will be accepted (because its parent O 300 is Accepted)
/// </summary>
[Fact]
[Fact(Skip = "Depends on cpvm transitive pinning")]
Copy link
Member

Choose a reason for hiding this comment

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

We usually include a link to a tracking issue either as the skip reason, or as a comment next to the skipped test. This makes it easier to figure out when a test can be unskipped, or maybe deleted permanently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

var solution = new SimpleTestSolutionContext(pathContext.SolutionRoot);
var packagesForSource = new List<SimpleTestPackageContext>();
var packagesForProject = new List<SimpleTestPackageContext>();
var framework = NuGetFramework.Parse("netcoreapp2.0");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var framework = NuGetFramework.Parse("netcoreapp2.0");
var framework = FrameworkConstants.CommonFrameworks.NetCoreApp20;

public static readonly NuGetFramework NetCoreApp20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

var projectA = SimpleTestProjectContext.CreateNETCore(
"projectA",
pathContext.SolutionRoot,
NuGetFramework.Parse("netcoreapp2.0"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
NuGetFramework.Parse("netcoreapp2.0"));
framwork);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Change looks good.

Make sure you link an issue for the PR + an issue for the follow-up.

@cristinamanum
Copy link
Contributor Author

Make sure you link an issue for the PR + an issue for the follow-up.

Done

@cristinamanum cristinamanum merged commit 087a711 into dev Oct 16, 2020
@cristinamanum cristinamanum deleted the dev-cmanu-revertcpvmtransitivepin branch October 16, 2020 16:52
kartheekp-ms pushed a commit that referenced this pull request Oct 17, 2020
Disable cpvm transitive pinning.
cristinamanum added a commit that referenced this pull request Oct 27, 2020
Disable cpvm transitive pinning.
marcin-krystianc added a commit to marcin-krystianc/NuGet.Client that referenced this pull request Apr 26, 2021
This reverts commit 087a711.

# Conflicts:
#	test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreNETCoreTest.cs
marcin-krystianc added a commit to marcin-krystianc/NuGet.Client that referenced this pull request Apr 28, 2021
@KirillOsenkov
Copy link
Contributor

Hi all, I've just spent a few hours debugging a build break and root caused it as this change breaking things. For the future, it would be really nice to avoid breaking things and just provide an option to the users who need to turn it off. The default transitive pinning behavior was good for the vast majority of cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants