Skip package when resolving package dependencies if it appears multiple times #30105
Conversation
…le times Resolves dotnet/NuGet.BuildTasks#86 NuGet shows error about conflict already so we just skip it
ghost
added
the
| resolvedPackageVersions.Remove(libraryName); | ||
| } | ||
|
|
||
| resolvedPackageVersions[libraryName] = library.Version.ToNormalizedString(); |
There was a problem hiding this comment.
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 libraryName = library.Name; | ||
| if (duplicatedPackages.Contains(libraryName)) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
Add some logging so that i.e. a binlog shows that duplicate packages are consumed?
There was a problem hiding this comment.
Can you point me to some example of logging, that would be helpful.
| { | ||
| duplicatedPackages.Add(libraryName); | ||
| resolvedPackageVersions.Remove(libraryName); | ||
| } |
There was a problem hiding this comment.
might be better with an else here as you can then avoid having to remove every time
| foreach (LockFileTargetLibrary library in target.Libraries) | ||
| { | ||
| string libraryName = library.Name; | ||
| if (duplicatedPackages.Contains(libraryName)) |
There was a problem hiding this comment.
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();
}
There was a problem hiding this comment.
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.
|
@dsplaisted - Can you also take a look at this? |
|
While looking at this, I remembered that as of #28405, we don't run the |
Resolves dotnet/NuGet.BuildTasks#86
NuGet shows error about conflict already so we just skip it