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.

Sign up for GitHub

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

Jump to bottom

Omit invalid waitpid flags on OpenBSD #787

Merged
merged 2 commits into from
Nov 11, 2017
Merged

Omit invalid waitpid flags on OpenBSD #787

merged 2 commits into from
Nov 11, 2017

Conversation

worr
Copy link
Contributor

@worr worr commented Oct 29, 2017

OpenBSD doesn't have WEXITED, WSTOPPED, or WNOWAIT, so omit those
from that platform.

@Susurrus
Copy link
Contributor

We don't like to blacklist OS targets and instead follow a whitelist strategy. Please correct this to a whitelist for appropriate platforms.

The test failure is due to an upstream change in libc that needs to be fixed.

@worr
Copy link
Contributor Author

worr commented Oct 31, 2017

I've updated the PR to be a whitelist of targets.

@Susurrus
Copy link
Contributor

Susurrus commented Nov 5, 2017

A few issues here:

  • Please order the target_os OSes alphabetically.
  • You have unrelated style changes wrapped up with functionality changes. This is widely considered bad-form with version control. Please separate this into two commits (their order doesn't matter) with your real code changes in one and style changes in the other.

@Susurrus
Copy link
Contributor

Susurrus commented Nov 5, 2017

Additionally, please rebase now that #790 was merged.

@worr worr force-pushed the master branch 2 times, most recently from c36f6cc to 29472a3 Compare November 6, 2017 06:41
@worr
Copy link
Contributor Author

worr commented Nov 6, 2017

Sorry about that, I messed up! I just pushed the latest rebased diffs.

@Susurrus
Copy link
Contributor

Susurrus commented Nov 7, 2017

No worries. But looking at this further, this seems to actually restrict these constants beyond OpenBSD. These three constants are available on haiku, ios, macos, and netbsd as well. Please re-add those platforms for those constants. You can search for them in the libc crate to see exactly which platforms support what for future reference.

worr added 2 commits November 11, 2017 10:44
@worr
Omit invalid waitpid flags on OpenBSD
e43d7db 
OpenBSD doesn't have `WEXITED`, `WSTOPPED`, or `WNOWAIT`, so omit those
from that platform.
@worr
Formatting with rustfmt
ecd0954
@worr
Copy link
Contributor Author

worr commented Nov 11, 2017

Whoops, I didn't think to check libc. Instead I checked the manpages, and it looks like I checked some out-of-date manpages.

I had no idea that y'all supported Haiku 😲

@Susurrus
Copy link
Contributor

@worr We don't technically support Haiiku OS (though there is work in #772 to do just that), but I like to support things on all platforms that libc is supporting.

Anyways, thanks for cleaning this up. Was this all that was necessary for OpenBSD to build again?

bors r+

bors bot added a commit that referenced this pull request Nov 11, 2017
@bors
Merge #787
8f2468c 
787: Omit invalid waitpid flags on OpenBSD r=Susurrus a=worr

OpenBSD doesn't have `WEXITED`, `WSTOPPED`, or `WNOWAIT`, so omit those
from that platform.
@bors
Copy link
Contributor

bors bot commented Nov 11, 2017

Build succeeded

@bors bors bot merged commit ecd0954 into nix-rust:master Nov 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@worr @Susurrus