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

kit-ty-kate · 2024-09-30T19:18:03Z

~~Queued on top of #6216~~
~~F.i.x.e.s #6215~~

~~For the backport to 2.3, do we want to backport the entire PR or just the OpenBSD fix?~~
~~Partially backported to 2.3 in #6228~~

~~Queued on #6230~~ merged

avsm · 2024-09-30T19:35:45Z

Works great on my OpenBSD desktop:

$ uname -a
OpenBSD 7.5 GENERIC.MP#82 amd64

<><> Global opam variables ><><><><><><><><><><><><><><><><><><><><><><><><><><>
arch              x86_64            # Inferred from system
exe                                 # Suffix needed for executable filenames (Windows)
jobs              1                 # The number of parallel jobs set up in opam configuration
make              gmake             # The 'make' command to use
opam-version      2.4.0~alpha1~dev  # The currently running opam version
os                openbsd           # Inferred from system
os-distribution   openbsd           # Inferred from system
os-family         bsd               # Inferred from system
os-version        7.5               # Inferred from system
root              /home/avsm/.opam2 # The current opam root directory
switch            default           # The identifier of the current switch
sys-ocaml-arch    x86_64            # Target architecture of the OCaml compiler present on your system
sys-ocaml-cc      cc                # Host C Compiler type of the OCaml compiler present on your system
sys-ocaml-libc    libc              # Host C Runtime Library type of the OCaml compiler present on your system
sys-ocaml-system  openbsd           # Target system of the OCaml compiler present on your system
sys-ocaml-version 4.14.1            # OCaml version present on your system independently of opam, if any

One test fails:

File "tests/reftests/dune.inc", line 2082, characters 0-243:
2082 | (rule
2083 |   (targets opam-repo-143dd2a2f59f5befbf3cb90bb2667f911737fbf8)
2084 |   (action
2085 |    (progn
2086 |     (run mkdir -p %{targets})
2087 |     (run tar -C %{targets} -xzf %{dep:opam-archive-143dd2a2f59f5befbf3cb90bb2667f911737fbf8.tar.gz} --strip-components=1))))

but looks unrelated (that tar needs to be gtar due to the --strip-components)

mndrix · 2024-10-01T13:49:20Z

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 uname via C stubs.

kit-ty-kate · 2024-10-01T19:25:29Z

turns out we still need to call the getconf process. sysconf(3) depends on the libc architecture and doesn't have the necessary query with musl (_SC_LONG_BIT is glibc only).

We could rely on Sys.word_size as an alternative but it would depend on the architecture of the opam binary, not the userspace architecture (e.g. you can call an arm64 binary just fine on Raspberry Pi OS 32bit, which has a 32bit userspace but 64bit kernel)

I've also removed the C stub for FreeBSD as __FreeBSD_version is defined at compile-time and does not show the actual runtime version.

rjbou

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 and getconf, or just update the API and highlight the change in the release note.

rjbou · 2024-10-08T15:10:14Z

src/core/opamStd.mli


  (** The output of the command "getconf", with the given argument. Memoised. *)
  val getconf: string -> string option
+  (** The memoized result of uname(2) *)
+  val uname : OpamStubs.uname Lazy.t


It's better to have a uname: void -> OpamStubs.uname to ease readability/writing when used

src/core/opamStd.ml

rjbou · 2024-10-08T15:23:11Z

src/core/opamStd.mli

-  (** 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. *)
+  val uname_freebsd_version: unit -> string option


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

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

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

src/core/opamStd.ml

rjbou · 2024-10-08T15:38:32Z

src/core/opamStd.mli

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


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

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

src/core/opamStubs.mli

rjbou · 2024-10-08T15:43:01Z

src/core/opamStd.mli

+  val getconf_long_bit: unit -> string option
+
+  (** The memoized result of uname(2) *)
+  val uname : OpamStubs.uname Lazy.t


uname should be before its specialised version for probing freebsd version

kit-ty-kate · 2024-10-08T19:11:45Z

I've split off the fix for OpenBSD to #6230 for simplification. Leaving this PR just for the uname improvement

src/core/opamStubs.mli

master_changes.md

kit-ty-kate added KIND: BUG AREA: PORTABILITY labels Sep 30, 2024

kit-ty-kate added this to the 2.4.0~alpha1 milestone Sep 30, 2024

kit-ty-kate mentioned this pull request Sep 30, 2024

getconf: LONG_BIT: unknown variable #6215

Closed

kit-ty-kate force-pushed the uname-stubs branch from 5645f51 to e2f67c1 Compare October 1, 2024 19:13

kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Oct 2, 2024

This was referenced Oct 7, 2024

[2.3 backport] Fix opam on OpenBSD #6227

Closed

[2.3 backport] System polling: Harden the process calls to account for failure #6228

Merged

rjbou reviewed Oct 8, 2024

View reviewed changes

kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Oct 8, 2024

kit-ty-kate mentioned this pull request Oct 8, 2024

System polling: Harden the process calls to account for failure #6230

Merged

kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Oct 8, 2024

kit-ty-kate changed the title ~~Fix opam on OpenBSD~~ Use a C stub to call uname(2) instead of calling the uname(1) command Oct 8, 2024

kit-ty-kate removed the KIND: BUG label Oct 10, 2024

kit-ty-kate force-pushed the uname-stubs branch from 05c71d0 to e857ce2 Compare October 21, 2024 13:37

rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Oct 24, 2024

rjbou reviewed Oct 24, 2024

View reviewed changes

src/core/opamStubs.mli Show resolved Hide resolved

master_changes.md Outdated Show resolved Hide resolved

master_changes.md Outdated Show resolved Hide resolved

rjbou reviewed Oct 24, 2024

View reviewed changes

master_changes.md Outdated Show resolved Hide resolved

rjbou reviewed Oct 24, 2024

View reviewed changes

master_changes.md Show resolved Hide resolved

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

f389885

kit-ty-kate force-pushed the uname-stubs branch from 90f38fb to f389885 Compare October 24, 2024 17:39

kit-ty-kate requested a review from rjbou October 24, 2024 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kit-ty-kate commented Sep 30, 2024 •

edited by rjbou

Loading

avsm commented Sep 30, 2024

mndrix commented Oct 1, 2024

kit-ty-kate commented Oct 1, 2024

rjbou left a comment

rjbou Oct 8, 2024

rjbou Oct 8, 2024

kit-ty-kate Oct 8, 2024

rjbou Oct 9, 2024

rjbou Oct 8, 2024

kit-ty-kate Oct 8, 2024

rjbou Oct 8, 2024

kit-ty-kate commented Oct 8, 2024

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

Are you sure you want to change the base?

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

Conversation

kit-ty-kate commented Sep 30, 2024 • edited by rjbou Loading

avsm commented Sep 30, 2024

mndrix commented Oct 1, 2024

kit-ty-kate commented Oct 1, 2024

rjbou left a comment

Choose a reason for hiding this comment

rjbou Oct 8, 2024

Choose a reason for hiding this comment

rjbou Oct 8, 2024

Choose a reason for hiding this comment

kit-ty-kate Oct 8, 2024

Choose a reason for hiding this comment

rjbou Oct 9, 2024

Choose a reason for hiding this comment

rjbou Oct 8, 2024

Choose a reason for hiding this comment

kit-ty-kate Oct 8, 2024

Choose a reason for hiding this comment

rjbou Oct 8, 2024

Choose a reason for hiding this comment

kit-ty-kate commented Oct 8, 2024

kit-ty-kate commented Sep 30, 2024 •

edited by rjbou

Loading