Skip to content

[keccak-hash] Bump version#159

Closed
dvdplm wants to merge 3 commits into
masterfrom
dp/chore/fix-keccak-hash-dependency
Closed

[keccak-hash] Bump version#159
dvdplm wants to merge 3 commits into
masterfrom
dp/chore/fix-keccak-hash-dependency

Conversation

@dvdplm
Copy link
Copy Markdown
Contributor

@dvdplm dvdplm commented May 16, 2019

In the switch to depend on primitive-types a local dependency was used ({ path = "../primitive-types" }). And a new version was never published, which causes trouble for depending crates higher up the stack.

Bump to 0.2.0

Question: should the version be 0.2 here? I think so.

@dvdplm dvdplm requested a review from ordian May 16, 2019 19:03
@dvdplm dvdplm self-assigned this May 16, 2019
@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 16, 2019

Not sure I understand what problem does this PR solve, could you elaborate?
(we need to publish keccak-hash 0.2, but that's a separate issue)

@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 17, 2019

what problem does this PR solve,

Using path = "../path/to/dep" to specify dependencies is not allowed when publishing crates, so that's one problem. Another problem is that if you use [patch] to load some crates from a git branch and some others a your local checkout and those two patched crates share a dependency, then you'll end up loading two different crates, which, even if they're exactly the same code can cause hard-to-debug issues.
I think the only time it's ok to use a path in parity-common is when the dependency is below, i.e. for sub-crates.

@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 17, 2019

I think the only time it's ok to use a path in parity-common is when the dependency is below, i.e. for sub-crates.

Hmm, and then I found this and now I'm confused again. How can that even work?

EDIT: aha, the code in the repo is not actually what was published

@dvdplm dvdplm changed the title [keccak-hash] Fix local dependency [keccak-hash] Bump version May 17, 2019
Copy link
Copy Markdown
Contributor

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Do we want to close #146 or tackle it in a separate PR?

@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 17, 2019

Do we want to close #146 or tackle it in a separate PR?

Separate PR. I think we have enough crate version churn to handle atm.

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 17, 2019

Hm, actually, let's wait for openethereum/parity-ethereum/pull/10670, seems like ethereum-types would work better as a dependency.

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 23, 2019

Closing in favor of #158.

@ordian ordian closed this May 23, 2019
@ordian ordian deleted the dp/chore/fix-keccak-hash-dependency branch May 23, 2019 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants