-
Notifications
You must be signed in to change notification settings - Fork 738
Full restore support for aliasing - make the assets file aliasing aware #6972
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
Conversation
4827827 to
e7cded1
Compare
7cbe95e to
863e076
Compare
Fixes # ### Context Design: NuGet/Home#12124 To allow duplicate frameworks in aliasing, the project reference protocol nearest framework selection needs to be updated to support matching by alias as well. Relevant part: https://github.com/NuGet/Home/blob/dev-nkolev92-tfmaliases/accepted/2025/Multiple-Equivalent-Framework-Support-TFM-As-Aliases.md#project-to-project-references NuGet/NuGet.Client#7011 NuGet.Client side adding the parameter. NuGet/NuGet.Client#6972 will add the full implementation at a later point. ### Changes Made - Pass CurrentProjectTargetFrameworkProperty if GetReferenceNearestTargetFrameworkTaskSupportsTargetFrameworKPropertyParameter is set. - If GetReferenceNearestTargetFrameworkTaskSupportsTargetFrameworKPropertyParameter is not set, but GetReferenceNearestTargetFrameworkTaskSupportsTargetPlatformParameter is set, call the old variation. - Otherwise calls the last variation. ### Testing - Manual testing. - I'd be happy to add tests if someone can point me in the right direction. ### Notes The idea here is to get ahead of things. Currently aliasing work can't be end to end tested because it requires an msbuild change. It makes it really hard to validate the NuGet changes are enough and good, but this is the only change needed on the msbuild side. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2c2a38f to
2d62c8b
Compare
91829c0 to
1d505fc
Compare
1d505fc to
0db929a
Compare
zivkan
left a comment
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.
Just finished going through the product code, but haven't started looking at the tests yet. Thought I'd share my comments so far.
src/NuGet.Core/NuGet.ProjectModel/LockFile/Utf8JsonStreamLockFileConverter.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/LockFile/Utf8JsonStreamLockFileTargetConverterV4.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs
Outdated
Show resolved
Hide resolved
zivkan
left a comment
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.
I always find tests hard to review. They all look so similar, making it hard to see the differences between them. I don't have any ideas how to make it better though.
src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs
Show resolved
Hide resolved
jeffkl
left a comment
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.
Thanks for undoing the breaking changes
Bug
Fixes: NuGet/Home#14534
Fixes: NuGet/Home#14536
Description
This PR implements the restore part of NuGet/Home#12124.
The short summary is:
Some more helpers for the review:
Assets file and dg spec (packagespec) read/write changes pointers:
The PackageSpecWriter, which writes the package spec, has a legacy and "new" writer. This writer is used in 3 different places. The assets file file, the dg spec writer (dgspec.json in the obj folder) and the no-op hash. In order to avoid having to version the package spec, we only use the legacy writer in the assets file, when the version is 3. This ensures all of our tests are actually covering the new codepath and we don't accidentally break the legacy projects. The way the package spec writer is implemented probably allows the legacy projects to have v4 of the assets file, but I'd rather do that at a later point to minimize the risk of this current change.
Every time we write the package spec now, we write a "framework", which is basically the effective framework. For simplicity, we write this every time.
LockFileTarget.Name is what's being written as the key, so in LockFileBuilder, we set the Name based on the version. Similarly, the LockFileTarget reader, v3 and v4, will set the name based on the version. If V3, we fill out the TargetAlias, if V4, we fill out the Framework (which is not longer written).
In LockFileFormat, the default version is 4. Whenever we use V4 of the LockFileFormat (or assets file), we use the default package spec writer, with version 3, we use the legacy writer.
I cleaned up any global suppressions whenever I saw it fit. The fewer of these we have, the better.
PR Checklist