-
-
Notifications
You must be signed in to change notification settings - Fork 177
Add Central Package Management (CPM) support to nbgv install command #1208
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
Conversation
Co-authored-by: AArnott <[email protected]>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could output
<ItemGroup>
<PackageReference Include="Nerdbank.GitVersioning" Condition="!Exists('packages.config')">
<PrivateAssets>all</PrivateAssets>
<Version Condition="$(ManagePackageVersionsCentrally) != 'true'">3.7.115</Version>
</PackageReference>
</ItemGroup>
and have that work in both cases?
Although I suppose the 3.7.115 version number in the file would seem somewhat misleading in the CPM-enabled case where it would not take effect but would attract developers to update it when a new version of NB.GV is released.
if (isCpmEnabled) | ||
{ | ||
// Remove Version metadata if CPM is enabled | ||
ProjectMetadataElement versionMetadata = item.Metadata.FirstOrDefault(m => m.Name == VersionMetadataName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't metadata names in MSBuild case-insensitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I didn't catch that. I'll send a follow-up PR to fix. Thanks.
private static bool IsCentralPackageManagementEnabled(string path) | ||
{ | ||
string directoryPackagesPropsPath = Path.Combine(path, "Directory.Packages.props"); | ||
if (!File.Exists(directoryPackagesPropsPath)) | ||
{ | ||
return false; | ||
} | ||
|
||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existence of Directory.Packages.props does not necessarily mean that central package management is enabled. The ManagePackageVersionsCentrally property might be set in neither Directory.Packages.props nor Directory.Build.props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the property must be set. And Copilot had originally included that check, but it was only looking in Directory.Packages.props for it, which as you say, would be an incomplete check.
Since no repo should have Directory.Packages.props without setting this property to true as well, I'm ignoring the property to keep it simple.
I agree with your concern: putting the version number in both places with a condition on it would be misleading and add to the maintenance burden without any gain. I'm not going to do that. |
The
nbgv install
command now recognizes and supports Central Package Management (CPM). Previously, the tool only worked with traditional PackageReference management and would always add version information directly to Directory.Build.props.Changes Made
ManagePackageVersionsCentrally=true
PackageVersion
to Directory.Packages.props andPackageReference
without Version to Directory.Build.propsPackageReference
including Version in Directory.Build.propsExample Usage
With CPM Enabled
Without CPM (Traditional)
Testing
All scenarios have been thoroughly tested:
The implementation is backward-compatible and makes minimal changes to the codebase (106 lines added, 7 removed in a single file).
Fixes #1031.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.