-
Notifications
You must be signed in to change notification settings - Fork 387
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
Fix TargetFramework caching and comparison in PackageRuleHandler #3082
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
FYI: This change is not really what i meant here when created this component. I was going to have a cache of unique TFMs, when now you could have same TFMs but specified by different short/long names or their aliases. Instead of doing this short term fix i would do a better TFM recognition in the line 43: var frameworkName = _nuGetFrameworkParser.ParseFrameworkName(shortOrFullName);
i.e. improve the way we parse and normalize to some common standard framework name (potentially keeping actual "alias" provided in the project).
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 agree we should be keeping the original alias. I'd prefer to call that the
ShortName
, and have a separateCanonicalShortName
for the value returned by_nuGetFrameworkParser.GetShortFrameworkName
, but I would need to ensure this does not affect semantics elsewhere.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.
Why should we be keeping the original alias? We'll only be tempted to compare to it. I've got a change where I throwaway usage of shortname completely apart from display purposes. The FullName is the canonical name.
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 primary purpose of keeping the alias here would be to avoid one extra check -- since the non-canonical shortnames do not occur in the cache, you have to parse the full name every time to get a cache hit. I don't think this is a bottleneck though so I'm fine leaving out this alias, and relying on the full name. This is ready to merge if I can get another signoff.
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.
There's three different names here:
Only the former two should used, the latter should never be used.
User written short name (
<TargetFramework>
property): Used in UI for display purposes and passed to NuGet so that it can write conditions in nuget.g.props/targets. It should never ever be interpreted, parsed or exchanged with any other feature.Canonical full name (
<TargetFrameworkMoniker>
property):: The canonical name that is used everywhere else and should be the exchange between Visual Studio features.Canonical short name: No MSBuild representation. This a concept that only exists in NuGet, and only used within NuGet packages. It should not be used anywhere else - the logic for interpreting/producing it only exists in NuGet.
Having the concept of a short name in our APIs has lead to an enormous amount of bugs - we should eradicate it and prevent anyone from consuming it unless used for display purposes.
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 issue is that this method is called with a "shortOrFullName" so we don't necessarily have the "user written short name" (
<TargetFramework>
property). I'll hold this change till I get back from vacation.