-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add LoadPrunePackageDataFromNearestFramework parameter for building against newer TFMs #51978
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
base: main
Are you sure you want to change the base?
Conversation
…ToPrune task Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
dsplaisted
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.
The LoadPackagesToPruneFromNearestFramework method duplicates a lot of code that's already in LoadPackagesToPrune. Instead of this duplication, make a recursive function that tries to load the data. If it can't find the data and it's supposed to load from the nearest framework, then it should call itself with the previous framework version, recursing down to the minimum supported version.
Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
Refactored to use a recursive function |
dsplaisted
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.
The value for LoadPrunePackageDataFromNearestFramework should be included in the CacheKey instead of passed as a parameter to LoadPackagesToPrune. Make sure to also update the hashcode for the CacheKey.
Also, the recursion should also be able to cover the case where useFrameworkPackageData is false, so that it can fall back from a framework where that would be false to one where it would be true. That will mean moving more code from LoadPackagesToPrune into TraLoadPackagesToPruneForVersion.
…eFrameworkPackageData logic into TryLoadPackagesToPruneForVersion Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
Updated to include |
Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
| { | ||
| string fallbackVersion = $"{targetVersion.Major - 1}.0"; | ||
| log.LogMessage($"LoadPrunePackageDataFromNearestFramework is enabled, trying to load from framework version {fallbackVersion}"); | ||
| packages = TryLoadPackagesToPruneForVersion(log, targetFrameworkIdentifier, fallbackVersion, frameworkReference, targetingPackRoots, prunePackageDataRoot, loadPrunePackageDataFromNearestFramework); |
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.
Do we want this to be recursive? That means someone can stack overflow if someone has this setting enabled and targets something like net5000.0? This also would be pretty expensive to fallback through each version 1-by-1... Find nearest might be better if you can determine which frameworks you have data for.
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.
You'll get an error way before this target runs if you target a higher version of .NET than the SDK supports:
NETSDK1045: The current .NET SDK does not support targeting .NET 20.0. Either target .NET 10.0 or lower, or use a version of the .NET SDK that supports .NET 20.0. Download the .NET SDK from https://aka.ms/dotnet/download
Recursion seemed simpler to me in this case.
| // If we can go to a lower version, recursively try it | ||
| if (targetVersion.Major > MinSupportedFrameworkMajorVersion) | ||
| { | ||
| string fallbackVersion = $"{targetVersion.Major - 1}.0"; |
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.
What if we have frameworks with minor versions?
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 think it's unlikely we'll ship minor versions again. I'm OK with this breaking or not working correctly if we eventually do create a minor version.
Implementation Plan
LoadPrunePackageDataFromNearestFrameworkparameter toGetPackagesToPrunetaskLoadPackagesToPrunemethod to accept and use the new parameterSummary
This PR successfully implements the
LoadPrunePackageDataFromNearestFrameworkparameter for theGetPackagesToPrunetask. The implementation:Adds a new boolean parameter
LoadPrunePackageDataFromNearestFrameworkto control fallback behavior (defaults tofalsefor backward compatibility)Includes the parameter in CacheKey - The value is now part of the cache key to ensure different parameter values produce different cached results
Uses a recursive approach -
TryLoadPackagesToPruneForVersionrecursively searches for prune package data from previous framework versions when the current version's data is not foundHandles framework version transitions - The recursive function now includes the
useFrameworkPackageDatalogic, allowing it to fall back from .NET 10+ (which uses prune package data) to .NET 9 and below (which uses framework package data)Tries multiple data sources in order of preference:
Eliminates code duplication - All loading logic is now in a single recursive function
Uses named constants - Both
MinSupportedFrameworkMajorVersion(3) andPrunePackageDataMinMajorVersion(10) are defined as constants for better maintainabilityIncludes proper logging to help diagnose which framework version's data is being used
Usage Example
When building .NET 11.0 with a .NET 10.0 SDK, set this property to avoid NETSDK1226 errors:
All changes have been verified to build successfully with no errors or security issues.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.