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

Code is broken for wasm32-unknow-emscripten #335

Closed
caiiiycuk opened this issue Nov 15, 2021 · 8 comments
Closed

Code is broken for wasm32-unknow-emscripten #335

caiiiycuk opened this issue Nov 15, 2021 · 8 comments
Labels
bug good first issue Perfect for new contributors

Comments

@caiiiycuk
Copy link

When I try to build project that depends on ron@0.7.0 for target wasm32-unknow-emscripten, I have lot of messages like this:

  --> /home/caiiiycuk/.cargo/registry/src/github.com-1ecc6299db9ec823/ron-0.7.0/src/de/id.rs:74:5
   |
74 | /     fn deserialize_i128<V>(self, _: V) -> Result<V::Value>
75 | |     where
76 | |         V: Visitor<'b>,
77 | |     {
78 | |         unimplemented!("IdDeserializer may only be used for identifiers")
79 | |     }
   | |_____^ not a member of trait `de::Deserializer`

I believe it's because emscripten target does not have i128/u182 types. Serde in that case does not have deserialize_i128/u128. As workaround I use version 0.5.1, it does not contains i128/u128 and works fine. However I would like to update to latest version if possible.

@torkleyy torkleyy added bug good first issue Perfect for new contributors labels Nov 15, 2021
@juntyr
Copy link
Member

juntyr commented Nov 16, 2021

Related to #304

@juntyr
Copy link
Member

juntyr commented Nov 20, 2021

I've done a tiny bit of investigation. As serde only provides serde_if_integer128 but no else variant, I did not find a way to tap into just serde to e.g. change the definition of

ron/src/parse.rs

Lines 89 to 103 in 7fefc00

#[derive(Clone, Debug, PartialEq)]
pub enum AnyNum {
F32(f32),
F64(f64),
I8(i8),
U8(u8),
I16(i16),
U16(u16),
I32(i32),
U32(u32),
I64(i64),
U64(u64),
I128(i128),
U128(u128),
}
. So I think #[cfg()]s are needed. While ron could mirror part of serde's build script, this does seem overkill. Adding a separate integer128 feature as suggested in #304 seems like the better option. This could then be tested by trying to build with target wasm32-unknown-emscripten on 1.36.0. Or ron could bump its MSRV to 1.40 which is when emscripten gained support for 128b integers. @caiiiycuk what Rust version are you using?

@caiiiycuk
Copy link
Author

1.53.0

@torkleyy
Copy link
Contributor

I think adding wasm to the build matrix makes sense. @kvark thoughts on this? I think a feature for integer128 would make sense, most of the time you don't need i128 anyways IMO. I see that features increase the complexity when it comes to testing and build scripts but this seems like a good reason.

@kvark
Copy link
Collaborator

kvark commented Dec 18, 2021

Yes, it all sounds reasonable. Although, I think @caiiiycuk 's original need for this issue is no longer active, if I understand correctly. This was needed for vange-rs, and the current plan is to use wasm32-unknown-unknown instead.

@caiiiycuk
Copy link
Author

Correct, but I think for consistency should be fixed at some time.
+1 for adding feature

@juntyr
Copy link
Member

juntyr commented Dec 23, 2021

@caiiiycuk Has this been fixed with #351?

@caiiiycuk
Copy link
Author

Just checked on vange-rs, works like a charm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Perfect for new contributors
Projects
None yet
Development

No branches or pull requests

4 participants