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.

Sign up for GitHub

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

Jump to bottom

Support for dotnet add package command for CPM projects #4792

Merged
merged 8 commits into from
Sep 28, 2022
Merged

Support for dotnet add package command for CPM projects #4792

merged 8 commits into from
Sep 28, 2022

Conversation

martinrrm
Copy link
Contributor

Bug

Fixes: NuGet/Home#9022

Regression? Last working version:

Description

This PR continues work from NuGet/Home#11890 and finishes the rest of scenarios for the command dotnet add package in a CPM project.

Scenarios 1-4 were added by Pragnya in #4700 and merged into the feature branch. This PR adds support for scenarios 9-16 (5-8 are not possible).
Design spec: https://github.com/NuGet/Home/blob/dev/proposed/2022/cpm-dotnet-cli-add-package-support.md

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A

  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@martinrrm martinrrm requested a review from a team as a code owner September 13, 2022 16:17
@jeffkl jeffkl mentioned this pull request Sep 13, 2022
Open
8 tasks
jeffkl
jeffkl reviewed Sep 13, 2022
View reviewed changes
src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated
{
var project = GetProject(packageReferenceArgs.ProjectPath);

// Get package version and VersionOverride if it already exists in the props file. Returns null if there is no matching package version.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be placed in the <returns /> section of the XML doc comment

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated Show resolved Hide resolved
nkolev92
nkolev92 reviewed Sep 13, 2022
View reviewed changes
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.

Great tests. I was able to correlate each test to one of the scenarios called out!

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx Outdated
@@ -757,4 +757,8 @@ Non-HTTPS access will be removed in a future version. Consider migrating to 'HTT
<value>PackageReference for package '{0}' added to '{1}' and PackageVersion added to central package management file '{2}'.</value>
<comment>0 - The package ID. 1 - Directory.Packages.props file path. 2 - Project file path.</comment>
</data>
<data name="Error_CPM_AddPkg_VersionsNotAllowed" xml:space="preserve">
Copy link
Member

Choose a reason for hiding this comment

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

I imagine this matches the NuGet.Commands resource?

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated Show resolved Hide resolved
test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetAddPackageTests.cs Show resolved Hide resolved
kartheekp-ms
kartheekp-ms reviewed Sep 14, 2022
View reviewed changes
...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs Outdated
var isValid = msBuild.AreCentralVersionRequirementsSatisfied(packageReferenceArgs);
if (!isValid)
{
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we throw an exception instead of returning error exit code i.e. 1

Copy link
Contributor

Choose a reason for hiding this comment

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

The AreCentralVersionRequirementsSatisfied method will log an error

nkolev92
nkolev92 reviewed Sep 16, 2022
View reviewed changes
src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated
}
}

// Get VersionOverride if it exisits in the package reference.
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a similar logic like restore?

I don't necessarily think we should expose public API and call it from here, but given that they are trying to assert the same thing, they should have the same code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nkolev92 In this case we dont want to look at the dgspec like we do in restore, I know this command uses a dgspec we dont know if it is up to date with the project file, since this tools is modifying the project XML, that should be the source of truth.

The user can modify the XML without doing a restore and try to add a package.

Copy link
Member

Choose a reason for hiding this comment

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

The dgspec is generated prior to the add package command running, so whether a restore was run shouldn't be relevant: https://github.com/dotnet/sdk/blob/a3d79ddc395389d0fe3f3ff9023bbf492323a606/src/Cli/dotnet/commands/dotnet-add/dotnet-add-package/Program.cs#L71-L99

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed some of the functionality to check in the dgspec, I'm still checking in csproj and Directory.Package.props files to see if the property comes from the desired file, I wasnt able to check this from the dgspec

jeffkl
jeffkl approved these changes Sep 16, 2022
View reviewed changes
...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs Outdated
var isValid = msBuild.AreCentralVersionRequirementsSatisfied(packageReferenceArgs);
if (!isValid)
{
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

The AreCentralVersionRequirementsSatisfied method will log an error

@martinrrm martinrrm force-pushed the dev-feature-cpm-dotnetaddpackage branch from 93da94e to 3a90203 Compare September 20, 2022 19:20
@martinrrm martinrrm force-pushed the dev-martinrrm-dotnet-add-package-support-cpm branch from ad0b6f4 to 352ba56 Compare September 20, 2022 21:51
@martinrrm martinrrm requested a review from nkolev92 September 23, 2022 17:48
nkolev92
nkolev92 reviewed Sep 24, 2022
View reviewed changes
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.

I think there's one small bug. Looks great other than that.

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated Show resolved Hide resolved
nkolev92
nkolev92 approved these changes Sep 27, 2022
View reviewed changes
src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs Outdated
string directoryPackagesPropsPath = project.GetPropertyValue(DirectoryPackagesPropsPathPropertyName);

// Get VersionOverride if it exisits in the package reference.
IEnumerable<LibraryDependency> dependenciesWithVersionOverride = packageSpec.TargetFrameworks.SelectMany(tfm => tfm.Dependencies.Where(d => !d.AutoReferenced && d.VersionOverride != null));
Copy link
Member

Choose a reason for hiding this comment

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

nit: A more performant version would be to have this after line 137 as you don't really need it beyond the scenarios in which package override is disabled.

@martinrrm martinrrm force-pushed the dev-martinrrm-dotnet-add-package-support-cpm branch from 78fda17 to 4122ec4 Compare September 28, 2022 19:47
@martinrrm martinrrm force-pushed the dev-feature-cpm-dotnetaddpackage branch from 3a90203 to 207a9eb Compare September 28, 2022 19:54
@martinrrm martinrrm force-pushed the dev-martinrrm-dotnet-add-package-support-cpm branch from 4122ec4 to 3adfc12 Compare September 28, 2022 19:55
@martinrrm martinrrm merged commit b827f77 into dev-feature-cpm-dotnetaddpackage Sep 28, 2022
@martinrrm martinrrm deleted the dev-martinrrm-dotnet-add-package-support-cpm branch September 28, 2022 21:02
@martinrrm martinrrm restored the dev-martinrrm-dotnet-add-package-support-cpm branch December 14, 2022 23:20
This was referenced Dec 16, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkolev92 nkolev92 nkolev92 approved these changes

@jeffkl jeffkl Awaiting requested review from jeffkl

@kartheekp-ms kartheekp-ms Awaiting requested review from kartheekp-ms

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martinrrm @nkolev92 @jeffkl @kartheekp-ms @pragnya17