tokio icon indicating copy to clipboard operation
tokio copied to clipboard

Should we vendor once_cell?

Open Darksonn opened this issue 3 years ago • 8 comments

This issue has been opened to resolve the question posed in https://github.com/tokio-rs/tokio/pull/3187#issuecomment-738258504, from the PR replacing lazy_static with once_cell.

This PR is fine as a first step.

However, I am noticing that once_cell has support for parking_lot as an optional feature. Ideally, we could enable that feature when we enable that feature, but cargo does not support this.

Instead, we may consider vendoring once_cell to work around this.

Darksonn avatar Dec 04 '20 09:12 Darksonn

cc @matklad in case there are additional thoughts on the feature problem.

carllerche avatar Dec 04 '20 18:12 carllerche

That's because parking_lot is a optional-package feature and not a [features] parking_lot = [] feature, right?

Yeah, as far as I know, there's not stable way to get what you want here. :+1: from me on vendoring -- code in OnceCell is pretty stable today.

Though, the difference between parking_lot and std is pretty small in this case (as fast path is just acquire load in either case). There is some difference in terms of size_of. So, if size_of is not super critical for you, I'd probably just :man_shrugging: and not vendor anything. This shouldn't matter either way, and if the users insist on using PL, they can always enable once_cell pl feature in their own package.

matklad avatar Dec 04 '20 18:12 matklad

Also you won't have it anymore when you will switch to the std Lazy types in the future.

paolobarbolini avatar Dec 04 '20 19:12 paolobarbolini

We can probably do this without vendoring by using dependency renaming.

# Cargo.toml

[features]
parking_lot = ["parking_lot-crate", "once_cell/parking_lot"]

[dependencies]
parking_lot-crate = { version = "...", package = "parking_lot", optional = true }
once_cell = { ... }
// lib.rs

#[cfg(feature = "parking_lot")]
extern crate parking_lot_crate as parking_lot;

However, unfortunately, this way always enables once_cell dependency when you enable the parking_lot feature. (until weak dependency features are stabilized.) So, this way is not ideal given that once_cell is an optional dependency.

taiki-e avatar May 09 '21 11:05 taiki-e

Probably the best long-term solution is to help https://github.com/rust-lang/rust/issues/74465 move forward. It's blocked on implementation & naming consensus, which should be fixable sinking by some hours into that (I am at the moment pretty swamped with near & rust-analyzer to so this myself :( )

matklad avatar May 09 '21 11:05 matklad

If once_cell bumps MSRV (https://github.com/matklad/once_cell/pull/198#issuecomment-1248738780) we may need to consider doing this to address it.

The policy on how to handle MSRV bumps from dependencies is not yet decided, but https://github.com/rust-lang/libc/pull/2845, https://github.com/rust-lang/libs-team/issues/72, and some discussions related to it even discussed the possibility of forking libc.

taiki-e avatar Sep 17 '22 02:09 taiki-e

once_cell is unlikely to be more aggressive than six-months MSRV of tokio. At the moment, we are considering upgrading to 1.58, which is 11 months old.

matklad avatar Sep 17 '22 09:09 matklad

once_cell is unlikely to be more aggressive than six-months MSRV of tokio.

Yeah, it is not a problem for normal releases, but might be a problem for LTS releases: https://github.com/rust-lang/libc/pull/2845#issuecomment-1195668773

taiki-e avatar Sep 17 '22 09:09 taiki-e