-
Notifications
You must be signed in to change notification settings - Fork 906
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
(#508) Uplift NuGet library #2740
(#508) Uplift NuGet library #2740
Conversation
Some initial thoughts:
|
7e08a13
to
8289892
Compare
I went for the low hanging fruit and got push working to my local nexus repository. There are still a number of things that are broken, but it is kinda working:
Other notes:
|
b28dcfe
to
5be7b43
Compare
So, I've got pack working as well. It works with NuGet compatible
|
9846e1d
to
2f09265
Compare
A couple of things I noted today in research:
|
1641023
to
5db4d45
Compare
In nuget-chocolatey, In NuGet.Client, Instead, I'm thinking of using This would require changes in lots of files, but theoretically the changes mostly should just be updating the names of things without too many larger changes. I talked with @gep13 about this, and it seems like a reasonable option to explore. |
While looking into getting
|
714836c
to
c11032b
Compare
With Chocolatey CLI versions that use Here is the integration test example. It has a starting situation of With NuGet.Core (where PackageManager is set to This does seem to be a breaking change, as it is behavior that is integration tested for, and the tests do fail when using NuGet SDK. However the NuGet SDK behavior where it installs the highest compatible version seems to be the correct behavior, given that it is set to I don't think there is a way to tweak this behavior without changing So, the question becomes, are there any operational cases where this breaking change would be a problem? If so, this needs to be investigated further to see if there is a way to mitigate or it that case. If not, then it should be documented during release, and the tests changed to reflect the new behavior I'll assume it is the second option for the moment, and modify the tests to assume the highest compatible version of parent package(s) will be installed, instead of the lowest. |
The Therefore, usages of Another change from NuGet.Core is that the For reading |
Proposal for a branching strategy for the NuGet.Client fork:
To keep the fork up to date, it may be a good idea to utilize a bot to create PRs to merge in changes on the
|
The NuGet There is not a distinction between upgrades and installs for the dependency resolver, so more or less the same setup can be used for installs and upgrades. Here is my explanation of what to pass to the
These have worked for all situations in the tests, and everything I have played with manually. There could be situations, say for example with multiple versions of packages installed, and complex dependency trees, that it does not work for, but nothing has popped out yet. As for uninstalls, instead of using |
Features to possibly addInvestigate adding support for v3 catalog reading/parsingThis would be for the purpose of adding a local index of packages: #820 Some NuGet v3 feeds support the catalog resource: https://docs.microsoft.com/en-us/nuget/api/catalog-resource It is a append-only resource that records all package additions/changes/unlistings in a json format. So it could be paged through and saved locally, and updated via a new There are a few samples and projects that use it, but it is not something available through NuGet.Client as far as I am aware, so it would have to be implemented within Chocolatey. https://github.com/NuGet/NuGet.Jobs/tree/main/src/NuGet.Protocol.Catalog Figure out version ranges, potentially get version range working for
|
As side by side is going to be removed, I'm updating this to not include side by side support. And here is the branch with the previous version of the NuGet.Client code: https://github.com/TheCakeIsNaOH/NuGet.Client/tree/sidebyside-backup |
0d9aea4
to
99ec19b
Compare
To be able to rebase this without a huge amount of hassle, I had to squash a bunch of commits, since otherwise changes would not merge smoothly in. As noted in the last comment, the previous version of this branch is still available here: https://github.com/TheCakeIsNaOH/choco/tree/nuget-v3-sidebyside |
239a7bc
to
a1b5469
Compare
Remove NuGet.Core dll and project references to it.
bee58d2
to
6072cb1
Compare
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.
These are just notes for us on the Chocolatey Team. No change is needed, as we will merge this code as is (after a few updates) and continue working on it.
(chocolatey#508) Update using references to new NuGet libraries (chocolatey#508) Update Constants references for new libraries update constants (chocolatey#508) ChocolateyNugetLogger update to nuget logger changes (chocolatey#508) Update method of getting remote repository This allows getting the remote repositories and remote resources from the config, with authentication and client certificates attached as available. (chocolatey#508) Get Push working (chocolatey#508) Get pack compiling This gets pack working, if the package is using the NuGet compatible schema without the added Chocolatey specific elements (chocolatey#508) Switch nupkg reading to PackageArchiveReader As OptimizedZipPackage is no longer available Switch to using my forked libraries Switch package result to use IPackageMetadata/IPackageServerMetadata Fix Pin, Template and Export commands Get List working Misc fixes Get Outdated, Install, Upgrade and Uninstall compiling Try initial manual package installation routine Remove Pending from Install Scenarios and tweak tests Fixes a couple of tests that have different behavior with newer NuGet Fixes a couple of tests that were pending, and were not written correctly Add better exception handling during setup to TinySpec ChocolateyPackageMetadata handle .nuspecs Fixes for List Make sure results are not duplicated, and that ordering is correct Add Identity to packageresult ChocolateyPackagePathResolver fix for side by side and use to get install location temp add debug asset to ensure metadata available NuGetCommon add more methods and tweak GetPackageDependencies This adds methods to get the local/installed package repository, to get dependencies for installed packages, and fixes GetPackageDependencies to get dependencys with ranges Switch to PackageMetadataResource to get all package versions Port over install improvements from parallel install branch misc cleanups Switch package service actions to include configuration Upgrade command work Switch depender to parent Add in package page to list verbose output Work on upgrade/uninstall Upgrade to nuget 6.4.0.4 Upgrade and Uninstall get passing tests Upgrade NuGet SDK build Update path resolver to specify correct manifest name upgrade nuget library version again Add tests for side by side nuspec and nupkg file names This adds tests to ensure that the manifest and package files are correctly named, that is with the version in the filenames if side by side, and without the version if not side by side. Remove now unused nugetpackagemanager related code Update my added integration tests Fixes and adds a test to the switch side by side to normal install scenario. Tweak install package resolution for side by side and add a test Update messages in some pester tests
6072cb1
to
031aaca
Compare
Failures on GitHub action here is expected, we have not made the NuGet.Client libraries we use public yet. |
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.
LGTM, thank you for your work on this @TheCakeIsNaOH. We have decided to pull these changes in and continue working on them to fix any outstanding issues in separate pull requests.
Description Of Changes
Uplift Chocolatey CLI from NuGet.Core to using NuGet.Client
Motivation and Context
This allows for features like V3 api support, and SemVer 2.0
Testing
Built and ran tests.
Things to test list
pack
(and/orpush
) to ensure the new.nuspec
element are not used if needed, or warn if they would be ignored currently?pack
token substitution, this issue seems to indicate that there may have been changes upstream: new - Token replacement with in nuspec should more closely match nuget #2100pack
encoding filenames, due to potential upstream changes: pack - URI encodes filenames #1628Tests to add:
chocolateyPackageVersionPrerelease
andchocolateyPackageVersionPackageRelease
to ensure that they are set correctly, e.g. normalized, etcKnown issues
choco install
andchoco search
by the network requests information logged to info by NuGet. (#508) Change NuGet normal logging to be verbose logging #2944.
as the source like.\choco.exe list -s .
, it erors withThe path '.' for the selected source could not be resolved.
. However, installs do seem to be working? (#508) Attempt to resolve relative paths for sources #2943--exact
and/or--all-versions
--page
,--page-size=
,--by-tags-only
are all likely broken in some cases. (#508) Fix --page and --page-size arguments #2949 and--by-tags-only
is working.PackageDownloader
class was overridden, but the class no longer actually downloads the packages inside it. (#9) Add Download Progress NuGet.Client#12search
with and without a search term,--exact
and--all-versions
, then compare to NuGet.Core quantity of calls. It may also be worth looking at install/upgrade when--version
is not passed, and for packages that have a large dependency chain.Features to add?
Moved to comment: #2740 (comment)
Completed
nuget-chocolatey
that should be ported/upstreamed/ignored/investigatedChange Types Made
Related Issue
Preparing for #508
Change Checklist