rust-bindgen icon indicating copy to clipboard operation
rust-bindgen copied to clipboard

Disable feature logging and clap on default

Open NobodyXu opened this issue 2 years ago • 3 comments

This crate is very popular and depends by a lot of other crates, which uses the default feature set of this crate.

However, by default, bindgen pulls in clap and logging, two features that never get used when using as a build-dev but just waste more time to compile.

Thus I remove it from the default feature set.

Signed-off-by: Jiahao XU [email protected]

NobodyXu avatar Jul 20 '22 08:07 NobodyXu

Hmm, I am not sure we can do this.

cargo clean
cargo build
cargo run

results in:

error: target `bindgen` in package `bindgen` requires the features: `clap`
Consider enabling them by passing, e.g., `--features="clap"`

I do not think we can require people to pass --features flags in order to get the bindgen binary.

I think this PR would need https://github.com/rust-lang/cargo/issues/1982#issuecomment-766433889 to be resolved first.

https://github.com/rust-lang/rust-bindgen/blob/18367d5af64c64035104e86c2bca6c3667389d79/Cargo.toml#L53-L54

kulp avatar Jul 22 '22 19:07 kulp

@kulp I don't think that can is a valid reason to keep the existing default feature set.

Looking at the dependents of bindgen on crates.io, it is clear that almost all softwares on page 1-30 that use bindgen's default feature set, including tokio::io-uring, zstd and etc.

This means bindgen is slowing the entire ecosystem down by requiring them to build unused dependencies, which IMHO is definitely more important than breaking people building/running bindgen binary on their own.

It will be great if cargo supports enabling specific feature set when building binaries, but even without it, I would say we should remove clap from default feature set.

NobodyXu avatar Jul 23 '22 02:07 NobodyXu

I think this PR would need https://github.com/rust-lang/cargo/issues/1982#issuecomment-766433889 to be resolved first.

Thanks!

I am glad that there is an issue to resolve this!

NobodyXu avatar Jul 23 '22 02:07 NobodyXu

@emilio @kulp would it be the worst idea in the world considering a possible split of bindgen into the regular build-script bindgen as a library and a bindgen-cli crate that uses the former? It would be possible to put both crates under the same workspace.

pvdrz avatar Sep 22 '22 20:09 pvdrz

That seems reasonable, though may need some wrangling to get the tests working, since they use the cli args.

emilio avatar Sep 23 '22 07:09 emilio

@emilio I'm going to do a PoC next week and submit it to see if we can find a sweet spot for this.

Edit: opened https://github.com/rust-lang/rust-bindgen/pull/2284 as an alternative to this.

pvdrz avatar Sep 24 '22 02:09 pvdrz

:umbrella: The latest upstream changes (presumably #2284) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Oct 05 '22 01:10 bors-servo

Closing this in favor of #2284. Feel free to reopen if you consider that something from here is missing there.

pvdrz avatar Oct 05 '22 02:10 pvdrz