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

Remove once_cell dependency #3520

Merged
merged 11 commits into from
Feb 25, 2024
Merged

Conversation

james7132
Copy link
Contributor

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

A follow up to #2313. Removes the once_cell dependency, instead using std::sync::OnceLock and a minimal polyfill for std::sync::LazyLock, which may be stablized soon (see rust-lang/rust#121377).

Should it be named LazyLock to match up with the proposed API? I kept it as Lazy for now.

This should not require a bump in MSRV, as OnceLock was stabilized in 1.70, which this crate is using.

@james7132
Copy link
Contributor Author

Didn't realize the type was in the public interface of the crate. Is this still acceptable as is?

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

I'm not convinced there is sufficient motivation to do this as long as:

  • once_cell is one of the most popular crates out there, e.g. our dependencies on Linux use it anyway, so removing it in Winit doesn't actually get rid of the dependency (on Linux).
  • Our manual implementation requires unsafe.
  • once_cell is a fairly small and well reviewed dependency.

It doesn't affect Web, so its up to the other maintainers.

@daxpedda
Copy link
Member

Didn't realize the type was in the public interface of the crate. Is this still acceptable as is?

I assume you mean because of all the CI failures? These things are not actually publicly exposed, they should rather be pub(crate) or pub(super).

@james7132
Copy link
Contributor Author

james7132 commented Feb 24, 2024

Our manual implementation requires unsafe.

This really only is needed if the poly-fill is kept. The actual implementation isn't too bad if we replace every Lazy with a OnceLock.

I assume you mean because of all the CI failures? These things are not actually publicly exposed, they should rather be pub(crate) or pub(super).

Would it be OK to turn those into pub(crate)/pub(super) in this PR?


With that said, I'm happy to close this out for now and revisit this when LazyLock gets stabilized, though probably undesirable until the MSRV bump is acceptable.

@madsmtm
Copy link
Member

madsmtm commented Feb 24, 2024

I'd rather go with converting our current use of once_cell to the stable OnceLock, since although it's minor, I don't want to add further unsafe to Winit if it can be avoided.

@notgull
Copy link
Member

notgull commented Feb 24, 2024

Agreed with moving away from OnceCell.

@james7132 james7132 requested a review from notgull February 24, 2024 23:55
@james7132
Copy link
Contributor Author

I eliminated the unsafe at @notgull's suggestion.

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Cool!

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Thanks!

@notgull notgull merged commit 352e70b into rust-windowing:master Feb 25, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants