Drastically simplify output when packages are missing in solve #11040

ElectreAAS · 2024-10-23T11:34:35Z

Fixes #10987.
When some packages are missing entirely, we only report that as that seems way more pressing than the wall of info about the state of the solver seen in the issue.

The fix was found while working on #11015, but is independent from it.

ElectreAAS · 2024-10-23T11:38:53Z

cc @v-gb with this PR the inscrutable wall you had should now be entirely replaced by "The following packages couldn't be found: - unix" 🙂

v-gb · 2024-10-23T12:48:02Z

Thanks, that'd be much more understandable.
The format of the list of missing packages seems a bit weird though. To me, one of these formats would make sense

  The following packages couldn't be found:
  - ocamlformat
  - whatever

  The following packages couldn't be found: ocamlformat, whatever

  The following packages couldn't be found: ocamlformat whatever

but it seems to be (from reading the code, I could be mistaken):

The following packages couldn't be found: - ocamlformat - whatever

ElectreAAS · 2024-10-23T13:59:46Z

You're right. Should be fixed now.

Alizter · 2024-10-25T16:29:23Z

If we are moving 0install to be our own library, we might as well just drop the messages being created in 0install and only have the data. We can then use our own Pp library to render the messages nicely.

rgrinberg · 2024-10-26T10:52:32Z

It might be independent from forking the solver, but I think we should finish forking the solver first. In general, we don't make changes in vendored repos without a lot of extra overhead (update scripts, maintaining commits in separate repos). If you fork the solver, you will avoid doing all that work in this PR.

Leonidas-from-XIV · 2024-11-01T10:22:18Z

Now that #11015 is merged this needs to be rebased and then we can proceed.

ElectreAAS · 2024-11-04T14:43:05Z

Rebased on main!

Leonidas-from-XIV

Thanks for taking care of this. Here some comments on the code.

src/opam-0install/lib/solver.ml

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

ElectreAAS · 2024-11-13T15:44:23Z

Moving to Pp introduces a fair amount of noise in the form of type t -> type 'tag t to accommodate for Pp's notion of tags that are not used at the moment. We could stylize solver failures further with them. What do you think?

Alizter · 2024-11-13T16:19:27Z

src/0install-solver/diagnostics.ml

-    let pp f = function
-      | UserRequested r -> pf f "User requested %s" (format_restrictions [ r ])
+    let pp = function
+      | UserRequested r -> Pp.verbatimf "User requested %s" (format_restrictions [ r ])


Pp.textf is preferred for this, otherwise things won't wrap well.

Basically, anywhere you have verbatim replace it with text.

That's what I did initially, but I ended up with this kind of formatting:

Rejected candidates: bar.0.0.1: In same conflict class (ccc) as foo

I expect a fair amount of these messages are wrapped in a vbox at some point, which means that basically all spaces in text are used as line breaks

That indicates that you have too few boxes around and you have some vboxes that need some more thought. I will give you some help once I've read some of the messages.

Alizter · 2024-11-13T17:02:19Z

src/0install-solver/diagnostics.ml

    ;;
  end

  (** Represents a single interface in the example (failed) selections produced by the solver.
      It partitions the implementations into good and bad based (initially) on the split from the
      impl_provider. As we explore the example selections, we further filter the candidates. *)
  module Component = struct
-    type rejection_reason =
+    type 'tag rejection_reason =


Suggested change

type 'tag rejection_reason =

type 'tag rejection_reason =

No need to do this, the tag will always be Stdune.User_message.Style.t.

It's a bit tricky to do, but replace everything with 'tag that you introduced with Stdune.User_message.Style.t. We can look at cleaning up that name at another time using our Import module, but there will be too many conflicts for now. Sometimes there is a _ instead of a 'tag in the interface just so that you are aware. Also any unit Pp.t can be taken as Stdune.User_message.Style.t Pp.t but that seems to only be for debug stuff.

If 'tag is always Stdune.User_message.Style.t, why is it parameterized? We could have a type alias in stdune if it's always the same

It's not always the same, sometimes it carries style information like color and italics etc. That's why it is polymorphic everywhere else. For the output of the solver none of this is relevant and we should try to keep types simple rather than making them needlessly polymorphic.

Should be better now.
I couldn't shadow the long Stdune.User_message.Style.t everywhere, so it spills out a little in dune_pkg, but that seems fine to me

Alizter · 2024-11-13T17:26:26Z

src/0install-solver/diagnostics.ml

-      (pp_rolemap ~verbose)
-      reasons
+    Pp.verbatim "Can't find all required implementations:"
+    ++ Pp.newline


This should be a break with a vbox surrounding the Pp so that it is formatted the correct way. You might need a hovbox on the sentence above.

src/0install-solver/sat.ml

Alizter · 2024-11-13T17:35:48Z

Try something like this:

Pp.vbox (
  Pp.concat
  [ Pp.paragraph "Sentece describing something, but not breaking:"
  ; Pp.enumerate ~f candidates])

The vbox will mean each entry is formatted vertically, and the Pp.paragraph will take care of wrapping text more generally.

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

ElectreAAS force-pushed the cleaner-solver-failure branch from 0867e55 to ef6efbe Compare November 4, 2024 14:42

rgrinberg added the package management label Nov 5, 2024

Leonidas-from-XIV reviewed Nov 5, 2024

View reviewed changes

src/opam-0install/lib/solver.ml Outdated Show resolved Hide resolved

src/opam-0install/lib/solver.ml Outdated Show resolved Hide resolved

src/opam-0install/lib/solver.ml Outdated Show resolved Hide resolved

ElectreAAS added 5 commits November 7, 2024 17:04

Drastically simplify output when packages are missing in solve

d7bf2c1

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Don't use List.is_empty, it's too new

beae97c

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Apply new error wording to other test

ddb6a53

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Better formatting of missing packages

cb44483

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Removed dependency on Fmt, changes in wording

7fc6f83

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

ElectreAAS force-pushed the cleaner-solver-failure branch from 5349da3 to 7fc6f83 Compare November 7, 2024 16:05

Transition from Format to Pp all around 0install

68e28d2

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Alizter reviewed Nov 13, 2024

View reviewed changes

src/0install-solver/sat.ml Show resolved Hide resolved

ElectreAAS added 3 commits November 14, 2024 17:23

Use text and paragraph instead of verbatim, add boxes

49e5169

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Un-polymorphize everything related to Pp

226f5b7

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Use paragraph instead of verbatim also in dune_pkg

8a7564b

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drastically simplify output when packages are missing in solve #11040

Drastically simplify output when packages are missing in solve #11040

ElectreAAS commented Oct 23, 2024

ElectreAAS commented Oct 23, 2024

v-gb commented Oct 23, 2024

ElectreAAS commented Oct 23, 2024

Alizter commented Oct 25, 2024

rgrinberg commented Oct 26, 2024

Leonidas-from-XIV commented Nov 1, 2024

ElectreAAS commented Nov 4, 2024

Leonidas-from-XIV left a comment

ElectreAAS commented Nov 13, 2024

Alizter Nov 13, 2024

Alizter Nov 13, 2024

ElectreAAS Nov 13, 2024

Alizter Nov 13, 2024

Alizter Nov 13, 2024

Alizter Nov 13, 2024

ElectreAAS Nov 13, 2024

Alizter Nov 13, 2024 •

edited

Loading

ElectreAAS Nov 14, 2024

Alizter Nov 13, 2024

Alizter commented Nov 13, 2024 •

edited

Loading

Drastically simplify output when packages are missing in solve #11040

Are you sure you want to change the base?

Drastically simplify output when packages are missing in solve #11040

Conversation

ElectreAAS commented Oct 23, 2024

ElectreAAS commented Oct 23, 2024

v-gb commented Oct 23, 2024

ElectreAAS commented Oct 23, 2024

Alizter commented Oct 25, 2024

rgrinberg commented Oct 26, 2024

Leonidas-from-XIV commented Nov 1, 2024

ElectreAAS commented Nov 4, 2024

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

ElectreAAS commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alizter Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alizter commented Nov 13, 2024 • edited Loading

Alizter Nov 13, 2024 •

edited

Loading

Alizter commented Nov 13, 2024 •

edited

Loading