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

Should we vendor once_cell? #3212

Closed
Darksonn opened this issue Dec 4, 2020 · 9 comments
Closed

Should we vendor once_cell? #3212

Darksonn opened this issue Dec 4, 2020 · 9 comments
Labels
A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup.

Comments

@Darksonn
Copy link
Contributor

Darksonn commented Dec 4, 2020

This issue has been opened to resolve the question posed in #3187 (comment), 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 Darksonn added C-maintenance Category: PRs that clean code up or issues documenting cleanup. A-tokio Area: The main tokio crate labels Dec 4, 2020
@carllerche
Copy link
Member

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

@matklad
Copy link
Contributor

matklad commented Dec 4, 2020

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. 👍 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 🤷‍♂️ 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.

@paolobarbolini
Copy link
Contributor

paolobarbolini commented Dec 4, 2020

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

@taiki-e
Copy link
Member

taiki-e commented May 9, 2021

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.

@matklad
Copy link
Contributor

matklad commented May 9, 2021

Probably the best long-term solution is to help rust-lang/rust#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 :( )

@taiki-e
Copy link
Member

taiki-e commented Sep 17, 2022

If once_cell bumps MSRV (matklad/once_cell#198 (comment)) 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 rust-lang/libc#2845, rust-lang/libs-team#72, and some discussions related to it even discussed the possibility of forking libc.

@matklad
Copy link
Contributor

matklad commented Sep 17, 2022

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.

@taiki-e
Copy link
Member

taiki-e commented Sep 17, 2022

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: rust-lang/libc#2845 (comment)

Noah-Kennedy added a commit that referenced this issue Sep 21, 2022
Fixes #3212.

With the release of `v1.15`, the once_cell crate is breaking our current MSRV. Vendoring it will allow us to keep using certain APIs from that crate without compromising our MSRV.
@taiki-e
Copy link
Member

taiki-e commented Nov 21, 2022

Addressed by #5048

@taiki-e taiki-e closed this as not planned Won't fix, can't repro, duplicate, stale Nov 21, 2022
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 a pull request may close this issue.

6 participants
@carllerche @Darksonn @matklad @paolobarbolini @taiki-e and others