dotnet add package cli support for cpm projects #4700

pragnya17 · 2022-06-23T22:49:24Z

Bug

Description

This feature is based on the design spec: NuGet/Home#11915. The goal was to allow the dotnet add package command to be executed for projects that were onboarded to CPM. Previously, the command threw a NU1008 error whenever packages were attempted to be added to projects onboarded to CPM. However, with this change users will now be able to add or update package references in projects onboarded to CPM. Directory.Packages.props files and project files are now modified accordingly whenever a package is added/updated.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- Functional test that checks if a package version is added to the props file correctly when there are no existing package versions in the props file already. It also checks that the package reference is added to the project file and that it does not have the package version metadata included in it.
- Functional test that checks if a package version is added to the props file correctly when there already other existing package versions in the props file. It also checks that the package reference is added to the project file and that it does not have the package version metadata included in it.
- Functional test that checks if a package version is updated with the correct package version attribute in the Directory.Packages.props file.
Documentation
- Documentation PR or issue filled: Document dotnet add package support for CPM projects Home#13480

test/NuGet.Core.Tests/NuGet.ProjectModel.Test/PackageSpecOperationsTests.cs

src/NuGet.Core/NuGet.ProjectModel/PackageSpecOperations.cs

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

test/NuGet.Core.Tests/NuGet.ProjectModel.Test/PackageSpecOperationsTests.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs

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

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs

nkolev92

Great job so far.
A few suggestions to improve clarity and I think we might be missing some scenarios.

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs

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

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs

nkolev92

This is looking great!

Let's add some no-op and negative tests.

In particular:

What if I specify a --version where the PackageVersion is already in the DBP.
We should no-op in this case, or do whatever the dotnet add package --version command does on a standard PackageReference project.

Think about the no-op version of every scenario you implemented.

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

pragnya17 · 2022-07-06T21:25:28Z

This is looking great!

Let's add some no-op and negative tests.

In particular:

What if I specify a --version where the PackageVersion is already in the DBP. We should no-op in this case, or do whatever the dotnet add package --version command does on a standard PackageReference project.

Think about the no-op version of every scenario you implemented.

I assume that by DBP you mean the Directory.Build.props file? However, in the scenarios that I have implemented I assume that the DBP does not exist since it is out of the scope of my project. In this comment, Jeff also mentions that the Directory.Packages.props file is mandatory for onboarding to CPM: NuGet/Home#11903 (comment) .

I am happy to create a follow up work item to ensure that we fallback onto existing behavior for the scenario where the Directory.Packages.props file does not exist.

nkolev92 · 2022-07-06T21:26:35Z

I meant to write if the version is already in DPP (Directory.Packages.props).

I don't really care about the scenarios where the project is not onboarded.

pragnya17 · 2022-07-06T22:02:00Z

I meant to write if the version is already in DPP (Directory.Packages.props).

I don't really care about the scenarios where the project is not onboarded.

