Remove feature flags #681

Susurrus · 2017-07-17T04:45:42Z

These are vestiges of the initial push to get this working on Rust 1.0. These feature flags are undocumented and so hard to discover (only learned about them today!), prevent functions being included that should be and this also affects documentation on docs.rs, and none of the features are tested in CI and the execvpe has been broken for forever.

The solution is to conditionally compile everything supported for a given platform and do away completely with the feature flags. The execvpe function is completely removed as it's not available for *nix platforms in libc and is already broken, so no loss removing it. We'll add it back once it's back in libc (rust-lang/libc#670).

Closes #98.
Closes #206.
Closes #306.
Closes #308.

asomers · 2017-07-17T04:58:04Z

Coincidentally, I first learned of those feature flags today, too. I agree with getting rid of them. Could you also change test_signalfd.rs into a normal test file? And there are also some feature gates in test_unistd.rs and sys/test_uio.rs to remove.

Susurrus · 2017-07-17T05:20:01Z

I'm working on fixing the tests but I get a "main function not found" when I "fix" things. You have this problem before?

Susurrus · 2017-07-17T05:36:22Z

Nvm, figured it out. Weird we have mount and signalfd tests using a custom test harness instead of the main one. This is some old code..

Susurrus · 2017-07-17T05:37:15Z

And this needs a CHANGELOG before it can be merged, but I'm working on getting CI passing first.

Susurrus · 2017-07-17T05:52:38Z

