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

Properly display error message when depext fails #323

Merged
merged 5 commits into from
Aug 1, 2022

Conversation

RyanGibb
Copy link

Addresses #258

@RyanGibb
Copy link
Author

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
Copy link
Member

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
Copy link
Author

RyanGibb commented Aug 1, 2022

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

Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for the fix!

@Leonidas-from-XIV we might want to add an integration test for this, I wonder if we can easily write a portable one.

@Leonidas-from-XIV
Copy link
Member

@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 :(

@Leonidas-from-XIV Leonidas-from-XIV merged commit 7903fa5 into tarides:main Aug 1, 2022
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Nov 21, 2022
CHANGES:

### Added

- Add support for specifying remote URLs in `x-opam-monorepo-repositories`
  (tarides/opam-monorepo#284, tarides/opam-monorepo#317, @Leonidas-from-XIV)

### Fixed

- Enable locking of packages with depexts even with uninitialized system
  package manager state (tarides/opam-monorepo#322, @Leonidas-from-XIV)
- Fix a bug where `pull` would crash if the lock file contained no package to
  vendor (tarides/opam-monorepo#321, @NathanReb)
- Display a better error message when the depext command fails when getting the
  status of the packages (tarides/opam-monorepo#258, tarides/opam-monorepo#323, @RyanGibb, @Julow)
- Take `archive-mirrors` from the global opam configuration into account to
  allow more local caches (tarides/opam-monorepo#337, @hannesm)
- Log at WARN level when opam-monorepo chooses a source for a package that
  doesn't match the package's version (tarides/opam-monorepo#352, @reynir)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants