gh-uuid-rs/uuid

Watch

Fork 0

Feature/wasm #351

Merged

zrzka merged 8 commits from feature/wasm into master

2018-12-01 09:11:06 -06:00

zrzka commented

2018-11-27 02:00:22 -06:00

(Migrated from github.com)

Description

This PR adds two new features:

stdweb
wasm-bindgen

These features are kind of passthrough features, because they do nothing in the uuid crate itself. They're just passed to the rand crate to make the uuid crate working for the wasm32-unknown-unknown target.

Motivation

I'm unable to generate random UUID (v4) when this crate is compiled for the wasm32-unknown-unknown target.

Tests

I just added these features ...

- cargo test --features "v3"
- cargo test --features "v3 stdweb"
- cargo test --features "v3 wasm-bindgen"

... for all v3 & v4 & v5 (rand crate is used in all these features) to the script section in the .travis.yml.

Not sure if it makes sense, but it can demonstrate that it's buildable at least.

I don't think that more tests are required unless you'd like to bring the whole wasm-bindgen, wasm-pack, ... machinery here. And it has no sense to do it, because goal of this PR is not to publish uuid-rs NPM package, just add the ability to compile & use it from the wasm32-unknown-unknown target.

Manual tests

My Cargo.toml:

[dependencies]
uuid = { features = ["v4"], git = "https://github.com/zrzka/uuid.git", branch = "feature/wasm" }

[target.wasm32-unknown-unknown.dependencies]
uuid = { features = ["wasm-bindgen"], git = "https://github.com/zrzka/uuid.git", branch = "feature/wasm" }

My index.js:

const bt = require('balena-temen');

console.log(
    bt.evaluate({
        "id": {
            "$$eval": "uuidv4()"
        }
    })
);

uuidv4() implementation.

Output:

{ id: 'cefa2919-ff48-48ef-a231-e13697e23ed2' }

# Description This PR adds two new features: * `stdweb` * `wasm-bindgen` These features are kind of _passthrough_ features, because they do nothing in the `uuid` crate itself. They're just passed to the `rand` crate to make the `uuid` crate working for the `wasm32-unknown-unknown` target. # Motivation I'm unable to generate random UUID (v4) when this crate is compiled for the `wasm32-unknown-unknown` target. # Tests I just added these features ... ``` - cargo test --features "v3" - cargo test --features "v3 stdweb" - cargo test --features "v3 wasm-bindgen" ``` ... for all `v3` & `v4` & `v5` (`rand` crate is used in all these features) to the `script` section in the `.travis.yml`. Not sure if it makes sense, but it can demonstrate that it's buildable at least. I don't think that more tests are required unless you'd like to bring the whole `wasm-bindgen`, `wasm-pack`, ... machinery here. And it has no sense to do it, because goal of this PR is not to publish `uuid-rs` NPM package, just add the ability to compile & use it from the `wasm32-unknown-unknown` target. # Related Issue(s) * #350 * [now() doesn't work](https://github.com/balena-io-modules/balena-temen/issues/37) # Manual tests My Cargo.toml: ``` [dependencies] uuid = { features = ["v4"], git = "https://github.com/zrzka/uuid.git", branch = "feature/wasm" } [target.wasm32-unknown-unknown.dependencies] uuid = { features = ["wasm-bindgen"], git = "https://github.com/zrzka/uuid.git", branch = "feature/wasm" } ``` My `index.js`: ``` const bt = require('balena-temen'); console.log( bt.evaluate({ "id": { "$$eval": "uuidv4()" } }) ); ``` [uuidv4() implementation](https://github.com/balena-io-modules/balena-temen/blob/master/src/builtin/function/uuidv4.rs#L9-L11). Output: ``` { id: 'cefa2919-ff48-48ef-a231-e13697e23ed2' } ```

👍 1

KodrAus (Migrated from github.com) reviewed

2018-11-27 02:00:22 -06:00

kinggoesgaming (Migrated from github.com) reviewed

2018-11-27 02:00:22 -06:00

