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

Update optional parking_lot dependency to 0.11.0 #2676

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

jbg
Copy link
Contributor

@jbg jbg commented Jul 22, 2020

Motivation

Part of a long-running quest to minimise binary size bloat in our projects by eliminating duplicate dependencies (dependencies with more than one version compiled in).

Solution

Update the optional parking_lot dependency in tokio to version 0.11.0. The MSRV is unchanged from 0.10.0, and parking_lot's API changes in 0.11.0 do not affect tokio.

@@ -104,7 +104,7 @@ memchr = { version = "2.2", optional = true }
mio = { version = "0.6.20", optional = true }
iovec = { version = "0.1.4", optional = true }
num_cpus = { version = "1.8.0", optional = true }
parking_lot = { version = "0.10.0", optional = true } # Not in full
parking_lot = { version = "0.11.0", optional = true } # Not in full
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about a more permissive requirement that allows both, like

Suggested change
parking_lot = { version = "0.11.0", optional = true } # Not in full
parking_lot = { version = ">= 0.10.0, <= 0.11.0", optional = true } # Not in full

If the goal is just to reduce duplicate dependency versions, this would be the ideal choice --- if other dependencies in tree are already pulling 0.10, Cargo would select that version. Tokio doesn't really use any of the APIs that have changed between those releases...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that prevent using 0.11.1 once that is released?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the advantage? Seems best to track deps?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that prevent using 0.11.1 once that is released?

Err, I meant < 0.12.0.

What would be the advantage? Seems best to track deps?

The idea is that Tokio should be able to use whatever parking_lot version is pulled in by another crate, if there is one in-tree, to minimize compiling duplicate versions. If another crate has an older dependency, we could just use it, and prefer the latest if no other crate depends on some other version.

I'm not sure if this is the right choice or not, but the only parking_lot APIs we depend on are the ones that mimic std, and therefore will probably not change very much. We can ignore breaking changes to unrelated APIs that we don't care about in order to reduce the total number of parking_lots in a crate where multiple dependencies use parking_lot.

This is a loosely-held opinion...I'm willing to be convinced this is a bad idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any data on whether or not it works well. If you think it is worth it, we can try it. I also do not know if the cargo dependency resolver is smart enough to dedup the way you are describing (it is a very hard problem).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall having heard issues with the resolver in cases like this before on the user's forum. I would want to test that it really works properly before doing this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not a big deal if this doesn't work, then!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless something changed within the last ~year, cargo will prefer using the latest versions possible for everything even if deduplication would be possible, unless the crate being deduplicated uses links (out of necessity) - see also rust-lang/cargo#4902 (comment).

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. labels Jul 25, 2020
@carllerche carllerche merged commit 084fcd7 into tokio-rs:master Jul 28, 2020
@dekellum
Copy link
Contributor

Forgive the late comment on this, but since the merged change its not released yet...

Besides this optional parking_lot tokio feature, I have two other deps that depend on parking_lot, and one of them tends to be slow to update. The change as merged (084fcd7) will ensure that there are duplicate parking_lot versions until the other deps upgrade.

The alternative that @hawkw proposed above (">=0.10.0, <0.12") would make it possible to avoid the duplicate. What @jebrosen states above matches my experience: cargo will prefer to use latest version over minimizing duplicates. But with the more permissive range its possible to selectively update or manually downgrade in the lock file to avoid the duplicate. Once all my deps permit parking_lot 0.11.0, I can then upgrade without a duplicate and without further change in tokio.

Are you willing to reconsider?

@Darksonn Darksonn mentioned this pull request Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants