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 lazy_static #180

Merged
merged 2 commits into from
Jan 19, 2025
Merged

Remove lazy_static #180

merged 2 commits into from
Jan 19, 2025

Conversation

vbasky
Copy link
Contributor

@vbasky vbasky commented Jan 16, 2025

  • Latest iteration of Rust since 1.71 can perform LazyLock which is what lazy_static was used for
  • Use standard library instead of another crate

* Latest iteration of Rust can perform LazyLock which is what lazy_static was used for
@akprasad akprasad changed the title Removed lazy_static Remove lazy_static Jan 16, 2025
Copy link
Contributor

@akprasad akprasad left a comment

Choose a reason for hiding this comment

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

This is really exciting! thanks for taking the initiative to do this.

There are three things I want to check before merge:

  • the code doesn't build for me on Rust 1.74 because LazyLock is not stable on that version. I think it was stabilized only in 1.80. So first we should upgrade the package version to 1.80 and see if that breaks anything.

  • Relatedly, I want to understand if there's some downside to using a newer Rust version generally. I don't understand this very well.

  • I've heard rumblings that LazyLock might have problems with no_std but lazy_static does not -- not sure if there's any truth to that, but it could potentially affect our Wasm bindings (can't recally if they need no_std or not).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what the usual style is, but I like having two blank spaces before heading so that sections are clear. Can you revert the changes to this file?

Copy link
Contributor Author

@vbasky vbasky Jan 16, 2025

Choose a reason for hiding this comment

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

image

This is the VSCode Extension that I use for Markdown Lint. The extension has well written rules that a markdown should validate against. The cleanup that I did was based on the extension's recommendation

image

This is how it offers suggestions to make markdown compliant. But you are right. The change here is unrelated to what this PR is inteded for. I should revert this change, will do.

@akprasad
Copy link
Contributor

I've done some reading on the second point (downside of upgrading rust version). The key phrase is MSRV, "minimum supported Rust version." Useful discussion for context and thinking through options: rust-lang/libs-team#72

The idea is that our MSRV is bounded below by all of our dependencies, and whatever version we choose, that will be the MSRV for any project that depends on Vidyut. So if we need to use 1.80 to support LazyLock, any project that depends on Vidyut also needs to use 1.80 at minimum.

I've checked a variety of libraries that we depend on, and they tend to be at most around version 1.63.

So my current inclination is that the cost of upgrading the MSRV outweighs the benefit of moving off of lazy_static, but I need more research to be sure. To change my mind on this, I would want to see:

  • examples of popular Rust libraries with high MSRVs, ideally a library we depend on currently.
  • some solution that lets older versions of Rust work with newer MSRVs
  • a way to use LazyLock in versions before 1.80

@akprasad
Copy link
Contributor

Actually, can we use use OnceCell? This was stabilized in 1.70 and we use RefCell already, so this might solve the problem without needing LazyLock.

@vbasky
Copy link
Contributor Author

vbasky commented Jan 18, 2025

Modified LazyLock with OnceLock because of MSRV compatibility to 1.70

@akprasad
Copy link
Contributor

Almost there!

Can you revert all changes to ARCHITECTURE.md ? I see there's still a stray line there.

I tried editing it directly to avoid some back-and-forth but wasn't able to. I can fix up future PRs if you enable this, I think: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.

* Made the switch for MSRV compatibility
@vbasky
Copy link
Contributor Author

vbasky commented Jan 19, 2025

Fixed the stray commit and squashed the commit.

@akprasad akprasad merged commit 66a9315 into ambuda-org:main Jan 19, 2025
19 checks passed
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.

2 participants