Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Consider re-exporting rocksdb crate from store #6301

Closed
matklad opened this issue Feb 16, 2022 · 2 comments · Fixed by #7703
Closed

Consider re-exporting rocksdb crate from store #6301

matklad opened this issue Feb 16, 2022 · 2 comments · Fixed by #7703
Assignees
Labels
C-housekeeping Category: Refactoring, cleanups, code quality Node Node team P-low Priority: low T-node Team: issues relevant to the node experience team

Comments

@matklad
Copy link
Contributor

matklad commented Feb 16, 2022

#6300 made me realize that we specify version of rocks we use in several places. This is a bit subomtimal. In the store/lib.rs we might want to add pub extern crate rocksdb; to have the version specified in just one Cargo.toml

@matklad matklad added C-housekeeping Category: Refactoring, cleanups, code quality T-node Team: issues relevant to the node experience team labels Feb 16, 2022
@nikurt nikurt added the P-low Priority: low label Jun 14, 2022
@mina86 mina86 self-assigned this Jun 14, 2022
@mina86
Copy link
Contributor

mina86 commented Jun 17, 2022

I don’t know if this is really an issue. Yes, we need to update the version in three places but that’s hardly a large number. Indexer uses rocksdb directly because it creates its own database unrelated to store. runtime-params-estimator uses the crate to bypass all of the abstraction. It doesn’t feel right to export rocksdb through store just to facilitate easy abstraction layer breaking.

@matklad
Copy link
Contributor Author

matklad commented Jun 20, 2022

Yeah, that's a valid point. I guess I could argue a bit that exposing just the crate (as opposed to as_rocks method) doesn't really break abstraction, but that's not a super-strong argument. But for me, on balance, one place to set version/flags of the crate feels more important than abstraction purity at this case.

There's also rust-lang/cargo#10497 which presumably would allow us to fix this in a more principled way in a near future?

near-bulldozer bot pushed a commit that referenced this issue Sep 27, 2022
Move all dependencies to root Cargo.toml file and refer to it using
inheritance in all members of the workspace.  This will hopefully help
reduce noise when updating a package in the future.  The only
exception is `rand` dependency in core/crypto crate which needs to be
held back at older version because of ed25519-dalek.

Doing this, put union on cargo features in the root Cargo.toml so
features don’t need to be enabled in member crates.  The one exception
to this rule is serde_json’s preserve_order feature which is only
enabled in tools that need it.

Fixes: #6301
Fixes: #7674
nikurt pushed a commit that referenced this issue Nov 9, 2022
Move all dependencies to root Cargo.toml file and refer to it using
inheritance in all members of the workspace.  This will hopefully help
reduce noise when updating a package in the future.  The only
exception is `rand` dependency in core/crypto crate which needs to be
held back at older version because of ed25519-dalek.

Doing this, put union on cargo features in the root Cargo.toml so
features don’t need to be enabled in member crates.  The one exception
to this rule is serde_json’s preserve_order feature which is only
enabled in tools that need it.

Fixes: #6301
Fixes: #7674
@gmilescu gmilescu added the Node Node team label Oct 19, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-housekeeping Category: Refactoring, cleanups, code quality Node Node team P-low Priority: low T-node Team: issues relevant to the node experience team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants