8000 Fix ruy rust 1.22 inccompatibility by sgeisler · Pull Request #431 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix ruy rust 1.22 inccompatibility #431

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
Merged
merged 1 commit into from
Jun 29, 2020
Merged

Fix ruy rust 1.22 inccompatibility #431

merged 1 commit into from
Jun 29, 2020

Conversation

sgeisler
Copy link
Contributor
@sgeisler sgeisler commented Jun 1, 2020 •
edited by elichai
Loading

Fixes #430 by pinning ryu (transitive dependency of serde_json) to an older version.

@sgeisler sgeisler requested review from stevenroose and elichai June 1, 2020 12:46
@sgeisler sgeisler force-pushed the 2020-06-01-ruy-compat branch from d8d25f3 to ea79479 Compare June 1, 2020 12:47
@codecov-commenter
Copy link
codecov-commenter commented Jun 1, 2020 •
edited
Loading

Codecov Report

Merging #431 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #431   +/-   ##
=======================================
  Coverage   86.19%   86.19%           
=======================================
  Files          39       39           
  Lines        7394     7394           
=======================================
  Hits         6373     6373           
  Misses       1021     1021

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4e26ca...139fc02. Read the comment docs.

@stevenroose
Copy link
Collaborator

I can confirm that it works, but I'd like to have a line of comment in Cargo.toml explaining why the ryu line is there. And probably put it right below serde_json for that reason.

@sgeisler sgeisler force-pushed the 2020-06-01-ruy-compat branch from ea79479 to 139fc02 Compare June 4, 2020 20:05
@sgeisler
Copy link
Contributor Author
sgeisler commented Jun 4, 2020

Done :)

@elichai
Copy link
Member
elichai commented Jun 6, 2020 •
edited
Loading

While it's easy to just accept this, I think we need to do something like rust-bitcoin/rust-secp256k1#204 instead.

EDIT: I just realized this is a dev-dependency, and we have a bunch of those already pinned so to support testing under 1.22.0 we'll need to pin all of those hmmm

@sgeisler
Copy link
Contributor Author
sgeisler commented Jun 6, 2020

@elichai Are you sure you linked the right PR? If so, I don't get what you mean.

@elichai
Copy link
Member
elichai commented Jun 6, 2020

Fixed hehe, right PR number, wrong repo :P

@sgeisler
Copy link
Contributor Author
sgeisler commented Jun 6, 2020

Yeah, I think that would be the right thing to do for real dependencies. As it's only a dev dependency I have no preference for either solution whatsoever. Both are a bit hacky. If anyone has a strong opinion on it I'd be happy to adapt the PR.

@kallewoof kallewoof mentioned this pull request Jun 10, 2020
Merged
stevenroose
stevenroose approved these changes Jun 28, 2020
View reviewed changes
Copy link
Collaborator
@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

LGTM

apoelstra
apoelstra approved these changes Jun 29, 2020
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

Sure, since it's only a dev dep it's fine to pin

@apoelstra apoelstra merged commit 4af1872 into rust-bitcoin:master Jun 29, 2020
@apoelstra apoelstra mentioned this pull request Jun 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenroose stevenroose stevenroose approved these changes

@apoelstra apoelstra apoelstra approved these changes

@elichai elichai Awaiting requested review from elichai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ryu 1.0.5 incompatible with Rust 1.22
5 participants
@sgeisler @codecov-commenter @stevenroose @elichai @apoelstra
0