-
Notifications
You must be signed in to change notification settings - Fork 160
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not do this globally? enable the default feature in the crates that want them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm fine with doing this globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no diff in Cargo.lock
, does this not pull in any additional dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other deps that pull in clap, such as criterion
and diplomat-tool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- @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.
clap
clap
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I had this different organisation before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You had "deps" and "dev deps" , but you didn't distinguish lib deps from datagen deps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.