-
Notifications
You must be signed in to change notification settings - Fork 27
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
Respect the "depext" field of global opam config #344
base: main
Are you sure you want to change the base?
Conversation
e68d926
to
201adb6
Compare
The context for this change is that I was trying to do the mirage tutorial on nixos and was running into an error when opam-monorepo tried to check the status of depexts:
Opam doesn't know how to install depexts on nixos, and even if it did, I'd rather it not mess with my system packages. Possibly relevant to #258 |
Thanks, could you also add a command line flag to control the setting? As-is it makes builds less reproducible as it reads more from opam settings (a thing that we're gradually trying to reduce to zero for higher reproduceability, with all settings controlled by the CLI/opam-files). |
(I am confused why even despite #323 you get this stacktrace, there is some kind of error still lurking in the code) |
The stacktrace only prints out when I run opam-monorepo as an opam plugin. If I run opam-monorepo instead, the output is:
|
201adb6
to
e31502f
Compare
I added a flag |
I would suggest to make the flag |
Prior to this change, opam-monorepo will attempt to install external dependencies, even if this field is set to false.
e31502f
to
88b08ad
Compare
Good suggestion. Updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is nice. Some small comments on how to make it more consistent/readable but the concept is solid now.
@@ -17,7 +17,8 @@ let available_packages pkgs = | |||
| available_pkgs, _not_found_pkgs -> Ok available_pkgs | |||
| exception Failure msg -> Error (`Msg msg) | |||
|
|||
let run (`Root root) (`Lockfile explicit_lockfile) dry_run (`Yes yes) () = | |||
let run (`Root root) (`Lockfile explicit_lockfile) dry_run resolve_depexts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, can you wrap this argument in a `` Resolve_depext
constructor?
@@ -17,6 +17,7 @@ | |||
vendor (#321, @NathanReb) | |||
- Display a better error message when the depext command fails when getting the | |||
status of the packages (#258, #323, @RyanGibb, @Julow) | |||
- Respect the "depext" config option when installing depexts (#344, @gridbugs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated since it's configurable now.
The following packages may need to be installed manually:" | ||
in | ||
l "%s\n%s" message | ||
(String.concat ~sep:"\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use Fmt.list ~sep:(Fmt.any "\n") string
to avoid that concat.
Or even (untested, but I enjoy how much one can do with Fmt
combinators):
let pp_sys_pkg = Fmt.of_to_string OpamSysPkg.to_string in
let pp_item ppf = Fmt.pf ppf "- %a" pp_sys_pkg in
l "%s\n%a" message (Fmt.list ~sep:(Fmt.any "\n") pp_item) (OpamSysPkg.Set.elements pkgs)
Prior to this change, opam-monorepo will attempt to install external dependencies, even if this field is set to false.