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

More clearly show installation warnings with the package name which generated them #2995

Open
dbuenzli opened this issue Jul 8, 2017 · 2 comments

Comments

@dbuenzli
Copy link
Contributor

dbuenzli commented Jul 8, 2017

The following is misleading, the WARNING is likely to be attributed to ocaml.4.03.0:

> opam install uchar 
The following actions will be performed:
  ∗  install ocaml      4.03.0                [required by uchar]
  ∗  install ocamlbuild 0.11.0                [required by uchar]
  ∗  install uchar      0.0.1
===== ∗  3 =====
Do you want to continue ? [Y/n] y

=-=- Gathering sources =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=  🐫 
[uchar.0.0.1] downloaded from https://opam.ocaml.org/2.0/cache
[ocamlbuild.0.11.0] downloaded from https://opam.ocaml.org/2.0/cache

=-=- Processing actions -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=  🐫 
∗  installed ocaml.4.03.0
[WARNING] Errors in /Users/dbuenzli/.opam/4.03.0/.opam-switch/build/ocamlbuild.0.11.0/ocamlbuild.install, some fields have been ignored:
            - At /Users/dbuenzli/.opam/4.03.0/.opam-switch/build/ocamlbuild.0.11.0/ocamlbuild.install:34:0:
              Invalid field docdir

∗  installed ocamlbuild.0.11.0
∗  installed uchar.0.0.1
@dra27
Copy link
Member

dra27 commented Oct 16, 2020

Issue is still present in opam 2.1. Simplest fix would to more visibly attach the warning to the package (i.e. have the blank line before rather than after).

@dra27 dra27 changed the title v2~beta3 install misleading output More clearly show installation warnings with the package name which generated them Oct 16, 2020
@dra27 dra27 added this to the Fix Me If You Can milestone Oct 16, 2020
@purplearmadillo77
Copy link
Contributor

The cause of the extra newline is this function call in show_errors.

OpamConsole.warning
             "Errors in %s, some fields have been ignored:\n%s"
             file
             (OpamStd.Format.itemize
                (fun e -> OpamPp.string_of_bad_format (Bad_format e))
               (List.rev_map snd errs))

OpamStd.Format.itemize will print the contents of a list with a new line after every element after given an appropriate stringification function. For example, [a; b; c] becomes "a\nb\nc\n". OpamConsole.warning prints the error message which contains the itemized string and then an additional newline. This results in not one but two newlines at the end (one from the last element of the list and one from the warning itself).

Not sure what the optimal solution would be. The extraneous newline seems like an unintentional interaction rather than a deliberate choice to add extra spacing after warnings. Changing itemize and warning in certain ways (such as the suggested deleting the final newline and moving it to the front) might ruin formatting in other places across opam.

Here are some other suggestions:

  1. Change itemize so it doesn’t include a newline after the last list element
  2. Change OpamConsole.warning so it doesn’t include a newline at the end of the warning
  3. Manually strip the extra newline in show_errors and move it to the front to preserve any important functionality of itemize and warning elsewhere in opam (if there is any)
  4. Add an "include newline" argument to OpamConsole.warning that defaults to true to itemize and/or warning, which gives the freedom to do 1 and 2 with the compatibility of 3
  5. Work around the double newlines by delaying warning printouts until the "installed packagename" line comes up (i.e. keep the current behavior but change the order of the output lines so the warning is attributed to the right package)
    (or add your own!)

The current behavior additionally means that any other combination of OpamConsole’s warning/error/note combined with itemize will have this double-newline problem. I'm happy to try to write a PR to fix this, but how do you recommend approaching the fix?

One more semi-related error: attempting to uninstall an uninstalled package will also have double newlines, for example:

$ ./opam uninstall astring uchar
[NOTE] uchar and astring are not installed.

Nothing to do.

but this is just because of the following in opamClient.ml

else
      OpamConsole.note "%s %s not installed.\n"
        (OpamStd.Format.pretty_list
           (List.map OpamFormula.short_string_of_atom not_installed))
        (match not_installed with [_] -> "is" | _ -> "are")

Since OpamConsole.note itself auto appends a newline, the one in the provided string is unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants