Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Counter fromListWith reversing constraints order #9547

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Dec 20, 2023

This does not change behaviour because (it does, see test failure with ghc >= 9.6) all constraints are passed to the solver but when we fix #9511, the order will matter if we pick the lastest version equality constraint as the winning one and discard the rest (per-package) before solving. I'm proposing this fix now so that whoever works on this or reviews it will not be surprised (like I was) when constraints are given to the solver in reversed order.

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:

@philderbeast
Copy link
Collaborator Author

With ghc-9.8.1 and ghc-9.6.3 the output is different for MultiRepl/CabalTooOld/cabal.test.hs, but not with ghc-9.4.8:

$ cabal run cabal-testsuite:cabal-tests -- \
  --with-cabal=./dist-newstyle/build/x86_64-linux/ghc-9.8.1/cabal-install-3.11.0.0/x/cabal/build/cabal/cabal \
  cabal-testsuite/PackageTests/MultiRepl/CabalTooOld/cabal.test.hs --accept
$ git diff
diff --git a/cabal-testsuite/PackageTests/MultiRepl/CabalTooOld/cabal.out b/cabal-testsuite/PackageTests/MultiRepl/CabalTooOld/cabal.out
index f2253c671..fd4691ed1 100644
--- a/cabal-testsuite/PackageTests/MultiRepl/CabalTooOld/cabal.out
+++ b/cabal-testsuite/PackageTests/MultiRepl/CabalTooOld/cabal.out
@@ -6,6 +6,7 @@ Error: [Cabal-7107]
 Could not resolve dependencies:
 [__0] trying: pkg-a-0 (user goal)
 [__1] next goal: pkg-a:setup.Cabal (dependency of pkg-a)
-[__1] rejecting: pkg-a:setup.Cabal-<VERSION>/installed-<HASH>, pkg-a:setup.Cabal-3.8.0.0 (constraint from --enable-multi-repl requires >=3.11)
+[__1] rejecting: pkg-a:setup.Cabal-<VERSION>/installed-<HASH> (constraint from --enable-multi-repl requires >=3.11)
+[__1] rejecting: pkg-a:setup.Cabal-3.8.0.0 (constraint from minimum version of Cabal used by Setup.hs requires >=3.10)
 [__1] fail (backjumping, conflict set: pkg-a, pkg-a:setup.Cabal)
 After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: pkg-a:setup.Cabal (3), pkg-a (2)

As the author of this test @mpickering, does this surprise you?

@Mikolaj Mikolaj requested a review from grayjay December 22, 2023 10:43
@grayjay
Copy link
Collaborator

grayjay commented Dec 24, 2023

Why is the constraint overriding being implemented in the solver? Wouldn't it be better to implement it in cabal-install, where the constraints from different files are initially combined? Then cabal-install could set the ConstraintSource and only pass the active constraints to the solver.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Dec 24, 2023

@grayjay was your comment intended for #9510?

Copy link
Collaborator

@gbaz gbaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we should make this change, because we should change what constraints are fed into the solver, not how the solver processes them. Certainly different orders of constraints will necessarily change the traversal order of the search tree, which will lead to different error messages on failure, and may potentially in very rare extreme cases change which solution is found or if one is found in an allowed number of steps. However, because there is not a clear "best" order to check constraints in, I think this could create churn and confusion to end-users with no corresponding benefit.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Dec 24, 2023

Here we're devaluing cabal developers' time (mine - I got stung by this unexpected reversing of constraints) to avoid some potential hazard to cabal users' time. Do users expect the cabal solver to always give the same solution? I wouldn't have this expectation as a cabal user unless I pinned down all my dependencies.

@grayjay
Copy link
Collaborator

grayjay commented Dec 24, 2023

@philderbeast Sorry, I hadn't seen #9510. I think that any feature that depends on the order of the constraints in the solver would be fragile, because the solver doesn't rely on the order anywhere. The constraint order currently should only affect the solution if it changes the order that the solver chooses values for package versions or flags (goal order). (EDIT: The benefit of this property is that it leaves open the opportunity to manipulate constraint order in the future to add additional testing or optimizations) I think that a better change to make this clearer for developers would be to change the list to a set.

@gbaz
Copy link
Collaborator

gbaz commented Dec 25, 2023

above i suggested that changing the order "may potentially in very rare extreme cases change which solution is found or if one is found in an allowed number of steps" and i think that's mistaken here because this change doesn't change the order in which the tree is traversed, just in which constraints are checked at each traversal step, but at each step they're all checked regardless. i.e. its not the order of "all constraints" but just the order of "constraints on a given package name" and that should affect only error messages at best.

I do agree with @grayjay that changing the order one way or the other wouldn't be any particular improvement, but moving to a set over a list is more semantically accurate.

@philderbeast philderbeast force-pushed the fix/constraints-to-solver-not-reversed branch from 0e72ff8 to d079583 Compare February 28, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Override version equality constraints
4 participants