0.1.9 seems to have broken wasm32-unknown-unknown about getrandom HOT 25 CLOSED

I'd like to add in my opinion that this was a breaking change and should be reverted, the default behavior for wasm32-unknown-unknown should follow the standard library by returning an error, not yielding a compile-time error.

from getrandom.

@josephlr But if we will usually get a compilation error in both cases, then why bother with the warning?

from getrandom.

If the dummy feature is on does that affect targets besides wasm32-unknown-unknown? I don't mind flipping it on if non wasm targets aren't affected.

To be clear: the flag enables the use of a dummy implementation only where no other implementation is available; i.e. it's safe to use everywhere but turns compile-time-failure into run-time-failure (should for any reason no impl be available). @newpavlov's answer to this may have caused confusion.

@josephlr no, I don't wish to go back to using fat cfgs all over the rand crate to control whether thread_rng is available. wasm-unknown-unknown will hopefully become a thing of the past in favour of wasm-wasi; in the mean-time we still tell users to enable stdweb or wasm-bindgen. @newpavlov's suggestion of exposing the dummy feature within rand may also be sensible, but perhaps a more specific name is appropriate there — getrandom-dummy? OTOH it's probably easier to tell users to add the following when running into @Lokathor's issue:

getrandom = { version = "0.1.9", features = ["dummy"] }

from getrandom.

This change was introduced in #71 and we decided not to treat it as a breaking one. If you want the old behavior, you can enable the dummy feature.

Are you really using getrandom in your CI tests? Another option would be to enable wasm-bindgen or stdweb feature for CI jobs.

from getrandom.

The use of getrandom is because it's a dev-dependency for benchmarks, but tests build all dev-dependency crates even if tests don't use that crate because cargo is just silly like that. I don't believe any actual tests use getrandom.

If the dummy feature is on does that affect targets besides wasm32-unknown-unknown? I don't mind flipping it on if non wasm targets aren't affected.

Also, you cannot conditional include a feature in cargo as far as I know, so this sort of thing really needs to be a breaking change when you do it.

from getrandom.

Argh... dummy features will influence all targets, initially there was also a wasm_dummy feature, but it got removed. One option could've been to write:

[dev-dependencies]
getrandom = { version = "0.1", features = ["dummy"] }

But unfortunately dev features leak to non-dev builds, due to the long-standing rust-lang/cargo#1796...

As a temporary solution you can run CI tests with --features=getrandom/dummy. I guess we could also add a dummy feature to rand for convenience.

@dhardy @josephlr
Thoughts?

from getrandom.

Alright I'll change the CI for now.

Side note: I don't use rand at all, so just a fix there will not help this case.

from getrandom.

@newpavlov I think we can learn from what other system crates (like libc) have done. These crates will build on any target, they will just be empty (or lacking key functions) on unsupported architectures.

I think we should change the behavior of getrandom to not have the getrandom() function on unsupported platforms, but otherwise build correctly. We should also proprobly make the current compiler error a warning to correctly point people to the docs on how to use unsupported targets.

@Lokathor would this work for your use case? If you don't actually call the getrandom function on unsupported platforms, everything should be fine.

EDIT: This would also allow us to get rid of certain hacks in our stdlib integration here.

from getrandom.

@josephlr
I don't think it's a good solutions for us. Don't forget that most users will not use getrandom directly, so your approach will result in a confusing compilation error "cannot find function getrandom" reported for rand or other crate, instead of the human-friendly one.

from getrandom.

@newpavlov this is a good point, and why I suggested the warning. This way:

• the user still gets good diagnostics (a human friendly warning about getrandom not being supported, and a link for what to do about it)
• the build will fail if someone is trying to use the crate on an unsupported target
• the build will succeed (with warnings) if the function is not actually being used

I’ll do an experiment to see if I can improve the diagnostics while keeping the above properties

from getrandom.

AFAIK there is no dedicated way to emit a compile time warning right now. Plus don't forget that it was requested in the std integration PR that getrandom should emit a compile-time warning on unsupported targets. So the question really is: what do we want as our default behavior? We could introduce no_dummy feature, but I am not sure if it will be a better default.

from getrandom.

Oh that’s too bad about not having compile_warning. We could just have the crate be empty for tests, and compile_error for builds? I think that not having a dummy implementation is the correct default, especially given that cargo features are supposed to be additive.

from getrandom.

We could just have the crate be empty for tests, and compile_error for builds?

I thought about it, but again, I doubt it will help much, since usually getrandom will be used indirectly, so compilation error during tests will still happen, but this time in rand, rand_core or some other crate.

from getrandom.

So, is the problem that there's no actual randomness to be had on wasm32-unknown-unknown, and the crate just never worked in the first place on that platform?

from getrandom.

Yes, without enabled wasm-bindgen or stdweb feature on wasm32-unknown-unknown the crate was using a dummy implementation which was always failing at runtime, in turn it would usually result in a panic if the getrandom function was used anywhere in a final binary.

from getrandom.

@newpavlov I've been experimenting, and I think I've figured out a way to get a warning with the text we want to display. Going back to your previous comment:

Plus don't forget that it was requested in the std integration PR that getrandom should emit a compile-time warning on unsupported targets.

This will still happen, as we would attempt to use the getrandom function, and it won't exist, so we will get a compiler error.

from getrandom.

@dhardy

OTOH it's probably easier to tell users to add the following when running into @Lokathor's issue:
getrandom = { version = "0.1.9", features = ["dummy"] }

Unfortunately I don't think we should make such recommendation. Doing it in a library crate will enable the dummy feature for all users of this crate, which will beat the purpose of doing compilation error in the first place. And enabling it only for dev-builds via:

[dev-dependencies]
getrandom = { version = "0.1", features = ["dummy"] }

Will not work properly either, due to the linked earlier feature leakage.

I think the workaround with manual --feature for CI jobs, which I've proposed earlier, will be a better way forward.

from getrandom.

Sorry, I missed this. Can you clarify in #89 then?

from getrandom.

Just to clarify. Do we all think that the leakage from dev-dependencies to dependencies only happen if a library is compiled on it's own (eg. cargo check, cargo build), but when it's is depended on by another crate, dev-dependencies and their features are not being considered by cargo?

If that is the case, when does a library ever get compiled stand-alone, except for tests/examples/benchmarks, when dev-dependencies apply?

from getrandom.

This seems to be the answer: tokio-rs/tokio#1318 (comment) to the feature leaking question.

from getrandom.

@najamelan

This seems to be the answer: tokio-rs/tokio#1318 (comment) to the feature leaking question.

Good find! I guess then projects can enable the dummy feature unconditionally in their dev-dependencies without any issues.

from getrandom.

Ran into this issue as well and was a bit surprised first, but managed to resolve it. Ideally don't want any of our dependencies to include getrandom or rand crates when we build for wasm32-unknown-unknown (which we run not in a browser) but a lot of crates do for utility functions or smaller other functionality that we do not use.

So the second best thing for us is to just continue to get a panic if getrandom is called in practice. Fortunately adding a dummy dependency in our top wasm crate with the dummy feature did work and forced the various other crate dependencies to include the dummy implementation and fail at runtime instead of compile time.

[target.'cfg(target_arch = "wasm32")'.dependencies]
getrandom = { version = "0.1.9", features = ["dummy"] } # https://github.com/rust-random/getrandom/issues/87

Hopefully we can make using rand in a lot of crates an optional feature that is not enabled by default, though I'm not too hopefully of that due to the additive nature of rand and how many crates that depend on it.

Update: Was able to just use rand crate with default-features = false everywhere where we build for wasm32-unknown-unknown so then getrandom is not included at all. And then if we do have some new code that tries to pull it in we would get a build error which is much better than a runtime panic on usage.

from getrandom.

I don't see why we should make exception for wasm32-unknown-unknown. (the exceptional std status of this target is already quite weird in my opinion, but it's a different topic...) We could "revert" it by making the dummy feature enabled by default, but I think many will agree that it's a bad default.

from getrandom.

0.1 needs to get reverted and stay with "it builds fine and panics at runtime", or whatever the runtime behavior was before. That's basically unarguable, that's just a semver thing.

In 0.2 you can do the whole feature thing, but since the getrandom already has a result return value you can just have it give an error result 100% of the time if no feature isn't enabled to select a real source, which seems fine enough.

And the crate should always build, all the time.

from getrandom.

@Lokathor @newpavlov I've got a patch in progress to fix this. It seems fine to just use the dummy implementation for wasm32-unknown-unknown at the moment. Then we can just release 1.10 and yank 1.9

We would still keep the dummy feature around. It just wouldn't affect the behavior of wasm32-unknown-unknown (as it would be considered a "supported" target).

from getrandom.