-
Notifications
You must be signed in to change notification settings - Fork 670
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
add haiku support #1703
add haiku support #1703
Conversation
@hoanga thanks for the pull request! I didn't have time to go through every file, but wanted to get you some feedback promptly. In particular, I was able to get through the following files: Most of my concerns are around making sure we don't unnecessarily limit functionality that exists on Haiku just because it isn't in https://github.com/rust-lang/libc today. I'm not particularly familiar with Haiku though, so if any of my feedback is incorrect please let me know. I'd also like to make sure the Haiku support doesn't regress over time, as it would appear to have done previously (there are existing references to "haiku" in the codebase). To that end, can you make changes similar to the ones made in #1603 to |
hi @rtzoeller! thanks for the feedback so far! i understand about the concerns regarding the Haiku port developing regressions over time and hope that won't be the case (if i can help prevent it). i have pushed some changes that hopefully enable the ci system to also enable builds for Haiku part of my motivation for being able to get better haiku support in is that one of the tools (delta) that i am hoping can work on Haiku relies on the nix library. with a patch build of nix, delta has been surprisingly functional on my test Haiku system. below is some sample runs showing delta running on Haiku as well as a delta as a substitute for the built-in git diff on Haiku: > delta --help | head
delta 0.12.0
A viewer for git and diff output
USAGE:
delta [OPTIONS] [ARGS]
ARGS:
<MINUS_FILE>
First file to be compared when delta is being used in diff mode
> git diff .cirrus.yml
Δ .cirrus.yml
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
─────────────┐
• 286: task: │
─────────────┘
- name: OpenBSD x86_64
env:
TARGET: x86_64-unknown-openbsd
- name: Haiku x86_64
env:
TARGET: x86_64-unknown-haiku
setup_script:
- rustup component add rust-src
<< : *BUILD |
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.
Ugh, it looks like this feedback didn't actually get posted when I made the original comment the other day. My bad.
src/errno.rs
Outdated
@@ -767,6 +772,80 @@ fn desc(errno: Errno) -> &'static str { | |||
} | |||
} | |||
|
|||
#[cfg(target_os = "haiku")] | |||
fn desc(errno: Errno) -> &'static str { |
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.
Instead of providing a Haiku-specific version of this function, can any Haiku-only (or missing on Haiku) errors be conditionally included in the existing definition of desc
? It looks like there are only a few differences, and this is how other platform support is handled.
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.
sure! i have merged desc
function so there is no Haiku-specific version
src/dir.rs
Outdated
@@ -239,7 +239,7 @@ impl Entry { | |||
} | |||
|
|||
// illumos and Solaris systems do not have the d_type member at all: |
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.
Update this comment as well.
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.
updated
#[cfg(not(any(target_os = "redox", target_os = "haiku")))] | ||
#[cfg_attr(docsrs, doc(cfg(all())))] | ||
O_NDELAY; |
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.
It seems like Haiku defines this; does libc need to be updated?
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.
after double checking, it looks like this is available in Haiku but the libc crate would need support for this. i have left this out for the time being.
src/sys/mod.rs
Outdated
@@ -52,7 +52,7 @@ feature! { | |||
pub mod memfd; | |||
} | |||
|
|||
#[cfg(not(target_os = "redox"))] | |||
#[cfg(not(any(target_os = "redox", target_os = "haiku")))] | |||
feature! { | |||
#![feature = "mman"] | |||
pub mod mman; |
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.
It seems like Haiku probably supports some of the functionality contained in mman
, such as madvise
?
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 have put in some updates that should enable the parts that Haiku supports (sans mlockall)
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 believe we also need to exclude munlockall
.
#[cfg(not(any(target_os = "redox", target_os = "fuchsia", target_os = "illumos", target_os = "haiku")))] | ||
feature! { | ||
#![feature = "resource"] |
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.
It seems like Haiku probably supports some of the functionality in resource
, such as setrlimit
?
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 did try enabling it but ran into some errors that i think originate from the libc crate so leaving it out for now.
an example of the messages i saw looked like the following (if it helps):
error[E0412]: cannot find type `rlimit` in this scope
--> src/sys/resource.rs:199:43
|
199 | let mut old_rlim = mem::MaybeUninit::<rlimit>::uninit();
| ^^^^^^
|
::: /boot/home/.cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.123/src/unix/haiku/mod.rs:1:1
|
1 | pub type rlim_t = ::uintptr_t;
| ------------------------------ similarly named type alias `rlim_t` defined here
|
help: a type alias with a similar name exists
|
199 | let mut old_rlim = mem::MaybeUninit::<rlim_t>::uninit();
| ~~~~~~
help: consider importing one of these items
|
2 | use crate::libc::rlimit;
|
2 | use libc::rlimit;
src/sys/mod.rs
Outdated
#[cfg(not(any(target_os = "redox", target_os = "haiku")))] | ||
feature! { | ||
#![feature = "socket"] | ||
#[allow(missing_docs)] |
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.
Can this be enabled for Haiku?
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.
updated to enable socket
src/sys/signal.rs
Outdated
#[cfg(not(target_os = "haiku"))] | ||
SIGIO, |
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.
#[cfg(not(target_os = "haiku"))] | |
SIGIO, | |
#[cfg(not(target_os = "haiku"))] | |
#[cfg_attr(docsrs, doc(cfg(all())))] | |
SIGIO, |
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.
updated
src/sys/signal.rs
Outdated
@@ -108,6 +109,7 @@ libc_enum!{ | |||
target_os = "redox")))] | |||
#[cfg_attr(docsrs, doc(cfg(all())))] | |||
/// Information request | |||
#[cfg(not(target_os = "haiku"))] |
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.
Merge this into the existing cfg
block.
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.
updated
src/sys/socket/mod.rs
Outdated
@@ -328,6 +328,7 @@ libc_bitflags!{ | |||
#[cfg(any(target_os = "android", | |||
target_os = "dragonfly", | |||
target_os = "freebsd", | |||
target_os = "haiku", |
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 don't actually see MSG_CMSG_CLOEXEC
in this list: https://github.com/haiku/haiku/blob/2bda927298d3491dcfe917de81b43ff85dd0c32d/headers/posix/sys/socket.h#L113-L124
Is it actually supported?
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.
from what i understand, i don't think so have removed support in Haiku
/// Read Only | ||
#[cfg(not(target_os = "haiku"))] | ||
ST_RDONLY; | ||
/// Do not allow the set-uid bits to have an effect | ||
#[cfg(not(target_os = "haiku"))] | ||
ST_NOSUID; |
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.
It seems like Haiku defines these?
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.
after double checking, it looks like these are defined in Haiku but the libc crate would (also) need support for this
thanks for the feedback! i have pushed updates and tried to respond to as much of the feedback as possible. some sections would likely need an update for the libc crate before support in Haiku could work properly. if the preference is to get these in libc before merging i can set this PR to WIP or such until they become available. |
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.
some sections would likely need an update for the libc crate before support in Haiku could work properly.
My preference would be for Haiku support to be as complete as possible, but I also recognize that it can be tedious to add libc definitions for functions you will never personally use. Let's try to get the nix support up to parity with the existing libc definitions; if there are any missing symbols where adding one or two to libc would allow a module to be entirely feature-complete let's do that, otherwise we can code to libc's definition (except where it's wrong, which we've seen a few times already).
I think this is really close to being ready. If you can go through and revert any changes which are no longer necessary such as places adding an extra any()
, that'd be helpful too.
Please add a CHANGELOG.md note as well in the Unreleased >> Added section.
README.md
Outdated
@@ -85,6 +85,7 @@ Tier 3: | |||
* x86_64-unknown-linux-gnux32 | |||
* x86_64-unknown-openbsd | |||
* x86_64-unknown-redox | |||
* x86_64-unknown-haiku |
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.
Please maintain alphabetical order (i.e. put this right below x86_64-unknown-dragonfly
).
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.
updated README for alphabetical order
src/sys/mod.rs
Outdated
@@ -52,7 +52,7 @@ feature! { | |||
pub mod memfd; | |||
} | |||
|
|||
#[cfg(not(target_os = "redox"))] | |||
#[cfg(not(any(target_os = "redox", target_os = "haiku")))] | |||
feature! { | |||
#![feature = "mman"] | |||
pub mod mman; |
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 believe we also need to exclude munlockall
.
src/sys/mod.rs
Outdated
@@ -126,7 +126,7 @@ feature! { | |||
pub mod signalfd; | |||
} | |||
|
|||
#[cfg(not(target_os = "redox"))] | |||
#[cfg(not(any(target_os = "redox")))] |
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.
This line can be reverted; we don't need any
for just one platform.
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.
updated and munlockall also excluded
src/sys/socket/addr.rs
Outdated
@@ -120,6 +121,7 @@ pub enum AddressFamily { | |||
#[cfg_attr(docsrs, doc(cfg(all())))] | |||
Rose = libc::AF_ROSE, | |||
/// DECet protocol sockets. | |||
#[cfg(not(any(target_os = "haiku")))] |
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.
Don't need any
.
src/sys/socket/addr.rs
Outdated
@@ -151,6 +153,7 @@ pub enum AddressFamily { | |||
#[cfg_attr(docsrs, doc(cfg(all())))] | |||
Rds = libc::AF_RDS, | |||
/// IBM SNA | |||
#[cfg(not(any(target_os = "haiku")))] |
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.
Don't need any
.
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.
updated
assert_eq!(Ok(BaudRate::B0), BaudRate::try_from(libc::B0)); | ||
#[cfg(not(target_os = "haiku"))] | ||
assert!(BaudRate::try_from(999999999).is_err()); |
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 guessing this is because Haiku uses a u8, and 999999999 isn't valid. Can the test use a different invalid number on Haiku, rather than disabling this part of it? E.g. assert!(BaudRate::try_from(99).is_err());
on Haiku?
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.
adding a check for haiku sounds good to me. i have added the recommended assert!
for the haiku-specific case
src/time.rs
Outdated
@@ -42,7 +42,7 @@ impl ClockId { | |||
} | |||
|
|||
/// Returns resolution of the clock id | |||
#[cfg(not(target_os = "redox"))] | |||
#[cfg(not(any(target_os = "redox", target_os = "haiku")))] | |||
#[cfg_attr(docsrs, doc(cfg(all())))] | |||
pub fn res(self) -> Result<TimeSpec> { | |||
clock_getres(self) |
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.
Haiku seems to support this.
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 tried enabling it but ran into messages like the one below. think this is a case of the functions being available in haiku but not necessarily available in libc:
error[E0425]: cannot find function `clock_getres` in crate `libc`
--> src/time.rs:220:30
|
220 | let ret = unsafe { libc::clock_getres(clock_id.as_raw(), c_time.as_mut_ptr()) };
| ^^^^^^^^^^^^ help: a function with a similar name exists: `clock_gettime`
|
src/time.rs
Outdated
@@ -212,7 +212,7 @@ impl std::fmt::Display for ClockId { | |||
|
|||
/// Get the resolution of the specified clock, (see | |||
/// [clock_getres(2)](https://pubs.opengroup.org/onlinepubs/7908799/xsh/clock_getres.html)). | |||
#[cfg(not(target_os = "redox"))] | |||
#[cfg(not(any(target_os = "redox", target_os = "haiku")))] | |||
#[cfg_attr(docsrs, doc(cfg(all())))] | |||
pub fn clock_getres(clock_id: ClockId) -> Result<TimeSpec> { |
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.
Haiku seems to support this.
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 think this is a similar situation as above, haiku does support it but seems like libc does not
src/unistd.rs
Outdated
@@ -2986,7 +2988,7 @@ impl From<User> for libc::passwd { | |||
} | |||
} | |||
|
|||
#[cfg(not(target_os = "redox"))] // RedoxFS does not support passwd | |||
#[cfg(not(any(target_os = "redox", target_os = "haiku")))] // RedoxFS does not support passwd |
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 see a definition for getpwnam_r
in libc; can we make Haiku support all of these (including the From
impls above)?
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.
sure, i gave it another shot and looks like this attempt was more successful than earlier attempts.
test/sys/mod.rs
Outdated
@@ -11,21 +11,21 @@ mod test_signal; | |||
target_os = "macos", | |||
target_os = "netbsd"))] | |||
mod test_aio; | |||
#[cfg(not(target_os = "redox"))] | |||
#[cfg(not(any(target_os = "redox")))] |
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.
Don't need any
.
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.
updated
thanks again for the prompt feedback! i have pushed more updates that should help address this round of feedback and enabled some more functionality for haiku. just for note, a few more tests have been enabled with these updates (the non-passing test is the same one):
|
looks like the most recent libc release also included haiku updates for clock, i have enabled that functionality and the associated tests which are passing at this time. cargo test now reports:
|
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.
@hoanga the changes look good! Can you amend the CHANGELOG entry to be of the form
- Added support for the `x86_64-unknown-haiku` target.
(#[1703](https://github.com/nix-rust/nix/pull/1703))
and then squash all of the commits into one?
Thanks!
i have pushed an update for the changelog as well as combining the previous commits into a squashed commit. thanks for all the feedback! |
* enabled as much functionality and defines that match updated libc definitions for haiku
bors r+ |
hello,
the following changeset(s) adds support for compiling on haiku.
below is an sample run showing compilation
and some results from running
cargo test
: