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

Enable central package management implicitly when Directory.Packages.props exists #5540

Closed

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Dec 12, 2023

Bug

Fixes: NuGet/Home#11834

Regression? Last working version:

Description

This change makes it so the presence of a Directory.Packages.props means that central package management is implicitly on. I've had countless people ask over the last year why you have to have the file and explicitly set ManagePackageVersionsCentrally to true. The logic now assumes if Directory.Packages.props exists then ManagePackageVersionsCentrally should be true unless the user has already set that property to a value, including if its set in Directory.Packages.props. This will make it easier for customers to onboard.

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

@jeffkl jeffkl marked this pull request as ready for review December 13, 2023 00:06
@jeffkl jeffkl requested a review from a team as a code owner December 13, 2023 00:06

<PropertyGroup>
<ImportDirectoryPackagesProps Condition="'$(ImportDirectoryPackagesProps)' == ''">true</ImportDirectoryPackagesProps>
<PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' != 'false' And '$(ImportDirectoryPackagesProps)' != 'false' And '$(DirectoryPackagesPropsPath)' == ''">
Copy link
Contributor

Choose a reason for hiding this comment

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

I found it difficult to understand the XML syntax while reviewing this pull request, so I have pasted the formatted XML here in case it helps to quickly grasp the changes.

<PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' != 'false' And '$(ImportDirectoryPackagesProps)' != 'false' And '$(DirectoryPackagesPropsPath)' == ''">
  <_DirectoryPackagesPropsFile Condition="'$(_DirectoryPackagesPropsFile)' == ''">
    Directory.Packages.props
  </_DirectoryPackagesPropsFile>
  <_DirectoryPackagesPropsBasePath Condition="'$(_DirectoryPackagesPropsBasePath)' == ''">
    $([MSBuild]::GetDirectoryNameOfFileAbove('$(MSBuildProjectDirectory)', '$(_DirectoryPackagesPropsFile)'))
  </_DirectoryPackagesPropsBasePath>
  <DirectoryPackagesPropsPath Condition="'$(_DirectoryPackagesPropsBasePath)' != '' and '$(_DirectoryPackagesPropsFile)' != ''">
    $([MSBuild]::NormalizePath('$(_DirectoryPackagesPropsBasePath)', '$(_DirectoryPackagesPropsFile)'))
  </DirectoryPackagesPropsPath>
</PropertyGroup>

Copy link
Member

Choose a reason for hiding this comment

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

@kartheekp-ms if you have github configured to use "unified" diffs, consider changing to "split":
image


<PropertyGroup Condition="'$(ImportDirectoryPackagesProps)' == 'true' and '$(DirectoryPackagesPropsPath)' != '' and Exists('$(DirectoryPackagesPropsPath)')">
<PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' != 'false' And '$(ImportDirectoryPackagesProps)' != 'false' And Exists('$(DirectoryPackagesPropsPath)')">
Copy link
Contributor

@kartheekp-ms kartheekp-ms Dec 15, 2023

Choose a reason for hiding this comment

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

How about just checking <PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' == 'true' > because ManagePackageVersionsCentrally is only true only when '$(ImportDirectoryPackagesProps)' != 'false' And Exists('$(DirectoryPackagesPropsPath)')?

@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Dec 25, 2023
@ghost
Copy link

ghost commented Dec 25, 2023

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DCR]: Central Package Management - Respect .props file as a way to opt-in to the feature.
4 participants