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

make statfs/statvfs to be available everywhere #832

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

ignatenkobrain
Copy link
Contributor

libc reads sys/statvfs.h on all OS except Windows which nix doesn't care
about.

Closes: #831
Signed-off-by: Igor Gnatenko i.gnatenko.brain@gmail.com

@ignatenkobrain
Copy link
Contributor Author

Unfortunately I can't run nix tests on those architectures because they depend on nix-test crate which requires nightly compiler..

@Susurrus
Copy link
Contributor

Susurrus commented Jan 7, 2018

What do you mean? The nix-test crate is no longer necessary and was removed by a PR of mine a little while back.

@ignatenkobrain
Copy link
Contributor Author

@Susurrus ah, that's because I run from latest release.. Didn't notice that.

Anyway, looking at travis, it seems it broke android builds while everything else seems good

@ignatenkobrain
Copy link
Contributor Author

I ended up sending rust-lang/libc#890

@ignatenkobrain
Copy link
Contributor Author

@Susurrus so only android fails now and PR to libc is sent. Should I temporarily guard statvfs for android here or wait for libc PR?

@Susurrus
Copy link
Contributor

Susurrus commented Jan 8, 2018

Please wait for the libc PR to resolve. They're pretty quick with merging those things, so I'd expect it to be merged tonight.

@ignatenkobrain ignatenkobrain force-pushed the statvfs branch 2 times, most recently from 2074ef6 to 52fa9bb Compare January 8, 2018 14:12
@ignatenkobrain
Copy link
Contributor Author

@Susurrus yay! now it passes!

@ignatenkobrain
Copy link
Contributor Author

ping @Susurrus

@Susurrus
Copy link
Contributor

This needs to be rebased and the CHANGELOG Added section updated. Otherwise LGTM.

@ignatenkobrain
Copy link
Contributor Author

@Susurrus done.

@Susurrus
Copy link
Contributor

CI failure with the aio_drop test, @asomers didnt you just touch this code?

@asomers
Copy link
Member

asomers commented Jan 28, 2018

I'm unable to reproduce the failure locally, so I restarted the offending test on Travis. However, I don't think the changelog message is accurate. Could you please change "all supported platforms" to "all OSX and Linux architectures"?

@Susurrus
Copy link
Contributor

I would also move the changelog entry under the "Added" section and explicitly list the platforms as @asomers said.

@Susurrus
Copy link
Contributor

Susurrus commented Feb 8, 2018

@ignatenkobrain We're pretty close to wrapping this up. Want to address the last two comments, then we can merge?

@ignatenkobrain
Copy link
Contributor Author

@Susurrus sorry, didn't have time to look at. Addressing it now.

libc reads sys/statvfs.h on all OS except Windows which nix doesn't care
about.

Closes: nix-rust#831
Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
@ignatenkobrain
Copy link
Contributor Author

@Susurrus pushed!

@Susurrus
Copy link
Contributor

Susurrus commented Feb 9, 2018

LGTM bors r+

@asomers
Copy link
Member

asomers commented Feb 9, 2018

bors needs to be on its own line.

bors r+ susurrus

bors bot added a commit that referenced this pull request Feb 9, 2018
832: make statfs/statvfs to be available everywhere r=asomers a=ignatenkobrain

libc reads sys/statvfs.h on all OS except Windows which nix doesn't care
about.

Closes: #831
Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
@bors
Copy link
Contributor

bors bot commented Feb 9, 2018

@bors bors bot merged commit 08624d0 into nix-rust:master Feb 9, 2018
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.

3 participants