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

Use a C stub to call uname(2) instead of calling the uname(1) command #6217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion master_changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ users)

## Internal

## Internal: Unix
* Use a C stub to call `uname` instead of calling the `uname` command [#6217 @kit-ty-kate]

## Internal: Windows

## Test
Expand Down Expand Up @@ -132,6 +135,9 @@ users)

## opam-core
kit-ty-kate marked this conversation as resolved.
Show resolved Hide resolved
* `OpamStd.Sys.{get_terminal_columns,uname,getconf,guess_shell_compat}`: Harden the process calls to account for failures [#6230 @kit-ty-kate - fix #6215]
* `OpamStd.Sys.{uname,getconf}`: now accepts only one argument as parameter, as per their documentation [#6230 @kit-ty-kate]
* `OpamStd.Sys.getconf`: was removed, replaced by `get_conf_long_bit` [#6217 @kit-ty-kate]
* `OpamStd.Sys.getconf_long_bit`: was added, which returns the output of the `getconf LONG_BIT` command [#6217 @kit-ty-kate]
* `OpamStd.Sys.uname`: now returns the memoized result of `uname` C binding [#6217 @kit-ty-kate]
* `OpamStd.Sys.uname_freebsd_version`: was added, which returns the output of the `uname -U` command [#6217 @kit-ty-kate]
* `OpamStubs.get_stdout_ws_col`: new Unix-only function returning the number of columns of the current terminal window [#6244 @kit-ty-kate]
* `OpamSystem`: add `is_archive_from_string` that does the same than `is_archive` but without looking at the file, only analysing the string (extension) [#6219 @rjbou]
1 change: 1 addition & 0 deletions src/core/opamCommonStubs.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@

#if OCAML_VERSION < 50000
#define caml_unix_access unix_access
#define caml_uerror uerror
#endif

CAMLprim value opam_is_executable(value path)
Expand Down
35 changes: 14 additions & 21 deletions src/core/opamStd.ml
Original file line number Diff line number Diff line change
Expand Up @@ -991,20 +991,13 @@ module OpamSys = struct

let etc () = "/etc"

let memo_command =
rjbou marked this conversation as resolved.
Show resolved Hide resolved
let memo = Hashtbl.create 7 in
fun cmd arg ->
try Hashtbl.find memo (cmd, arg) with Not_found ->
let r =
match process_in cmd [arg] with
| None -> None
| Some x -> Some (OpamString.strip x)
in
Hashtbl.add memo (cmd, arg) r;
r
let uname =
let uname = lazy (OpamStubs.uname ()) in
fun () ->
Lazy.force uname

let uname = memo_command "uname"
let getconf = memo_command "getconf"
let uname_freebsd_version () = process_in "uname" ["-U"]
let getconf_long_bit () = process_in "getconf" ["LONG_BIT"]

let system =
let system = Lazy.from_fun OpamStubs.getPathToSystem in
Expand All @@ -1026,14 +1019,14 @@ module OpamSys = struct
let os = lazy (
match Sys.os_type with
| "Unix" -> begin
match uname "-s" with
| Some "Darwin" -> Darwin
| Some "Linux" -> Linux
| Some "FreeBSD" -> FreeBSD
| Some "OpenBSD" -> OpenBSD
| Some "NetBSD" -> NetBSD
| Some "DragonFly" -> DragonFly
| _ -> Unix
match (uname ()).sysname with
| "Darwin" -> Darwin
| "Linux" -> Linux
| "FreeBSD" -> FreeBSD
| "OpenBSD" -> OpenBSD
| "NetBSD" -> NetBSD
| "DragonFly" -> DragonFly
| _ -> Unix
end
| "Win32" -> Win32
| "Cygwin" -> Cygwin
Expand Down
11 changes: 7 additions & 4 deletions src/core/opamStd.mli
Original file line number Diff line number Diff line change
Expand Up @@ -510,11 +510,14 @@ module Sys : sig
(** Queried lazily *)
val os: unit -> os

(** The output of the command "uname", with the given argument. Memoised. *)
val uname: string -> string option
(** The output of the command "uname -U". FreeBSD only. *)
Copy link
Collaborator

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]. *)

Copy link
Member Author

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

val uname_freebsd_version: unit -> string option
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not just uname, but uname -U to retrieve freebsd version. Maybe something like "freebsd_version"

Copy link
Member Author

Choose a reason for hiding this comment

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

freebsd_version doesn't give the impression that a command is going to be called so i'd rather keep the current name

Copy link
Collaborator

Choose a reason for hiding this comment

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

uname gives the impression that a command is called but it is not the case. This kind of information should be in the documentation.


(** The output of the command "getconf", with the given argument. Memoised. *)
val getconf: string -> string option
(** The output of the command "getconf LONG_BIT". *)
val getconf_long_bit: unit -> string option

(** The memoized result of uname(2) *)
val uname : unit -> OpamStubs.uname

(** Append .exe (only if missing) to executable filenames on Windows *)
val executable_name : string -> string
Expand Down
11 changes: 11 additions & 0 deletions src/core/opamStubs.mli
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,14 @@ val get_stdout_ws_col : unit -> int
linked with stdout. If stdout isn't linked to any terminal
(e.g. redirection), then this function will return 0. A valid number
of columns should be strictly above 0. *)

type uname = {
sysname : string; (** uname -s *)
release : string; (** uname -r *)
machine : string; (** uname -m *)
}
(** A subset of the [struct utsname] C structure, as modified by uname(2),
converted to OCaml datatypes. *)

kit-ty-kate marked this conversation as resolved.
Show resolved Hide resolved
val uname : unit -> uname
(** Unix only. Returns info from uname(2) *)
1 change: 1 addition & 0 deletions src/core/opamStubs.unix.ml
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,4 @@ let getVersionInfo = that's_a_no_no
let get_initial_environment = that's_a_no_no

external get_stdout_ws_col : unit -> int = "opam_stdout_ws_col"
external uname : unit -> uname = "opam_uname"
6 changes: 6 additions & 0 deletions src/core/opamStubsTypes.ml
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,12 @@ type win32_version_info = {
(** Non-fixed string table. First field is a pair of Language and Codepage ID. *)
}

type uname = {
sysname : string;
release : string;
machine : string;
}

external is_executable : string -> bool = "opam_is_executable"
(** faccessat on Unix; _waccess on Windows. Checks whether a path is executable
for the current process. On Unix, unlike Unix.access, this is checked using
Expand Down
17 changes: 17 additions & 0 deletions src/core/opamUnix.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,20 @@ CAMLprim value opam_stdout_ws_col(value _unit) {
}
return Val_int(win.ws_col);
}

#include <sys/utsname.h>

CAMLprim value opam_uname(value _unit) {
struct utsname buf;
value ret;

if (-1 == uname(&buf)) {
caml_uerror("uname", Nothing);
}
ret = caml_alloc(3, 0);
Store_field(ret, 0, caml_copy_string(buf.sysname));
Store_field(ret, 1, caml_copy_string(buf.release));
Store_field(ret, 2, caml_copy_string(buf.machine));

return ret;
}
1 change: 1 addition & 0 deletions src/core/opamWin32Stubs.win32.ml
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,4 @@ external get_initial_environment : unit -> string list = "OPAMW_CreateEnvironmen
let that's_a_no_no _ = failwith "Unix only. This function isn't implemented."

let get_stdout_ws_col = that's_a_no_no
let uname = that's_a_no_no
12 changes: 6 additions & 6 deletions src/state/opamSysPoll.ml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ let normalise_arch raw =

let poll_arch () =
let raw = match Sys.os_type with
| "Unix" | "Cygwin" -> OpamStd.Sys.uname "-m"
| "Unix" | "Cygwin" -> Some (OpamStd.Sys.uname ()).machine
| "Win32" ->
begin match OpamStubs.getArchitecture () with
| OpamStubs.AMD64 -> Some "x86_64"
Expand All @@ -56,7 +56,7 @@ let poll_arch () =
| "Unix" | "Cygwin" ->
(match normalised with
| Some ("x86_64" | "arm64" | "ppc64" as arch) ->
(match OpamStd.Sys.getconf "LONG_BIT", arch with
(match OpamStd.Sys.getconf_long_bit (), arch with
| Some "32", "x86_64" -> Some "x86_32"
| Some "32", "arm64" -> Some "arm32"
| Some "32", "ppc64" -> Some "ppc32"
Expand All @@ -74,7 +74,7 @@ let normalise_os raw =
let poll_os () =
let raw =
match Sys.os_type with
| "Unix" -> OpamStd.Sys.uname "-s"
| "Unix" -> Some (OpamStd.Sys.uname ()).sysname
| s -> norm s
in
match raw with
Expand Down Expand Up @@ -130,7 +130,7 @@ let poll_os_distribution () =
| Some "win32" ->
let kind =
OpamStd.Sys.get_windows_executable_variant
?search_in_first:(OpamCoreConfig.(!r.cygbin)) "cygpath.exe"
?search_in_first:(OpamCoreConfig.(!r.cygbin)) "cygpath.exe"
in
begin match kind with
| `Msys2 -> Some "msys2"
Expand Down Expand Up @@ -158,9 +158,9 @@ let poll_os_version () =
Scanf.sscanf s "%_s@[ Version %s@]" norm
with Scanf.Scan_failure _ | End_of_file -> None)
| Some "freebsd" ->
OpamStd.Sys.uname "-U" >>= norm
OpamStd.Sys.uname_freebsd_version () >>= norm
| _ ->
OpamStd.Sys.uname "-r" >>= norm
norm (OpamStd.Sys.uname ()).release
let os_version = Lazy.from_fun poll_os_version

let poll_os_family () =
Expand Down
Loading