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

Revised proposal for Adding dotnet CLI support for projects onboarded to CPM #11873

Merged
merged 17 commits into from
Jun 17, 2022
Merged

Revised proposal for Adding dotnet CLI support for projects onboarded to CPM #11873

Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
11916ba
basic formatting
Jun 7, 2022
467dfc6
more work
Jun 8, 2022
0dd4eee
update
Jun 8, 2022
f710e80
update
Jun 8, 2022
f311fcb
update
Jun 8, 2022
85e973b
update
Jun 8, 2022
1cbbefe
update
Jun 8, 2022
bb5c20b
update
Jun 8, 2022
cb92d59
feedback
Jun 8, 2022
8c62378
update
kartheekp-ms Jun 11, 2022
099a091
update
kartheekp-ms Jun 11, 2022
6e501b9
update
kartheekp-ms Jun 11, 2022
a599d0e
add version override attribute in case of update
kartheekp-ms Jun 16, 2022
24c13de
If `VersionOverride` is already specified then the value should be up…
kartheekp-ms Jun 16, 2022
c7af6be
updated spec for example 2 the package reference does exist
kartheekp-ms Jun 16, 2022
0d3fcf2
update
kartheekp-ms Jun 16, 2022
2ad4cbf
update
kartheekp-ms Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 85 additions & 22 deletions proposed/2022/cpm-dotnet-cli-add-package-support.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,46 @@
# Adding Dotnet CLI add package command support to projects onboarded with Central Package Management

