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

Fix emscripten build failures due to lack of i128 support #281

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

jstarry
Copy link
Contributor

@jstarry jstarry commented Oct 20, 2019

Problem

Bincode only partially relies on whether serde decides to enable i128. And so, building bincode for Emscripten targets is totally broken on rustc >= 1.26.0.

Solution

Remove autocfg and simply rely on serde to determine whether i128 is supported

Background

autocfg checks for i128 support by attempting to compile code to llvm-ir and checking for success or failure. When targeting Emscripten, i128 can still be compiled to llvm-ir and so autocfg believes i128 support should be enabled. Unfortunately, the Emscripten runtime does not support i128 despite the LLVM backend supporting it and so it needs to be explicitly disabled. This is the route that serde takes here: https://github.com/serde-rs/serde/blob/master/serde/build.rs#L56

Since serde does not add the deserialize_u128 method to the serde::Deserializer trait for Emscripten targets, but autocfg believes that 128bit integers are supported and implements the method, I'm seeing this build error:

error[E0407]: method `deserialize_u128` is not a member of trait `serde::Deserializer`
   --> /Users/jstarry/Workspace/bincode/src/de/mod.rs:65:9
    |
65  | /         fn $dser_method<V>(self, visitor: V) -> Result<V::Value>
66  | |             where V: serde::de::Visitor<'de>,
67  | |         {
68  | |             try!(self.read_type::<$ty>());
69  | |             let value = try!(self.reader.$reader_method::<O::Endian>());
70  | |             visitor.$visitor_method(value)
71  | |         }
    | |_________^ not a member of trait `serde::Deserializer`
...
112 |       impl_nums!(u128, deserialize_u128, visit_u128, read_u128);
    |       ---------------------------------------------------------- in this macro invocation

Full CI Failure here: https://travis-ci.com/yewstack/yew/jobs/247388659#L541

@jstarry
Copy link
Contributor Author

jstarry commented Nov 14, 2019

Hey @jdm, could I bother you for a review on this? Thanks :)

@jstarry
Copy link
Contributor Author

jstarry commented Dec 10, 2019

Hi @dtolnay would you mind giving this a review? Yew has been waiting on this change for awhile, thanks!

Copy link
Contributor

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me.

@dtolnay dtolnay merged commit 8ef2231 into bincode-org:master Dec 10, 2019
@jstarry jstarry deleted the fix-emscripten-builds branch December 10, 2019 06:25
@dtolnay
Copy link
Contributor

dtolnay commented Dec 10, 2019

I published 1.2.1 with this fix.

@jstarry
Copy link
Contributor Author

jstarry commented Dec 10, 2019

Thanks!

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.

None yet

2 participants