Skip to content

Use a C stub to call the uname function from the C standard library instead of calling the uname POSIX command#6217

Merged
rjbou merged 1 commit intoocaml:masterfrom
kit-ty-kate:uname-stubs
Dec 2, 2024
Merged

Use a C stub to call the uname function from the C standard library instead of calling the uname POSIX command#6217
rjbou merged 1 commit intoocaml:masterfrom
kit-ty-kate:uname-stubs

Conversation

@kit-ty-kate
Copy link
Copy Markdown
Member

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

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

@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha1 milestone Sep 30, 2024
@kit-ty-kate kit-ty-kate mentioned this pull request Sep 30, 2024
Closed
@avsm
Copy link
Copy Markdown
Member

avsm commented Sep 30, 2024

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
Copy link
Copy Markdown
Contributor

mndrix commented Oct 1, 2024

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
Copy link
Copy Markdown
Member Author

kit-ty-kate commented Oct 1, 2024

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.

@kit-ty-kate 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
Closed
Merged
rjbou
rjbou reviewed Oct 8, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

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

src/core/opamStd.mli Outdated

(** 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. *)
Copy link
Copy Markdown
Collaborator

@rjbou rjbou Oct 8, 2024

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
Copy Markdown
Member Author

@kit-ty-kate kit-ty-kate Oct 8, 2024

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

@kit-ty-kate 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 kit-ty-kate mentioned this pull request Oct 8, 2024
Merged
@kit-ty-kate 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 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
Copy link
Copy Markdown
Member Author

kit-ty-kate commented Oct 8, 2024

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

@rjbou rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Oct 24, 2024
rjbou
rjbou reviewed Oct 24, 2024
View reviewed changes
rjbou
rjbou reviewed Oct 24, 2024
View reviewed changes
rjbou
rjbou reviewed Oct 24, 2024
View reviewed changes
@kit-ty-kate kit-ty-kate requested a review from rjbou October 24, 2024 17:40
@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Nov 22, 2024

Except the naming, LGTM! We can discuss that monday.

rjbou
rjbou reviewed Nov 26, 2024
View reviewed changes
@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Nov 26, 2024

As discussed on dev meeting, we should use uname function instead of uname(2) and uname binary or tool instead of uname(1).

@kit-ty-kate kit-ty-kate requested a review from rjbou November 30, 2024 22:58
@kit-ty-kate kit-ty-kate changed the title Use a C stub to call uname(2) instead of calling the uname(1) command Use a C stub to call the uname function from the C standard library instead of calling the uname POSIX command Nov 30, 2024
@rjbou rjbou merged commit 8531b1a into ocaml:master Dec 2, 2024
@kit-ty-kate kit-ty-kate deleted the uname-stubs branch December 2, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rjbou rjbou rjbou approved these changes

Assignees

No one assigned

Labels

AREA: PORTABILITY

Projects

None yet

Milestone

2.4.0~alpha1

Development

Successfully merging this pull request may close these issues.

4 participants

@kit-ty-kate @avsm @mndrix @rjbou