-
Notifications
You must be signed in to change notification settings - Fork 69
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
haiku support #154
haiku support #154
Conversation
* pipe2() not available 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.
Could you add tests to CI to make sure it builds on Haiku? Just a cargo check
would suffice.
Actually this could probably be folded into the pipe-based impl used by non-ESP-IDF systems. Just replace the |
i have added an extra ci check that runs cargo check for haiku (x86-64) per suggestion. in regards to replacing |
I would just modify the pipe-based notifier to do something like this: #[cfg(not(target_os = "haiku"))]
let pipe = pipe_with(...).ok();
#[cfg(target_os = "haiku")]
let pipe = None;
let (read, write) = pipe.unwrap_or_else(|| {
let (read, write) = pipe();
...
}); |
ah, gotcha. makes sense now. i have pushed some updates that folds the haiku and non-ESP-IDF sections as per suggestion and also keeps tests passing (and on haiku as well). thanks for the pointers! |
src/poll.rs
Outdated
#[cfg(not(target_os = "haiku"))] | ||
let (read_pipe, write_pipe) = pipe_with(PipeFlags::CLOEXEC).or_else(|_| { | ||
let (read_pipe, write_pipe) = pipe()?; | ||
fcntl_setfd(&read_pipe, fcntl_getfd(&read_pipe)? | FdFlags::CLOEXEC)?; | ||
fcntl_setfd(&write_pipe, fcntl_getfd(&write_pipe)? | FdFlags::CLOEXEC)?; | ||
io::Result::Ok((read_pipe, write_pipe)) | ||
})?; | ||
|
||
#[cfg(target_os = "haiku")] | ||
let pipe_with = None; | ||
|
||
#[cfg(target_os = "haiku")] | ||
let (read_pipe, write_pipe) = pipe_with.unwrap_or_else(|| { | ||
let (read_pipe, write_pipe) = pipe()?; | ||
fcntl_setfd(&read_pipe, fcntl_getfd(&read_pipe)? | FdFlags::CLOEXEC)?; | ||
fcntl_setfd(&write_pipe, fcntl_getfd(&write_pipe)? | FdFlags::CLOEXEC)?; | ||
io::Result::Ok((read_pipe, write_pipe)) | ||
})?; | ||
|
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'd rather not duplicate the pipe creation code twice. Instead, can you make a closure that creates the pipe, then call it in the Haiku case and use it in or_else
in the other case?
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/poll.rs
Outdated
#[cfg(not(target_os = "haiku"))] | ||
let pipe_init = pipe_with; | ||
|
||
#[cfg(target_os = "haiku")] | ||
let pipe_init = |_| { |
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"))] | |
let pipe_init = pipe_with; | |
#[cfg(target_os = "haiku")] | |
let pipe_init = |_| { | |
let fallback_pipe = || { |
src/poll.rs
Outdated
}; | ||
|
||
let (read_pipe, write_pipe) = pipe_init(PipeFlags::CLOEXEC)?; |
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.
}; | |
let (read_pipe, write_pipe) = pipe_init(PipeFlags::CLOEXEC)?; | |
#[cfg(not(target_os = "haiku"))] | |
let (read_pipe, write_pipe) = pipe_with(PipeFlags::CLOEXEC).or_else(fallback_pipe)?; | |
#[cfg(target_os = "haiku")] | |
let (read_pipe, write_pipe) = fallback_pipe()?; |
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 per suggestions
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.
Thanks!
hello!
this changeset fixes an error that occurs when trying to compile on haiku that would typically show up like so:
haiku doesn't offer pipe2() which appears to be the problem. this changeset updates the haiku case to prefer pipe() instead. an example test run with patch applied is shown below: