Organize workspace deps and reenable default features where appropriate #4844
Conversation
|
As long as it's not pulling in more deps I'm fine with it. |
Cargo.toml
Outdated
| @@ -211,7 +211,7 @@ diplomat-tool = { git = "https://github.com/rust-diplomat/diplomat.git", rev = " | |||
|
|
|||
| # External deps from crates.io | |||
| bincode = { version = "1.3.1", default-features = false } | |||
| clap = { version = "4.2.0", default-features = false } | |||
| clap = { version = "4.2.0" } # clap has a lot of default features and we want them | |||
There was a problem hiding this comment.
can we not do this globally? enable the default feature in the crates that want them.
There was a problem hiding this comment.
I don't think we'd ever want clap without its default features, which largely enable diagnostics and help text and things, but ok
There was a problem hiding this comment.
Okay, I made this change, but I'm not convinced it's an improvement. It would be nice if we never had to rebuild the clap dependency when different icu4x projects use it with different features. Why allow projects to use clap with a different set?
There was a problem hiding this comment.
Yeah I'm fine with doing this globally.
There was a problem hiding this comment.
There's no diff in Cargo.lock, does this not pull in any additional dependencies?
There was a problem hiding this comment.
There are other deps that pull in clap, such as criterion and diplomat-tool
There was a problem hiding this comment.
- @sffc - We don't want to take on no-default-features liability.
- @Manishearth - Our no-default-features deps for code library crates are either tiny or maintained by BurntSushi who follows an even stricter version of the no-default-features stability policy.
- @robertbastian - If we are explicit about why deps have different features in Cargo.toml (in groups like allowlist.rs) it's fine with me.
sffc
changed the title
clapclap and other datagen deps
|
Autosubmit = true |
|
|
||
| ## Dev only | ||
| ## External Deps Group 3: Dev and Datagen deps. Include default features. |
There was a problem hiding this comment.
I did prefer the split between dev deps and non-dev deps. For example this looks like datagen depends on rkyv, so someone might introduce the dep somewhere else.
There was a problem hiding this comment.
I'll land this because it fixes problems and is likely to bitrot, and if you want to propose a different organization you're welcome to do so
There was a problem hiding this comment.
Well I had this different organisation before
There was a problem hiding this comment.
You had "deps" and "dev deps" , but you didn't distinguish lib deps from datagen deps
There was a problem hiding this comment.
For example this looks like datagen depends on rkyv, so someone might introduce the dep somewhere else.
I consider depcheck allowlist.rs to be the source of truth for what depends on what.
robertbastian
changed the title
clap and other datagen deps
Fixes #4843
Alternatively we could re-enable all the features manually, but that's brittle, and I'd rather use the default features.
We should probably check all the other crates, too. In general I think we should use default features for datagen deps.