Skip to content

Skip package when resolving package dependencies if it appears multiple times #30105

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

Closed
wants to merge 1 commit into from
Closed

Skip package when resolving package dependencies if it appears multiple times #30105

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
22 changes: 20 additions & 2 deletions src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageDependencies.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,26 @@ private void RaiseLockFileTargets()

private void GetPackageAndFileDependencies(LockFileTarget target)
{
var resolvedPackageVersions = target.Libraries
.ToDictionary(pkg => pkg.Name, pkg => pkg.Version.ToNormalizedString(), StringComparer.OrdinalIgnoreCase);
// In case of there are duplicated packages in libraries with differing versions,
// We skip package altogether
var resolvedPackageVersions = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
var duplicatedPackages = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (LockFileTargetLibrary library in target.Libraries)
{
string libraryName = library.Name;
if (duplicatedPackages.Contains(libraryName))
Copy link
Member

Choose a reason for hiding this comment

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

If we only use the duplicatedPackages HashSet here, is it worth the perf hit of even having it. You could just have the below as it simplifies the code (unless the hashset check is so much faster than the dictionary containsKey check that it's worth it)

  if (!resolvedPackageVersions.ContainsKey(libraryName))
                {
resolvedPackageVersions[libraryName] = library.Version.ToNormalizedString();
                }

Copy link
Author

Choose a reason for hiding this comment

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

If we use this, then first package of duplicate packages will get added to the dictionary. Current code avoids adding any duplicate packages to dictionary. Happy to take whichever path you like. Letting the first one pass through certainly has better perf.

{
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Add some logging so that i.e. a binlog shows that duplicate packages are consumed?

Copy link
Author

Choose a reason for hiding this comment

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

Can you point me to some example of logging, that would be helpful.

}

if (resolvedPackageVersions.ContainsKey(libraryName))
{
duplicatedPackages.Add(libraryName);
resolvedPackageVersions.Remove(libraryName);
}
Copy link
Member

Choose a reason for hiding this comment

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

might be better with an else here as you can then avoid having to remove every time


resolvedPackageVersions[libraryName] = library.Version.ToNormalizedString();
Copy link
Author

Choose a reason for hiding this comment

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

If we let first one pass-through then we can avoid hashset lookup & dict.Remove calls.
If we want to minimize the impact then perhaps try/catch could be useful so we only run additional code only in error case.

}

string frameworkAlias = _targetNameToAliasMap[target.Name];

Expand Down