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

optional rocksdb for frame-benchmarking-cli #7649

Merged
merged 3 commits into from
Feb 24, 2025
Merged

optional rocksdb for frame-benchmarking-cli #7649

merged 3 commits into from
Feb 24, 2025

Conversation

vedhavyas
Copy link
Contributor

@vedhavyas vedhavyas commented Feb 20, 2025 •
edited
Loading

sc-cli brings rocksdb dependency into frame-benchmarking-cli when used with default-features = false.
This PR makes rocksdb deps optional that sc-cli brings in some of the crates. I think I covered all the crates that depend on sc-cli but please let me know if I missed any.

Fixes: #3793

@vedhavyas vedhavyas mentioned this pull request Feb 20, 2025
Merged
nazar-pc
nazar-pc reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Also PR description has a typo, --no-default-feature should be replaced with --default-feature = true

@vedhavyas vedhavyas force-pushed the rocks-db-optional-sc-cli branch from 4fe1b92 to 21604a8 Compare February 21, 2025 10:46
@vedhavyas vedhavyas force-pushed the rocks-db-optional-sc-cli branch from 21604a8 to 04d8968 Compare February 21, 2025 10:47
bkchr
bkchr approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

In the workspace rocksdb will still be enabled. Did you check if this doesn't build rocksdb when you only install the benchmarking cli?

@bkchr bkchr requested a review from ggwpez February 24, 2025 09:55
@vedhavyas
Copy link
Contributor Author

In the workspace rocksdb will still be enabled. Did you check if this doesn't build rocksdb when you only install the benchmarking cli?

With default-features=False benchmarking-cli will not bring rocksdb deps in. I have tested this on our repo here autonomys/subspace@afae258 (#3394)

@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Feb 24, 2025
@bkchr
Copy link
Member

bkchr commented Feb 24, 2025

/cmd fmt

@github-actions GitHub Actions
Copy link
Contributor

Command "fmt" has failed ❌! See logs here

ggwpez
ggwpez approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Thanks! Confirmed that cargo tree does not show rocksdb anymore for frame-benchmarking-cli when run with --no-default-features.

@ggwpez ggwpez enabled auto-merge February 24, 2025 16:05
@ggwpez ggwpez added this pull request to the merge queue Feb 24, 2025
Merged via the queue into paritytech:master with commit 2edabef Feb 24, 2025
248 of 254 checks passed
@vedhavyas vedhavyas deleted the rocks-db-optional-sc-cli branch February 24, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nazar-pc nazar-pc nazar-pc left review comments

@ggwpez ggwpez ggwpez approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

frame-benchmarking-cli should not build RocksDB
4 participants
@vedhavyas @bkchr @nazar-pc @ggwpez