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

Update dependencies and fix builds #123

Closed
wants to merge 3 commits into from

Conversation

rbtcollins
Copy link
Contributor

No description provided.

The mem::replace idiom was unneeded, so replace it with more direct
code.

This fixes the lint issues breaking the build on master.
Updates to latest major (or minor for <0 releases) versions of
dependencies.
1.32 was released in Jan 2019 and set as MSRV a year ago; 1.41 was
released a year after 1.32, giving about the same gap between release
and setting it as MSRV.
@Stebalien
Copy link
Owner

What's the motivation for the new MSRV?

@rbtcollins
Copy link
Contributor Author

rbtcollins commented Aug 25, 2020

Required to pass CI, without it there are a tonne of errors from use of unstabilised things. Coming up from crossbeam I think, via remove_dir_all's new parallel codepath on windows that I added in 0.6.

https://ci.appveyor.com/project/Stebalien/tempfile/builds/34838839

error[E0658]: use of unstable library feature 'maybe_uninit' (see issue #53491)
   --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\crossbeam-channel-0.4.3\src\flavors\array.rs:236:30
    |
236 |         slot.msg.get().write(MaybeUninit::new(msg));
    |                              ^^^^^^^^^^^^^^^^

@Stebalien
Copy link
Owner

Oh, I see. remove_dir_all has added a dependency on rayon. Making tempfile depend on something like rayon (and, more generally, starting threads) isn't an option.

@rbtcollins
Copy link
Contributor Author

Oh, I see. remove_dir_all has added a dependency on rayon. Making tempfile depend on something like rayon (and, more generally, starting threads) isn't an option.

Ok; that wasn't clear.

I can't do this immediately, but:... - for rustups test suite we'd benefit from the performance improvements the remove_dir_all new codepath has being offered from tempfile as well. The primary reason to add that was for the toolchain component removal codepath - removing 10's of thousands of files serially was awfully slow.

We would also benefit from this parallel removal codepath in our test suites use of tempfile. What if the remove_dir_all parallelism was controlled via a feature, so tempfile could default to a conservative serial behaviour and serial dependencies, and users of tempfile could opt into faster behaviour, with the associated dependencies and complexity.

@rbtcollins
Copy link
Contributor Author

Oh, and I've split out the master-fixing patch to #124 for you while we figure out what to do with remove_dir_all

@Stebalien
Copy link
Owner

We would also benefit from this parallel removal codepath in our test suites use of tempfile. What if the remove_dir_all parallelism was controlled via a feature, so tempfile could default to a conservative serial behaviour and serial dependencies, and users of tempfile could opt into faster behaviour, with the associated dependencies and complexity.

An opt-in feature works. My concern is that, by default at least, a temporary file library needs to be:

  1. Light weight. I've even had requests to switch to lighter random libraries (ref: use SmallRng #119).
  2. Predictable. I would not expect a temporary file library to spawn a bunch of threads. Honestly, I wouldn't expect a remove_dir_all function to do that either, I'm quite surprised this was enabled by default.

However, if the user explicitly opts in to removing files in parallel, that's their choice. I'd still prefer it if it were completely opt-in as anyone explicitly pulling in remove_dir_all (or some other library that depends on it) will be opted into this behavior even if they don't know about it, but I can live without that.

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