Refactor cabal-install solver config log output #10854

erikd · 2025-03-26T01:21:00Z

Includes:

Apply some of @grayjay and @mpickering comments
Fix Solver "rejecting" message is too verbose #4251

This is the PR #9541 rebased and fixed to build.

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
- Is the change significant? If so, remember to add significance: significant in the changelog file.
The documentation has been updated, if necessary.
Manual QA notes have been included.
Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

ulysses4ever · 2025-03-26T02:04:04Z

Any chance you could add examples of what the new output looks like? Say, in the PR description.

grayjay · 2025-03-27T01:01:05Z

I haven't had a chance to look at the code yet, but, if I remember correctly, #9541 was a followup to #9159. Would it make sense to first update and merge #9159?

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

erikd · 2025-03-27T23:04:33Z

I haven't had a chance to look at the code yet, but, if I remember correctly, #9541 was a followup to #9159. Would it make sense to first update and merge #9159?

According to this comment this seems like a precursor to #9159 .

grayjay · 2025-03-31T08:01:24Z

According to this comment this seems like a precursor to #9159 .

The original change in #9159 was split into a refactoring change and a fix for #4251. Now the refactoring change is in #9159, and the fix for #4251 is in #9541. #9541 contains #9159, because the fix depends on the refactoring.

#9560 has also been merged since #9541 was written and helps address #4251. Do you know how this fix compares now?

erikd · 2025-04-01T06:39:23Z

Current version of this PR aims to minimize the differences in the cabal-install:unit-test output.

Still need to provide a information about how this version improves the solver output compared to the current output.

erikd · 2025-04-02T02:39:13Z

The changes between the output on master and the output in this PR is mostly incredibly minor (as shown by the tiny patch to the tests).

This is the only difference I could find in the cabal-install:unit-tests output:
master

      minimize conflict set with --minimize-conflict-set:                                                                              FAIL
        tests/UnitTests/Distribution/Solver/Modular/DSL/TestCaseUtils.hs:274:
        Unexpected error:
        Could not resolve dependencies:
        [__0] trying: A-3.0.0 (user goal)
        [__1] next goal: D (dependency of A)
        [__1] rejecting: D-1.0.0 (conflict: A => D==2.0.0)
        [__1] fail (backjumping, conflict set: A, D)
        After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: A (5), D (4)
        Use -p '/minimize conflict set with --minimize-conflict-set/' to rerun this test only.
      show original conflict set with --no-minimize-conflict-set:                                                                      FAIL
        tests/UnitTests/Distribution/Solver/Modular/DSL/TestCaseUtils.hs:274:
        Unexpected error:
        Could not resolve dependencies:
        [__0] trying: A-3.0.0 (user goal)
        [__1] next goal: B (dependency of A)
        [__1] rejecting: B-1.0.0 (conflict: A => B==2.0.0)
        [__1] fail (backjumping, conflict set: A, B)
        After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: A (7), B (2), C (2), D (2)
        Try running with --minimize-conflict-set to improve the error message.
        Use -p '/show original conflict set with --no-minimize-conflict-set/' to rerun this test only.

New:

      minimize conflict set with --minimize-conflict-set:                                                                              FAIL
        tests/UnitTests/Distribution/Solver/Modular/DSL/TestCaseUtils.hs:268:
        Unexpected solver log:
        targets: A
        constraints: 
          any.base installed (non-reinstallable package)
          any.ghc-bignum installed (non-reinstallable package)
          any.ghc-internal installed (non-reinstallable package)
          any.ghc-prim installed (non-reinstallable package)
          any.ghc installed (non-reinstallable package)
          any.integer-gmp installed (non-reinstallable package)
          any.integer-simple installed (non-reinstallable package)
          any.template-haskell installed (non-reinstallable package)
        preferences: 
        strategy: PreferLatestForSelected
        reorder goals: False
        count conflicts: True
        fine grained conflicts: True
        minimize conflict set: True
        independent goals: False
        avoid reinstalls: False
        shadow packages: False
        strong flags: False
        allow boot library installs: False
        only constrained packages: OnlyConstrainedNone
        max backjumps: infinite
        [__0] trying: A-3.0.0 (user goal)
        [__1] next goal: B (dependency of A)
        [__1] rejecting: B-1.0.0 (conflict: A => B==2.0.0)
        [__1] fail (backjumping, conflict set: A, B)
        [__0] trying: A-2.0.0
        [__1] trying: B-1.0.0 (dependency of A)
        [__2] next goal: C (dependency of A)
        [__2] rejecting: C-1.0.0 (conflict: A => C==2.0.0)
        [__2] fail (backjumping, conflict set: A, C)
        [__0] trying: A-1.0.0
        [__1] trying: B-1.0.0 (dependency of A)
        [__2] trying: C-1.0.0 (dependency of A)
        [__3] next goal: D (dependency of A)
        [__3] rejecting: D-1.0.0 (conflict: A => D==2.0.0)
        [__3] fail (backjumping, conflict set: A, D)
        [__0] fail (backjumping, conflict set: A, B, C, D)
        Found no solution after exhaustively searching the dependency tree. Rerunning the dependency solver to minimize the conflict set ({A, B, C, D}).
        Trying to remove variable "A" from the conflict set.
        [__0] trying: A-3.0.0 (user goal)
        [__1] next goal: B (dependency of A)
        [__1] rejecting: B-1.0.0 (conflict: A => B==2.0.0)
        [__1] fail (backjumping, conflict set: A, B)
        [__0] trying: A-2.0.0
        [__1] trying: B-1.0.0 (dependency of A)
        [__2] next goal: C (dependency of A)
        [__2] rejecting: C-1.0.0 (conflict: A => C==2.0.0)
        [__2] fail (backjumping, conflict set: A, C)
        [__0] trying: A-1.0.0
        [__1] trying: B-1.0.0 (dependency of A)
        [__2] trying: C-1.0.0 (dependency of A)
        [__3] next goal: D (dependency of A)
        [__3] rejecting: D-1.0.0 (conflict: A => D==2.0.0)
        [__3] fail (backjumping, conflict set: A, D)
        [__0] fail (backjumping, conflict set: A, B, C, D)
        Failed to remove "A" from the conflict set. Continuing with {A, B, C, D}.
        Trying to remove variable "B" from the conflict set.
        [__0] tryingE: A-3.0.0 (user goal)
        [__1] tryingE: C-1.0.0 (dependency of A)
        [__2] next goal: D (dependency of A)
        [__2] rejecting: D-1.0.0 (conflict: A => D==2.0.0)
        [__2] fail (backjumping, conflict set: A, D)
        [__0] skipping: A; 1.0.0, 2.0.0 (has the same characteristics that caused the previous version to fail: depends on 'D' but excludes version 1.0.0)
        [__0] fail (backjumping, conflict set: A, D)
        Successfully removed "B" from the conflict set. Continuing with {A, D}.
        Trying to remove variable "D" from the conflict set.
        [__0] trying: A-3.0.0 (user goal)
        [__1] next goal: D (dependency of A)
        [__1] rejecting: D-1.0.0 (conflict: A => D==2.0.0)
        [__1] fail (backjumping, conflict set: A, D)
        [__0] skipping: A; 1.0.0, 2.0.0 (has the same characteristics that caused the previous version to fail: depends on 'D' but excludes version 1.0.0)
        [__0] fail (backjumping, conflict set: A, D)
        Failed to remove "D" from the conflict set. Continuing with {A, D}.

I suppose the main benefit of this PR is that in the file cabal-install-solver/src/Distribution/Solver/Modular/Message.hs detection of errors is separated from reporting of errors.

erikd · 2025-04-02T03:35:47Z

I haven't had a chance to look at the code yet, but, if I remember correctly, #9541 was a followup to #9159. Would it make sense to first update and merge #9159?

I have had a look at #9159 (against master from 18 months ago) but its rather difficult what really changes between them. I am studying #9159 more closely to figure out what it actually does.

Update: I am not able find any significant differences between the behavior of the code in this PR and in #9541 .

grayjay · 2025-04-03T07:17:23Z

Update: I am not able find any significant differences between the behavior of the code in this PR and in #9541 .

Isn't this PR an updated version of #9541? Do you mean #9560?

erikd · 2025-04-06T22:19:23Z

Update: I am not able find any significant differences between the behavior of the code in this PR and in #9541 .

Isn't this PR an updated version of #9541? Do you mean #9560?

Yes, I got confused. This is an updated version of #9541. Ad for #9560, that has been merged but does not seem to be related this PR. I do think that #9159 is related.

So the correct comment is, "I am not able find any significant differences between the behavior of the code in this PR and in #9159 ".

grayjay · 2025-04-08T20:16:39Z

Here is the current state of each of the PRs, as I understand it:

#9159: It contains two commits (b20ea53 and f10dbcf) that refactor the code in preparation for the improvement to error messages in #9541.
#9541: It contains the two commits from #9159, as well as a third commit (e4775b4) to improve error messages by removing duplication of package names.
#9560: It removes duplication of package names in error messages, without significant refactoring.

Since #9560 was already merged, it seems like the main difference between this PR and master is the refactoring. Are you interested in merging this just for the refactoring? Are you planning to make additional changes to the solver log that depend on it?

erikd · 2025-04-08T21:28:38Z

My hope is to get this refactor merged (after the fix suggested by @mpickering ). Then I intend to work on improving error messages as per commit e4775b4 .

grayjay · 2025-04-09T00:30:57Z

I meant that this PR already contains the contents of e4775b4 (removing duplicate package names, similar to #9560), so I was wondering if you were planning to make more changes beyond e4775b4 that depend on the refactoring.

ulysses4ever · 2025-06-04T07:30:59Z

Try rebasing on top of current master (if not already).

grayjay

I responded to some of your latest comments and changes, but I haven't done another full review yet.

cabal-install-solver/src/Distribution/Solver/Types/Progress.hs

cabal-install-solver/src/Distribution/Solver/Modular.hs

grayjay · 2025-06-04T06:47:27Z

cabal-install-solver/src/Distribution/Solver/Modular.hs

+    retryMap :: (t -> step) -> RetryLog t fail done -> RetryLog step fail done
+    retryMap f l = fromProgress $ (\p -> foldProgress (\x xs -> Step (f x) xs) Fail Done p) $ toProgress l


I'm not sure that it's possible to convert the RetryLogss efficiently, but I think that it would be easier to remove the calls to retryMap, as I described in my previous comment.

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

erikd · 2025-06-04T07:56:44Z

Try rebasing on top of current master (if not already).

It says:

Current branch erikd/cosmetic-changes-2 is up to date.

ulysses4ever · 2025-06-04T15:06:42Z

Let me try to restart the failing jobs then. It's the same test in all configuration, so this test may be up for the flaky marker if it fails like that ...

erikd · 2025-06-05T01:27:03Z

I have left the new fixes for PR comments discrete for easier review. When this is approved for merge I will squash them down as appropriate.

erikd · 2025-06-05T03:42:24Z

One of the "Validate" failures is with ghc-9.6.7 and cabal-install-3.12.1.0. With those versions it passes locally.

The "Bootstrap" failure is a failure in Python code and my PR does not touch any Python code.

I have already tried rebasing against master.

ulysses4ever · 2025-06-05T12:57:25Z

I'll try to patch up the failing test (it's the same one again) today.

ulysses4ever · 2025-06-05T13:44:38Z

I restarted the bootstrap job, but it failed again in the same way: with http.client.IncompleteRead: IncompleteRead(3307 bytes read). https://github.com/haskell/cabal/actions/runs/15456775662/job/43546021620?pr=10854 So, network problems in GitHub runners strike again. I feel like GH is forcing us to make the testsuite more hermetic, which is a great thing!

@grayjay

Includes: * Apply some of @grayjay and @mpickering comments * Fix haskell#4251 Co-Authored-By: Erik de Castro Lopo <[email protected]>

ulysses4ever · 2025-06-05T14:50:27Z

I rebased the branch here on the latest master, which contains a fix for the failing test. Please, reset your local branch when you get there.

ulysses4ever

This PR needs a changelog entry.

Also, the PR description is missing a self-contained description of the change — please add one. I'd expect an example of solver output after this change.

erikd · 2025-06-06T01:12:59Z

I'd expect an example of solver output after this change.

Example solver output is in: #10854 (comment)

Solver output has not changed since that.

These fixes are require due to improvements in solver error reporting.

Extensive effort was put in to simplify the solver. In the end only a pretty minor simplification was possible.

ulysses4ever · 2025-06-06T03:31:35Z

Example solver output is in: #10854 (comment)

Thank you. I think it should be referenced from or inserted in the PR description. Currently, it's hard to find.

So, the new output is longer than the old one. It's the opposite of what most people I talk to want. @Mikolaj @ffaf1 @geekosaur, thoughts?

erikd · 2025-06-06T03:50:19Z

So, the new output is longer than the old one. It's the opposite of what most people I talk to want. @Mikolaj @ffaf1 @geekosaur, thoughts?

The output in that example is actually a rare one. In most cases the differences are really minor as show by the differences requried to make the validate tests pass: 1b701f5

grayjay · 2025-06-06T07:05:23Z

I'm confused now. I had thought that we agreed that this PR was just a refactoring change to add an intermediate log message type (SummarizedMessage) to the processing of the solver log. I thought that all of the behavior changes from #9541 were already merged in #9560 and that any remaining changes to strings in the solver unit tests were just bugs that needed to be worked out.

Was the change in #10854 (comment) automatically caught by tests, or did you need to manually compare the solver output?

geekosaur · 2025-06-06T13:51:53Z

the new output is longer than the old one. It's the opposite of what most people I talk to want

I think longer is fine as long as it's more comprehensible. That's the real problem most people have with the current solver output, it's a lot of gobbledygook that requires someone who has some idea of what's going on inside the solver to interpret.

erikd requested review from mpickering and grayjay March 26, 2025 01:21

erikd mentioned this pull request Mar 26, 2025

Cosmetic changes to cabal-install summarized solver output #9541

Closed

5 tasks

erikd force-pushed the erikd/cosmetic-changes-2 branch 5 times, most recently from e602461 to 8c1868b Compare March 26, 2025 01:53

erikd force-pushed the erikd/cosmetic-changes-2 branch 5 times, most recently from c8f419c to 5a2528d Compare March 26, 2025 04:06

Bodigrim reviewed Mar 27, 2025

View reviewed changes

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs Outdated Show resolved Hide resolved

erikd force-pushed the erikd/cosmetic-changes-2 branch from 5a2528d to 78733cd Compare April 1, 2025 06:37

erikd force-pushed the erikd/cosmetic-changes-2 branch from 78733cd to b7b0c64 Compare April 2, 2025 00:27

erikd force-pushed the erikd/cosmetic-changes-2 branch from b7b0c64 to 6fbd8d7 Compare April 6, 2025 21:38

erikd force-pushed the erikd/cosmetic-changes-2 branch from 6fbd8d7 to dce06cb Compare April 8, 2025 01:52

grayjay reviewed Jun 4, 2025

View reviewed changes

erikd force-pushed the erikd/cosmetic-changes-2 branch 2 times, most recently from 7f2599e to b77ca13 Compare June 5, 2025 02:07

Refactor cabal-install solver config log output

051b4ec

Includes: * Apply some of @grayjay and @mpickering comments * Fix haskell#4251 Co-Authored-By: Erik de Castro Lopo <[email protected]>

ulysses4ever force-pushed the erikd/cosmetic-changes-2 branch from b77ca13 to af042db Compare June 5, 2025 14:48

ulysses4ever requested changes Jun 5, 2025

View reviewed changes

erikd added 8 commits June 6, 2025 12:11

Rename SolverState back to Message

2cca4c9

tests: Fixes to expected message strings

1b701f5

These fixes are require due to improvements in solver error reporting.

Address PR comments

281308f

Modular-solver: Simplify

81a7372

Extensive effort was put in to simplify the solver. In the end only a pretty minor simplification was possible.

Use showOption instead of showOptions for singleton lists

7849a47

Move Message type back to ../Solver/Modular/Message.hs

f38afd8

Remove all calls to retryMap and remove it

120b793

Add changelog entry

e0b480a

erikd force-pushed the erikd/cosmetic-changes-2 branch from af042db to e0b480a Compare June 6, 2025 02:27

erikd requested a review from ulysses4ever June 6, 2025 05:49

grayjay mentioned this pull request Jun 7, 2025

Refactor cabal-install solver config log output #9159

Closed

1 task

		retryMap :: (t -> step) -> RetryLog t fail done -> RetryLog step fail done
		retryMap f l = fromProgress $ (\p -> foldProgress (\x xs -> Step (f x) xs) Fail Done p) $ toProgress l

Refactor cabal-install solver config log output #10854

Are you sure you want to change the base?

Refactor cabal-install solver config log output #10854

Uh oh!

Conversation

erikd commented Mar 26, 2025 • edited by ulysses4ever Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses4ever commented Mar 26, 2025

Uh oh!

grayjay commented Mar 27, 2025

Uh oh!

Uh oh!

erikd commented Mar 27, 2025

Uh oh!

grayjay commented Mar 31, 2025

Uh oh!

erikd commented Apr 1, 2025

Uh oh!

erikd commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikd commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grayjay commented Apr 3, 2025

Uh oh!

erikd commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grayjay commented Apr 8, 2025

Uh oh!

erikd commented Apr 8, 2025

Uh oh!

grayjay commented Apr 9, 2025

Uh oh!

ulysses4ever commented Jun 4, 2025

Uh oh!

grayjay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grayjay Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erikd commented Jun 4, 2025

Uh oh!

ulysses4ever commented Jun 4, 2025

Uh oh!

erikd commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikd commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses4ever commented Jun 5, 2025

Uh oh!

ulysses4ever commented Jun 5, 2025

Uh oh!

ulysses4ever commented Jun 5, 2025

Uh oh!

ulysses4ever left a comment

Choose a reason for hiding this comment

Uh oh!

erikd commented Jun 6, 2025

Uh oh!

ulysses4ever commented Jun 6, 2025

Uh oh!

erikd commented Jun 6, 2025

Uh oh!

grayjay commented Jun 6, 2025

Uh oh!

geekosaur commented Jun 6, 2025

Uh oh!

Uh oh!

erikd commented Mar 26, 2025 •

edited by ulysses4ever

Loading

erikd commented Apr 2, 2025 •

edited

Loading

erikd commented Apr 2, 2025 •

edited

Loading

erikd commented Apr 6, 2025 •

edited

Loading

erikd commented Jun 5, 2025 •

edited

Loading

erikd commented Jun 5, 2025 •

edited

Loading