Android failures are because of missing libc items (see rust-lang/libc#671), but I'll probably just disable signalfd on android so this can be part of the current release.

Susurrus · 2017-07-17T05:53:14Z

There are ECHILD errors from running the tests, which I can also produce locally, so I'll need to figure those out as well.

Susurrus · 2017-07-17T18:25:50Z

@asomers Want to take a look-see over this for me, as I think the new signalfd test should be good but I'm not too comfortable with fork and would appreciate a second pair of eyes on it? Should be ready to go if it looks good to you.

asomers · 2017-07-18T00:12:10Z

test/sys/test_signalfd.rs

@@ -0,0 +1,52 @@
+#[cfg(target_os = "linux")]


Isn't this line redundant with test/sys/mod.rs:5 ?

asomers · 2017-07-18T00:16:06Z

test/sys/test_signalfd.rs

+    mask.thread_block().unwrap();
+
+    // Start a child process
+    match unistd::fork() {


I don't think you should fork here. For one thing, cargo's test harness has deadlocks when a multithreaded test program forks. That's why I had to add std::process:exit(0) all over the place. For another thing, it's not necessary. You can just create the eventfd and send the signal all from the same thread, just like the old test did.

I remember why I was doing this, because if I didn't I saw errors like "process didn't exit successfully (signal: 10)" even just running my test by itself. This is ECHILD, which states there are no child processes. Would you have another suggestion rather than forking?

ECHILD is an errno, not a signal. Signal 10, on Linux, is SIGUSR1. It sounds like you weren't correctly blocking the signal. I can take a look at it, if you want.

asomers · 2017-07-18T00:20:06Z

test/sys/test_signalfd.rs

+
+    // Set up a signalfd to listen for SIGUSR1 on this thread
+    let mut mask = SigSet::empty();
+    mask.add(SIGUSR1);


SIGUSR1 is a global resource, so this test will conflict with any other test that does anything with it. I dealt with that in PR #638 by creating a mutex to protect SIGUSR2. You should probably do the same for SIGUSR1

I was going to use SIGUSR2, but I actually think this implementation isn't quite right. According to [this](https://stackoverflow.com/questions/11679568/signal-handling-with-multiple-threads-in-linux#11679770] signal disposition is shared across all threads in a process. So I think we should actually have a SIGNAL_MTX and not one specific to each signal.

Susurrus · 2017-07-18T05:10:42Z

Thanks for the review, @asomers, everything should be addressed now!

Note that I b roadened the SIGUSR2_MTX to a general SIGNAL_MTX because depending on how signal handling is modified, it might not be updated in one test in a way that doesn't clobber the signal mask in another test. So instead of worrying about that, just make it general to all signal handling. This also makes it more in line with the rest of the mutexes which are pretty broad.

Also, I wanted to note that this includes a commit to fix unused imports introduced by you @asomers in 2cd4420. You might be running an older version of Rust that doesn't output warnings about unused variables in tests by default, which is why this was easy to miss. Might be worth updating Rust depending on your specific dev needs!

asomers

Apart from that one kill/raise typo, it all looks good to me. Fix that and r+ asomers

BTW, I submitted a PR to libc last night to support preadv/pwritev on more platforms. That can be a nix 0.10.0 addition.

asomers · 2017-07-18T19:58:21Z

test/sys/test_signalfd.rs

+    // Send a SIGUSR1 signal to the current process. Note that this uses `raise` instead of `kill`
+    // because `kill` with `getpid` isn't correct during multi-threaded execution like during a
+    // cargo test session. Instead use `raise` which does the correct thing by default.
+    raise(signal::SIGUSR1).ok().expect("Error: Kill Failed");


s/Kill/raise

Note that this is now only available for Linux as support is missing in libc for Android (see rust-lang/libc#671). As part of this work the SIGUSR2 signal mutex was altered to be a general signal mutex. This is because all signal handling is shared across all threads in the Rust test harness, so if you alter one signal, depending on whether it's additive or may overwrite the mask for other signals, it could break the other ones. Instead of putting this on the user, just broaden the scope of the mutex so that any altering of signal handling needs to use it.

It was broken when enabled, and currently the libc definition is only available for windows. This will be re-added when a new libc is released that supports it across all available platforms

Susurrus · 2017-07-18T20:05:55Z

Cool, thanks for reviewing it!

As for supporting preadv/pwritev on more platforms, can you open an issue for it? It'd be great to have more mentored/good-first-bug issues.

bors r+ asomers

681: Remove feature flags r=Susurrus These are vestiges of the initial push to get this working on Rust 1.0. These feature flags are undocumented and so hard to discover (only learned about them today!), prevent functions being included that should be and this also affects documentation on docs.rs, and none of the features are tested in CI and the `execvpe` has been broken for forever. The solution is to conditionally compile everything supported for a given platform and do away completely with the feature flags. The `execvpe` function is completely removed as it's not available for *nix platforms in libc and is already broken, so no loss removing it. We'll add it back once it's back in libc (rust-lang/libc#670). Closes #98. Closes #206. Closes #306. Closes #308.

bors · 2017-07-19T00:31:30Z

Build failed

continuous-integration/travis-ci/push

asomers · 2017-07-19T00:38:44Z

We hit the test_double_close timeout on powerpc again. But we also had failures in the epoll tests on x86_64. I haven't personally seen that one yet.

Susurrus · 2017-07-19T02:26:28Z

I'm thinking it's time to deprecate powerpc to Tier 2 as you suggested, and I propose we do that for the 0.9 release so we aren't promising too much to our customers.

Susurrus · 2017-07-19T02:45:56Z

I'm seeing those epoll failures on #686 as well....Hmmm.

Susurrus · 2017-07-19T02:48:58Z

I'm re-running master by itself to see what's going on: https://travis-ci.org/nix-rust/nix/builds/254708956

Susurrus · 2017-07-19T06:27:59Z

bors r+ asomers

681: Remove feature flags r=Susurrus These are vestiges of the initial push to get this working on Rust 1.0. These feature flags are undocumented and so hard to discover (only learned about them today!), prevent functions being included that should be and this also affects documentation on docs.rs, and none of the features are tested in CI and the `execvpe` has been broken for forever. The solution is to conditionally compile everything supported for a given platform and do away completely with the feature flags. The `execvpe` function is completely removed as it's not available for *nix platforms in libc and is already broken, so no loss removing it. We'll add it back once it's back in libc (rust-lang/libc#670). Closes #98. Closes #206. Closes #306. Closes #308.

bors · 2017-07-19T06:55:04Z

Build succeeded

Susurrus force-pushed the rem_features branch 2 times, most recently from 4cb46ad to 4c8c94f Compare July 17, 2017 05:35

Susurrus force-pushed the rem_features branch 2 times, most recently from 0d39c9d to fcc22f2 Compare July 17, 2017 14:25

This was referenced Jul 17, 2017

Release 0.9.0 #632

Closed

Add execvpe support #682

Closed

asomers requested changes Jul 18, 2017

View reviewed changes

Susurrus mentioned this pull request Jul 18, 2017

Make aio, chdir, and wait tests thread safe #638

Merged

Susurrus added 2 commits July 17, 2017 21:52

Remove eventfd feature in favor of conditional include

c1ffefa

Remove preadv_pwritev feature in favor of conditional include

79ad5fa

Susurrus force-pushed the rem_features branch from 5693610 to eb6882c Compare July 18, 2017 05:05

Susurrus mentioned this pull request Jul 18, 2017

Re-adding execvpe support, conditional on platform #683

Closed

asomers approved these changes Jul 18, 2017

View reviewed changes

Susurrus added 4 commits July 18, 2017 13:04

Remove broken execvpe implementation

d3f733c

It was broken when enabled, and currently the libc definition is only available for windows. This will be re-added when a new libc is released that supports it across all available platforms

Update changelog

12c71d1

Remove unused imports

bee13c8

Susurrus force-pushed the rem_features branch from eb6882c to bee13c8 Compare July 18, 2017 20:04

Susurrus mentioned this pull request Jul 19, 2017

Fix testing #689

Merged

bors bot merged commit bee13c8 into nix-rust:master Jul 19, 2017

Susurrus deleted the rem_features branch July 19, 2017 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove feature flags #681

Remove feature flags #681

Susurrus commented Jul 17, 2017 •

edited

Loading

asomers commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

asomers Jul 18, 2017

asomers Jul 18, 2017

Susurrus Jul 18, 2017

asomers Jul 18, 2017

asomers Jul 18, 2017

Susurrus Jul 18, 2017

Susurrus commented Jul 18, 2017

asomers left a comment

asomers Jul 18, 2017

Susurrus commented Jul 18, 2017

bors bot commented Jul 19, 2017

asomers commented Jul 19, 2017

Susurrus commented Jul 19, 2017

Susurrus commented Jul 19, 2017

Susurrus commented Jul 19, 2017

Susurrus commented Jul 19, 2017

bors bot commented Jul 19, 2017

Remove feature flags #681

Remove feature flags #681

Conversation

Susurrus commented Jul 17, 2017 • edited Loading

asomers commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus commented Jul 17, 2017

asomers Jul 18, 2017

Choose a reason for hiding this comment

asomers Jul 18, 2017

Choose a reason for hiding this comment

Susurrus Jul 18, 2017

Choose a reason for hiding this comment

asomers Jul 18, 2017

Choose a reason for hiding this comment

asomers Jul 18, 2017

Choose a reason for hiding this comment

Susurrus Jul 18, 2017

Choose a reason for hiding this comment

Susurrus commented Jul 18, 2017

asomers left a comment

Choose a reason for hiding this comment

asomers Jul 18, 2017

Choose a reason for hiding this comment

Susurrus commented Jul 18, 2017

bors bot commented Jul 19, 2017

Build failed

asomers commented Jul 19, 2017

Susurrus commented Jul 19, 2017

Susurrus commented Jul 19, 2017

Susurrus commented Jul 19, 2017

Susurrus commented Jul 19, 2017

bors bot commented Jul 19, 2017

Build succeeded

Susurrus commented Jul 17, 2017 •

edited

Loading