Fix: prefer disabled project nodes to external node #10223
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10223 +/- ##
==========================================
- Coverage 88.69% 88.67% -0.03%
==========================================
Files 180 180
Lines 22449 22449
==========================================
- Hits 19912 19906 -6
- Misses 2537 2543 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| # node may already exist from package or running project (even if it is disabled), | ||
| # in which case we should avoid clobbering it with an external node | ||
| if ( | ||
| node.unique_id not in self.manifest.nodes |
There was a problem hiding this comment.
Do we want to warn user in this case?
There was a problem hiding this comment.
I think this could end up being quite noisy as a warning because it would likely be multiple project nodes (e.g. from a package) being favoured over a collection of external nodes. I've been viewing it as something similar to our other precedence / resolution behaviour for resolving macros, refs, etc which is just 'expected' behaviour. Definitely warrants documentation though!
cc: @jtcohen6
There was a problem hiding this comment.
Agree, I don't think we want a warning here. We will document here that, in all cases where identical resources from the same namespace are installed via both "project" and "package" dependency, the "package" dependency is preferred, whether or not the resources from that package are actually enabled.
There was a problem hiding this comment.
resolves #10224
Problem
Currently, an external node can clobber an existing project node if that project node is disabled. This should not be the case as it leads to duplicative nodes in the manifest! (one in manifest.nodes, one in manifest.disabled)
Solution
When injecting an external node to the manifest that already exists from the FS, handle merges by:
Checklist