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 on Dragonfly #799

Merged
merged 1 commit into from Dec 19, 2017

Conversation

Projects
None yet
2 participants
@mneumann
Copy link
Contributor

mneumann commented Nov 17, 2017

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.

@mneumann mneumann referenced this pull request Nov 17, 2017

Closed

Fix nix for Dragonfly #684

@@ -630,7 +630,7 @@ pub fn execvp(filename: &CString, args: &[CString]) -> Result<Void> {
///
/// This function is similar to `execve`, except that the program to be executed
/// is referenced as a file descriptor instead of a path.
#[cfg(any(target_os = "android", target_os = "dragonfly", target_os = "freebsd",
#[cfg(any(target_os = "android", target_os = "freebsd",

This comment has been minimized.

@Susurrus

Susurrus Nov 18, 2017

Contributor

Can you alphabetize this list and wrap them with one target per line?

target_os = "netbsd",
target_os = "openbsd",
target_os = "linux", ))] {
execve_test_factory!(test_execve, execve, &CString::new("/bin/sh").unwrap());
execve_test_factory!(test_fexecve, fexecve, File::open("/bin/sh").unwrap().into_raw_fd());
} else if #[cfg(any(target_os = "ios", target_os = "macos", ))] {
} else if #[cfg(any(target_os = "ios", target_os = "macos", target_os = "dragonfly"))] {

This comment has been minimized.

@Susurrus

Susurrus Nov 18, 2017

Contributor

Please keep the targets in alphabetical order.

@Susurrus

This comment has been minimized.

Copy link
Contributor

Susurrus commented Nov 18, 2017

Also, is there a reason this is kept as a separate crate inside nix? I'm not certain we can do that and still publish nix to crates.io honestly. Additionally it just seems like an odd organization, can we not integrate everything with a lot less boilerplate by having it be part of the nix crate itself?

@mneumann

This comment has been minimized.

Copy link
Contributor

mneumann commented Nov 18, 2017

@Susurrus : Added two further commits to address your suggestions. Dunno, why the buildbot is failing :(

@Susurrus

This comment has been minimized.

Copy link
Contributor

Susurrus commented Nov 18, 2017

Yeah, I don't know why the buildbots are having problems either. @asomers any idea here?

Also, you'll want to rebase this now that #798 was merged that touches on this code.

@Susurrus
Copy link
Contributor

Susurrus left a comment

Another thing that I don't see in here is documentation for why we need to do this or a reference to something within Rust or libc that we're waiting on to not need this workaround. I'd like to make sure that it's clear why we need this code here.

unsafe fn errno_location() -> *mut c_int {
extern { fn __dfly_error() -> *mut c_int; }
__dfly_error()
#[link(name="errno_dragonfly", kind="static")]

This comment has been minimized.

@Susurrus

Susurrus Nov 18, 2017

Contributor

I thought this was thread-local. Do we just not use that yet because Rust doesn't support it on stable yet?

This comment has been minimized.

@mneumann

mneumann Nov 20, 2017

Contributor

Exactly. And it's unclear when this will become stable. The suggestion to use a C extension for this purpose came from @alexcrichton in this thread rust-lang/rust#29594 (comment)

@mneumann mneumann force-pushed the mneumann:fix-nix-on-dragonfly branch from 3e87de6 to 6c9e48d Nov 20, 2017

@mneumann

This comment has been minimized.

Copy link
Contributor

mneumann commented Nov 20, 2017

@Susurrus Rebased on top of master, and stashed the errno changes into one single commit, describing why we do it this way (thread_local is not stable).

@Susurrus
Copy link
Contributor

Susurrus left a comment

You'll want to rebase this once #800 lands as well.

@@ -5,7 +5,7 @@ pub use libc::{socket, listen, bind, accept, connect, setsockopt, sendto, recvfr

use libc::{c_int, c_void, socklen_t, ssize_t};

#[cfg(not(target_os = "macos"))]
#[cfg(not(any(target_os = "macos", target_os = "dragonfly")))]

This comment has been minimized.

@Susurrus

Susurrus Nov 21, 2017

Contributor

These should be alphabetized as well.

#[cfg(target_os = "dragonfly")]
pub type type_of_cmsg_data = c_int;

#[cfg(not(any(target_os = "macos", target_os = "dragonfly")))]

This comment has been minimized.

@Susurrus

Susurrus Nov 21, 2017

Contributor

Also alphabetized.

@@ -23,7 +23,10 @@ pub type type_of_cmsg_len = socklen_t;
#[cfg(target_os = "macos")]

This comment has been minimized.

@Susurrus

Susurrus Nov 21, 2017

Contributor

I'd actually suggest putting these in a cfg_if! macro, as this will be a lot easier to understand.

@Susurrus

This comment has been minimized.

Copy link
Contributor

Susurrus commented Dec 3, 2017

@mneumann With #800 merged, now would be a good time to rebase this and get it merged.

@mneumann mneumann force-pushed the mneumann:fix-nix-on-dragonfly branch from 6c9e48d to 3d2c56a Dec 5, 2017

@mneumann

This comment has been minimized.

Copy link
Contributor

mneumann commented Dec 5, 2017

@Susurrus Rebased, alphabetized and used the cfg_if! in socket/ffi.rs. Hoping it still builds everywhere and we are finally ready to merge :)

@Susurrus
Copy link
Contributor

Susurrus left a comment

Just 2 minor changes, and then please squash all of these commits into 1 and we'll get this merged!

// works or not heavily depends on which pthread implementation is chosen
// by the user at link time. For this reason we do not want to run aio test
// cases on DragonFly.
#[cfg(any(target_os = "freebsd", target_os = "ios",

This comment has been minimized.

@Susurrus

Susurrus Dec 13, 2017

Contributor

Can you use a single OS per line as done in other places?

build.rs Outdated
}

#[cfg(not(target_os = "dragonfly"))]
fn main() {

This comment has been minimized.

@Susurrus

Susurrus Dec 13, 2017

Contributor

Please put the braces on the same line.

Fix support for DragonFly
* DragonFly does not have a O_DSYNC flag
* Fix type_of_cmsg_data on DragonFly
* No fexecve() on DragonFly
* Do not run aio test cases on DragonFly
* Keep target lists in alphabetical order
* Unscrable #[cfg] directives and use cfg_if! macro instead
* Fix errno on DragonFly

Below follows an explanation why we have to use a C extension
to get errno working on DragonFly:

DragonFly uses a thread-local errno variable, but #[thread_local] is
feature-gated and not available in stable Rust as of this writing (Rust
1.21.0). We have to use a C extension (src/errno_dragonfly.c) to access
it.

Tracking issue for `thread_local` stabilization:

    rust-lang/rust#29594

Once this becomes stable, we can remove build.rs, src/errno_dragonfly.c,
remove the build-dependency from Cargo.toml, and use:

    extern {
        #[thread_local]
        static errno: c_int;
    }

Now all targets will use the build.rs script, but only on DragonFly this
will do something. Also, there are no additional dependencies for
targets other than DragonFly (no gcc dep).

@mneumann mneumann force-pushed the mneumann:fix-nix-on-dragonfly branch from 28fa6c0 to 709cbdf Dec 19, 2017

@mneumann

This comment has been minimized.

Copy link
Contributor

mneumann commented Dec 19, 2017

@Susurrus : Fixed your above suggestions and squashed everything into one single commit.

@Susurrus

This comment has been minimized.

Copy link
Contributor

Susurrus commented Dec 19, 2017

LGTM, thanks @mneumann!

bors r+

bors bot added a commit that referenced this pull request Dec 19, 2017

Merge #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.
@bors

This comment has been minimized.

@bors bors bot merged commit 709cbdf into nix-rust:master Dec 19, 2017

4 checks passed

bors Build succeeded
buildbot/nix-rust/nix amd64_fbsd11 Build done.
Details
buildbot/nix-rust/nix i386_fbsd11 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mneumann

This comment has been minimized.

Copy link
Contributor

mneumann commented Dec 20, 2017

@Susurrus : Thank you very much for your reviews :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment