search

tarides / opam-monorepo

Assemble dune workspaces to build your project and its dependencies as a whole
ISC License
130 stars 27 forks source link

Properly display error message when depext fails #323

Closed RyanGibb closed 2 years ago

RyanGibb commented 2 years ago

Addresses #258

RyanGibb commented 2 years ago

Before:

opam-monorepo: internal error, uncaught exception:
               Failure("External dependency handling not supported for OS family 'nixos'.")
               Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
               Called from CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 31, characters 17-27
               Re-raised at CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 36, characters 4-11
               Called from OpamSysInteract.packages_status in file "duniverse/opam/src/state/opamSysInteract.ml", line 224, characters 8-17
               Called from Duniverse_cli__Depext.run.(fun) in file "cli/depext.ml", line 29, characters 20-56
               Called from OpamGlobalState.with_ in file "duniverse/opam/src/state/opamGlobalState.ml", line 186, characters 14-18
               Re-raised at OpamStd.Exn.finalise in file "duniverse/opam/src/core/opamStd.ml", line 1372, characters 4-38
               Called from Cmdliner_term.app.(fun) in file "duniverse/cmdliner/src/cmdliner_term.ml", line 24, characters 19-24
               Called from Cmdliner_term.app.(fun) in file "duniverse/cmdliner/src/cmdliner_term.ml", line 22, characters 12-19
               Called from Cmdliner_eval.run_parser in file "duniverse/cmdliner/src/cmdliner_eval.ml", line 34, characters 37-44

After:

opam-monorepo: [ERROR] External dependency handling not supported for OS family 'nixos'.
Leonidas-from-XIV commented 2 years ago

Hi @RyanGibb, thanks for your PR! This is a pretty large try block, could you rewrite it in a way where it exits directly on the error and only continues if no exception is thrown? It looks like this could be lifted into the Error monad, that would make reading the code easier since it would avoid nested try blocks and pin-point the location of the failure (OpamSysInteract.packages_status) directly.

RyanGibb commented 2 years ago

Hi @Leonidas-from-XIV, thanks for the feedback! Yes of course. Is this a little better?

Leonidas-from-XIV commented 2 years ago

@NathanReb Both #258 as well as this PR mention nixos so I looked at how the failure happens and this is is when OPAM tries to determine the OS family. It does so by determining the OS and only if it is Linux it goes ahead and checks /etc/os-release. So we would need to fake /etc/os-release and fake uname -s output (for it to also fail on macOS) which seems quite complicated to do portably. Integration tests around OPAM are unfortunately quite a pain :(