Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 15, 2025

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

  • CPM Detection: Added logic to detect when CPM is enabled by checking for Directory.Packages.props with ManagePackageVersionsCentrally=true
  • Dual Mode Support: The install command now handles both CPM and non-CPM scenarios:
    • CPM enabled: Adds PackageVersion to Directory.Packages.props and PackageReference without Version to Directory.Build.props
    • CPM disabled: Continues existing behavior with PackageReference including Version in Directory.Build.props
  • Conversion Support: Handles upgrading from non-CPM to CPM by removing Version attributes from existing PackageReference entries

Example Usage

With CPM Enabled

<!-- Directory.Packages.props -->
<Project>
  <PropertyGroup>
    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
  </PropertyGroup>
  <ItemGroup>
    <PackageVersion Include="Nerdbank.GitVersioning" Version="3.7.115" />
  </ItemGroup>
</Project>

<!-- Directory.Build.props -->
<Project>
  <ItemGroup>
    <PackageReference Include="Nerdbank.GitVersioning" Condition="!Exists('packages.config')">
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>
  </ItemGroup>
</Project>

Without CPM (Traditional)

<!-- Directory.Build.props -->
<Project>
  <ItemGroup>
    <PackageReference Include="Nerdbank.GitVersioning" Condition="!Exists('packages.config')">
      <PrivateAssets>all</PrivateAssets>
      <Version>3.7.115</Version>
    </PackageReference>
  </ItemGroup>
</Project>

Testing

All scenarios have been thoroughly tested:

  • ✅ CPM enabled: Correctly manages versions in Directory.Packages.props
  • ✅ CPM disabled: Preserves existing behavior
  • ✅ CPM conversion: Successfully migrates from traditional to CPM approach
  • ✅ Existing functionality: All existing tests continue to pass

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.

@Copilot Copilot AI changed the title [WIP] nbgv tool doesn't support Central Package Management Add Central Package Management (CPM) support to nbgv install command Jun 15, 2025
@Copilot Copilot AI requested a review from AArnott June 15, 2025 00:14
Copilot finished work on behalf of AArnott June 15, 2025 00:14
@AArnott AArnott marked this pull request as ready for review June 15, 2025 03:33
@AArnott AArnott enabled auto-merge (squash) June 15, 2025 03:33
@AArnott AArnott merged commit f3793b0 into main Jun 15, 2025
12 checks passed
@AArnott AArnott deleted the copilot/fix-1031 branch June 15, 2025 03:52
@KalleOlaviNiemitalo

This comment has been minimized.

Copy link

@KalleOlaviNiemitalo KalleOlaviNiemitalo left a 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);

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?

Copy link
Collaborator

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.

Comment on lines +907 to +916
private static bool IsCentralPackageManagementEnabled(string path)
{
string directoryPackagesPropsPath = Path.Combine(path, "Directory.Packages.props");
if (!File.Exists(directoryPackagesPropsPath))
{
return false;
}

return true;
}

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.

Copy link
Collaborator

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.

@AArnott
Copy link
Collaborator

AArnott commented Jun 15, 2025

I wonder if this could output...

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.

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.

nbgv tool doesn't support Central Package Management

3 participants