Skip to content
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

[MC-1766] Use mobilecoinofficial/serde_cbor fork #356

Merged
merged 1 commit into from
Aug 7, 2020

Conversation

kylefleming
Copy link
Contributor

Motivation

serde_cbor currently uses serde in such a way that it fails to compile if the serde_cbor/std feature is not enabled but serde/std is. The error is:

error[E0277]: the trait bound `error::Error: std::error::Error` is not satisfied
   --> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_cbor-0.11.1/src/de.rs:781:18
    |
781 |     type Error = Error;
    |                  ^^^^^ the trait `std::error::Error` is not implemented for `error::Error`

This issue has been fixed in pyfisch/cbor#198, but the maintainer of serde_cbor is no longer merging PRs (see pyfisch/cbor#179 (comment)).

A fork has been created that fixes pyfisch/cbor#198, located here: https://github.com/mobilecoinofficial/cbor/tree/fix-serde-std

In this PR

Future Work

  • Removing serde_cbor as a dependency entirely since it's no longer being maintained and the maintainer is no longer accepting PRs. (MC-1767)

@kylefleming kylefleming requested a review from a team August 7, 2020 02:04
@cbeck88
Copy link
Contributor

cbeck88 commented Aug 7, 2020

does this have the patch to serde_cbor? https://github.com/mobilecoinofficial/cbor/commits/master
I couldn't find the commit

@kylefleming
Copy link
Contributor Author

does this have the patch to serde_cbor? mobilecoinofficial/cbor@master (commits)
I couldn't find the commit

Yea, in the https://github.com/mobilecoinofficial/cbor/tree/fix-serde-std branch. I'll push it to master as well though.

@cbeck88
Copy link
Contributor

cbeck88 commented Aug 7, 2020

do you want to turn on branch protection on master also? (or i can do it)

@kylefleming
Copy link
Contributor Author

do you want to turn on branch protection on master also? (or i can do it)

If you want, go for it. Nothing is referencing master though, so I think it's less of an issue than for a repo with more active development.

@kylefleming kylefleming merged commit ef7259b into master Aug 7, 2020
@kylefleming kylefleming deleted the serde_cbor_fork branch August 7, 2020 22:00
@cbeck88
Copy link
Contributor

cbeck88 commented Aug 7, 2020

i did it -- its just that philosophically, if someone force pushed this repo, that would break historical mobilecoin builds, so i would like to prevent that, to preserve the ability to checkout old commits and build

@kylefleming
Copy link
Contributor Author

i did it -- its just that philosophically, if someone force pushed this repo, that would break historical mobilecoin builds, so i would like to prevent that, to preserve the ability to checkout old commits and build

Not unless they also delete the fix-serde-std branch and the v0.11.1-rev1 tag, but I take your point.

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.

4 participants