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

Removed the last dependencies breaking no-std build. #669

Merged
merged 6 commits into from
Sep 1, 2023

Conversation

lvella
Copy link
Contributor

@lvella lvella commented Aug 30, 2023

Removed to_binary, which was used only in a debug display.

Replaced once_cell with lazy_static. The no-std build of once_cell did not support the Sync version of OnceCell, that was needed when creating static variables. On the other hand, lazy_static is the complete solution for lazyly initialized (Sync) static variables, and is no-std.

With this changes, revm builds on a no-std platform.

Removed to_binary, which was used only in a debug display.

Replaced once_cell with lazy_static. The no-std build of once_cell did
not support the Sync version of OnceCell, that was needed when creating
static variables. On the other hand, lazy_static is the complete
solution for lazyly initialized (Sync) static variables, and is no-std.

With this changes, revm builds on a no-std platform.
sha2 = { version = "0.10.5", default-features = false }
sha3 = { version = "0.10.7", default-features = false }
lazy_static = "1.4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

lazy_static is unmaintained. Please just use once_cell with default-features = false.

Copy link
Contributor Author

@lvella lvella Aug 31, 2023

Choose a reason for hiding this comment

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

That was my first try, doesn't work with once_cell::unsync::OnceCell, which is not Sync, thus it can't be used to initialize a static variable. It seems lazy_static manages to do it by using a spin-lock, what once_cell doesn't have.

Anyway, the other crate I removed, to-binary, is also unmaintained and is older, so I don't see the issue of using lazy_static.

Copy link
Collaborator

@DaniPopes DaniPopes Aug 31, 2023

Choose a reason for hiding this comment

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

See the once_cell FAQ:

  • Does this crate support no_std
  • Should I use std::cell::OnceCell, once_cell, or lazy_static?

I say we enable the critical-section feature. cc @rakita

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just found once_cell::race. I'll use it instead of lazy_static.

Copy link
Member

Choose a reason for hiding this comment

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

OnceCell was stabilised inside rust, I would check if it works
https://doc.rust-lang.org/nightly/core/cell/struct.OnceCell.html#:~:text=Struct%20core%3A%3Acell%3A%3AOnceCell&text=A%20cell%20which%20can%20be,borrow%20checks%20(unlike%20RefCell%20).

to-binary is not relevant at all as it had very simpler usecase,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it seems once_cell::race doesn't provide a generic OnceCell, instead it is specialized for some types that can be handled by atomic instructions. I'll change this PR to use critical-section.

Copy link
Member

@rakita rakita Aug 31, 2023

Choose a reason for hiding this comment

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

Sorry, it seems once_cell::race doesn't provide a generic OnceCell, instead it is specialized for some types that can be handled by atomic instructions. I'll change this PR to use critical-section.

Just came here to write this. We can use once_cell::race::OnlyBox, it does not require std and it fits nicely with the code

Copy link
Contributor Author

@lvella lvella Aug 31, 2023

Choose a reason for hiding this comment

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

Well, I just pushed a version using critical-section, had to add a new feature and all. Should I change?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I just pushed a version using critical-section, had to add a new feature and all. Should I change?

It would be better to change it, fewer features to maintain and it would work out of the box ( pun intended :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm! waiting for CL to pass

@rakita rakita merged commit 175aaec into bluealloy:main Sep 1, 2023
7 checks passed
@rakita
Copy link
Member

rakita commented Sep 4, 2023

@lvella what is the best build command to add to CL for no_std check?

@lvella
Copy link
Contributor Author

lvella commented Sep 4, 2023

@lvella what is the best build command to add to CL for no_std check?

I don't know the best, but what I have been using is this target that has no std available, so a dependency can't inadvertedly rely on it.

$ rustup target add riscv32imac-unknown-none-elf
$ cargo check --target riscv32imac-unknown-none-elf --no-default-features

mikelodder7 pushed a commit to LIT-Protocol/revm that referenced this pull request Sep 12, 2023
* Removed the last dependencies breaking no-std build.

Removed to_binary, which was used only in a debug display.

Replaced once_cell with lazy_static. The no-std build of once_cell did
not support the Sync version of OnceCell, that was needed when creating
static variables. On the other hand, lazy_static is the complete
solution for lazyly initialized (Sync) static variables, and is no-std.

With this changes, revm builds on a no-std platform.

* Reverting back to OnceCell.

* Hex encoding debug print.

* Using race::OnceBox instead of critical-section.

* Lint error.
Evalir pushed a commit to Evalir/revm that referenced this pull request Sep 14, 2023
* Removed the last dependencies breaking no-std build.

Removed to_binary, which was used only in a debug display.

Replaced once_cell with lazy_static. The no-std build of once_cell did
not support the Sync version of OnceCell, that was needed when creating
static variables. On the other hand, lazy_static is the complete
solution for lazyly initialized (Sync) static variables, and is no-std.

With this changes, revm builds on a no-std platform.

* Reverting back to OnceCell.

* Hex encoding debug print.

* Using race::OnceBox instead of critical-section.

* Lint error.
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

3 participants