-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix has_command, CentOS detection, source installation and add OS update support #7
Conversation
``` $ command -v fooooo $ echo $? 1 $ /bin/sh -c command -v foooo $ echo $? 0 ``` We now just call `command` directly, which fixes ocaml-opam#6
The distribution name is "CentOS", not "Centos". Just force lowercase comparison to simplify this, as this may have varied between CentOS 6 and 7. Also improve the error message to distinguish between an unknown distribution and other parsing error.
@@ -41,11 +41,11 @@ let string_split char str = | |||
aux 0 | |||
|
|||
let has_command c = | |||
let cmd = Printf.sprintf "/bin/sh -c command -v %s" c in | |||
let cmd = Printf.sprintf "command -v %s" c in |
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.
I'm not sure this will work well. Based on experience with cmdliner that's a correct and multiplatform way of checking whether a command exists or not.
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.
command
is POSIX, but I can believe that it's not portable despite that. Would it be possible for cmdliner
to expose the find_cmd
logic so that we dont have to cut and paste it here?
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.
Would it be possible for cmdliner to expose the find_cmd logic so that we dont have to cut and paste it here?
I don't think cmdliner
is the place to expose such things. I'd say either just c&p or use Bos which exposes this logic in its API (but that's another dependency).
…ion. This is useful on Debian or Ubuntu where package installation may fail if the sets are not up-to-date with respect to latest security updates.
This pull request has become a bit feature packed :) I'm using it in the bulk builds with all this now though... |
Awesome! :) |
Fix has_command, CentOS detection, source installation and add OS update support
We now just call
command
directly, which fixes #6