Author: Pragnya Pandrate <br>
Issue: https://github.com/NuGet/Home/issues/11807 <br>
Status: In Review <br>
- Author: [Pragnya Pandrate](https://github.com/pragnya17), [Kartheek Penagamuri](https://github.com/kartheekp-ms)
- Issue: [11807](https://github.com/NuGet/Home/issues/11807)
- Status: In Review

## Problem Background

The dotnet add package command allows users to add or update a package reference in a project file through the Dotnet CLI. However, when this command is used in a project that has been onboarded to Central Package Management (CPM), it poses an issue as this error is thrown: `error: NU1008: Projects that use central package version management should not define the version on the PackageReference items but on the PackageVersion items: [PackageName]`.

Projects onboarded to CPM use a `Directory.packages.props` file in the root of the repo where package versions are defined centrally. Ideally, when the `dotnet add package` command is used, the package version should only be added to the corresponding package in the `Directory.packages.props` file. However, currently the command attempts to add the package version to the `<PackageReference />` in the project which conflicts with the CPM requirements that package versions must only be in the `Directory.packages.props` file.
Projects onboarded to CPM use a `Directory.packages.props` file in the root of the repo where package versions are defined centrally. Ideally, when the `dotnet add package` command is used, the package version should only be added to the corresponding package in the `Directory.packages.props` file. However, currently the command attempts to add the package version to the `<PackageReference />` in the project which conflicts with the CPM requirements that package versions must only be in the `Directory.packages.props` file.

## Goals

The main goal is to add support for `dotnet add package` to be used with projects onboarded onto CPM. Regardless of whether the package has already been added to the project or not, the command should allow users to add packages or update the package version in the `Directory.packages.props` file.

## Customers

Users wanting to use CPM onboarded projects and dotnet CLI commands.

## Solutions
## Solution

When `dotnet add package` is executed in a project onboarded to CPM (meaning that the `Directory.packages.props` file exists) there are a few scenarios that must be considered.

| Scenario # | PackageReference exists? | Directory.Packages.Props file exists? | Is ManagePackageVersionsCentrally property set to true? | Is VersionOverride? | Current behavior | New behavior in dotnet CLI | In Scope |
| ---- |-----| -----|-----|----------|----------|----------|----------|
| 1 | ❌ | ✔️ | ✔️ (in Directory.Packages.Props or .csproj file) | ❌ | Restore failed with NU1008 error and NO edits were made to the csproj file (same in VS and dotnet CLI) | `PackageReference` should be added to .(cs/vb)proj file and `PackageVersion` should be added to the closest `Directory.Packages.Props` file | ✔️ |
| 2 | ❌ | ❌ | ✔️ in (cs/vb)proj file | ❌ | Restore failed with NU1008 error and NO edits were made to the csproj file (same in VS and dotnet CLI) | `PackageReference` and `Version` should be added to .(cs/vb)proj file.| ✔️ |
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved
| 3 | ❌ | ✔️ | ❌ | ❌ | `PackageReference` and `Version` added to .(cs/vb)proj file. | In addition to the current behavior `Directory.packages.props` file should be deleted if it exists in the project folder | ❌ |
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved
| 4 | ✔️ | ✔️ | ✔️ | ❌ | **dotnet CLI** - Restore failed with NU1008 error and NO edits were made to the csproj file. **VS** - Clicked on Update package in PM UI. New `PackageVersion` added to csproj file and `Directory.packages.props` file was not updated | No changes should be made to the `PackageReference` item in .(cs/vb)proj file but `PackageVersion` item should be updated with the new version in the appropriate `Directory.Packages.Props` file.|✔️ [More Info](https://github.com/NuGet/Home/pull/11849#discussion_r890639808) |
Copy link
Member

Choose a reason for hiding this comment

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

I don't think add package should update versions.
This is something that's not readily apparent to the user.

Imo, the behavior should be either version override or an explciit command to update in CPM depending on whether --version has been passed it or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We went down on this path because dotnet add package currently updates the package version if the reference already exists. In the case of CPM, updating version in the Directory.Packages.Props file is an option.

Your suggestion to add VersionOverride if the package reference already exists is also a good one. @jeffkl what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, its just more work for a user to use a different gesture for add vs update. They might be running it on a bunch of projects via automation and they'd have to determine whether not that package is already referenced. So I think add package should just update the version for them.

I'd rather not make VersionOverride easier to use personally. Its an escape hatch/last restore and I'd much rather people try to unify their graph. If we wanted to use VersionOverride, I would see that as a command-line argument for you opt into that functionality.

Copy link
Member

@nkolev92 nkolev92 Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion, its just more work for a user to use a different gesture for add vs update. They might be running it on a bunch of projects via automation and they'd have to determine whether not that package is already referenced. So I think add package should just update the version for them.

The reason why I bring it up is because we have issues and customer interviews that have suggested otherwise.
There's also an ask for a dotnet update package command, which is something we'd want to do eventually as well.

I think dotnet add package should match the VS experience for install package.

When you go to an individual project and run add package, you don't expect the package to be updated for other projects as well.

You could introduce potential compatibility issues but you wouldn't know about that until you get to your build, because there's no prevalidation for that.

Copy link
Contributor Author

@kartheekp-ms kartheekp-ms Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think dotnet add package should match the VS experience for install package.

Current behavior in VS - "Clicked on Update package in PM UI. New PackageVersion added to csproj file and Directory.packages.props file was not updated". I think it is a bug because the tooling adds PackageVersion XML element not the VersionOverride.

EDIT - When I launched @jeffkl's PR branch NuGet/NuGet.Client#4642 in an experimental instance, I noticed that updating package version in Visual Studio for CPM project didn't update .csproj file also. But the updated version is present in the assets.json file and solution explorer dependencies node except the props file.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed an issue for the <PackageVersion /> being added to the project: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1544117

Copy link
Contributor Author

@kartheekp-ms kartheekp-ms Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jeffkl - Hope you are okay with changing the logic to add VersionOverride when add package is trying to update the package version. I will update the proposal shortly.

cc @pragnya17

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 updated the spec to capture the summary of this conversation. No changes should be made to the Directory.Packages.Props file. Add VersionOverride attribute to the existing PackageReference item in .(cs/vb)proj file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes if VersionOverride is already specified, we should update that value.

Copy link
Contributor Author

@kartheekp-ms kartheekp-ms Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes if VersionOverride is already specified, we should update that value.

I agree. Updated the spec.

Can I get approval on the updated spec?

| 5 | ✔️ | ✔️ | ✔️ | ✔️ | **dotnet CLI** - Restore failed with NU1008 error and NO edits were made to the csproj file. **VS** - Clicked on Update package in PM UI. New `PackageVersion` added to csproj file and `VersionOverride` in .csproj file was not updated | Update `VersionOverride` attribute value in the corresponding `PackageReference` item in .(cs/vb)proj file. | ✔️ |

> [!NOTE]
> Scenarios with multiple Directory.Packages.props are out of scope for now.

### 1. The package reference does not exist

If the package does not already exist, it should be added along with the appropriate package version to `Directory.packages.props`. The package version should either be the latest version or the one specified in the CLI command. Only the package name (not the version) should be added to `<PackageReference>` in the project file.

#### Before `add package` is executed:<br>
#### Before `add package` is executed

The props file:

The props file: <br>
```xml
<Project>
<PropertyGroup>
Expand All @@ -33,7 +50,9 @@ The props file: <br>
</ItemGroup>
</Project>
```
The .csproj file: <br>

The .csproj file:

```xml
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
Expand All @@ -47,9 +66,12 @@ The .csproj file: <br>
</Project>
```

#### After `add package` is executed:<br>
#### After `add package` is executed

`dotnet add ToDo.csproj package Newtonsoft.Json -v 13.0.1`

The props file:

The props file: <br>
```xml
<Project>
<PropertyGroup>
Expand All @@ -60,7 +82,9 @@ The props file: <br>
</ItemGroup>
</Project>
```
The .csproj file: <br>

The .csproj file:

```xml
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
Expand Down Expand Up @@ -88,15 +112,18 @@ Repository
```

In the above example, the following scenarios are possible:

1. Project1 will evaluate the Directory.Packages.props file in the Repository\Solution1\ directory.
2. Project2 will evaluate the Directory.Packages.props file in the Repository\ directory.

***Sourced from https://devblogs.microsoft.com/nuget/introducing-central-package-management/
***Sourced from <https://devblogs.microsoft.com/nuget/introducing-central-package-management/>

### 2. The package reference does exist

If the package already exists in `Directory.packages.props` the version should be updated in `Directory.packages.props`. The package version should either be the latest version or the one specified in the CLI command. The `<PackageReference>` in the project file should not change.

#### Before `add package` is executed:<br>
#### Before `add package` is executed

```xml
<Project>
<PropertyGroup>
Expand All @@ -108,7 +135,10 @@ If the package already exists in `Directory.packages.props` the version should b
</Project>
```

#### After `add package` is executed:<br>
#### After `add package` is executed

`dotnet add ToDo.csproj package Newtonsoft.Json -v 13.0.1`

```xml
<Project>
<PropertyGroup>
Expand All @@ -120,15 +150,48 @@ If the package already exists in `Directory.packages.props` the version should b
</Project>
```

In case there are multiple `Directory.packages.props` files in the repo, all of them should be searched for a matching package reference. Once a matching package reference is found, its package version can be updated appropriately.
### 3. The package reference does exist with an VersionOverride

If the package already exists in `Directory.packages.props` the version should be updated in `Directory.packages.props`. The package version should either be the latest version or the one specified in the CLI command. The `<PackageReference>` in the project file should not change.

#### Before `add package` is executed

```xml
<Project>
<PropertyGroup>
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
</PropertyGroup>
<ItemGroup>
<PackageVersion Include="Newtonsoft.Json" Version="11.0.1"/>
</ItemGroup>
</Project>
```

```xml
<Project>
<PropertyGroup>
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
</PropertyGroup>
<ItemGroup>
<PackageVersion Include="Newtonsoft.Json" VersionOverride="12.0.1"/>
</ItemGroup>
</Project>
```

### Other Scenarios that can be considered
#### After `add package` is executed

While the above cases must be considered to solve the issue, there are a few other cases that can be considered to make the product more usable:
- No changes should be made to the `Directory.packages.props` file.
- The version specified in the `VersionOverride` atribute value of `PackageReference` element in the project file should be updated.

1. If the project has the `<ManagePackageVersionsCentrally>` set to true, but the `Directory.packages.props` file has not been created yet the file should be created. At this point, package references can be updated according to the cases discussed above.
2. If the `Directory.packages.props` file has been created but the `<ManagePackageVersionsCentrally>` property has not been set to true, it should be set to true.
3. If the project has the `<ManagePackageVersionsCentrally>` set to false, the project should not be considered as onboarded to CPM. Therefore the `Directory.packages.props` file should be deleted if it exists and only `.csproj` files should be modified.
4. If there are `Directory.packages.props` files that inherit other props files, all inherited files should be considered when `add package` is executed.
5. If the project has a `VersionOverride` attribute defined in the `csproj` file and the `add package` command is run in order to update a package version, then the `VersionOverride` attribute should be updated with the appropriate package version.
`dotnet add ToDo.csproj package Newtonsoft.Json -v 13.0.1`

```xml
<Project>
<PropertyGroup>
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
</PropertyGroup>
<ItemGroup>
<PackageVersion Include="Newtonsoft.Json" VersionOverride="13.0.1"/>
</ItemGroup>
</Project>
```