Skip to content

chore: switch from zstd-codec to no-zstd#9462

Closed
joshieDo wants to merge 4 commits intomainfrom
joshie/no-zstd
Closed

chore: switch from zstd-codec to no-zstd#9462
joshieDo wants to merge 4 commits intomainfrom
joshie/no-zstd

Conversation

@joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Jul 11, 2024 •
edited
Loading

closes #8889
ref #9456

I don't know what's the best approach, tbh

This PR still has zstd as default. However, if default features are turned off it will fail compilatio, unless the developer chooses either no-zstd or zstd through reth-node-builder

@joshieDo joshieDo added the A-dependencies Pull requests or issues that are about dependencies label Jul 11, 2024
mattsse
mattsse requested changes Jul 12, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment •
edited
Loading

Choose a reason for hiding this comment

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

can we please document these features in lib.rs, because these are very delicate and it's not obvious how they behave

crates/primitives/Cargo.toml
c-kzg = ["dep:c-kzg", "revm-primitives/c-kzg", "dep:tempfile", "alloy-eips/kzg"]
zstd-codec = ["dep:zstd"]
zstd = ["dep:zstd"]
no-zstd = []
Copy link
Collaborator

@mattsse mattsse Jul 12, 2024

Choose a reason for hiding this comment

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

is this even necessary?

crates/primitives/Cargo.toml
Comment on lines +102 to +103
zstd = ["dep:zstd"]
no-zstd = []
Copy link
Collaborator

@mattsse mattsse Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I find these a bit confusing because not obvious why no-zstd isn't the absence of zstd

@mattsse
Copy link
Collaborator

mattsse commented Jul 12, 2024 •
edited
Loading

This PR still has zstd as default. However, if default features are turned off it will fail compilation

this shouldn't be the case

@joshieDo joshieDo closed this Jul 12, 2024
@joshieDo joshieDo deleted the joshie/no-zstd branch September 5, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattsse mattsse mattsse requested changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes

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

@fgimenez fgimenez Awaiting requested review from fgimenez

@onbjerg onbjerg Awaiting requested review from onbjerg

Assignees

No one assigned

Labels

A-dependencies Pull requests or issues that are about dependencies

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Switch feature logic from zstd-codec to no-zstd-codec

2 participants

@joshieDo @mattsse