Dylan-DPC-zz commented

2018-11-27 05:13:04 -06:00

(Migrated from github.com)

Thanks. But I don't think it is a good idea to depend on docs for messages that it is supported only on certain targets with a feature gate. I'm looking for a better way of doing this.

zrzka commented

2018-11-27 05:24:09 -06:00

(Migrated from github.com)

Agreed, I don't like it too. Maybe hide new_v4() & friends ...

    #[cfg(any(
        not(target_arch = "wasm32"),
        all(
            target_arch = "wasm32",
            any(feature = "stdweb", feature = "wasm-bindgen")
        )
    ))]
    pub fn new_v4() -> Self { ... }

... , remove # Panics docstring and add some note to the lib.rs what to do with the wasm32?

Agreed, I don't like it too. Maybe hide `new_v4()` & friends ... ```rust #[cfg(any( not(target_arch = "wasm32"), all( target_arch = "wasm32", any(feature = "stdweb", feature = "wasm-bindgen") ) ))] pub fn new_v4() -> Self { ... } ``` ... , remove `# Panics` docstring and add some note to the `lib.rs` what to do with the `wasm32`?

zrzka commented

2018-11-27 06:03:56 -06:00

(Migrated from github.com)

Did update this PR and gated all v3, v4, v5 modules behind features. If you target wasm32-... and you forget to enable either stdweb or wasm-bindgen feature, it fails to compile:

warning: unused manifest key: package.maintenance
   Compiling rand v0.6.1
   Compiling uuid v0.7.1 (https://github.com/zrzka/uuid.git?branch=feature/wasm#0ccfd12f)
   Compiling balena-temen v0.0.20 (/Users/robertvojta/Work/balena/balena-temen)
error[E0599]: no function or associated item named `new_v4` found for type `uuid::Uuid` in the current scope
  --> src/builtin/function/uuidv4.rs:10:22
   |
10 |     Ok(Value::String(Uuid::new_v4().to_hyphenated().to_string()))
   |                      ^^^^^^^^^^^^ function or associated item not found in `uuid::Uuid`

Did update this PR and gated all v3, v4, v5 modules behind features. If you target `wasm32-...` and you forget to enable either `stdweb` or `wasm-bindgen` feature, it fails to compile: ``` warning: unused manifest key: package.maintenance Compiling rand v0.6.1 Compiling uuid v0.7.1 (https://github.com/zrzka/uuid.git?branch=feature/wasm#0ccfd12f) Compiling balena-temen v0.0.20 (/Users/robertvojta/Work/balena/balena-temen) error[E0599]: no function or associated item named `new_v4` found for type `uuid::Uuid` in the current scope --> src/builtin/function/uuidv4.rs:10:22 | 10 | Ok(Value::String(Uuid::new_v4().to_hyphenated().to_string())) | ^^^^^^^^^^^^ function or associated item not found in `uuid::Uuid` ```

Dylan-DPC-zz (Migrated from github.com) reviewed

2018-11-27 06:19:53 -06:00

src/lib.rs

do we need both features to be mutually exclusive? what will happen if this condition doesn't exist and both features exist?

wasm-pack.log

this file should be added to .gitignore

this file should be added to `.gitignore`

zrzka (Migrated from github.com) reviewed

2018-11-27 06:27:09 -06:00

wasm-pack.log

Ouch, that's my fault, this file and bin folder should not have been commited. I was just playing with the wasm-pack, will remove them.

Ouch, that's my fault, this file and `bin` folder should not have been commited. I was just playing with the `wasm-pack`, will remove them.

zrzka (Migrated from github.com) reviewed

2018-11-27 06:33:36 -06:00

src/lib.rs

According to this and this I assume that the compilation will fail, because of two imp modules. And we should probably add not(target_os = "emscripten") as well, to mimick rand behavior.

According to [this](https://github.com/rust-random/rand/blob/master/src/rngs/os.rs#L1004-L1006) and [this](https://github.com/rust-random/rand/blob/master/src/rngs/os.rs#L1107-L1110) I assume that the compilation will fail, because of two `imp` modules. And we should probably add `not(target_os = "emscripten")` as well, to mimick `rand` behavior.

Dylan-DPC-zz (Migrated from github.com) reviewed

2018-11-27 06:37:19 -06:00

src/lib.rs

Sounds good to me 👍

Sounds good to me :+1:

zrzka (Migrated from github.com) reviewed

2018-11-27 06:38:13 -06:00

src/lib.rs

Not 100% sure if not(target_os = "emscripten") should be added, because at the top level only wasm32 & stdweb | wasm-bindgen is used.

Not 100% sure if `not(target_os = "emscripten")` should be added, because [at the top level](https://github.com/rust-random/rand/blob/master/src/lib.rs#L169-L170) only `wasm32` & `stdweb` | `wasm-bindgen` is used.

Dylan-DPC-zz commented

2018-11-27 06:38:59 -06:00

(Migrated from github.com)

Currently this doesn't test the crate on wasm-* but just the features. We need to add another build to the CI so that it will run all the tests (or just the feature-gated ones) on wasm.

zrzka (Migrated from github.com) reviewed

2018-11-27 06:48:04 -06:00

src/lib.rs

My assumption was wrong.

cargo build --target wasm32-unknown-unknown --features "stdweb wasm-bindgen" works and stdweb is picked.

Rand contains ...

#[cfg(all(target_arch = "wasm32",
          not(target_os = "emscripten"),
feature = "stdweb"))]

... which works when stdweb is enabled. And it also contains ...

#[cfg(all(target_arch = "wasm32",
          not(target_os = "emscripten"),
          not(feature = "stdweb"),
feature = "wasm-bindgen"))]

... which utilizes wasm-bindgen only if stdweb is disabled.

It means that stdweb & wasm-bindgen does not need to be mutually exclusive. rand can handle this
Still not sure if not(target_os = "emscripten") should be added

My assumption was wrong. `cargo build --target wasm32-unknown-unknown --features "stdweb wasm-bindgen"` works and `stdweb` is picked. Rand contains ... ``` #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten"), feature = "stdweb"))] ``` ... which works when `stdweb` is enabled. And it also contains ... ``` #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten"), not(feature = "stdweb"), feature = "wasm-bindgen"))] ``` ... which utilizes `wasm-bindgen` only if `stdweb` is disabled. * It means that `stdweb` & `wasm-bindgen` does not need to be mutually exclusive. `rand` can handle this * Still not sure if `not(target_os = "emscripten")` should be added

Dylan-DPC-zz (Migrated from github.com) reviewed

2018-11-27 06:53:16 -06:00

src/lib.rs

I don't think RNGs are supported on emscripten which means if we don't bother about it, then compilation will fail and if we include it in the feature gate, it will compile to nothing.

zrzka commented

2018-11-27 07:02:28 -06:00

(Migrated from github.com)

@Dylan-DPC can you elaborate on tests pls? There're two options:

a) make a crate buildable for wasm32-... and that's it, test the features only,
b) make a crate buildable for wasm32-... and provide "NPM package" (JS wrapper) and test it with the wasm-pack test ..., something like this

If a) is the right answer then I don't understand what do you mean with tests. Because you can't do things like cargo test --target wasm32-unknown-unknown --features "v5 stdweb" for example.

Also I don't think b) needs to be added.

