Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

"unimplemented!" encode/decode implementations for wasm #190

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrysn
Copy link

@chrysn chrysn commented Dec 4, 2017

The cfg() guards around the encode/decode implementations result in the
crate being unusable on wasm32-unknown-unknown.

These lines don't implement the functionality for that platform (there
is file system equivalent there), which makes the crate usable again for
file system unrelated tasks like encoding using the "cbor" crate.

@CryZe
Copy link

CryZe commented Jan 16, 2018

This implementation is wrong. If you use the emscripten target, this now compiles in both the unix implementation and the unknown implementation.

@CryZe
Copy link

CryZe commented Jan 16, 2018

Also, can we get this merged? At the moment the num crate doesn't compile because of rustc-serialize not compiling. Not sure why they still use it, but I guess they can't switch to serde before the next major version.

@chrysn
Copy link
Author

chrysn commented Jan 16, 2018

I was unaware that emscripten sets the "unix"-cfg; is there any way to have an "else" part in the cfg variations to do this cleaner, or should I rephrase the non-implementation to be
#[cfg(not(any(target_os="redox", unix, windows)))]
guarded?

@CryZe
Copy link

CryZe commented Jan 16, 2018

You can't do else afaik, so you could either do the not(any(...)) or specify wasm32, not emscripten.
I think the former makes more sense, as it automatically supports all other targets too

@cdetrio
Copy link

cdetrio commented Jan 18, 2018

We'd like this merged too, and rand version bumped 0.4, which had a wasm fix in rust-random/rand#197.

The cfg() guards around the encode/decode implementations result in the
crate being unusable on wasm32-unknown-unknown or any non-unix
non-windows non-redox target.

These lines mark the functionality as unimplemented on those platforms
(which is a sane behavior for platforms like wasm or bare-metal
microprocessors given those have no file system), which makes the crate
as a whole usable again for file system unrelated tasks like encoding
using the "cbor" crate.
@chrysn
Copy link
Author

chrysn commented Jan 18, 2018

Updated the commit to reflect CryZe's findings about wasm-emscripten; tested under unix and wasm32-unknown-unknown.

@kennytm
Copy link

kennytm commented Jan 22, 2018

Rust recently added CloudABI support, and this will cause encode/decode to become unimplemented! for target_os = "cloudabi", which I don't think is correct.

@CryZe
Copy link

CryZe commented Jan 22, 2018

The CI fails to compile this because the latest rand doesn't compile on 1.0 anymore. So this PR itself compiles just fine. It'd be nice if someone could tell us what we should do about this (maybe just remove the 1.0 compilation?) or just merge the PR regardless.

@CryZe
Copy link

CryZe commented Jan 22, 2018

@kennytm well that didn't compile before at all then. But I guess we can fix it in this PR too.

@kennytm
Copy link

kennytm commented Jan 22, 2018

Instead of removing 1.0.0, better just restrict the rand dependency to a version which 1.0.0 can work, something like:

[dev-dependencies]
rand = ">=0.3.0, <=0.3.20"

(Note: I haven't checked which rand version does work on 1.0.0. The last successful build uses 0.3.16)

@chrysn
Copy link
Author

chrysn commented Jan 22, 2018

May I ask that we keep unrelated issues (like rand versions) in their own issue?

I'm happy to address regressions this issue introduces, but target_os="cloudabi" looks like something that wouldn't work before this pull request (unless windows or unix are set); so if cloudabi yet another implementation of Encodable and Decodable, I suggest that a version be branched off this PR that adds the implementations and adds ,target_os="cloudabi" to the not-any clause to avoid merge conflicts.

@tomaka
Copy link
Contributor

tomaka commented Mar 14, 2018

Is there a way to get this moving somehow?

@cwervo
Copy link

cwervo commented Mar 26, 2018

@alexcrichton is there a way to get this PR moving again? From what I understand it seems critical to getting a lot of Rust projects that still depend on rustc-serialize into wasm. Obviously the best solution would be to switch projects to serde, but this seems like a good stopgap for cases where that's unfeasible.

@alexcrichton
Copy link
Contributor

This library is currently unmaintained and not publishing new releases, so there is currently not process or timeline for publishing this. Crates depending on rustc-serialize which also want to support new platforms will need to update to a serialization framework like Serde

@CryZe
Copy link

CryZe commented Apr 2, 2018

@alexcrichton There's still a lot of ecosystem critical libraries like num that still depend on rustc-serialize for semver reasons. So I think a new release for this makes sense.

If there's no maintainer here willing to merge and publish this, would it make sense to give someone like tomaka the rights to just push this real quick? This doesn't seem like it would take a lot of effort and affects the new wasm target a lot.

@sallar
Copy link

sallar commented May 27, 2018

Is it possible to get this going? Many community packages are depending on this. rust-crypto for example.

@BelfordZ
Copy link

BelfordZ commented Jun 8, 2018

Please fix this.

@varkor
Copy link

varkor commented Sep 21, 2018

This library is currently unmaintained and not publishing new releases

The README states:

No new feature development will happen in this crate, although bug fixes proposed through PRs will still be merged.

This PR looks to me like a bug fix and one that really affects the experience of targeting WASM from Rust.

Crates depending on rustc-serialize which also want to support new platforms will need to update to a serialization framework like Serde

This is an option for crate maintainers, but not users of those crates (which may no longer be maintained).

@jethrogb
Copy link

Why don't we make the implementation that's currently used for redox work for not(any(unix,windows))? That should work for most platforms.

@jethrogb
Copy link

Filed #195 with the same

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet