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

Is flate2's rust_backend feature really used? #8019

Closed
est31 opened this issue Mar 19, 2020 · 7 comments · Fixed by #8023
Closed

Is flate2's rust_backend feature really used? #8019

est31 opened this issue Mar 19, 2020 · 7 comments · Fixed by #8023
Labels
E-easy Experience: Easy

Comments

@est31
Copy link
Member

est31 commented Mar 19, 2020

The flate2 dependency is included by cargo in the following way:

flate2 = { version = "1.0.3", features = ["zlib"] }

This includes the on-by-default rust_backend feature, which adds the miniz_oxide dependency to flate2 and the adler32 transitive dependency. In the rustc repo you can find the following note though, saying that adler32 is not used:

rust-lang/rust@61fe2e4#diff-3ac60df36be32d72842bf5351fc2bb1dR43

It might be better to turn off the default features of flate2 so that the unused crate doesn't have to be compiled by people.

@est31 est31 changed the title Is flate2 Is flate2's rust_backend feature really used? Mar 19, 2020
@ehuss
Copy link
Contributor

ehuss commented Mar 19, 2020

I believe the answer is "no". Removing it saves about 0.1 seconds on my system, so I'm not sure it really matters, but if you want to send a PR to disable default features, it should probably be OK.

@est31
Copy link
Member Author

est31 commented Mar 20, 2020

Can you label it easy and help wanted? Thanks!

@aleksator
Copy link
Contributor

Done. I'll create a PR to rust repository to remove these 2 dependencies from their whitelist once 8023 is merged.

@est31
Copy link
Member Author

est31 commented Mar 20, 2020

@aleksator thanks! Don't forget to also update the cargo submodule

@ehuss
Copy link
Contributor

ehuss commented Mar 20, 2020

Done. I'll create a PR to rust repository to remove these 2 dependencies from their whitelist once 8023 is merged.

That won't be necessary. flate2 is used in a number of other projects in the rust repository, which use the default features.

@est31
Copy link
Member Author

est31 commented Mar 20, 2020

Good point. The comment still needs updating because it's not just an unused dep of cargo but used inside the compiler, too.

@aleksator
Copy link
Contributor

aleksator commented Mar 20, 2020

@ehuss

That won't be necessary. flate2 is used in a number of other projects in the rust repository, which use the default features.

Certainly true in regards to this constant in deps.rs then:

/// These are exceptions to Rust's permissive licensing policy, and
/// should be considered bugs. Exceptions are only allowed in Rust
/// tooling. It is _crucial_ that no exception crates be dependencies
/// of the Rust runtime (std/test).
const EXCEPTIONS: &[(&str, &str)]

But what about

/// Whitelist of crates rustc is allowed to depend on. Avoid adding to the list if possible.
///
/// This list is here to provide a speed-bump to adding a new dependency to
/// rustc. Please check with the compiler team before adding an entry.
const WHITELIST: &[&str]

? This one is internal for rustc if I understand it correctly, so probably should be changed. Is it?

@est31

Good point. The comment still needs updating because it's not just an unused dep of cargo but used inside the compiler, too.

What would you change it to?

bors added a commit that referenced this issue Mar 23, 2020
…xcrichton

Remove unused transitive dependencies: miniz_oxide, adler32

Fixes #8019
@bors bors closed this as completed in 38aa4a2 Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Experience: Easy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants