Skip to content
This repository has been archived by the owner on Jul 12, 2022. It is now read-only.

Added support for PackageDownload and PackageVersion #986

Merged

Conversation

david-driscoll
Copy link
Contributor

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Feature

⤵️ What is the current behavior?

Testing with and without PackageDownload and PackageVersion.

🆕 What is the new behavior (if this is a feature change)?

PackageDownload - This is seriously a thing, today Nuke is using it ensure that packages are downloaded, but they are not implicitly referenced. This is used in the Nuke context to ensure that the binaries exist. I could not however find any specific documentation on this behavior, but it is a thing.
PackageVersion - This just shipped with the latest dotnet sdk 3.1.300 with support for centrally managed package versions. While the building blocks will be the same this central managed solution will be supported inside VisualStudio at a future date.

💥 Does this PR introduce a breaking change?

No

🐛 Recommendations for testing

Testing with and without PackageDownload and PackageVersion.

📝 Links to relevant issues/docs

I created a new PR using my older one as a reference so I merged the two together. Hopefully the code is a little cleaner this time around, let me know.

Supersedes: #895
Relates: NuGet/Home#7339
Centrally managing NuGet package-versions

🤔 Checklist before submitting

  • All projects build
  • Relevant documentation was updated

Let me know if you have any questions! ❤️ Nukeeper ❤️

@skolima
Copy link
Collaborator

skolima commented May 28, 2020

Hey @david-driscoll , thanks for the feature PR! Would you be open to splitting + restructuring it a somewhat? Here's what I have in mind (sorry for a large-ish brain dump, but I'm trying to minimise the back-and-forth of async communication - this was pending for quite a while already and I'm sorry for that):

  • <PackageVersion> : reading https://github.com/NuGet/Home/wiki/Centrally-managing-NuGet-package-versions does not make me feel like this is ready or implemented yet, really, especially the note "The name is not definitive and we are looking for better name pattern.". The PR to implement [PackageReference] Centrally manage NuGet package versions for a solution or a repo NuGet/Home#6764 is also still open. I'd prefer to postpone this work until the NuGet team has it finished (and do it as separate PR then).
  • <PackageDownload>: neat!
  • version parsing: currently you're string-trimming [ and ], no need to do this any more Handle single-version-wide exact version range. #971 (yes, I only spotted the bug because of your PR)
  • ProjectFileReader changes: the changes you're making are not covered by tests right now, I'd really prefer if they were
  • your initial version had a breaking change (forcing [1.2.3.4] exact version match everywhere), this one has it fixed now 👍
  • GetVersion in ProjectFileReader and DirectoryBuildTargetsReader : I'd skip the null handling and optional parameter and instead have two different ones (with and without namespace) in respective files, as needed

@david-driscoll
Copy link
Contributor Author

I doubt PackageVersion will be changed without at least support being maintained because it's now been shipped with a release (non beta ) sdk. I can't find definitive proof outside it has shipped in the latest 3.1.300 sdk, though I will ask around.

I can clean up the trimming no problem and look at the rest of the feedback as well this weekend.

@slang25
Copy link
Member

slang25 commented May 30, 2020

I can't seem to get PackageVersion working at all with 3.1.300 or the latest .NET 5 preview. Is it definitely shipped?

@david-driscoll
Copy link
Contributor Author

They have to be in the Directory.Packages.props otherwise they don't get picked up.

This commit (and it's branch) have them enabled. OmniSharp/csharp-language-server-protocol@ed0beb4 (https://github.com/OmniSharp/csharp-language-server-protocol/tree/pin-sdk ). It hasn't been merged yet due to other outstanding work, but it was working correctly on my machine with sdk 3.1.300.

@slang25
Copy link
Member

slang25 commented May 30, 2020

Ah, I think I need to opt-in with <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>

Update; Yes, it's opt-in

@cristinamanum
Copy link

I doubt PackageVersion will be changed without at least support being maintained because it's now been shipped with a release (non beta ) sdk. I can't find definitive proof outside it has shipped in the latest 3.1.300 sdk, though I will ask around.

The PackageVersion element name is not expected to be changed. The feature is in preview. We encourage customers to try it and add feedback/issues at https://github.com/nuget/home/issues as needed.

@david-driscoll
Copy link
Contributor Author

@skolima based on the rest of the feedback from @cristinamanum would you still two like different PRs?

@david-driscoll
Copy link
Contributor Author

Besides the splitting of the PR in two, I think the remaining feedback has now been addressed.

@AnthonySteele AnthonySteele merged commit 0281fda into NuKeeperDotNet:master Jun 3, 2020
@david-driscoll david-driscoll deleted the feature/package-version branch June 4, 2020 02:23
@david-driscoll
Copy link
Contributor Author

🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants