Fix bug when determining transitive NoWarn #4256
Conversation
jeffkl
force-pushed
the
dev-jeffkl-fix-nowarn-transitive-slowness
branch
from
31c44db
to
6e9d0d3
Compare
| @@ -191,7 +191,7 @@ public static class TransitiveNoWarnUtils | |||
| // Merge the node's package specific no warn to the one in the path. | |||
| var mergedPackageSpecificNoWarn = MergePackageSpecificNoWarn(pathPackageSpecificNoWarn, nodePackageSpecificNoWarn); | |||
|
|
|||
| AddDependenciesToQueue(nodeDependencies, | |||
| AddDependenciesToQueue(nodeDependencies.Where(i => !seen.ContainsKey(i.Name)), | |||
There was a problem hiding this comment.
What happens if a package dependency that was seen before has a different version than the one, we are processing currently?
There was a problem hiding this comment.
Good point.. looks to me like this should be based on PR, which are version specific.
https://docs.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#suppressing-nuget-warnings
There was a problem hiding this comment.
I learnt from NuGet/Home#5740 (comment) comment that NoWarn can be set at the project level as a MSBuild property. This means that NoWarn applies to all the package dependencies including transitive ones.
There was a problem hiding this comment.
What happens if a package dependency that was seen before has a different version than the one, we are processing currently?
That can't happen (currently at least. With package shading things will get more complex). The point of restore is to resolve the graph and select a single version for each package. Different projects could use different versions, but each project goes through this method independently.
There was a problem hiding this comment.
I tried reading the tests for this class, but it was very difficult for me to determine if the scenario is actually tested or not. I haven't even attempted to read the algorithm, but it seems to me that if each dependency is visited exactly once, then the following scenario can never work:
Both packages A and B depend on package C. Package A has NoWarn="1", package B has NoWarn="2". Depending on the algorithm, C should get either both 1 and 2, or neither. However, with this change, wouldn't it get one of the two, depending on whether package A or B is visited first?
My gut feeling, without any actual evidence, is that this change is breaking behaviour.
Also, the title of this PR to me sounds like there's a bug in outcomes that is being fixed. The PR is intended to "only" improve performance, not change the result of computation, right?
|
@zivkan I agree, I've been thinking about this PR all night and since I've provided a workaround for the only affected customer, I think this PR is rushed. I see a few other potential perf gains in this method and I want to add unit tests that are more targeted (to verify its only visiting each node once) I'm going to close this for now and just try to get it in by 17.1 |
Bug
Fixes: NuGet/Home#11222
Regression? No
Description
When a set of projects has a lot of dependencies, the code that determines the NoWarn can end up searching for transitive dependencies incorrectly. Its walking the tree and adding dependencies but only checks if they've been visited after they're added to the queue. In one repo, this resulted in millions of searches.
The fix is to only add dependencies to the queue for processing if they have not been visited already.
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Existing tests already verify the result of this algorithm, this is just a perf optimization
Documentation