Skip to content
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 ppoll() #520

Merged
merged 1 commit into from
Feb 26, 2017
Merged

Add ppoll() #520

merged 1 commit into from
Feb 26, 2017

Conversation

Susurrus
Copy link
Contributor

This will currently fail CI, as the necessary changes haven't hit libc yet. That is tracked in rust-lang/libc#537. This does build on my computer using the changes tracked on that PR, so I'd appreciate any visual review of this code as it should be "done".

I also wanted to get this submitted so hopefully it'd be in the queue for the 0.8 release.

Copy link
Member

@kamalmarhubi kamalmarhubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of small changes, otherwise this looks great!

src/poll.rs Outdated
@@ -47,3 +50,19 @@ pub fn poll(fds: &mut [PollFd], timeout: libc::c_int) -> Result<libc::c_int> {

Errno::result(res)
}

pub fn ppoll(fds: &mut [PollFd], timeout: Duration) -> Result<libc::c_int> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you take timeout: TimeSpec until we decide how to resolve #516?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should accept a sigmask here, too. @fiveop @chaosagent could you suggest how to represent it in nix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should accept a sigmask here, too. @fiveop @chaosagent could you suggest how to represent it in nix?

@Susurrus
Copy link
Contributor Author

I wasn't sure if you wanted me to tack on additional commits to fix this or not, so I force-pushed a new version addressing both the sigmask issue (as I thought it could currently be solved) and using TimeSpec instead of Duration.

@Susurrus
Copy link
Contributor Author

I've pushed a rebased version to re-trigger Travis now that ppoll() was merged into libc.

@kamalmarhubi
Copy link
Member

@homu r+

@homu
Copy link
Contributor

homu commented Feb 25, 2017

📌 Commit cce4dec has been approved by kamalmarhubi

homu added a commit that referenced this pull request Feb 25, 2017
Add ppoll()

This will currently fail CI, as the necessary changes haven't hit libc yet. That is tracked in rust-lang/libc#537. This does build on my computer using the changes tracked on that PR, so I'd appreciate any visual review of this code as it should be "done".

I also wanted to get this submitted so hopefully it'd be in the queue for the 0.8 release.
@homu
Copy link
Contributor

homu commented Feb 25, 2017

⌛ Testing commit cce4dec with merge 65bfec5...

@homu
Copy link
Contributor

homu commented Feb 25, 2017

💔 Test failed - status

@Susurrus
Copy link
Contributor Author

I'm not certain what's going on with travis and tempfile, which seems to be causing errors, but I'll make sure to only expose this function to non-Mac OSes:

+#[cfg(any(target_os = "linux", target_os = "android"))]

@Susurrus
Copy link
Contributor Author

So ppoll is actually available on more OSes that this would imply, like OpenBSD and FreeBSD both support it through libc. But I don't think nix is worrying about those OSes right now, yes?

Anyways, this should resolve failures on Mac. I still don't know what's up with the other failing platforms, but they're allow_failure, so I guess I'll wait on you guys to tell me what, if anything, to do about those.

@kamalmarhubi
Copy link
Member

Looks like you didn't add your cfg? When you do could you also modify the changelog line to specify the OSes it's added for. I'll r+ again after and see if we can't get this past CI!

@Susurrus
Copy link
Contributor Author

@kamalmarhubi Whoops, don't know what happened there! Anyways, I've added the cfg and targets to the CHANGELOG.

@kamalmarhubi
Copy link
Member

thanks!

@homu r+

@homu
Copy link
Contributor

homu commented Feb 25, 2017

📌 Commit c96d818 has been approved by kamalmarhubi

homu added a commit that referenced this pull request Feb 25, 2017
Add ppoll()

This will currently fail CI, as the necessary changes haven't hit libc yet. That is tracked in rust-lang/libc#537. This does build on my computer using the changes tracked on that PR, so I'd appreciate any visual review of this code as it should be "done".

I also wanted to get this submitted so hopefully it'd be in the queue for the 0.8 release.
@homu
Copy link
Contributor

homu commented Feb 25, 2017

💔 Test failed - status

@@ -1,3 +1,6 @@
use sys::time::TimeSpec;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to conditional include this and SigSet. This is why the build fails on macos.

@Susurrus
Copy link
Contributor Author

Alright, I think I got it this time!

Note that all the Rust 1.7.0 errors are still occuring but we can't tell if they're a result of this change because they're all failing on the tempfile crate.

@Susurrus
Copy link
Contributor Author

All targets pass on Travis. This look good to merge?

@fiveop
Copy link
Contributor

fiveop commented Feb 26, 2017

@kamalmarhubi must still accept the changes. I think his concerns are addressed, but I do not want to press the link 'Dismiss review'.

@fiveop
Copy link
Contributor

fiveop commented Feb 26, 2017

@homu r+

@homu
Copy link
Contributor

homu commented Feb 26, 2017

📌 Commit 5d4967e has been approved by fiveop

homu added a commit that referenced this pull request Feb 26, 2017
Add ppoll()

This will currently fail CI, as the necessary changes haven't hit libc yet. That is tracked in rust-lang/libc#537. This does build on my computer using the changes tracked on that PR, so I'd appreciate any visual review of this code as it should be "done".

I also wanted to get this submitted so hopefully it'd be in the queue for the 0.8 release.
@homu
Copy link
Contributor

homu commented Feb 26, 2017

⌛ Testing commit 5d4967e with merge d4d790e...

@homu
Copy link
Contributor

homu commented Feb 26, 2017

☀️ Test successful - status

@homu homu merged commit 5d4967e into nix-rust:master Feb 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants