Fix TargetFramework caching and comparison in PackageRuleHandler #3082
Conversation
|
/cc @dotnet/project-system |
| _cachedTargetFrameworks.Add(targetFramework); | ||
| var frameworkName = _nuGetFrameworkParser.ParseFrameworkName(shortOrFullName); | ||
| if (frameworkName != null && | ||
| !TryGetCachedTargetFramework(frameworkName.FullName, out targetFramework)) |
There was a problem hiding this comment.
TryGetCachedTargetFramework [](start = 29, length = 27)
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.
I agree we should be keeping the original alias. I'd prefer to call that the ShortName, and have a separate CanonicalShortName 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.
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.
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.
There's three different names here:
- User written short name
- Canonical Full name
- Canonical short name
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.
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.
|
Ping. I think this is ready to merge |
|
@davkean can you take another look? |
|
I'll go ahead and merge this in later today to catch the snap |
|
Just saw Dave's comment. I can make the additional change to drop the line that is picking up NuGet's canonical short name |
Customer scenario
Projects created with non-canonical target frameworks (e.g. netstandard20 instead of netstandard2.0) can leave dependency tree in unresolved state. Two bugs conspired to make this possible (1) TargetFrameworkProvider caches duplicate target frameworks in the face of non-canonical short names (2) comparison operator overload does not work with interfaces, hence in PackageRuleHandler two identical (i.e. duplicate) target frameworks are treated as unequal.
Bugs this fixes:
Fixes #2772
Workarounds, if any
N/A
Risk
Low
Performance impact
Slight improvement since cache is working
Is this a regression from a previous update?
No
Root cause analysis:
When caching, we use the canonical shortname, even if the input was non-canonical. This means future checks do not find anything in the cache, and create a duplicate. Fix is to also look for TargetFrameworks using the full name, and avoid the duplicate creation.
How was the bug found?
Customer reported