Prefer updating only the conditioned packages when updating in VS and dotnet add package #5420
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nkolev92
changed the title
jeffkl
approved these changes
Sep 23, 2023
|
I have reviewed the changes and about to click approve but noticed that PR has been merged. The proposed changes look good👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/2237
Covers large parts of NuGet/Home#4681 - due to the urgency of this fix, I'll validate all scenarios in that issues have been addressed afterwards and I'll have any follow up fixes if needed.
Regression? Last working version:
Description
Note that this started as a larger change, so I had written up a spec on this, however as I was implementing it and adding tests I discovered the accidental bug introduced in 4.4! You can read more in depth in #5420, but it should not be necessary to review this PR.
For a conditionally installed PackageReference, NuGet does not update conditional reference correctly consistenly. Currently all updates of conditional packages lead to attempting to install the package to all frameworks, thus often causing an NU1504 warning. The change here is for the update gesture to update a package only in the frameworks it's installed in.
Note that this behavior appears to be a regression of f9569ac.
I verified that
1.1.14of .NET SDK does not have the problem at hand. It is extremely likely that Visual Studio did not have the same issues for updating, but very likely had issues where no-op was constantly incorrect after a package installation.The preview step boils down to a call to
PackageSpecOperation.AddOrUpdate(..).In f9569ac, the AddOrUpdate call was changed to always add to all framework. That was done because the AddOrUpdate without frameworks did not update the correct list for PackageReference based projects.
PackageReference based projects always use the TargetFrameworkInformation object dependency list.
The correct fix is to update the original AddOrUpdate without frameworks method to account for PackageReference based projects.
dotnet add package.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation