Fix conditional package updating in update all scenarios, add more tests for conditional package operations by nkolev92 · Pull Request #5499 · NuGet/NuGet.Client

nkolev92 · 2023-11-08T23:21:16Z

Bug

Regression? Last working version:

Description

Conditional package updating was previously fixed in fed7892.

This PR fixes the update all scenario, ensuring all packages are updated instead of just 1.
This PR also improves the tests cases for the scenarios.

Some technical details to help with the review of this PR:

BuildIntegratedProjectAction is always the result for a project action on a PackageReference project, even if there are multiple packages being updated. Note that this is not something we're adding in this PR.
For each package being updated as part of an action, the compatible frameworks should be consider with that package id in mind. That allows, Newtonsoft.Json to be updated for net472, but NuGet.Common to be updated for net5.0.
BuildIntegratedProjectAction types are not expected to be used outside of the NuGet.PackageManagement, and given how overexposed are APIs are, I'm adding these as internal. This is probably not the best design, but it avoids adding future work for us. An alternative would be to complete break the type.
Added nullable annotation for BuildIntegratedProjectAction

PR Checklist

…sts for conditional package operations

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/BuildIntegratedProjectAction.cs

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/BuildIntegratedProjectAction.cs

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs

kartheekp-ms · 2023-11-13T20:04:59Z

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/BuildIntegratedProjectAction.cs

+#pragma warning disable CS0618 // Type or member is obsolete
+            OriginalActions = originalActionsAndInstallationContexts.Select(e => e.Item1).ToList();
+            InstallationContext = originalActionsAndInstallationContexts[0].Item2;
+#pragma warning restore CS0618 // Type or member is obsolete


I'm really curious to understand why we should access the obsolete members here. We already assigned the new property ActionAndContextList with the correct value in this constructor, which isn't obsolete. And have we done the same in the constructor that is marked as obsolete in this PR.

The constructor is new, there's a new property, but that doesn't mean that the old contract should be broken.
Normally, if this wasn't a public API, I'd remove the old properties, but that's not possible without a breaking change currently.
So until then, we make sure that the type is correctly populated.

The two properties OriginalActions and InstallationContext are currently unused in the product code. If a caller uses the new constructor, we'll assign a value to the newly introduced property in this PR, ActionAndContextList, which the product code references. However, if the caller uses the old constructor, we'll generate the ActionAndContextList property using OriginalActions and InstallationContext. This new property will then be used throughout the rest of the product code. Therefore, I'm uncertain about the necessity of assigning values to the old or obsolete properties in the new constructor.

We don't know who/how people use our code unfortunately.
If people say we're not worried about that, I can remove it, but I was erring on the cautious side.

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/BuildIntegratedProjectAction.cs

test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerUtilityTests.cs

...Get.Core.Tests/NuGet.PackageManagement.Test/PackageReferenceBasedNuGetPackageManagerTests.cs

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs

nkolev92 · 2023-11-14T01:17:08Z

@kartheekp-ms
Since you're adding comments a considerable time apart, can you please let me know when you're done adding comments so I can address the feedback in a single commit?

Note that I've marked comments where my next commit will resolve the feedback as Resolved already.

kartheekp-ms · 2023-11-14T01:58:18Z

I am also wondering if we need a new (or update existing) apex test to assert the new behavior.

kartheekp-ms · 2023-11-14T01:59:53Z

@kartheekp-ms Since you're adding comments a considerable time apart, can you please let me know when you're done adding comments so I can address the feedback in a single commit?

Note that I've marked comments where my next commit will resolve the feedback as Resolved already.

I think I am done for now. Please feel free to push new commit(s).

May be out of scope for this PR. If I click on "Update" a package version in the "Installed" tab, then the versions in the csproj are not updated correctly when there are multiple target frameworks. I can open a separate bug for this scenario.

nkolev92 · 2023-11-14T02:12:40Z

I am also wondering if we need a new (or update existing) apex test to assert the new behavior.

There's no multi targeted templates, so that makes Apex tests extremely difficult to add. .NET templates in previews also require constant source updates right now as packages are not always published to nuget.org, or a source that we control. Created an issue for that https://github.com/NuGet/Client.Engineering/issues/2589.
I'll ask the vendors to add manual tests once the whole thing is complete.

nkolev92 · 2023-11-14T02:14:21Z

May be out of scope for this PR. If I click on "Update" a package version in the "Installed" tab, then the versions in the csproj are not updated correctly when there are multiple target frameworks. I can open a separate bug for this scenario.

With my changes?
It'd be great if you can give me a setup.

If you want to consider it out scope, you can just create an issue and assign it to me, and I can validate that afterwards.

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs

kartheekp-ms · 2023-11-14T18:38:23Z

With my changes?
It'd be great if you can give me a setup.

Yes Nikolche. Here are the repro steps.

Clone this PR branch locally
Build the repo
Launch the experimental instance
Create a new project
Edit the csproj manually

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net472;net8.0</TargetFrameworks>  
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>
    <ItemGroup Condition="'$(TargetFramework)' == 'net472'">
	    <PackageReference Include="Newtonsoft.Json">
		    <Version>13.0.1</Version>
	    </PackageReference>
    </ItemGroup>
    <ItemGroup Condition="'$(TargetFramework)' == 'net8.0'">
	    <PackageReference Include="Newtonsoft.Json">
		    <Version>13.0.2</Version>
	    </PackageReference>
    </ItemGroup>
</Project>

Open NuGet Package Manager UI for the project
Navigate to "Installed" tab
Click on the update icon with text 13.0.3
As you can see in the below screenshot, package version changed only for "net472" target framework.

If you want to consider it out scope, you can just create an issue and assign it to me, and I can validate that afterwards.

Please take a look at the repro steps and let me know your feedback so that we can plan accordingly.

nkolev92 · 2023-11-14T19:09:32Z

@kartheekp-ms
Turns out project-system is having issues with where the condition is:

For example, I tested a similar scenario as yours, but the conditions are on the package reference item instead, and that does work!

I'll create an issue for follow up. That change may not be on the nuget side.

nkolev92 · 2023-11-14T19:13:19Z

Actually, nevermind you have 2 frameworks in your test and I have 3.
The problem is that we don't consider the fact that all frameworks have a conditional reference.

I'll create a follow up for that.
That may be something we can detect, but I'm not sure we can have a perfect solution.

kartheekp-ms · 2023-11-20T22:58:54Z

I'll create a follow up for that.
That may be something we can detect, but I'm not sure we can have a perfect solution.

Has the follow-up issue been created?

nkolev92 · 2023-11-20T23:28:46Z

No, I was waiting for approval on this first.

I created it now NuGet/Home#13034

kartheekp-ms

There are some unresolved comments in this pull request, primarily the ones we discussed earlier. Feel free to review them, but I don't want to block this PR until those comments are addressed.

Fix conditional package updating in update all scenarios, add more te…

e91d377

…sts for conditional package operations

nkolev92 force-pushed the dev-nkolev92-conditionalInstallationInUpdateAll branch from 4ee220c to e91d377 Compare November 8, 2023 23:22

nkolev92 changed the title ~~Dev nkolev92 conditional installation in update all~~ Fix conditional package updating in update all scenarios, add more tests for conditional package operations Nov 8, 2023

nkolev92 marked this pull request as ready for review November 9, 2023 00:03

nkolev92 requested a review from a team as a code owner November 9, 2023 00:03

zivkan reviewed Nov 10, 2023

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/BuildIntegratedProjectAction.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs Show resolved Hide resolved

zivkan previously approved these changes Nov 10, 2023

View reviewed changes

move nullable location

0fa5b4a

nkolev92 dismissed zivkan’s stale review via 0fa5b4a November 10, 2023 17:11

nkolev92 requested a review from zivkan November 10, 2023 17:11

kartheekp-ms reviewed Nov 10, 2023

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/BuildIntegratedProjectAction.cs Show resolved Hide resolved

kartheekp-ms reviewed Nov 11, 2023

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/BuildIntegratedProjectAction.cs Show resolved Hide resolved

kartheekp-ms reviewed Nov 11, 2023

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs Show resolved Hide resolved

kartheekp-ms reviewed Nov 13, 2023

View reviewed changes

nkolev92 requested a review from kartheekp-ms November 13, 2023 20:35

kartheekp-ms reviewed Nov 13, 2023

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/BuildIntegratedProjectAction.cs Show resolved Hide resolved

kartheekp-ms reviewed Nov 14, 2023

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs Show resolved Hide resolved

address feedback

ccfa180

nkolev92 requested a review from kartheekp-ms November 14, 2023 02:16

donnie-msft approved these changes Nov 14, 2023

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs Show resolved Hide resolved

kartheekp-ms approved these changes Nov 21, 2023

View reviewed changes

nkolev92 merged commit ecf0277 into dev Nov 21, 2023

nkolev92 deleted the dev-nkolev92-conditionalInstallationInUpdateAll branch November 21, 2023 18:17

nkolev92 mentioned this pull request Nov 21, 2023

Conditional updating when *all* packages are conditioned is broken NuGet/Home#13034

Closed

Conversation

nkolev92 commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kartheekp-ms Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkolev92 Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

kartheekp-ms Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

nkolev92 Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkolev92 commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kartheekp-ms commented Nov 14, 2023

Uh oh!

kartheekp-ms commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkolev92 commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkolev92 commented Nov 14, 2023

Uh oh!

Uh oh!

kartheekp-ms commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkolev92 commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkolev92 commented Nov 14, 2023

Uh oh!

kartheekp-ms commented Nov 20, 2023

Uh oh!

nkolev92 commented Nov 20, 2023

Uh oh!

kartheekp-ms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nkolev92 commented Nov 8, 2023 •

edited

Loading

kartheekp-ms Nov 13, 2023 •

edited

Loading

nkolev92 commented Nov 14, 2023 •

edited

Loading

kartheekp-ms commented Nov 14, 2023 •

edited

Loading

nkolev92 commented Nov 14, 2023 •

edited

Loading

kartheekp-ms commented Nov 14, 2023 •

edited

Loading

nkolev92 commented Nov 14, 2023 •

edited

Loading

kartheekp-ms left a comment •

edited

Loading