Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Replace std::env::temp_dir with tempdir in tests #8103

Merged
merged 2 commits into from
Mar 14, 2018
Merged

Replace std::env::temp_dir with tempdir in tests #8103

merged 2 commits into from
Mar 14, 2018

Conversation

niklasad1
Copy link
Collaborator

@niklasad1 niklasad1 commented Mar 13, 2018 •
edited
Loading

Attempt to close #6147

The remaining occurrences of std::env::temp_dir() are the following:

$ git grep -n "::std::env::temp_dir"
ethcore/src/ethereum/mod.rs:36:         None => Spec::load(&::std::env::temp_dir(), b)
ethcore/src/ethereum/mod.rs:148:                let spec = new_morden(&::std::env::temp_dir());
ethcore/src/ethereum/mod.rs:163:                let morden = new_morden(&::std::env::temp_dir());
ethcore/src/ethereum/mod.rs:174:                let frontier = new_foundation(&::std::env::temp_dir());
ethcore/src/spec/spec.rs:416:                   &::std::env::temp_dir(),
evmbin/src/main.rs:243:                         spec::Spec::load(&::std::env::temp_dir(), file)?
evmbin/src/main.rs:246:                         ethcore::ethereum::new_foundation(&::std::env::temp_dir())
parity/account.rs:74:   let spec = spec.spec(&::std::env::temp_dir())?;

@parity-cla-bot
Copy link

It looks like @niklasad1 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

debris
debris suggested changes Mar 13, 2018
View reviewed changes
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

TempDir::new("") needs to be assigned to a value. Otherwise it's dropped to early, and temp directory is never cleaned up

ethash/src/lib.rs Outdated
let ethash = EthashManager::new(&::std::env::temp_dir(), None);
use tempdir::TempDir;

let ethash = EthashManager::new(TempDir::new("").unwrap().path(), None);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It won't work as expected, cause TempDir is dropped before EthashManager, so the directory won't be cleaned up

ethcore/node_filter/src/lib.rs Outdated

/// Contract code: https://gist.github.com/arkpar/467dbcc73cbb85b0997a7a10ffa0695f
#[test]
fn node_filter() {
let contract_addr = Address::from_str("0000000000000000000000000000000000000005").unwrap();
let data = include_bytes!("../res/node_filter.json");
let spec = Spec::load(&::std::env::temp_dir(), &data[..]).unwrap();
let spec = Spec::load(&TempDir::new("").unwrap().path(), &data[..]).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, TempDir will be dropped to early

ethash/src/compute.rs Outdated
@@ -387,26 +388,26 @@ mod test {
];
let nonce = 0xd7b3ac70a301a249;
// difficulty = 0x085657254bd9u64;
let light = NodeCacheBuilder::new(None).light(&::std::env::temp_dir(), 486382);
let light = NodeCacheBuilder::new(None).light(TempDir::new("").unwrap().path(), 486382);
Copy link
Collaborator

Choose a reason for hiding this comment

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

and here ;)

@debris debris added the A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. label Mar 13, 2018
@niklasad1
Copy link
Collaborator Author

niklasad1 commented Mar 13, 2018 •
edited
Loading

Well, technically I think the first commit is fine because the argument<path> in this case must have a bigger lifetime than the function itself and therefore the tmpdir will be cleaned up.

Am I wrong?

@niklasad1 niklasad1 force-pushed the tempdir_crate branch 2 times, most recently from 7a92291 to 08a9856 Compare March 13, 2018 19:37
@debris
Copy link
Collaborator

debris commented Mar 14, 2018

Well, technically I think the first commit is fine because the argument in this case must have a bigger lifetime than the function itself and therefore the tmpdir will be cleaned up.

Am I wrong?

You are right when it comes to function calls, but in many places the path was used to initialize a structure (e.g. EthashManager), and because TempDir was dropped before EthashManager, the directory was never cleaned ;)

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Mar 14, 2018
@debris debris merged commit 23fc551 into master Mar 14, 2018
@debris debris deleted the tempdir_crate branch March 14, 2018 11:26
@5chdn 5chdn added this to the 1.11 milestone Mar 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@debris debris debris approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
1.11 Prosperity
Development

Successfully merging this pull request may close these issues.

Replace usages of ::std::env::temp_dir with tempdir crate in tests
4 participants
@niklasad1 @parity-cla-bot @debris @5chdn