Makes sense. I actually did already handle the scenario you were talking about (#4700 (review)), I just did not name it properly, so I have now updated the name of the test. For the first 4 scenarios that I implemented, after discussing with @kartheekp-ms and reviewing the spec again (https://github.com/NuGet/Home/blob/dev-kartheekp-ms-cpmaddpackagesupport-revised/proposed/2022/cpm-dotnet-cli-add-package-support.md), I don't believe there are any other no-op/fail scenarios that need to be included in the integration tests.

kartheekp-ms

Great job. I know CI is failing on other platforms. I will create a separate issue to track that work. Given that this work is getting merged into a feature branch, I will fix the CI scripts before creating a new pull request for merging these changes into the dev branch.

kartheekp-ms · 2022-07-08T03:54:04Z

test/NuGet.Core.Tests/NuGet.CommandLine.Xplat.Tests/MSBuildAPIUtilityTests.cs

+            Assert.Contains(@$"<PackageReference Include=""X"" />", File.ReadAllText(Path.Combine(testDirectory, "projectA.csproj")));
+            Assert.DoesNotContain(@$"<Version = ""1.0.0"" />", File.ReadAllText(Path.Combine(testDirectory, "projectA.csproj")));


I think we can slightly improve the performance by reading text from the project only once.

Agreed, will fix that.

test/NuGet.Core.Tests/NuGet.CommandLine.Xplat.Tests/MSBuildAPIUtilityTests.cs

This reverts commit 4e49d93.

kartheekp-ms · 2022-07-12T01:10:01Z

@pragnya17 - I have updated MSBuildAPIUtilityTests.cs to run only on Windows. All CI checks passed now for this PR. Please feel to merge this pull request. thanks

pragnya17 · 2022-07-12T22:08:38Z

test/NuGet.Core.Tests/NuGet.CommandLine.Xplat.Tests/MSBuildAPIUtilityTests.cs

-            Assert.DoesNotContain(@$"<Version = ""1.0.0"" />", File.ReadAllText(Path.Combine(testDirectory, "projectA.csproj")));
+            string updatedProjectFile = File.ReadAllText(Path.Combine(testDirectory, "projectA.csproj"));
+            Assert.Contains(@$"<PackageReference Include=""X"" />", updatedProjectFile);
+            Assert.DoesNotContain(@$"<Version = ""1.0.0"" />", updatedProjectFile);


This always return true, so I think it should be changed to $"PackageReference Includes =""X"" <Version = ""1.0.0"" />". This is along the lines of this comment: #4700 (comment)

* dotnet add package cli support for cpm projects (#4700)

* dotnet add package cli support for cpm projects (#4700) Co-authored-by: pragnya17 <59893188+pragnya17@users.noreply.github.com>

* dotnet add package cli support for cpm projects (#4700)

pragnya17 requested a review from a team as a code owner June 23, 2022 22:49

kartheekp-ms changed the title ~~Dev pragnya17 add package cli support for cpm projects~~ dotnet add package cli support for cpm projects Jun 23, 2022

pragnya17 commented Jun 23, 2022

View reviewed changes

test/NuGet.Core.Tests/NuGet.ProjectModel.Test/PackageSpecOperationsTests.cs Outdated Show resolved Hide resolved

pragnya17 commented Jun 23, 2022

View reviewed changes

src/NuGet.Core/NuGet.ProjectModel/PackageSpecOperations.cs Outdated Show resolved Hide resolved

kartheekp-ms force-pushed the dev-pragnya17-AddPackageCLISupportForCPMProjects branch from 7c0188c to a331f78 Compare June 24, 2022 00:35

kartheekp-ms mentioned this pull request Jun 28, 2022

Revised proposal for Adding dotnet CLI support for projects onboarded to CPM (Take 3) NuGet/Home#11915

Merged

jeffkl reviewed Jun 28, 2022

View reviewed changes

kartheekp-ms requested changes Jun 28, 2022

View reviewed changes

pragnya17 force-pushed the dev-pragnya17-AddPackageCLISupportForCPMProjects branch 2 times, most recently from c2074f5 to e83381a Compare June 29, 2022 21:59

nkolev92 reviewed Jun 29, 2022

View reviewed changes

jeffkl approved these changes Jun 30, 2022

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated Show resolved Hide resolved

nkolev92 reviewed Jul 1, 2022

View reviewed changes

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetAddPackageTests.cs Outdated Show resolved Hide resolved

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetAddPackageTests.cs Show resolved Hide resolved

kartheekp-ms force-pushed the dev-pragnya17-AddPackageCLISupportForCPMProjects branch from 6236de4 to 48834f8 Compare July 1, 2022 17:44

jeffkl mentioned this pull request Jul 1, 2022

Refactor CentralPackageManagementFile #4708

Merged

8 tasks

pragnya17 and others added 8 commits July 5, 2022 10:39

Add package support for cpm projects

1de7506

fix bug in test

41e9474

Added package version parameter

8d78db9

Modify test names to be more readable, remove comment in strings.resx

3457cb8

Change when info is logged

2d5c9e4

fix indentation and existing package references lines

991539a

[skip ci] add basic unit test for msbuild api

1211852

Change name of test

9c4e593

pragnya17 force-pushed the dev-pragnya17-AddPackageCLISupportForCPMProjects branch from 8455b4f to 9c4e593 Compare July 5, 2022 17:42

Add unit tests

4dbde7b

Pragnya Pandrate added 2 commits July 6, 2022 14:44

Change name of integration test

1740334

Change name to noop test

e3c1b34

kartheekp-ms approved these changes Jul 8, 2022

View reviewed changes

Pragnya Pandrate added 2 commits July 8, 2022 11:52

Revert "[skip ci] Attempt to modify unit test, still need to debug"

c7aef21

This reverts commit 4e49d93.

[skip ci] remove repetition when reading file from disk

60655e8

kartheekp-ms mentioned this pull request Jul 11, 2022

[Test]: MSBuildAPIUtilityTests are failing on Linux and Mac NuGet/Home#11960

Open

updated tests

58a5ddf

pragnya17 commented Jul 12, 2022

View reviewed changes

pragnya17 merged commit 93da94e into dev-feature-cpm-dotnetaddpackage Jul 12, 2022

pragnya17 deleted the dev-pragnya17-AddPackageCLISupportForCPMProjects branch July 12, 2022 22:09

pragnya17 mentioned this pull request Aug 4, 2022

Added dotnet add package CPM support documentation dotnet/docs#30496

Merged

martinrrm mentioned this pull request Sep 13, 2022

Support for dotnet add package command for CPM projects #4792

Merged

8 tasks

martinrrm pushed a commit that referenced this pull request Sep 20, 2022

dotnet add package cli support for cpm projects (#4700)

3a90203

martinrrm pushed a commit that referenced this pull request Sep 20, 2022

dotnet add package cli support for cpm projects (#4700)

f73b5ea

martinrrm pushed a commit that referenced this pull request Sep 28, 2022

dotnet add package cli support for cpm projects (#4700)

cf2cb7f

martinrrm pushed a commit that referenced this pull request Sep 28, 2022

dotnet add package cli support for cpm projects (#4700)

0123d4d

martinrrm pushed a commit that referenced this pull request Sep 28, 2022

dotnet add package cli support for cpm projects (#4700)

207a9eb

martinrrm pushed a commit that referenced this pull request Sep 28, 2022

dotnet add package cli support for cpm projects (#4700)

43f1095

martinrrm added a commit that referenced this pull request Sep 28, 2022

Support for dotnet add package command for CPM projects (#4792)

b827f77

* dotnet add package cli support for cpm projects (#4700)

martinrrm pushed a commit that referenced this pull request Oct 5, 2022

dotnet add package cli support for cpm projects (#4700)

7eda4b2

martinrrm pushed a commit that referenced this pull request Oct 12, 2022

dotnet add package cli support for cpm projects (#4700)

eba10a2

martinrrm pushed a commit that referenced this pull request Oct 17, 2022

dotnet add package cli support for cpm projects (#4700)

36cc894

martinrrm pushed a commit that referenced this pull request Oct 19, 2022

dotnet add package cli support for cpm projects (#4700)

6dc5e77

martinrrm added a commit that referenced this pull request Oct 21, 2022

CPM dotnet add package feature (#4832)

eb19de0

* dotnet add package cli support for cpm projects (#4700) Co-authored-by: pragnya17 <59893188+pragnya17@users.noreply.github.com>

slang25 mentioned this pull request Dec 12, 2022

[Epic]: Central Package Management improvements for 17.5 NuGet/Home#12301

Closed

4 tasks

martinrrm pushed a commit that referenced this pull request Dec 15, 2022

dotnet add package cli support for cpm projects (#4700)

2a3aae8

martinrrm pushed a commit that referenced this pull request Dec 15, 2022

dotnet add package cli support for cpm projects (#4700)

35507c1

martinrrm pushed a commit that referenced this pull request Dec 15, 2022

dotnet add package cli support for cpm projects (#4700)

8f83ddc

martinrrm added a commit that referenced this pull request Dec 16, 2022

Dotnet add package support cpm (#4986)

6d1a300

* dotnet add package cli support for cpm projects (#4700)

martinrrm mentioned this pull request Dec 16, 2022

Dotnet add package support cpm (#4986) #4987

Merged

8 tasks

martinrrm added a commit that referenced this pull request Dec 21, 2022

Dotnet add package support cpm (#4986) (#4987)

d61b1ad

* dotnet add package cli support for cpm projects (#4700)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dotnet add package cli support for cpm projects #4700

dotnet add package cli support for cpm projects #4700

pragnya17 commented Jun 23, 2022 •

edited

Loading

nkolev92 left a comment

nkolev92 left a comment

pragnya17 commented Jul 6, 2022

nkolev92 commented Jul 6, 2022

pragnya17 commented Jul 6, 2022 •

edited

Loading

kartheekp-ms left a comment

kartheekp-ms Jul 8, 2022

pragnya17 Jul 8, 2022

kartheekp-ms commented Jul 12, 2022

pragnya17 Jul 12, 2022

		Assert.Contains(@$"<PackageReference Include=""X"" />", File.ReadAllText(Path.Combine(testDirectory, "projectA.csproj")));
		Assert.DoesNotContain(@$"<Version = ""1.0.0"" />", File.ReadAllText(Path.Combine(testDirectory, "projectA.csproj")));

dotnet add package cli support for cpm projects #4700

dotnet add package cli support for cpm projects #4700

Conversation

pragnya17 commented Jun 23, 2022 • edited Loading

Bug

Description

PR Checklist

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

pragnya17 commented Jul 6, 2022

nkolev92 commented Jul 6, 2022

pragnya17 commented Jul 6, 2022 • edited Loading

kartheekp-ms left a comment

Choose a reason for hiding this comment

kartheekp-ms Jul 8, 2022

Choose a reason for hiding this comment

pragnya17 Jul 8, 2022

Choose a reason for hiding this comment

kartheekp-ms commented Jul 12, 2022

pragnya17 Jul 12, 2022

Choose a reason for hiding this comment

pragnya17 commented Jun 23, 2022 •

edited

Loading

pragnya17 commented Jul 6, 2022 •

edited

Loading