Refactor cabal-install solver config log output

[erikd]

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]

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

[grayjay]

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 .

[grayjay]

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 .

[grayjay]

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

[grayjay]

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]

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]

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.

[grayjay]

Which doctests failed? Do you mean the ones in Message.hs? Those are just testing an internal function, so I think that it would be better to update the doctests than change the solver's output.

[grayjay]

I did another full review. I have some followup comments, and I saw that there are two unresolved comments above (documentation for new types, and removal of the EntrySkipping constructor).

[erikd]

I have tried to keep commits that directly address review comments above separate. I think I have addressed them all, but I am finding it extremely difficult to keep track of what has and has not been addressed.

There are no longer any changes at all in the file cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs.

[grayjay]

@erikd Thanks for keeping the commits separate for review. I've had a lot of trouble finding the unresolved comments in the Github UI for this PR, too. I looked through the comments again, and I think these are the only remaining ones, including one new one:

• Removing the EntrySkipping constructor:

  From #9159: I think it is a little confusing to have a separate Log Entry constructor for a case that should never happen (EntrySkipping). Can this case just use ErrorMsg?

• Documentation for the new types in SummarizedMessage.hs:

  From #9159: It would help to have some comments describing how the new types and Message relate to each other.

• Reverting the changes to Progress.hs

• Reverting the renaming of createErrorMsg, rerunSolverForErrorMsg, and finalErrorMsg

[erikd]

* Removing the `EntrySkipping` constructor:

That constructor is used. In displayMessage it gets rendered as:

displayMessage (EntrySkipping cs) = "skipping: " ++ showConflicts cs

and that constructor is the one of two that has the string "skipping: " as part of its rendering. If I modify the string for that constructor to "skipping single: " but all the unit tests pass, so obviously it is not needed.

However, just removing it not so straight forward. Currently in summarizeMessages there is a case:

go !l (Step (Skip conflicts)         ms) = Step (SummarizedMsg $ AtLevel l $ (EntrySkipping conflicts)) (go l ms)

This cannot just be dropped or compiler says:

src/Distribution/Solver/Modular/Message.hs:110:5: warning: [GHC-62161] [-Wincomplete-patterns]
    Pattern match(es) are non-exhaustive
    In an equation for ‘go’:
        Patterns of type ‘Int’, ‘Progress Message a b’ not matched:
            _ (Step (Skip _) _)
    |
110 |     go !_ (Done x)                           = Done x

Its also not possible to just replace EntrySkipping with EntrySkipMany because that constructor has extra parameters that are not available in case above.

The _ (Step (Skip _) _) case case cannot be removed either because Step (Skip _) is used elsewhere.

  From [Refactor `cabal-install` solver config log output #9159](https://github.com/haskell/cabal/pull/9159): I think it is a little confusing to have a separate ~`Log`~ `Entry` constructor for a case that should never happen (`EntrySkipping`). Can this case just use `ErrorMsg`?

Just replacing EntrySkipping with ErrorMsg (now called StringMsg) is not so simple. The EntrySkippping constructor has Set CS.Conflict parameter, which would need to be rendered to put it in a StringMsg. This would pull rendering of messages back into the summarizeMessages function.

There are skipping: .... messages in cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs but as I pointed out above, none of the EntrySkipping renderings are in the unit tests.

* Documentation for the new types in `SummarizedMessage.hs`:

Have added documentation but as I did not actually write this code I can only guess the original intent. Suggestions for improvement welcome.

  From [Refactor `cabal-install` solver config log output #9159](https://github.com/haskell/cabal/pull/9159): It would help to have some comments describing how the new types and `Message` relate to each other.

Done, but again I can only guess at the original intent. Suggestions for improvement welcome.

* Reverting the changes to `Progress.hs`

Done. The changes were basically re-exporting types from Distribution.Solver.Types.SummarizedMessage. Reverting the Progess.hs changes required SummarizedMessage to be imported elsewhere.

* Reverting the renaming of `createErrorMsg`, `rerunSolverForErrorMsg`, and `finalErrorMsg`

Done.

[grayjay]

* Removing the `EntrySkipping` constructor:

...

There are skipping: .... messages in cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs but as I pointed out above, none of the EntrySkipping renderings are in the unit tests.

I realized that this issue is more involved than I initially thought, and there may be other impossible cases in summarizeMessages, too, so it doesn't make sense to handle the issue in this PR. I'm fine with leaving the EntrySkipping constructor.

* Documentation for the new types in `SummarizedMessage.hs`:

Have added documentation but as I did not actually write this code I can only guess the original intent. Suggestions for improvement welcome.

I added a few suggestions.

[grayjay]

Thanks! Looks good!

[erikd]

Squashed into a single commit. Checked that the diff between the squashed version of the previous commit was zero length.

[grayjay]

Great! Could you please remove the part of the commit message about changing the solver's output and fixing #4251, now that it only contains the refactoring?

Has been addressed.

[grayjay]

@erikd Thanks! I think this is ready for a merge label.

[erikd]

Rebased against master.

[erikd]

The CI failure is an unhandled error in Python code.

[geekosaur]

That's a "shouldn't happen". Hackage has been hiccuping since the planned maintenance window ended.