-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add missing utmpx apis for linux musl #4332
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
Conversation
It compiles and I tested it by building uutils-coreutils with it as dependency, however I haven't managed to run cargo test on ubuntu 24.04:
Do you know what the issue is? I do have the required header-file:
|
8f26ab6
to
f03c76b
Compare
Please also consider this for backporting to 0.2 |
@rustbot label +stable-nominated |
cef026a
to
ad97bd5
Compare
The header-file error seems to be a generic error when compiling with musl-gcc: https://stackoverflow.com/questions/42128091/linking-to-musl-with-other-headers-from-usr-include |
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.
Considering musl's stance here, what failures motivate this addition? It seems like the API consumer may be better off disabling utmpx functionality on musl rather than having surprising untested nop behavior, and I'm wondering if we wouldn't be better off leaving these undefined to discourage running into that footgun.
If you still think this is worth having, it would be preferable to move the existing definitions from linux/gnu/mod.rs
to linux/mod.rs
so glibc, musl, and uclibc all get covered.
@tgross35 I understand your concern. I was thinking about this as well before the implementation and was hesitating as well. However my reasoning for including it in libc is the following:
Regarding your request to make those generic, please note that since constants are prefixed with "__" in glibc, but not in musl, the constant is e.g. "UT_HOSTSIZE" in musl, but "__UT_HOSTSIZE" in glibc. Do you want me to move a subset to the generic file? |
I think bug-for-bug tools like uutils are probably somewhat on the rare side. Maybe let's do this: we can add the functions but immediately mark them deprecated so the user is likely to get a warning. That way they are available if needed but less likely to be used by complete accident, and // NB: not actually intended for removal; this is a user warning that the function may not
// behave as expected
#[deprecated(since="0.2.172",
note="musl provides `utmp` as stubs and an alternative should be preferred; see \
https://wiki.musl-libc.org/faq.html"
)] If that sounds reasonable, would you mind adding the same attribute to other functions that musl has stubbed out?
Didn't realize that, nope it's fine! |
ad97bd5
to
7be3b83
Compare
I think it is a good idea. I have added the deprecation warnings using a macro. I've also tested that the warnings are printed when compiling uutils-coreutils:
Edit: I've removed the macro because the CI was failing
|
7be3b83
to
ca90edc
Compare
ca90edc
to
3d7eee5
Compare
Close rust-lang#4322 Also add a deprecation warning, because those functions are only implemented as stubs inside musl. Signed-off-by: Etienne Cordonnier <[email protected]>
3d7eee5
to
42ead6d
Compare
This is ready to be merged IMO. |
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.
Sorry this fell off my radar, thanks for the changes. LGTM
Close rust-lang#4322 Also add a deprecation warning, because those functions are only implemented as stubs inside musl. Signed-off-by: Etienne Cordonnier <[email protected]> (backport <rust-lang#4332>) (cherry picked from commit 42ead6d)
Close rust-lang#4322 Also add a deprecation warning, because those functions are only implemented as stubs inside musl. Signed-off-by: Etienne Cordonnier <[email protected]> (backport <rust-lang#4332>) (cherry picked from commit 42ead6d)
Description
Add the utmp APIs which were missing from musl.
Close #4322
Sources
This is based on https://git.musl-libc.org/cgit/musl/tree/include/utmpx.h
Note that musl implements only stub functions for utmp functionalities, because they consider it unsafe.
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
); (unrelated error about #include <linux/errqueue.h>)especially relevant for platforms that may not be checked in CI
Signed-off-by: Etienne Cordonnier [email protected]