Proposal: Allow transitive constraints #1489
Conversation
This is a prototype for keeping track of the path through the selection process to a project. It is used to help dep ignore "stale" transitive constraints: constraints that when created applied to a descendent but should no longer apply now that that the project has moved to another location in the dependency graph. Questions: * I put bmi on atomWithPackages because it already had a bmi method to recreate the original bmi. Not sure if it's safe to live there considering the comments on the original bmi function about avoiding copys of the package list. So it may need to shift elsewhere or path should be be split out from the bmi struct so that it can be attached directly to an atom. * Is a non-bimodal solve ever a possibility in dep (outside of the unit tests?). I think we could drop "dep.isTransitive" in favor of using bmi.path exclusively but the unit tests have a non-bimodal set of tests that cause this to fail.
One test relied upon constraints NOT being applied transitively. The other was named incorrectly, it was testing a non-root scenario but was named "transitive" which was misleading.
carolynvs
force-pushed
the
transitive-constraints
branch
from
41150b2 to
d93117f
Compare
| pl: awp.pl, | ||
| } | ||
| a atom | ||
| bmi bimodalIdentifier |
There was a problem hiding this comment.
haven't fully grokked yet, but the complexity of this area is sufficient that, unless there is copy-avoiding performance argument to the contrary, it is preferable to not reuse types even if they share many fields. instead, types should be created and clearly described for the specific purpose for which they are used.
| @@ -152,12 +188,12 @@ var bimodalFixtures = map[string]bimodalFixture{ | |||
| }, | |||
| r: mksolution( | |||
| "a 1.0.0", | |||
| "b 1.1.0", | |||
| "b 1.0.0", // Now that constraints can be applied transitively, the constraint from root applies | |||
There was a problem hiding this comment.
We don't need the historical trace in comments - the whole test (and existing comments) should be updated to make sure that the important bits are still being tested, or the test should be removed entirely in favor of new tests.
| @@ -177,6 +177,8 @@ type bimodalIdentifier struct { | |||
| prefv Version | |||
| // Indicates that the bmi came from the root project originally | |||
| fromRoot bool | |||
| // The path to the atom in the graph, e.g. root -> foo -> bar | |||
| path []atom | |||
There was a problem hiding this comment.
OK, so here's where i think the problem is gonna be. This is effectively precaching the path to a given atom from the root - but there's no guarantee that it's the only possible path to the atom. That itself may not necessarily be a problem (i haven't thought it through in at the level of the purest white driven snow algorithm), but it likely will not work well in terms of the way we populate the unselected queue.
Have a look at selectAtom(). We only add a bmi to the unselected queue if it induces a new package requirement on the target atom. i believe that'll mean that it's possible for a transitive constraint on P to be ignored if project A that expresses it induces no new packages to be selected within P.
That may not be true, but it's not covered by the current set of tests. It also may be the case that this isn't true, but i'm reasoning that it is by relying on cached thoughts around the the restricted purpose of bimodalIdentifier. If that's the case, it's evidence for why the reuse of similarly-shaped types for different purposes inside the algorithm is not a great idea - it complicates the already-difficult task of reasoning about the algorithm.
What does this do / why do we need it?
See the full proposal
tldr: Allow constraints to apply to transitive dependencies.
Users no longer need to know the difference between direct and transitive dependencies, and can stop using overrides or empty imports + requires to force dep to apply a constraint to a transitive dependency.
What should your reviewer look out for in this PR?
bimodalIdentifierwhich keeps track of the path through the selection process to a project. It is used to help dep ignore "stale" transitive constraints: constraints that when created applied to a descendant but should no longer apply now that that the project has moved to another location in the dependency graph. (this is explained in more detail in the proposal doc).Do you need help or clarification on anything?
atomWithPackagesbecause it already had a bmi method to recreate the original bmi. Not sure if it's safe to live there considering the comments on the original bmi function about avoiding copies of the package list. So it may need to shift elsewhere or path should be be split out from the bmi struct so that it can be attached directly to an atom.dep.isTransitivein favor of usingbimodalIdentifier.pathexclusively but the unit tests have a non-bimodal set of tests that cause this to fail.Which issue(s) does this PR fix?
Fixes #1124
TODO
These are additional items that need to be addressed if the proposed solution is accepted.