-
Notifications
You must be signed in to change notification settings - Fork 654
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
Fix nix for Dragonfly #684
Conversation
Function __dfly_error() is gone from Rust since a long time. Rust's libstd uses the thread_local attribute for errno, but thread_local cannot be used from stable Rust. For this reason I created errno_dragonfly which is a C FFI wrapper for errno.
@@ -27,6 +27,9 @@ bitflags = "0.9" | |||
cfg-if = "0.1.0" | |||
void = "1.0.2" | |||
|
|||
[target.'cfg(target_os = "dragonfly")'.dependencies] | |||
errno-dragonfly = "0.1" |
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.
We shouldn't need an extra crate just for this. The basic symbols should all be defined in libc, and nix's won src/errno.rs defines errno_location
. Can you merge whatever you need into nix and libc so we don't depend on the extra crate?
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.
No, there is no other possibility!!! I talked with @alexcrichton to make the thread_local attribute attribute stable, and he actually suggested me to write a C extension like errno-dragonfly. libc
does not expose the errno variable directly AFAIK.
The problem is that in DragonFly, the errno variable is not a global, but a thread locale variable, so a simple "extern __errno" as used by the other OSes simply does not work. In libstd, errno can be accessed by using "#[thread_local]", as unstable features are allowed for libstd.
If you want to support DragonFly there really isn't any other option to go via C atm.
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.
That's ... sad. I took a stab at this myself. It doesn't look as though we can use #[thread_local]
unless we enable it for all builds, not just Dragonfly's. Still, what would you say to moving the C extension into Nix instead of making it its own crate? Cargo just seems like an awfully heavyweight way to manage such a tiny amount of code.
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 have a problem with this crate staying external, though I'd much rather rely on a crate that had proper semver (i.e. was 1.0+).
@@ -20,7 +20,10 @@ pub type type_of_cmsg_len = socklen_t; | |||
#[cfg(target_os = "macos")] | |||
pub type type_of_cmsg_data = c_uint; | |||
|
|||
#[cfg(not(target_os = "macos"))] | |||
#[cfg(target_os = "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.
I think this should be #[cfg(any(target_os = "dragonfly", target_os = "macos"))]
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.
No, this is correct! type_of_cmsg_data
is already defined above as c_uint. Here it is defined for DragonFly as c_int. Below it is defined for all other OSes.
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.
Oh, you're right. I didn't notice the "u".
@@ -145,27 +145,34 @@ mod status { | |||
target_os = "netbsd"))] | |||
mod status { | |||
use sys::signal::Signal; | |||
use libc::c_int; |
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.
Why is it necessary to change all of these types?
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 understand why you use i32
here instead of c_int
. The latter is the correct type for those APIs, as waitpid returns a c_int, not a i32. On most platforms these types are identical, but c_int should be the correct one.
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, if you want to be pedantically correct. But it's got nothing to do with DragonFly. It you're going to change those types, please do it in a separate commit, not comingled with DragonFly specific changes.
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 agree with @asomers here, please remove this changes from this PR.
There's big changes coming to the FFI declarations in the |
Ok, I will wait for #647 to land, then rebase on top of that. |
} | ||
|
||
#[cfg(not(target_os = "dragonfly"))] | ||
pub fn signaled(status: c_int) -> bool { | ||
wstatus(status) != WSTOPPED && wstatus(status) != 0 && status != 0x13 |
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.
What does the magic number 0x13 mean? This sounds like a constant that should be in libc.
@@ -145,27 +145,34 @@ mod status { | |||
target_os = "netbsd"))] | |||
mod status { | |||
use sys::signal::Signal; | |||
use libc::c_int; |
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, if you want to be pedantically correct. But it's got nothing to do with DragonFly. It you're going to change those types, please do it in a separate commit, not comingled with DragonFly specific changes.
@mneumann I'd like to go ahead and get these changes except the errno-dragonfly parts merged. We can open that as an issue and defer on resolving it for now. Do you have time coming up here to address this? |
@mneumann I think I'd be fine with errno-dragonfly being a dependency of |
Closing in favor of #799 |
799: Fix nix on Dragonfly r=Susurrus a=mneumann This commit replaces pull request #684. It fixes building `nix` on DragonFly. All tests pass. This requires most recent libc to build: rust-lang/libc#851.
No description provided.