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.

Sign up for GitHub

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

Jump to bottom

Disable default features for reth-storage-api in workspace manifest #14466

Merged
merged 7 commits into from
Feb 13, 2025
Merged

Disable default features for reth-storage-api in workspace manifest #14466

merged 7 commits into from
Feb 13, 2025

Conversation

emhane
Copy link
Member

@emhane emhane commented Feb 13, 2025

Disables default features for reth-storage-api in workspace manifest, ref #14465

@emhane emhane added C-debt A clean up/refactor of existing code A-dependencies Pull requests or issues that are about dependencies labels Feb 13, 2025
@emhane emhane requested a review from gakonst as a code owner February 13, 2025 09:48
@emhane emhane enabled auto-merge February 13, 2025 09:48
mattsse
mattsse requested changes Feb 13, 2025
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

smol fix:

we need std enabled int he reth-provider crate because there we impl the feature gate trait fns

@emhane
Copy link
Member Author

emhane commented Feb 13, 2025 •
edited
Loading

smol fix:

we need std enabled int he reth-provider crate because there we impl the feature gate trait fns

made me realise that std should be explicitly enabled for all crates with pending no-std support, due to the little perf degradation that could happen with the lesser precision of Tokio time.

@mattsse
Copy link
Collaborator

mattsse commented Feb 13, 2025 •
edited
Loading

run make lint-toml

with the lesser precision of Tokio time.

idk what this is referring to

@emhane emhane added this pull request to the merge queue Feb 13, 2025
@mattsse mattsse removed this pull request from the merge queue due to a manual request Feb 13, 2025
@emhane
Copy link
Member Author

emhane commented Feb 13, 2025

idk what this is referring to

tokio-rs/tokio#5996 (comment)

@emhane emhane enabled auto-merge February 13, 2025 12:18
@mattsse
Copy link
Collaborator

mattsse commented Feb 13, 2025

what's the connection to this pr here?

@emhane
Copy link
Member Author

emhane commented Feb 13, 2025

what's the connection to this pr here?

this pr makes storage api available as no-std using Tokio time, but uses std by default

we haven't benchmarked with Tokio time enabled for storage api, so we use std time by default still

@emhane emhane added this pull request to the merge queue Feb 13, 2025
Merged via the queue into main with commit 58275b8 Feb 13, 2025
44 checks passed
@emhane emhane deleted the emhane/storage-api-no-std branch February 13, 2025 12:44
mattsse
mattsse reviewed Feb 13, 2025
View reviewed changes
crates/storage/storage-api/src/chain_info.rs
Comment on lines +3 to +9
#[cfg(feature = "std")]
use std::time::Instant;
#[cfg_attr(feature = "std", allow(unused_imports))]
#[cfg(feature = "std")]
use tokio::time as _;
#[cfg(not(feature = "std"))]
use tokio::time::Instant;
Copy link
Collaborator

Choose a reason for hiding this comment

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

so, I overlooked this,

I want to undo that, this isn't useful

This was referenced Feb 13, 2025
Merged
Merged
frankudoags pushed a commit to frankudoags/reth that referenced this pull request Feb 13, 2025
@emhane @mattsse @frankudoags
Disable default features for reth-storage-api in workspace manifest (…
0fda3b3 
…paradigmxyz#14466)

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@rakita rakita Awaiting requested review from rakita rakita is a code owner

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@fgimenez fgimenez Awaiting requested review from fgimenez fgimenez is a code owner

@joshieDo joshieDo Awaiting requested review from joshieDo joshieDo is a code owner

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

@shekhirin shekhirin Awaiting requested review from shekhirin shekhirin is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@klkvr klkvr Awaiting requested review from klkvr klkvr is a code owner

Assignees
No one assigned
Labels
A-dependencies Pull requests or issues that are about dependencies C-debt A clean up/refactor of existing code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emhane @mattsse