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

bb8: Make parking_lot an optional feature enabled by default #152

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

r-ml
Copy link
Contributor

@r-ml r-ml commented Feb 9, 2023

Rust 1.62 improved std::sync::Mutex speed and 1.63 constified its constructor. It can now be used in the same contexts parking_lot's implementation can.

Currently, bb8 is enabling the parking_lot tokio feature for its users. A library should probably not do that without a good reason. With the recent-ish improvements to std::sync::Mutex, I believe we can drop parking_lot from the dependency tree entirely.

@djc
Copy link
Owner

djc commented Feb 9, 2023

Well, bb8 currently has an MSRV of 1.57, so we cannot rely on the const constructor. On top of that, the 1.62 release notes only discuss Linux, not any of the other platforms. So I don't think this makes sense as a default. I'd be okay to take this as a Cargo feature that is enabled by default, though.

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.16 🎉

Comparison is base (5737736) 74.11% compared to head (04ee589) 74.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
+ Coverage   74.11%   74.28%   +0.16%     
==========================================
  Files           6        6              
  Lines         537      556      +19     
==========================================
+ Hits          398      413      +15     
- Misses        139      143       +4     
Impacted Files Coverage Δ
bb8/src/internals.rs 94.92% <ø> (ø)
bb8/src/lib.rs 100.00% <100.00%> (ø)
bb8/src/api.rs 62.42% <0.00%> (-1.33%) ⬇️
bb8/src/inner.rs 86.24% <0.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@r-ml
Copy link
Contributor Author

r-ml commented Feb 9, 2023

Well, bb8 currently has an MSRV of 1.57, so we cannot rely on the const constructor. On top of that, the 1.62 release notes only discuss Linux, not any of the other platforms. So I don't think this makes sense as a default. I'd be okay to take this as a Cargo feature that is enabled by default, though.

The pool constructor isn't const anyway.

To put it behind a feature flag I'll check what other libraries do in this case, because std::sync::Mutex::lock does not have a compatible signature to parking_lot's impl.

@r-ml r-ml changed the title bb8: Drop reliance on parking_lot bb8: Make parking_lot an optional feature enabled by default Feb 9, 2023
@r-ml
Copy link
Contributor Author

r-ml commented Feb 9, 2023

Done. Please tell me whether the mod is a good enough way to implement this or would you prefer I sprinkle #[cfg(feature ...)] everywhere?

@r-ml
Copy link
Contributor Author

r-ml commented Feb 9, 2023

Well, cargo's dep: feature was added in Rust 1.60 sigh...

@r-ml
Copy link
Contributor Author

r-ml commented Feb 9, 2023

I installed 1.57 and I can work around the issue by naming the feature parking-lot instead of parking_lot. Let me know whether that's good enough.

Another thing, compilation failed here with "there is no argument named err". This appears to be fixed with 1.58. Want me to make a PR to make it compile? CI should have caught that though 🤔

@djc
Copy link
Owner

djc commented Feb 26, 2023

Okay, let's bump the MSRV to 1.60 (in a separate commit) so we can use dep:?

The CI setup, as I recall it, is a bit strange because the redis crate has a newer MSRV than the core bb8 crates. But bumping the MSRV should fix this anyway.

@r-ml
Copy link
Contributor Author

r-ml commented Mar 8, 2023

Done. (For some reason I did not get notified by Github of your comment)

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks!

@djc djc merged commit 03f95c9 into djc:main Mar 13, 2023
@nmldiegues nmldiegues mentioned this pull request Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants