Comments (25)
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 cfg
s 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.
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 thatgetrandom
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.
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.
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.
Related Issues (20)
- Deadlock in when poll is hooked by coroutine lib HOT 4
- `wasm-opt` fails to optimize the wasm, due to `getrandom::getrandom` HOT 2
- Should the "rdrand" feature take precedence over the "target_os"? HOT 4
- Create a Security Policy
- Cannot compile with rust 1.70 and x86_64-unknown-linux-musl target HOT 15
- Support for non-blocking getrandom HOT 2
- Hash pin github workflow dependencies HOT 4
- Constant `__getrandom_internal` should have UPPER_SNAKE_CASE name, e.g. `__GETRANDOM_INTERNAL`rust-analyzernon_upper_case_globals HOT 3
- What versions do platform support track? HOT 4
- Broken link to sys_read_entropy HOT 4
- Rust embedded standard library (no_std) target is not supported HOT 9
- Don't fall back to file I/O on Linux when Rust only supports kernels guaranteed to have it HOT 5
- Potential undefined behavior in hermit implementation. HOT 3
- Cannot use synchronous feature in node 18 HOT 7
- Add support for targets without atomic operations
- Support for UEFI HOT 3
- Failed to build for wasm32-unknown-unknown with no_std HOT 1
- Is the ESPIDF implementation actually correct? HOT 3
- Spurious failures of Web tests on Windows HOT 1
- Port from libc to rustix HOT 7
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from getrandom.