add package cli support for CPM projects #4681

pragnya17 · 2022-06-15T21:40:06Z

Bug

Regression? No.

Description

This feature is based on the design spec: NuGet/Home#11873. 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

pragnya17 · 2022-06-15T21:44:08Z

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

@@ -26,7 +26,7 @@ public static void AddOrUpdateDependency(PackageSpec spec, PackageDependency dep

            if (!existing.Any())
            {
-                AddDependency(spec.Dependencies, dependency.Id, range);
+                AddDependency(spec.Dependencies, dependency.Id, range, spec.RestoreMetadata?.CentralPackageVersionsEnabled ?? false);


All the modifications in this file are already in review based on @jeffkl PR (#4642) so I will remove these changes once his PR is merged into dev.

pragnya17 · 2022-06-15T21:45:01Z

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

@@ -253,6 +253,102 @@ public void AddOrUpdateDependency_ToSpecificFrameworks_AddsNewDependency()
                spec.TargetFrameworks[1].Dependencies[0].LibraryRange.VersionRange.MinVersion);
        }

+        [Fact]


All the modifications in this file are already in review based on @jeffkl PR (#4642) so I will remove these changes once his PR is merged into dev.

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

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

jeffkl · 2022-06-16T18:13:29Z

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

+            AddPackageReferenceIntoPropsItemGroup(propsItemGroup, libraryDependency);
+
+            // Save the updated props file.
+            directoryBuildPropsRootElement.Save();


I'm very curious if formatting is preserved. I think there might be something special we have to do to get the MSBuild APIs to do this. Can you test it out with a Directory.Packages.props that has some special formatting like:

<Project> <PropertyGroup> <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally> </PropertyGroup> <ItemGroup>  <PackageVersion Include="SomePackage" Version="1.0.0" /> </ItemGroup> </Project>

Since we are no longer updating the package version in the props file and rather adding a VersionOverride attribute to the project file instead, I think this no longer needs to be addressed. But please let me know if there are similar concerns with updating the VersionOverride attribute (I don't think there are because we are updating the project file not the props file, but I could be wrong).
EDIT: after discussion during our PR review meeting, we decided that this was still an issue that needed to be investigated and documented.

After further investigation, it seems that formatting is not preserved in the Directory.Packages.props file when something is modified (ex: a PackageVersion is added). I remember you mentioned that there is a flag that you can set in order to preserve the formatting, where might I be able to find that?

Consider using ProjectRootElement.PreserveFormatting to preserve formatting. I didn't try this API :)

…ot exist

…ce of Directory.Packages.Props file

… up comments.

…ce of Directory.Packages.Props file

…ate package scenario.

… functional tests

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

kartheekp-ms · 2022-06-22T01:45:24Z

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

+        {
+            // Only add the package reference information using the PACKAGE_REFERENCE_TYPE_TAG.
+            ProjectItemElement item = itemGroup.AddItem(PACKAGE_REFERENCE_TYPE_TAG, libraryDependency.Name);
+            string packageVersion = "No package version added because this project is onboarded to CPM";


Consider adding strings to the resources file so that they can be localized. We can add a new entry in the resource file for CPM projects. For example, info : Adding PackageReference for package 'Newtonsoft.Json' into project 'C:\Users\user1\source\repos\ConsoleApp9\ConsoleApp9\ConsoleApp9.csproj' and Version into the Directory.Packages.Props file

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

kartheekp-ms · 2022-06-22T16:32:45Z

There is another scenario we should consider. When add package command is executed with --no-restore switch then we should account for CPM onboarded project. We can also add a test for this scenario.

NuGet.Client/src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

Lines 35 to 62 in 84e277c

    
           if (packageReferenceArgs.NoRestore) 
        
           { 
        
               packageReferenceArgs.Logger.LogWarning(string.Format(CultureInfo.CurrentCulture, 
        
                   Strings.Warn_AddPkgWithoutRestore)); 
        
               VersionRange versionRange = default; 
        
               if (packageReferenceArgs.NoVersion) 
        
               { 
        
                   versionRange = packageReferenceArgs.Prerelease ? 
        
                                       VersionRange.Parse("*-*") : 
        
                                       VersionRange.Parse("*"); 
        
               } 
        
               else 
        
               { 
        
                   versionRange = VersionRange.Parse(packageReferenceArgs.PackageVersion); 
        
               } 
        
               var libraryDependency = new LibraryDependency 
        
               { 
        
                   LibraryRange = new LibraryRange( 
        
                       name: packageReferenceArgs.PackageId, 
        
                       versionRange: versionRange, 
        
                       typeConstraint: LibraryDependencyTarget.Package) 
        
               }; 
        
               msBuild.AddPackageReference(packageReferenceArgs.ProjectPath, libraryDependency); 
        
               return 0; 
        
           }

kartheekp-ms · 2022-06-22T17:50:35Z

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

+        {
+            // Get the Directory.Packages.props path.
+            string directoryPackagesPropsPath = project.GetPropertyValue("DirectoryPackagesPropsPath");
+            Logger.LogInformation("Modifying the Directory.Packages.props file at: " + directoryPackagesPropsPath);


Consider using resource file for managing strings. The advantage is that the strings in the resource file will be localized.

to expand on @kartheekp-ms's suggestion, the rule is that every string that is user facing should be localized, and come from a resource file.

A suggestion for the message would be Modifying {0}.

It'll be obvious from the name that it's a directory.build.props

kartheekp-ms · 2022-06-22T18:08:21Z

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

+                else
+                {
+                    // MOdify the project file with version override.
+                    UpdatePackageReferenceItemsWithVersionOverride(project, libraryDependency);


I think we should pass existingPackageReferences to this method to avoid computing package references again.

existingPackageReferences is only in a specific project I believe, whereas I am trying to search the whole solution for existing package references. Please let me know if I am misinterpreting that though.

kartheekp-ms · 2022-06-22T18:36:49Z

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

+                if (packageVersion == null)
+                {
+                    // Modifying the props file if project is onboarded to CPM.
+                    AddPackageVersionIntoItemGroupCPM(project, libraryDependency);
+                    //Modify the project file.
+                    AddPackageReferenceIntoItemGroupCPM(itemGroup, libraryDependency);
+                }


I think we should split this logic into two categories.

if (packageVersion == null) { // add package version to the props file. } if (!existingPackageReferences.Any()) { // add package reference to the csproj file // add the version override only if package version in the props file is different from the version passed to the add package command. } else { // add/update version override for the existing package references }

I'm a little confused as to what is being proposed. Is the change meant to be for all the if/else branches present?

nkolev92

I did one quick pass.

I'll a more in depth review once the spec is updated so I know what to review :D

nkolev92 · 2022-06-23T02:53:56Z

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

-                AddPackageReferenceIntoItemGroup(itemGroup, libraryDependency);
+                if (!existingPackageReferences.Any())
+                {
+                    //Modify the project file.


Some of these seem like helper comments but sometimes add clutter, especially when they are describe 1 method call.

nkolev92 · 2022-06-23T02:56:07Z

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

+        /// </summary>
+        /// <param name="libraryDependency">Package Dependency of the package to be added.</param>
+        /// <param name="item">Item to add the metadata to.</param>
+        private void AddExtraMetadata(LibraryDependency libraryDependency, ProjectItemElement item)


naming: Probably want to call out what metadata that is.

Example: AddFlagsMetadata.

nkolev92 · 2022-06-23T02:57:04Z

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

+        {
+            // Get the Directory.Packages.props path.
+            string directoryPackagesPropsPath = project.GetPropertyValue("DirectoryPackagesPropsPath");
+            Logger.LogInformation("Modifying the Directory.Packages.props file at: " + directoryPackagesPropsPath);


to expand on @kartheekp-ms's suggestion, the rule is that every string that is user facing should be localized, and come from a resource file.

nkolev92 · 2022-06-23T02:57:38Z

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

+        {
+            // Get the Directory.Packages.props path.
+            string directoryPackagesPropsPath = project.GetPropertyValue("DirectoryPackagesPropsPath");
+            Logger.LogInformation("Modifying the Directory.Packages.props file at: " + directoryPackagesPropsPath);


A suggestion for the message would be Modifying {0}.

It'll be obvious from the name that it's a directory.build.props

pragnya17 · 2022-06-23T20:45:26Z

Closing this PR as the design spec has changed.

pragnya17 requested a review from a team as a code owner June 15, 2022 21:40

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

pragnya17 commented Jun 15, 2022

View reviewed changes

kartheekp-ms reviewed Jun 16, 2022

View reviewed changes

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs Outdated Show resolved Hide resolved

jeffkl reviewed Jun 16, 2022

View reviewed changes

jeffkl and others added 24 commits June 21, 2022 14:52

Set VersionCentrallyManaged when installing a package in Visual Studio

371d9ba

Update logic to handle null RestoreMetadata and add test

5bc224d

Address comments

eb9e776

Add package support for package references that do not exist already

2efd1b5

Refactor code for scenario when package reference does not exist

019e3d6

Refactor code for scenario for scenario when package reference does n…

6647a2b

…ot exist

Use cpm flag that is already set rather than checking for the existen…

ccd4015

…ce of Directory.Packages.Props file

Adding functional test

9c318b8

Refactor code to add packages for CPM onboarded projects and rename test

7afa520

Finish add package implementation and fix bugs with tests

7f475c7

Add update package scenario and corresponding functional test

2263bb8

Reformat code for update package scenario, add logging back in, clean…

fc92313

… up comments.

Add validating project path back in

cc2e238

Use jeff's implementation

003483f

Add package support for package references that do not exist already

5f37ad1

Use cpm flag that is already set rather than checking for the existen…

68408c3

…ce of Directory.Packages.Props file

Refactor code to add packages for CPM onboarded projects and rename test

db518ff

Finish add package implementation and fix bugs with tests

88f7a0b

Add validating project path back in

196e0ca

Use Jeff's implementation of CPM on restore

1ef0aa0

Refactor/change order of a few functions. Add version override to upd…

f951436

…ate package scenario.

Switch order

3f78bab

Add documentation for all functions.

13b7f73

Remove logging statement, it was only meant to be for debugging purposes

bbb542a

pragnya17 added 5 commits June 21, 2022 15:09

Update package scenario with version override

151c880

Finish implementation of update package with version override and add…

cb65df1

… functional tests

change variable name

b772a23

Add return values in the summary statements of the functions.

5c363bc

Adding log information, modifying documentation.

2be0195

pragnya17 force-pushed the dev-pragnya17-AddPackageCLISupportForCPMProjects branch from a47da65 to 2be0195 Compare June 21, 2022 22:09

kartheekp-ms reviewed Jun 22, 2022

View reviewed changes

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

kartheekp-ms reviewed Jun 22, 2022

View reviewed changes

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

kartheekp-ms reviewed Jun 22, 2022

View reviewed changes

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

kartheekp-ms reviewed Jun 22, 2022

View reviewed changes

pragnya17 added 2 commits June 22, 2022 15:52

Remove unnecessary parameters/variables

7468457

remove unnecessary vars/parameters

96543bb

nkolev92 reviewed Jun 23, 2022

View reviewed changes

pragnya17 closed this Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add package cli support for CPM projects #4681

add package cli support for CPM projects #4681

pragnya17 commented Jun 15, 2022 •

edited

Loading

pragnya17 Jun 15, 2022 •

edited

Loading

pragnya17 Jun 15, 2022

jeffkl Jun 16, 2022

pragnya17 Jun 20, 2022 •

edited

Loading

pragnya17 Jun 21, 2022

kartheekp-ms Jun 21, 2022

kartheekp-ms Jun 22, 2022

kartheekp-ms commented Jun 22, 2022 •

edited

Loading

kartheekp-ms Jun 22, 2022

nkolev92 Jun 23, 2022

nkolev92 Jun 23, 2022

kartheekp-ms Jun 22, 2022

pragnya17 Jun 22, 2022

kartheekp-ms Jun 22, 2022 •

edited

Loading

pragnya17 Jun 22, 2022

nkolev92 left a comment

nkolev92 Jun 23, 2022

nkolev92 Jun 23, 2022

nkolev92 Jun 23, 2022

nkolev92 Jun 23, 2022

pragnya17 commented Jun 23, 2022

add package cli support for CPM projects #4681

add package cli support for CPM projects #4681

Conversation

pragnya17 commented Jun 15, 2022 • edited Loading

Bug

Description

PR Checklist

pragnya17 Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pragnya17 Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms commented Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pragnya17 commented Jun 23, 2022

pragnya17 commented Jun 15, 2022 •

edited

Loading

pragnya17 Jun 15, 2022 •

edited

Loading

pragnya17 Jun 20, 2022 •

edited

Loading

kartheekp-ms commented Jun 22, 2022 •

edited

Loading

kartheekp-ms Jun 22, 2022 •

edited

Loading