Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix TargetFramework caching and comparison in PackageRuleHandler #3082

Merged
merged 1 commit into from
Jan 22, 2018
Merged

Fix TargetFramework caching and comparison in PackageRuleHandler #3082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions ...stem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/CrossTarget/TargetFrameworkProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.ComponentModel.Composition;
using System.Linq;
Expand Down Expand Up @@ -37,20 +38,17 @@ public ITargetFramework GetTargetFramework(string shortOrFullName)
{
lock (_targetsLock)
{
// use linear search here, since there not many target frameworks and it would most efficient.
targetFramework = _cachedTargetFrameworks.FirstOrDefault(x => x.Equals(shortOrFullName));
if (targetFramework != null)
if (!TryGetCachedTargetFramework(shortOrFullName, out targetFramework))
{
return targetFramework;
}

var frameworkName = _nuGetFrameworkParser.ParseFrameworkName(shortOrFullName);
if (frameworkName != null)
{
var shortName = _nuGetFrameworkParser.GetShortFrameworkName(frameworkName);
targetFramework = new TargetFramework(frameworkName, shortName);
// remember target framework - there can not bee too many of them across the solution.
_cachedTargetFrameworks.Add(targetFramework);
var frameworkName = _nuGetFrameworkParser.ParseFrameworkName(shortOrFullName);
if (frameworkName != null &&
!TryGetCachedTargetFramework(frameworkName.FullName, out targetFramework))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

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 separate CanonicalShortName for the value returned by _nuGetFrameworkParser.GetShortFrameworkName, but I would need to ensure this does not affect semantics elsewhere.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@davkean davkean Jan 8, 2018 •
edited
Loading

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:

  • 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.

Copy link
Contributor Author

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.

{
var shortName = _nuGetFrameworkParser.GetShortFrameworkName(frameworkName);
targetFramework = new TargetFramework(frameworkName, shortName);
// remember target framework - there can not be too many of them across the solution.
_cachedTargetFrameworks.Add(targetFramework);
}
}
}
}
Expand All @@ -63,6 +61,13 @@ public ITargetFramework GetTargetFramework(string shortOrFullName)
return targetFramework;
}

private bool TryGetCachedTargetFramework(string shortOrFullName, out ITargetFramework targetFramework)
{
// use linear search here, since there not many target frameworks and it would most efficient.
targetFramework = _cachedTargetFrameworks.FirstOrDefault(x => x.Equals(shortOrFullName));
return targetFramework != null;
}

public ITargetFramework GetNearestFramework(ITargetFramework targetFramework,
IEnumerable<ITargetFramework> otherFrameworks)
{
Expand Down
2 changes: 1 addition & 1 deletion ...tSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Subscriptions/PackageRuleHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ private IDependencyModel GetDependencyModel(
&& unresolvedChanges.Contains(metadata.Name));
isTarget = metadata.IsTarget;
var packageTargetFramework = TargetFrameworkProvider.GetTargetFramework(metadata.Target);
if (packageTargetFramework != targetFramework)
if (!(packageTargetFramework?.Equals(targetFramework) == true))
{
return null;
}
Expand Down