Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Use crates from parity-common #9058

Closed
wants to merge 22 commits into from
Closed

Use crates from parity-common #9058

wants to merge 22 commits into from

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Jul 6, 2018

ref. #9011

Update parity-ethereum to use the new parity-common repo. In this PR the dependencies are fetched via the-crate = { git = "…" } rather than from crates.io as they are not all published/updated.

This affects the following crates in parity-ethereum:

hashdb
keccak-hash
kvdb
kvdb-rocksdb
memorydb
parity-bytes
parity-crypto
path
patricia_trie
plain_hasher
rlp
target
trie-standardmap
triehash

The crates above are not deleted from parity-ethereum to allow pending PRs to be merged without too much merge conflicts fun.

@dvdplm dvdplm self-assigned this Jul 6, 2018
@dvdplm dvdplm added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies. F5-task 🎬 Generic label for things that just have to get done but are no issues in first place. labels Jul 6, 2018
@5chdn 5chdn added this to the 2.0 milestone Jul 6, 2018
@5chdn 5chdn removed the F5-task 🎬 Generic label for things that just have to get done but are no issues in first place. label Jul 8, 2018
Cargo.toml Outdated
@@ -133,6 +133,7 @@ members = [
"transaction-pool",
"whisper",
"whisper/cli",
"util/triehash-ethereum",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing tab!

where
I: IntoIterator<Item = (A, B)>,
A: AsRef<[u8]>,
B: AsRef<[u8]>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant trait bounds i.e, A and B is referring to the same trait!

If this is for readability I would want these to be changed to (K, V) instead because they are referring to Key, Value right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

K/V makes sense, changing that. Not sure how to remove the bound though, can you show me how? Without the bound the call to triehash::sec_trie_root() does not work. Do you know a better way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

well my first comment is that you could do:

pub fn sec_trie_root<I, A>(input: I) -> H256
where
    I: IntoIterator<Item = (A, A)>,
    A: AsRef<[u8]>,
{}

Otherwise this is also ok IMO:

pub fn sec_trie_root<I, K, V>(input: I) -> H256
where
    I: IntoIterator<Item = (K, V)>,
    K: AsRef<[u8]>,
    V: AsRef<[u8]>,
{}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@niklasad1 that's not true. Right now, A and B can be different types (and they, very often are), whereas in your snippet they need to be exactly the same type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@niklasad1 it's probably easier to see if you look at the actual sec_trie_root() method here – this little crate here is just a "proxy" to the real deal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@debris @dvdplm Marek is right even if the types are [u8; 4] and [u8; 16] it wouldn't compile because they are different types so ignore me and continue :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore me

Never! It was a great q, forced me to think.

and continue

So that's a thumbs-up from you? ;)

* master:
  Clean up deprecated options and add CHECK macro (#9036)
  Replace `std::env::home_dir` with `dirs::home_dir` (#9077)
  fix warning in secret-store test (#9074)
  SeedHashCompute remove needless `new` impl (#9063)
  remove trait bounds from several structs (#9055)
  docs: add changelog for 1.10.9 stable and 1.11.6 beta (#9069)
  Enable test in `miner/pool/test` (#9072)
@dvdplm dvdplm requested a review from debris July 10, 2018 08:38
Cargo.toml Outdated
@@ -33,7 +33,7 @@ fdlimit = "0.1"
ctrlc = { git = "https://github.com/paritytech/rust-ctrlc.git" }
jsonrpc-core = { git = "https://github.com/paritytech/jsonrpc.git", branch = "parity-1.11" }
ethcore = { path = "ethcore", features = ["work-notify", "price-info", "stratum"] }
ethcore-bytes = { path = "util/bytes" }
parity-bytes = { git = "https://github.com/paritytech/parity-common", rev = "1803c0f" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove rev from Cargo.toml. Version is already locked in Cargo.lock file. This field will only force us to use sed before every dependency update

Copy link
Collaborator Author

@dvdplm dvdplm Jul 10, 2018

Choose a reason for hiding this comment

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

Yes, it's terrible. It's here because I could not for the world convince cargo to go out and fetch the latest version from git. When there are mismatching versions (or when rustc thinks the versions don't match) errors pop up like popcorn.

Do you happen to know how to force cargo to do a git fetch?

My plan here was to get the PR reviewed and then remove the rev = … as the last step before merging. In a follow-up PR I'll remove the crates now moved to parity-common and switch to using them from crates.io if I can't get git updates to work as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you happen to know how to force cargo to do a git fetch?

For jsonrpc crates it's enough to do cargo update -p X, where X is the name of one of the crates from jsonrpc repo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, it's possible that I messed up (I did discover one instance of a crate getting fetched with path so perhaps that's what tripped me up yesterday).

I'll remove the rev so we can merge this and then we'll see what happens when ppl star using it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants