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.

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

Proposal: Adding dotnet CLI support for projects onboarded to CPM #11849

Merged
merged 16 commits into from
Jun 6, 2022

Conversation

pragnya17
Copy link
Contributor

@pragnya17 pragnya17 commented Jun 1, 2022

Tracking issue #11858

Rendered Design Spec

Proposal for 11807

@pragnya17 pragnya17 requested a review from a team as a code owner June 1, 2022 19:36
@ghost
Copy link

ghost commented Jun 1, 2022

CLA assistant check
All CLA requirements met.

@erdembayar
Copy link
Contributor

@pragnya17
Your spec looks great. Probably need to rename to cpm-dotnet-cli-add-package-support.md or something like that to express what problem it's solving, otherwise CPMDotnetCLISupport.md is too generic. Also, please remove image files since you're using embedded codes.

@pragnya17 pragnya17 merged commit 78b5f27 into dev Jun 6, 2022
@pragnya17 pragnya17 deleted the dev-pragnya17-CPMDotnetCLISupport branch June 6, 2022 21:42
@pragnya17
Copy link
Contributor Author

The spec has been reviewed and changes have been made according to the feedback received.

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.

Nice job @pragnya17!

It would be great if we can clarify the second scenario about the updating package version.

I wonder if @JonDouglas had a chance to look over that.

I also have some minor spec comments.

</Project>
```

#### After `add package` is executed:<br>
Copy link
Member

Choose a reason for hiding this comment

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

I think this behavior might not be super obvious to the end user.

If I am already managing a package centrally, it feels like the version that should be installed is the existing one, and ideally not update it.

Version updates should only happen explicitly or through an update command.

Copy link
Contributor

Choose a reason for hiding this comment

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

We went down this path because dotnet add package command currently updates the package version if the package refence already exists.

If it is not ideal for dotnet add package command to update the package version then should we raise a warning instead of updating the version in the directory.packages.props file for projects onboarded onto CPM?

</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.
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 the intention here is if multiple Directory.Packages.props file apply correct?

This is something that the user can only do manually.

Copy link
Contributor

@kartheekp-ms kartheekp-ms Jun 7, 2022

Choose a reason for hiding this comment

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

I imagine the intention here is if multiple Directory.Packages.props file apply correct?

You are correct. Incase if others are curious, as per Central Package Management rules, only the closest Directory.Packages.props file is evaluated for a given project.

Repository
 |-- Directory.Packages.props
 |-- Solution1
     |-- Directory.Packages.props 
     |-- Project1
 |-- Solution2
     |-- Project2

However, I think customers can specify an Import statement in Solution1\Directory.Packages.props file to import properties from Repository\Directory.Packages.props

Repository
 |-- Directory.Packages.props
 |-- Solution1
     |-- Directory.Packages.props (specifies an import statement pointing to the props file in the parent folder)
     |-- Project1
 |-- Solution2

The current spec suggests that dotnet add package command should walk through the hierarchy of directory.packages.props file (using Import statements) and update the package version in the appropriate props file.

This is something that the user can only do manually.

Once we get clarity on https://github.com/NuGet/Home/pull/11849/files#r890672222 conversation then we consider this scenario as out of scope / future possibilities for this work.

Copy link
Member

Choose a reason for hiding this comment

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

There might be some implementational complexity with multiple files.
Example, what if both files have it?
You'd then need to know where it's coming from, which is probably more complicated than a V1.

I'd encourage explicitly saying that scenarios with multiple Directory.Packages.props are out of scope for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed about this scenario offline with @jeffkl. Yes, scenarios with multiple Directory.Packages.props are out of scope for now. Thanks Nikolche. Created a PR with revised proposal #11873


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.

### Other Scenarios that can be considered
Copy link
Member

Choose a reason for hiding this comment

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

This is probably something best handled in a Future possibilities section.


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:

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.
Copy link
Member

Choose a reason for hiding this comment

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

It's worth calling out what would actually happen here today, even if we're not promising/committing to the improvements themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a table in the revised proposal #11873 explaining current and new behavior for every scenario.


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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
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 in the project folder should be deleted if it exists and only `.csproj` files should be modified

@pragnya17 pragnya17 restored the dev-pragnya17-CPMDotnetCLISupport branch June 7, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants