-
Notifications
You must be signed in to change notification settings - Fork 228
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.
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
Use workspace metadata and deps #900
Conversation
@bkchr Since polkadot-sdk still has not begun the deprecation process of rocksdb, what do you think about releasing a new version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@ordian Sorry, I made a mistake. Could you help re-trigger the CI? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully last small changes and ready to go
criterion = "0.5.1" | ||
rand = { version = "0.8.0", default-features = false } | ||
hex-literal = "0.4.1" | ||
scale-info = { version = ">=1.0, <3", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was >= 0.9, < 3
, why did you change it? the reason we set it this way is to allow upsteam users to update scale-info
without having to update crates in parity-common
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this version range comes from https://github.com/paritytech/parity-common/pull/900/files#diff-c949d3988c105a100fde1d974b36302b74911bbb90efeb2581f94e6379a39ce5L20
I align with the highest one. Should I need to separate them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, i see. ok, let's keep it >= 1.0, < 3
. i think nobody should be using scale-info 0.9 at this point
Co-authored-by: ordian <[email protected]>
impl-codec = { workspace = true, optional = true } | ||
impl-num-traits = { workspace = true, optional = true } | ||
impl-rlp = { workspace = true, optional = true } | ||
scale-info = { workspace = true, features = ["derive"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically, renaming an optional dependency is a breaking change, because someone could have used scale-info-crate
feature, but hopefully nobody did 🤞🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was struggling here. I should keep codes as is, but workspace dependency isn't flexible.
I had to add a scale-info-crate
in the workspace, but I felt odd and changed the code.
CI has passed
Should I add uint/fuzz to the workspace? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍
I made three following PRs:
Thank you all for helping review my PR! |
In this PR:
This PR shouldn't change any dependencies. Just move them to the workspace Cargo.toml