@Dylan-DPC can you elaborate on tests pls? There're two options: a) make a crate buildable for `wasm32-...` and that's it, test the features only, b) make a crate buildable for `wasm32-...` and provide "NPM package" (JS wrapper) and test it with the `wasm-pack test ...`, something like [this](https://github.com/balena-io-modules/balena-temen/blob/master/src/engine/helper.rs#L288) If a) is the right answer then I don't understand what do you mean with tests. Because you can't do things like `cargo test --target wasm32-unknown-unknown --features "v5 stdweb"` for example. Also I don't think b) needs to be added.

zrzka commented

2018-11-27 07:17:24 -06:00

(Migrated from github.com)

rand is doing this for WASM, it takes quite a long time and the output is:

test result (async): ok. 0 passed; 0 failed
test result (async): ok. 0 passed; 0 failed

`rand` is doing [this](https://github.com/rust-random/rand/blob/master/.travis.yml#L127-L163) for WASM, it takes quite a long time and the output is: ``` test result (async): ok. 0 passed; 0 failed test result (async): ok. 0 passed; 0 failed ```

zrzka commented

2018-11-27 07:22:12 -06:00

(Migrated from github.com)

Did add builds for the wasm32 arch with features as a minimum.

Did add builds for the `wasm32` arch with features as a minimum.

KodrAus commented

2018-11-27 07:22:20 -06:00

(Migrated from github.com)

Thanks for the PR @zrzka. Personally I'm not a fan of adding transitive stdweb or wasm-bindgen features to uuid for the sake of rand. It just passes the problem of figuring out feature gates upstream to the next consumer of uuid.

Right now, I think depending on rand alongside uuid in your project with one of these features set is a better way to go.

If we're going to support random uuids in wasm then I think we should do it without needing any special feature gates.

Thanks for the PR @zrzka. Personally I'm not a fan of adding transitive `stdweb` or `wasm-bindgen` features to `uuid` for the sake of `rand`. It just passes the problem of figuring out feature gates upstream to the next consumer of `uuid`. Right now, I think depending on `rand` alongside `uuid` in your project with one of these features set is a better way to go. If we're going to support random uuids in wasm then I think we should do it without needing any special feature gates.

Dylan-DPC-zz commented

2018-11-27 07:22:32 -06:00

(Migrated from github.com)

@Dylan-DPC can you elaborate on tests pls? There're two options:

a) make a crate buildable for wasm32-... and that's it, test the features only,
b) make a crate buildable for wasm32-... and provide "NPM package" (JS wrapper) and test it with the wasm-pack test ..., something like this

This will test only the features, it won't test whether uuid works on the wasm architectures or not, which could lead to false truths.

> @Dylan-DPC can you elaborate on tests pls? There're two options: > > a) make a crate buildable for `wasm32-...` and that's it, test the features only, > b) make a crate buildable for `wasm32-...` and provide "NPM package" (JS wrapper) and test it with the `wasm-pack test ...`, something like [this](https://github.com/balena-io-modules/balena-temen/blob/master/src/engine/helper.rs#L288) This will test only the features, it won't test whether uuid works on the wasm architectures or not, which could lead to false truths.

zrzka commented

2018-11-27 07:29:24 -06:00

(Migrated from github.com)

@KodrAus not sure if you can do it without feature gates now, because there's stdweb and wasm-bindgen, which are different things / approaches. That doesn't mean I like it, because it's weird and it looks like that it's fixing an issue which lies elsewhere.

Anyway, I'm fine with adding rand as a dependency and copy & pasting new_v4.

I'm going to close this PR, trash branch. Decide if you'd like to keep #350 open and if not, feel free to close it.

@KodrAus not sure if you can do it without feature gates now, because there's `stdweb` and `wasm-bindgen`, which are different things / approaches. That doesn't mean I like it, because it's weird and it looks like that it's fixing an issue which lies elsewhere. Anyway, I'm fine with adding `rand` as a dependency and copy & pasting `new_v4`. I'm going to close this PR, trash branch. Decide if you'd like to keep #350 open and if not, feel free to close it.

Dylan-DPC-zz commented

2018-11-27 07:40:58 -06:00

(Migrated from github.com)

@KodrAus I don't think you can do it without the feature gates, and since the rand features are different depending on whether they want stdweb or bindgen. I don't see another way of doing this at this point.

zrzka commented

2018-11-27 08:06:10 -06:00

(Migrated from github.com)

@Dylan-DPC is right :)

Anyway, I have fixed my issue with this commit, works for me. Ugly as well, but at least it works.

@Dylan-DPC is right :) Anyway, I have fixed my issue [with this commit](https://github.com/balena-io-modules/balena-temen/pull/41/commits/737b578ae6a0efdbf6a8a197ae5702e5b7ddbbdd), works for me. Ugly as well, but at least it works.

KodrAus commented

2018-11-30 04:37:26 -06:00

(Migrated from github.com)

@zrzka So it looks like this is actually the way we're recommending doing this right now, so if you'd like to re-open this PR as-is then I'm on board with the approach.

Sorry about the misdirection here :) I can live with a solution that has drawbacks if it's consistent with what other libraries are doing.

@zrzka So it looks like this is actually the way we're recommending doing this right now, so if you'd like to re-open this PR as-is then I'm on board with the approach. Sorry about the misdirection here :) I can live with a solution that has drawbacks if it's consistent with what other libraries are doing.

zrzka commented

2018-11-30 04:55:59 -06:00

(Migrated from github.com)

@KodrAus np, next week.

zrzka commented

2018-12-01 06:37:06 -06:00

(Migrated from github.com)

Restored & reopened as requested. Summary:

rand version bumped
stdweb & wasm-bindgen features added, they just transitively enables same features in the rand crate
if anyone wants to use v3, v4, v5 on wasm32 arch, stdweb and/or wasm-bindgen feature(s) must be enabled otherwise these modules wont be available
Travis CI builds these combinations for wasm32-unknown-unknown
Note added to lib.rs about this behavior

Restored & reopened as requested. Summary: * `rand` version bumped * `stdweb` & `wasm-bindgen` features added, they just transitively enables same features in the `rand` crate * if anyone wants to use `v3`, `v4`, `v5` on `wasm32` arch, `stdweb` and/or `wasm-bindgen` feature(s) must be enabled otherwise these modules wont be available * Travis CI builds these combinations for `wasm32-unknown-unknown` * Note added to `lib.rs` about this behavior

Dylan-DPC-zz commented

2018-12-01 07:24:41 -06:00

(Migrated from github.com)

This fails on stable & beta with an issue on stdweb:

error[E0554]: #![feature] may not be used on the stable release channel
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/stdweb-0.4.10/src/lib.rs:115:5
    |
115 | /     feature(use_extern_macros)
116 | | )]
    | |_^

This fails on stable & beta with an issue on stdweb: ``` error[E0554]: #![feature] may not be used on the stable release channel --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/stdweb-0.4.10/src/lib.rs:115:5 | 115 | / feature(use_extern_macros) 116 | | )] | |_^ ```

zrzka commented

2018-12-01 08:10:01 -06:00

(Migrated from github.com)

Yep, stdweb requires nightly. I reschuffled .travis.yml little bit. Also 1.22 doesn't have support form wasm32-unknown-unknown, so, I removed it from the main script.

Yep, `stdweb` requires nightly. I reschuffled .travis.yml little bit. Also 1.22 doesn't have support form `wasm32-unknown-unknown`, so, I removed it from the main script.

Dylan-DPC-zz commented

2018-12-01 08:19:29 -06:00

(Migrated from github.com)

Yeah they fixed the issue in this commit (github.com/koute/stdweb@f1fc5e3206) and it will work on stable once it is released.

Yeah they fixed the issue in this commit (https://github.com/koute/stdweb/commit/f1fc5e320645a9284b3f0e651dba05bcb4720b3b) and it will work on stable once it is released.

Dylan-DPC-zz (Migrated from github.com) approved these changes

2018-12-01 09:03:02 -06:00

Dylan-DPC-zz commented

2018-12-01 09:04:19 -06:00

(Migrated from github.com)

bors: r+

bors[bot] commented

2018-12-01 09:11:04 -06:00

(Migrated from github.com)

Build succeeded

# Build succeeded * [continuous-integration/travis-ci/push](https://travis-ci.org/uuid-rs/uuid/builds/462187519?utm_source=github_status&utm_medium=notification) * [continuous-integration/appveyor/branch](https://ci.appveyor.com/project/KodrAus/uuid/builds/20698831)