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

Disable feature logging and clap on default #2238

Closed
wants to merge 1 commit into from

Conversation

NobodyXu
Copy link

@NobodyXu NobodyXu commented Jul 20, 2022

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 Jiahao_XU@outlook.com

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@kulp
Copy link
Member

kulp commented Jul 22, 2022

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 rust-lang/cargo#1982 (comment) to be resolved first.

rust-bindgen/Cargo.toml

Lines 53 to 54 in 18367d5

# This kinda sucks: https://github.com/rust-lang/cargo/issues/1982
clap = { version = "3", optional = true }

@NobodyXu
Copy link
Author

@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
Copy link
Author

NobodyXu commented Jul 23, 2022

I think this PR would need rust-lang/cargo#1982 (comment) to be resolved first.

Thanks!

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

@kulp kulp added the dependencies Pull requests that update a dependency file label Jul 25, 2022
@pvdrz
Copy link
Contributor

pvdrz commented Sep 22, 2022

@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.

@emilio
Copy link
Contributor

emilio commented Sep 23, 2022

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

@pvdrz
Copy link
Contributor

pvdrz commented Sep 24, 2022

@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 #2284 as an alternative to this.

@bors-servo
Copy link

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

@pvdrz
Copy link
Contributor

pvdrz commented Oct 5, 2022

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

@pvdrz pvdrz closed this Oct 5, 2022
@NobodyXu NobodyXu deleted the disable-clap-on-default branch October 5, 2022 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants