[skunkworks] rust: use workspace dependencies #19375
Conversation
ParkMyCar
changed the title
ParkMyCar
requested review from
a team,
aalexandrov and
umanwizard
as code owners
Cargo.toml
Outdated
| yansi = "0.5.1" | ||
| zeroize = "1.5.7" | ||
|
|
||
| mz-adapter = { path = "src/adapter" } |
There was a problem hiding this comment.
do we get any particular benefit from putting all the path deps here, too? if not, i'm inclined to say we should leave them inlined
There was a problem hiding this comment.
I only separated them because that's what we used to do with imports, I'll inline them!
| tikv-jemalloc-ctl = "0.5.0" | ||
| tikv-jemallocator = "0.5.0" |
There was a problem hiding this comment.
These are optional dependencies. It would be great if we could keep it this way because several memory profiling tools have a hard time with jemalloc and work much better with the system allocator.
There was a problem hiding this comment.
(This is also why the lint fails)
There was a problem hiding this comment.
Huh, I'm surprised this doesn't work as is. AFAICT the dependencies are still optional in the package itself; it's just that if they are enabled they're meant to use the version here.
There was a problem hiding this comment.
Totally agree that we should keep these as optional deps, it was through investigating this lint that I found default-features don't really work with workspace dependencies. I filed rust-lang/cargo#12162 to discuss this
There was a problem hiding this comment.
The described behavior seems workable for us. Seems like it'll be annoying for the author of this PR (:D), but not too annoying to deal with once it's in place. There are very few crates where we want default-features = false in some places but not others; usually every Materialize crates wants to turn of the default features or none.
| uncased = { workspace = true } | ||
| uuid = { workspace = true, features = ["v5"]} | ||
| proptest = { workspace = true, default-features = false, features = ["std"] } | ||
| proptest-derive = { workspace = true, features = ["boxed_union"]} |
There was a problem hiding this comment.
If we can define features in addition to the version number in the workspace Cargo.toml, I think it makes sense to also mandate features = ["boxed_union"] by default (see #16809).
There was a problem hiding this comment.
Yep, looks like you can specify features on the workspace dependency that get unioned with each package's features.
ParkMyCar
changed the title
|
Moving this to the draft stage because I realized workspace dependencies don't work well with default features, I filed an issue against Cargo here rust-lang/cargo#12162. tl;dr if you inherit a dependency from the workspace, there is no way to disable default features for that dependency, without also disabling them for the entire workspace. What you need to do is first disable them for the entire workspace and then enable them in the crates which you want them, which is presumably most. |
|
Very out of date, will need to re-open |
Motivation
This PR moves all of our dependency version into the workspace's
Cargo.toml. Then all of the crates in the workspace specify a version with{ workspace = true }. This makes it easier to upgrade dependencies and to make sure we're relying on the same version of a dependency all throughout the code base.Slack thread for context: https://materializeinc.slack.com/archives/CMH6PG4CW/p1684502539940499
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.