Skip to content

feat: disable default features in workspace dependencies#9456

Closed
jtguibas wants to merge 2 commits intoparadigmxyz:mainfrom
jtguibas:john/disable-default-features
Closed

feat: disable default features in workspace dependencies#9456
jtguibas wants to merge 2 commits intoparadigmxyz:mainfrom
jtguibas:john/disable-default-features

Conversation

@jtguibas
Copy link

@jtguibas jtguibas commented Jul 11, 2024

Disables default features in the workspace dependencies.

This is done so that features that do not compile in wasm32-wasi are not automatically enabled. This change was originally in #9430 but was seperated at the request of @mattsse.

@jtguibas jtguibas requested review from gakonst and mattsse as code owners July 11, 2024 19:06
@jtguibas jtguibas force-pushed the john/disable-default-features branch from dc2d88f to db4a543 Compare July 11, 2024 19:07
reth-payload-primitives = { path = "crates/payload/primitives" }
reth-payload-validator = { path = "crates/payload/validator" }
reth-primitives = { path = "crates/primitives" }
reth-primitives = { path = "crates/primitives", default-features = false, features = ["std"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a bit dangerous because this now disables "zstd-codec" which is problematic, right @joshieDo ?
so we'd need to enable this somewhere else, I'm thinking on node builder

Copy link
Author

Choose a reason for hiding this comment

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

IIRC zstd doesn't work on 32-bit architectures btw, so that's why it's not enabled by default.

@mattsse mattsse added the A-dependencies Pull requests or issues that are about dependencies label Jul 11, 2024
@jtguibas
Copy link
Author

@mattsse Noticing EF tests failed. Any ideas why compilation flags would effect this?

Will do some A/B testing with dependencies and try find source of issue.

@jtguibas jtguibas force-pushed the john/disable-default-features branch from 7592419 to db4a543 Compare July 11, 2024 22:41
@jtguibas
Copy link
Author

jtguibas commented Jul 11, 2024

Should be fixed now, seems like an issue with c-kzg not being enabled.

reth-prune = { path = "crates/prune/prune" }
reth-prune-types = { path = "crates/prune/types" }
reth-revm = { path = "crates/revm" }
reth-revm = { path = "crates/revm", default-features = false, features = ["std", "c-kzg"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we missing blst feature here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah this as well

I need to think about all of this a bit, because messing with features like this can result in giga footguns

Copy link
Author

@jtguibas jtguibas Jul 12, 2024

Choose a reason for hiding this comment

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

Yeah so in the zkVM context, I would ideally like to disable both c-kzg and blst by default. Ideally what would happen here is only std would be enabled.

Copy link
Author

Choose a reason for hiding this comment

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

Could we just only have features = ["std"] here and then in the crates that need c-kzg/blst we just enable it there?

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.

superseded by #9998

we need to be a bit more careful with this

@mattsse mattsse closed this Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-dependencies Pull requests or issues that are about dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants