add unistd::getcwd and unistd::mkdir #416
Conversation
…to expect into proper try handling), needs testing still
… directory names, need to be created with mkdir first which doesn't exist yet)
|
oh, failing tests for Rust 1.2, I'll check the errors in a few hours |
Looks like the errors should be pretty easy to resolve. At some point, I think we're OK with dropping 1.2 but in this case, it should be pretty easy to satisfy the compiler. |
|
@posborne indeed, I'm almost there. I ended up with an exact copy of the implementation in std (which I somehow was not able to make running but found out that I needed to add |
| // ERANGE means buffer was too small to store directory name | ||
| if error != Errno::ERANGE { | ||
| return Err(Error::Sys(error)); | ||
| } |
There was a problem hiding this comment.
Thanks for the hint. I've corrected those now. Any idea why cargo clippy did not warn about those?
…copy of the implementation in std
| "/private/tmp/" | ||
| } else { | ||
| "/tmp/" | ||
| }; |
There was a problem hiding this comment.
This seems a little fragile. I think it would be OK to just verify that the nix getcwd results match those of libstd without hardcoding absolute base paths which may change based on the OS and system (My tmpfs may not be at /tmp).
There was a problem hiding this comment.
good idea, I'll change this and add documentation for getcwd and mkdir
I would like to see the documentation of nix improve. We would welcome documentation (including examples) for new code. |
…pares against std::env::current_dir
|
@posborne I fixed the test and added documentation for |
| use std::mem; | ||
| use std::ffi::CString; | ||
| use std::ffi::{CString,CStr,OsString}; |
…ust 1.2.0 error in doc-test
| assert!(mkdir(tmp_dir.as_path(), stat::S_IRWXU).is_ok()); | ||
| } | ||
| assert!(chdir(tmp_dir.as_path()).is_ok()); | ||
| assert_eq!(getcwd().unwrap(), current_dir().unwrap()); |
There was a problem hiding this comment.
I just noticed that this code is indented with 2-spaces for some reason. Should be 4.
There was a problem hiding this comment.
Running rustfmt as a last step of preparing a PR should be added to our CONTRIBUTING.md.
|
@homu r+ |
|
📌 Commit 7dd12c6 has been approved by |
|
⚡ Test exempted - status |
add unistd::getcwd and unistd::mkdir As a (late) followup of [this withdrawn PR](rust-lang/libc#326) I have added getcwd (wrapper around `libc::getcwd`) and mkdir (wrapper around `libc::mkdir`) and added testing. A few notes: - I'm new to rust so I would appreciate some pair of eyes testing the code, plus I'm open for revision of code or general remarks about my coding style - I have run the tests both on OSX as on Linux (Ubuntu) - I've run `clippy` to see if my code is well formatted, however clippy issues many warnings about the project. I think I didn't add any more warnings - the methods in unistd are not documented so I also left out the documentation of `getcwd` and `mkdir`, although I think it'd probably be good to add some documentation, especially some example code how to use the methods - the base idea of `getcwd` is [taken from std](https://github.com/rust-lang/rust/blob/1.9.0/src/libstd/sys/unix/os.rs#L95-L119), should I mention that somewhere?
|
Yay, thanks a lot @posborne for the helpful comments and the merge. I'm feeling proud - that was my first bigger PR into a rust project |
|
Thank you! Could you add entries to the CHANGELOG.md detailing your changes? Preferably before we cut the release. |
Update CHANGELOG for pull request 416 As [requested by @fiveop](#416 (comment))
As a (late) followup of this withdrawn PR I have added getcwd (wrapper around
libc::getcwd) and mkdir (wrapper aroundlibc::mkdir) and added testing.A few notes:
clippyto see if my code is well formatted, however clippy issues many warnings about the project. I think I didn't add any more warningsgetcwdandmkdir, although I think it'd probably be good to add some documentation, especially some example code how to use the methodsgetcwdis taken from std, should I mention that somewhere?