Refactor cabal-install solver config log output

[erikd]

Includes:

• Apply some of @grayjay and @mpickering comments
• Fix #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:

• [x] 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]

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

[sebright]

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?

[erikd]

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 .

[sebright]

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]

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]

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]

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 .

[sebright]

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]

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 ".

[sebright]

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

#9159: It contains two commits (b20ea537f7eee5058a50204e2e16856233052613 and f10dbcf7872927622250f25b463bb608882737a9) 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 (e4775b426c8aac4229509b51f98cc0f024c60478) 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]

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 https://github.com/haskell/cabal/commit/e4775b426c8aac4229509b51f98cc0f024c60478 .

[sebright]

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

[erikd]

@grayjay I had not planned to do any further refactoring after this, but would be happy to be pointed to work that needs to be done.

[sebright]

@erikd I feel like I still don't understand the reason for this PR, given that #9560 was already merged. What is your goal in updating #9541?

[erikd]

@erikd I feel like I still don't understand the reason for this PR, given that #9560 was already merged. What is your goal in updating #9541?

I think the replacement of a String type with SummarizedMessage is a significant cosmetic improvement.

If you disagree, please say so an I will close this PR.

[sebright]

I think that it's fine to merge just the refactoring, especially just replacing String with SummarizedMessage. #9159 still has a lot of unaddressed code review comments, though. I could try to copy the relevant ones to this PR if that would help.

[erikd]

@grayjay Let me look at https://github.com/haskell/cabal/pull/9159 and try to address those comments.

[erikd]

@grayjay Sorry for the delay. Here is the updated PR with the change requests make in #9159.

• LogSucessMsg -> LogSuccess
• LogFailureMsg -> LogFailure
• All constructors of Entry data type ( file cabal-install-solver/src/Distribution/Solver/Types/Progress.hs) renamed from Log* to Entry* .
• EntryMsg data type renamed to EntryAtLevel.
• Make SummarizedMsg type opaque (constructors not exported).
• Undo some un-needed formatting changes for imports

[erikd]

One of the CI failures is with ghc-9.6.7 and cabal-install-3.12.1.0 . When I run the validation tests locally with those two versions everything passes.

[ulysses4ever]

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

[erikd]

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

It says:

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

[ulysses4ever]

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]

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]

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]

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

[ulysses4ever]

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!

[ulysses4ever]

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.

[erikd]

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

Example solver output is in: https://github.com/haskell/cabal/pull/10854#issuecomment-2771197952

Solver output has not changed since that.

[ulysses4ever]

Example solver output is in: https://github.com/haskell/cabal/pull/10854#issuecomment-2771197952

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]

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: https://github.com/haskell/cabal/pull/10854/commits/1b701f53af6046a10b6b3220021c33507565bb07