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

deps: update tempfile to 3 #900

Merged
merged 3 commits into from Sep 2, 2018
Merged

deps: update tempfile to 3 #900

merged 3 commits into from Sep 2, 2018

Conversation

@ignatenkobrain
Copy link
Contributor

@ignatenkobrain ignatenkobrain commented May 11, 2018

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

@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented May 11, 2018

I'm not sure if you want to have prefixed directories, but kept it as it was.

@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented May 11, 2018

(except for the doctest for simplicity sake)

Copy link
Member

@asomers asomers left a comment

Overall, it looks good. But I think we should be consistent about using either anonymous or named temporary files/directories. The preference should probably be anonymous tempfile > named tempfile > anonymous tempdir > named tempdir.


let tempdir = TempDir::new("test_getsockname").unwrap();
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@asomers

asomers May 11, 2018
Member

Shouldn't this test work even with an anonymous tempdir, like this? I think anonymous tempdirs still have names; only anonymous tempfiles don't.

let tempdir = TempDir::new();
let sockname = tempdir.path().join("sock")

This comment has been minimized.

@ignatenkobrain

ignatenkobrain May 11, 2018
Author Contributor

it should, just wanted to keep prefixes as it was in original code. If this is not needed, I'm going to drop it.

@@ -114,7 +113,10 @@ fn test_pwrite() {
fn test_pread() {
use std::io::Write;

let tempdir = TempDir::new("nix-test_pread").unwrap();

This comment has been minimized.

@asomers

asomers May 11, 2018
Member

In this case, I think you can use an anonymous temp file like this. Less code, and no Rust cleanup required. test_pwrite already does just this.

let mut file = tempfile().unwrap();
@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Jun 2, 2018

@ignatenkobrain friendly ping

@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented Jun 22, 2018

sorry for delay, was busy with $dayjob. Updating now.

@ignatenkobrain ignatenkobrain force-pushed the ignatenkobrain:master branch from b3ca1a7 to 6d9e090 Jul 4, 2018
@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented Jul 4, 2018

rebased + updated.

@asomers
Copy link
Member

@asomers asomers commented Jul 4, 2018

Looks like you have a basic syntax error. Did you test locally before pushing?

@ignatenkobrain ignatenkobrain force-pushed the ignatenkobrain:master branch from 6d9e090 to bfdb0f8 Jul 4, 2018
@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented Jul 7, 2018

@asomers passes now

@asomers
asomers approved these changes Jul 7, 2018
Copy link
Member

@asomers asomers left a comment

LGTM. @Susurrus any feelings?

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Jul 11, 2018

I'd agree with consistently using anonymous directories. I'd also suggest we be consistent with using .unwrap() versus .unwrap_or_else() on the Result returned by tempdir(). It's an unrecoverable error, but having some more details is nice, so I'd suggest using .expect(...) here.

@asomers
Copy link
Member

@asomers asomers commented Jul 15, 2018

@Susurrus does that mean you don't approve? If so, please give a negative review to indicate that the PR still needs some work.

Copy link
Contributor

@Susurrus Susurrus left a comment

We should have consistent usages of tempdir(), and I don't think we need to prefix or do any real error checking on this. If temp directories are failing, we likely have bigger issues here.

@@ -30,7 +29,9 @@ fn test_openat() {

#[test]
fn test_readlink() {
let tempdir = TempDir::new("nix-test_readdir")
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -32,7 +32,9 @@ exit 23";

const NONE: Option<&'static [u8]> = None;
pub fn test_mount_tmpfs_without_flags_allows_rwx() {
let tempdir = TempDir::new("nix-test_mount")
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -89,7 +91,9 @@ exit 23";
}

pub fn test_mount_rdonly_disallows_write() {
let tempdir = TempDir::new("nix-test_mount")
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -107,7 +111,9 @@ exit 23";
}

pub fn test_mount_noexec_disallows_exec() {
let tempdir = TempDir::new("nix-test_mount")
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -146,12 +152,16 @@ exit 23";
}

pub fn test_mount_bind() {
let tempdir = TempDir::new("nix-test_mount")
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -106,7 +115,10 @@ fn test_stat_fstat_lstat() {

#[test]
fn test_fchmod() {
let tempdir = TempDir::new("nix-test_fchmod").unwrap();
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -128,7 +140,10 @@ fn test_fchmod() {

#[test]
fn test_fchmodat() {
let tempdir = TempDir::new("nix-test_fchmodat").unwrap();
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -84,7 +81,10 @@ fn test_mkstemp_directory() {

#[test]
fn test_mkfifo() {
let tempdir = TempDir::new("nix-test_mkfifo").unwrap();
let tempdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -286,7 +286,10 @@ fn test_fchdir() {
#[allow(unused_variables)]
let m = ::CWD_MTX.lock().expect("Mutex got poisoned by another test");

let tmpdir = TempDir::new("test_fchdir").unwrap();
let tmpdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@@ -302,7 +305,10 @@ fn test_getcwd() {
#[allow(unused_variables)]
let m = ::CWD_MTX.lock().expect("Mutex got poisoned by another test");

let tmpdir = TempDir::new("test_getcwd").unwrap();
let tmpdir = tempfile::Builder::new()

This comment has been minimized.

@Susurrus

Susurrus Jul 15, 2018
Contributor

Just tempfile::tempdir().unwrap(); should be fine.

@ignatenkobrain ignatenkobrain force-pushed the ignatenkobrain:master branch 2 times, most recently from fd85572 to 9e1caa4 Jul 17, 2018
@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented Jul 17, 2018

Updated to address all points from @Susurrus

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Jul 24, 2018

Test failures on i386 freebsd 11. Travis reported a spurious failure I'm rerunning.

@@ -90,9 +90,9 @@ pub fn test_abstract_uds_addr() {
pub fn test_getsockname() {
use nix::sys::socket::{socket, AddressFamily, SockType, SockFlag};
use nix::sys::socket::{bind, SockAddr};
use tempdir::TempDir;
use tempfile;

This comment has been minimized.

@Susurrus

Susurrus Jul 24, 2018
Contributor

This isn't necessary, correct?

This comment has been minimized.

@bachp

bachp Aug 9, 2018
Contributor

@Susurrus I don't understand what you mean?

This comment has been minimized.

@Susurrus

Susurrus Aug 10, 2018
Contributor

Sorry, don't know what I was talking about here. But since none of these are conditionally compiled I'd suggest we put a single use statement at the top of the file and get rid of all of the additional ones.

@bachp
Copy link
Contributor

@bachp bachp commented Aug 8, 2018

It seems like tempfile requires rand 0.5 which in turn requires Rust 1.22 or higher:

The 0.5 release of Rand requires Rustc version 1.22 or greater. Rand 0.4 and 0.3 (since approx. June 2017) require Rustc version 1.15 or greater. Subsets of the Rand code may work with older Rust versions, but this is not supported.

Is it possible to bump the required rust version for nix to 1.22 or higher?

I'm also interested in having this in for #930

@asomers
Copy link
Member

@asomers asomers commented Aug 9, 2018

There's nothing special about 1.20.0. You can bump the minimum compiler version if it's for a good reason.

@bachp
Copy link
Contributor

@bachp bachp commented Aug 9, 2018

@asomers I already tried this in #930 but I don't know how to bump the version for the freebsd buildbot? Does this need to be done by somebody?

@asomers
Copy link
Member

@asomers asomers commented Aug 10, 2018

Correct. I have to update the buildbots outside of Github. I'll do that after you settle on a Rustc version. Will it be 1.22.0 or 1.24.1?

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Aug 11, 2018

These changes all look good to me. Just needs the version stuff sorted out. I will say that I'd like to bump the version as little as possible. If 1.22.0 works, I'd prefer we use that.

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Aug 15, 2018

I believe the buildbots have been updated to 1.22.1, @ignatenkobrain will you have time to work on this in the near future? We'd like to get this merged before some other PRs.

@ignatenkobrain ignatenkobrain force-pushed the ignatenkobrain:master branch 2 times, most recently from b8e8454 to 6ccfc37 Aug 15, 2018
@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented Aug 15, 2018

we should be all good.

Copy link
Contributor

@bachp bachp left a comment

As a general remark, I would try to use tempdir() and tempfile() functions wherever possible. They should be imported with use tempfile::{tempdir, tempfile} (whichever is required). This would make the whole thing more consistent and there is no need to import self. The only exception is the NamedTempFile that can't be replaced by tempfile().

///
/// fn main() {
/// let tmp_dir1 = TempDir::new("test_mkdir").unwrap();
/// let tmp_dir1 = TempDir::new().unwrap();

This comment has been minimized.

@bachp

bachp Aug 15, 2018
Contributor

use tempfile::tempdir().unwrap() here

/// extern crate nix;
///
/// use nix::unistd;
/// use nix::sys::stat;
/// use tempdir::TempDir;
/// use tempfile::TempDir;

This comment has been minimized.

@bachp

bachp Aug 15, 2018
Contributor

only use tempfile or tempfile::tempdir so it can be uses as tempdir().unwrap()

///
/// fn main() {
/// let tmp_dir = TempDir::new("test_fifo").unwrap();
/// let tmp_dir = TempDir::new().unwrap();

This comment has been minimized.

@bachp

bachp Aug 15, 2018
Contributor

use tempfile::tempdir().unwrap() here, same as above

@@ -5,8 +5,7 @@ use std::{cmp, iter};
use std::fs::{OpenOptions};
use std::os::unix::io::AsRawFd;

use tempdir::TempDir;
use tempfile::tempfile;
use tempfile::{self, tempfile};

This comment has been minimized.

@bachp

bachp Aug 15, 2018
Contributor

use tempfile::{tempdir, tempfile} no need to import self

@@ -114,7 +113,7 @@ fn test_pwrite() {
fn test_pread() {
use std::io::Write;

let tempdir = TempDir::new("nix-test_pread").unwrap();
let tempdir = tempfile::tempdir().unwrap();

This comment has been minimized.

@bachp

bachp Aug 15, 2018
Contributor

With use tempfile::{tempdir, tempfile} this becomes just tempdir()

Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
@ignatenkobrain ignatenkobrain force-pushed the ignatenkobrain:master branch from 1550373 to 19e420f Aug 15, 2018
@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented Aug 15, 2018

@bachp fixed.

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Aug 15, 2018

Looks good, but needs a CHANGELOG entry under the CHANGED entry with "Increased required Rust version to 1.22.1". Should also update the required version listed in the README. Please do these changes in the last commit. Then LGTM. @asomers anything on your end?

@asomers
Copy link
Member

@asomers asomers commented Aug 15, 2018

No, I agree with you, @Susurrus . Just needs a CHANGELOG entry.

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Aug 17, 2018

@ignatenkobrain Would you be able to update the CHANGELOG and README and squash that into your last commit? This is otherwise ready to merge.

@asomers asomers mentioned this pull request Aug 31, 2018
@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Sep 1, 2018

Pinging you on this @ignatenkobrain.

@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented Sep 1, 2018

on it.

Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
@ignatenkobrain ignatenkobrain force-pushed the ignatenkobrain:master branch from 19e420f to ecad72a Sep 1, 2018
@ignatenkobrain
Copy link
Contributor Author

@ignatenkobrain ignatenkobrain commented Sep 1, 2018

done

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Sep 2, 2018

bors r+

bors bot added a commit that referenced this pull request Sep 2, 2018
Merge #900
900: deps: update tempfile to 3 r=Susurrus a=ignatenkobrain

Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>

Co-authored-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
@bors bors bot merged commit ecad72a into nix-rust:master Sep 2, 2018
4 checks passed
4 checks passed
bors Build succeeded
Details
buildbot/nix-rust/nix amd64_fbsd11 Build done.
Details
buildbot/nix-rust/nix i386_fbsd11 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants