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

Use remove_dir_all crate for more robust Windows CI #604

Merged
merged 4 commits into from
Apr 8, 2021

Conversation

trevyn
Copy link
Contributor

@trevyn trevyn commented Apr 7, 2021

std::fs::remove_dir_all has serious issues on Windows: rust-lang/rust#29497

@XAMPPRocky, who wrote and owns the remove_dir_all crate, is on the Rust release team (see https://www.rust-lang.org/governance/teams/release), notes that the crate is used in cargo and rustup, and suggests moving the crate's implementation into std (rust-lang/rust#29497 (comment)), so I think we can trust this dependency. Given that it improves a real issue on Windows, I think that until this is fixed in std, it would be wise to avoid fs::remove_dir_all wherever possible.

@quentinlesceller
Copy link
Member

Good idea though not a big fan of having yet another dependency...

@trevyn
Copy link
Contributor Author

trevyn commented Apr 7, 2021

remove_dir_all 0.5.3 (vs current 0.7.0) is already in the Cargo.lock as a dependency of tempfile, would you prefer to use that version?

It's also only added in dev-dependencies since it's only for tests.

Let's see if it actually works...

@trevyn
Copy link
Contributor Author

trevyn commented Apr 7, 2021

Ok, the Windows test passes and it doesn't look like there are any bug fixes between 0.5.3 and 0.7.0: https://github.com/XAMPPRocky/remove_dir_all/commits/master

Shall I use 0.5.3 instead and replace the other test instances of fs::remove_dir_all with remove_dir_all::remove_dir_all, and keep the Cargo.lock changes minimal?

@quentinlesceller
Copy link
Member

Probably better to use the 0.7.0 version then.

@trevyn trevyn marked this pull request as ready for review April 7, 2021 18:20
@quentinlesceller quentinlesceller merged commit bdc5bd7 into mimblewimble:master Apr 8, 2021
GeneFerneau pushed a commit to GeneFerneau/grin-wallet that referenced this pull request Apr 21, 2021
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.

None yet

2 participants