-
Notifications
You must be signed in to change notification settings - Fork 371
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
Use a C stub to call the uname function from the C standard library instead of calling the uname POSIX command #6217
Conversation
Works great on my OpenBSD desktop:
One test fails:
but looks unrelated (that |
Thanks. This PR fixes #6215 for me. My opinion may not matter on the patch, but that looked good to me too. I like the switch to |
5645f51
to
e2f67c1
Compare
turns out we still need to call the We could rely on I've also removed the C stub for FreeBSD as |
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.
- master changes needs to be updated with changes of the PR itself (freebsd uname, stubs, + API ones)
Fix opam on FreeBSD
commit- oneliner commit message is not enough explicit, maybe something like "Fix FreeBSD version probing"
- commit message is explaining the process_in changes but not the uname call update
- I don't know what is better, have everything on the same commit as freebsd fix, or two seperate commit each one having its purpose (process_in fix and uname call code simplification)
- I don't know if we should keep a backward compatiblity for the old
uname
andgetconf
, or just update the API and highlight the change in the release note.
src/core/opamStd.mli
Outdated
@@ -510,11 +510,12 @@ module Sys : sig | |||
(** Queried lazily *) | |||
val os: unit -> os | |||
|
|||
(** The output of the command "uname", with the given argument. Memoised. *) | |||
val uname_cmd: string -> string option | |||
(** The output of the command "uname -U". FreeBSD only. *) |
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.
Also, it's intended to be used only on freebsd, but no check is done for that. Proposal:
(** FreeBSD version, probed via "uname -U".
To use only on FreeBSD, otherwise returns [None]. *)
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 about this suggestion. The fact that the function returns the freebsd version is already described in the function name. I prefer my version
I've split off the fix for OpenBSD to #6230 for simplification. Leaving this PR just for the |
05c71d0
to
e857ce2
Compare
90f38fb
to
f389885
Compare
Except the naming, LGTM! We can discuss that monday. |
As discussed on dev meeting, we should use |
…nstead of calling the uname POSIX command
f389885
to
bf82d64
Compare
Queued on top of #6216F.i.x.e.s #6215For the backport to 2.3, do we want to backport the entire PR or just the OpenBSD fix?Partially backported to 2.3 in #6228Queued on #6230merged