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

Onboard to central package management in NuGet.Client repo #4899

Merged
merged 6 commits into from
Nov 16, 2022
Merged

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Nov 8, 2022

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1969

Regression? Last working version:

Description

  • I tried using git mv on packages.targets but I changed too much of the contents so it looks like a delete/add 😢
  • Organized all of the package versions in Directory.Packages.props
  • Added GlobalPackageReference items for PackageReference items that were in common imports
  • Used <PackageReference /> instead of <PackageDownload /> since CPM doesn't support PackageDownload. This brought in new dependencies which weren't being pulled in before so I had to resolve all of the conflicts with a few new package versions and/or PrivateAssets/ExcludeAssets.
  • Removed logic that verified package versions were declared in one place since CPM does that
  • Used GeneratePathProperty in a few places
  • Had to include some interfaces for project.json in Test.Utility since I think they were removed from newer versions of Microsoft.VisualStudio.ProjectSystem.Managed. They are just using ComImport so it should be okay.
  • Writing an empty Directory.Packages.props to the .test folder

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 changed the title Adopt central package management in NuGet.Client repo Onboard to central package management in NuGet.Client repo Nov 8, 2022
Base automatically changed from dev-jeffkl-directory-build-props to dev November 9, 2022 17:37
@jeffkl jeffkl marked this pull request as ready for review November 9, 2022 21:48
@jeffkl jeffkl requested a review from a team as a code owner November 9, 2022 21:48
zivkan
zivkan previously approved these changes Nov 10, 2022
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

I don't like the "enumerate every dependency of package X and add a <PackageReference Inlcude="x" ExcludeAssets="all" /> as an alternative to <PackageDownload. But everything else is opinionated, so if others approve I won't complain if it gets merged as is.

</ItemGroup>

<ItemGroup Condition=" '$(UsePublicApiAnalyzer)' == 'true' ">
<GlobalPackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="$(MicrosoftCodeAnalysisPublicApiAnalyzersVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

We probably need to wait to merge this until we know 100% all the CIs have 17.4.

We should also update our requirements section for building the repo.

zivkan
zivkan previously approved these changes Nov 15, 2022
@zivkan zivkan dismissed their stale review November 15, 2022 01:37

I forgot about the comment about changing the repo readme.md or contribution.md to mention minimum VS version numbers required to build the repo

@jeffkl
Copy link
Contributor Author

jeffkl commented Nov 15, 2022

I forgot about the comment about changing the repo readme.md or contribution.md to mention minimum VS version numbers required to build the repo

Good idea, I have updated the CONTRIBUTING.md to indicate you need VS 17.4+ and .NET SDK 7.0+

@jeffkl jeffkl merged commit 20be995 into dev Nov 16, 2022
@jeffkl jeffkl deleted the dev-jeffkl-cpm branch November 16, 2022 15:43
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.

3 participants