Limit qualifier depth #3220
Limit qualifier depth #3220
Conversation
In #3170 we introduced a cycle check to the solver. This check is necessary to reject cycling solutions (which would previously have resulted in an internal error when we verify the install plan). However, this by itself is not sufficient. If we have a cycle through setup dependencies, the solver loops because it starts building an infinite tree. This is explained in detail in In this commit we just add some unit tests that provide a minimal example that exposes the bug.
A package path now consists of a namespace and a qualifier. The namespace is either DefaultNamespace or Independent _i_, for some _i_; this is used for independent top-level goals. Then the qualifier is either Unqualified (default), Setup _pn_ for the setup dependencies of package _pn_, or Base _pn_, for a dependency on base by package _pn_ (used only when we detect the presence of a base shim). Qualifiers are not tested anymore. This avoids non-termination in the solver. The unit tests now pass.
|
@edsko Do you want this to also go into 1.24? |
|
Hmmmm. That's a harder question. I think I'm going to say yes, I think that's a good idea. After all, qualified goals are new right? So any kind of qualification already increases the search space over any existing cabal releases? So then the question is, is a single qualifier enough? But if there aren't situations where it isn't, then at least it's not worse than it is now. However, without this change the solver may loop (and I fear quite frequently will, especially once there are one or two commonly used packages on Hackage that have explicit setup dependencies). I think that is a significant degradation from a users perspective, and will not improve cabal's image. So yes, I think I would tentatively suggest to include this. But I would be keen to hear other people's opinions, in particular @dcoutts and @kosmikus.
|
|
OK, noted. |
|
I just read #3160, and I wanted to float an idea. I'm not very familiar with the implementation of independent goals, so I don't know if it makes sense. I was trying to understand why the solver would choose different instances for Since Would it make sense to also qualify the goal with the package version, as in EDIT: The solver could also pick different instances for |
|
@edsko cherry-picked to nix-local-build branch. This resolves the solver looping that we were seeing when building cabal-install itself with 7.10. That is, |
|
@grayjay Sorry for the slow reply, very busy at the moment. It is indeed tempting to try to somehow incorporate version numbers in the qualifiers; I too have considered this. For setup dependencies it does seem to be the right solution: how else could the two uses of C have different requirements on D for their setup script? However, I don't think it quite works. After all, in your example, A and B might not be specifying an explicit version of C; perhaps the choice of a version of C is forced by another setup dependency, maybe something like In general, when we qualify these goals, we might not yet have that version information; after all, it is the task of the solver to figure out those versions. Really we want to say "the version that we need here (whatever that version may be), but of course that is precisely the purpose of qualifiers. Whenever I found myself thinking along these lines (wanting to have version numbers in qualifiers), I always ended up concluding that no, that doesn't work, because we are putting the cart before the horse. Somewhat more philosophically, the idea of qualifiers is that within a certain namespace (certain package path), we can pick only one version; if you want to allow versions to differ, you need to introduce different namespaces. Adding version numbers into the qualifiers kind of confuses these two concepts. Of course, that's not to say that the idea of qualifiers is necessarily the right one :) |
|
@edsko I didn't consider that the solver doesn't need to know a package's version before it adds goals for the setup dependencies, and that my suggestion mixes up variables and values. Thank you for the detailed explanation! |
|
@23Skidoo having discussed this with @dcoutts once more, I think I will stick to my proposal above to include this in the release. One qualifier is strictly more general than none, so compared to the released cabal increasing it to a single qualifier cannot make anything worse, and can only result in more solutions. It may or may not turn out that a single qualifier is not sufficient and that we need to increase the search space some more, but we can do that in a later release and anyway it would be much better to do that given some real examples than just speculate, because I think any further increase (which does not go all the way to infinity) will necessarily be of somewhat empirical nature. Having an infinite number of qualifiers I'm afraid will make the solver loop relatively often, which is not something we want to release. I suggest to merge. |
|
OK, merging. |
|
Merged into |
|
Yes, I'm aware this has already been merged. Nevertheless: I have finally looked at this and I'm ok with this change. Thanks. |
|
@kosmikus Mosterd. Maaltijd. 😁 |
This PR changes the structure of package paths to:
This separates out a namespace from a qualifier; the same space is just used to distinguish top-level goals, which are supposed to be entirely independent from each other. Qualifiers are used when we want to make independent choices for dependencies. Crucially, qualifiers do not have any kind of nesting structure. This addresses the problem described in detail in #3160, but it also introduces a change in behaviour; we will find fewer solutions (though still strictly more solutions than existing released versions of cabal).
My suggestion would be that @dcoutts first cherry-picks this PR into the nix-local-build branch so we can experiment with it, before we merge this into master.