Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Update rand to 0.7.0 #195

Closed
wants to merge 2 commits into from
Closed

Update rand to 0.7.0 #195

wants to merge 2 commits into from

Conversation

rubdos
Copy link

@rubdos rubdos commented Jul 20, 2019

Since a bit, rand has small_rng as a separate feature. I thus had to add it as required feature.

@rubdos rubdos requested a review from a team as a code owner July 20, 2019 13:05
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ithinuel (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rubdos
Copy link
Author

rubdos commented Jul 20, 2019

There's still an issue with no_std in rand itself.

@therealprof
Copy link
Contributor

@rubdos Indeed. I also noticed that while trying to upgrade some other crates of mine a while ago. Just curious: What's the benefit of this version bump?

@rubdos
Copy link
Author

rubdos commented Jul 20, 2019

@rubdos Indeed. I also noticed that while trying to upgrade some other crates of mine a while ago. Just curious: What's the benefit of this version bump?

Fair point... I was bumping it because I was trying to get futures + async_await to compile, and I noticed that futures-preview contained 0.7.0. Without the version bump, I'd have multiple versions of the same crate...

It's nothing urgent though, I don't even have my test boards yet :D

@therealprof
Copy link
Contributor

@rubdos The use of rand in this crate is somewhat special and also a bit unfortunate, bumping it to the latest version sounds good to me in general, but it would be even better if we could completely eliminate it. 😅

@rubdos
Copy link
Author

rubdos commented Jul 20, 2019

Ah, tbqf, I didn't look at why it needed rand at all 😓

It's not lost energy though, since that issue I found in rand itself applies to any no_std environment, and futures-preview uses it too if you enable async-await.

I've had a look at the use, and it's just to generate some unique identifiers with a bunch of macros? I don't get why it'd need to be no_std compatible then, if it's presumably only used on compile time, and not even included in the output?

EDIT: It's already only in dev-dependencies! Why is it still compiled for the target itself then? 😮
EDIT2: It's for the example, then... So in fact, rand is not even ever used!

@therealprof
Copy link
Contributor

@rubdos You can only have one set of dependencies with one set of feature-flags (feature flags are always additive) in your application. So it's normally only used at build time, but in case you want to use it in an application you're stuck with the same one...

@rubdos
Copy link
Author

rubdos commented Jul 20, 2019

@rubdos You can only have one set of dependencies with one set of feature-flags (feature flags are always additive) in your application. So it's normally only used at build time, but in case you want to use it in an application you're stuck with the same one...

I'd agree, but in this case, there's one rand in the macro's library, and another rand in the root crate. Both were at different versions (and probably features too?) before 7e31e50.

If I remove rand from the main Cargo.toml, everything except the example still compiles, afaict.

@therealprof
Copy link
Contributor

If I remove rand from the main Cargo.toml, everything except the example still compiles, afaict.

Sure.

@therealprof therealprof mentioned this pull request Jul 23, 2019
bors bot added a commit that referenced this pull request Nov 24, 2019
205: Stop using randomized symbol names r=therealprof a=jonas-schievink

It isn't possible to do this by incrementing a global counter, since the expansion order of macros isn't guaranteed and might change between compiler invocations.

Fixes #212
Closes #196
Closes #195

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
@bors bors bot closed this in #205 Nov 24, 2019
@bors bors bot closed this in 7c8778f Nov 24, 2019
@rubdos rubdos deleted the rand-0.7 branch November 24, 2019 16:51
adamgreig pushed a commit to rust-embedded/cortex-m that referenced this pull request Jan 12, 2022
205: Stop using randomized symbol names r=therealprof a=jonas-schievink

It isn't possible to do this by incrementing a global counter, since the expansion order of macros isn't guaranteed and might change between compiler invocations.

Fixes #212
Closes rust-embedded/cortex-m-rt#196
Closes rust-embedded/cortex-m-rt#195

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants