Update rand dependency to 0.7 + some tweaks

[dhardy]

trafficstars

... I realise this is jumping the gun since quickcheck does not yet support Rand 0.7.

Of course this is a breaking change (part of pub API).

Some of the distributions used here are somewhat arbitrary, but I guess are fine for testing purposes. However, this does lead to some slightly surprising code when re-used (nphysics_testbed):

                    color = rand::thread_rng().gen();
                    color *= 1.5;
                    color.x = color.x.min(1.0);
                    color.y = color.y.min(1.0);
                    color.z = color.z.min(1.0);

[newpavlov]

I think you also can make rand optional. Plus I think it may be worth to add alloc to the list of features enabled by std and simplify the associated cfgs. IIUC it will bump MSRV to 1.36, but on the other hand we will not need the weird std-without-rand feature. Also I believe you can remove the stdweb feature, since it's only used for enabling rand/stdweb.

[newpavlov]

I've submitted a PR with the described changes: dhardy/nalgebra#1

[sebcrozet]

Thank you for this PR and sorry for the delay before this review!

Right now the CI fails when trying to compile for a no-std target. This is due to some non-existent std imports. The extern crate core as std removed by this PR made it easier to handle this so I suggest adding it back and reverting all the use core:: added by this PR so they remain use std:: as they were before.

[newpavlov]

I've added a fix for the import in dhardy#2. Generally extern crate core as std is a more error-prone approach, since in modules you can't really see at the first glance from where you import stuff. And AFAIK it's is the most common (albeit a bit weird since you turn on no_std unconditionally) approach of handling optional std functionality across no_std crates.

[sebcrozet]

Thanks @dhardy and @newpavlov! The CI still fails, likely due to some missing use of rand traits.

[dhardy]

Yes, looks like it's just missing a trait use. Unfortunately I can't fix this until next Wednesday.

[dhardy]

Taking a look... testing without the std feature has many failures (DMatrix not defined). Testing with it with just alloc has many failures (Vec not in scope; suggestion to import from prelude which is not included from the alloc crate by default). Testing with the std feature has the same effect, plus the format! macro is undefined.

[sebcrozet]

@dhardy Our tests are not designed to be run without the std feature.

I’m surprised you get errors when running tests with the std feature since they are alway run by our CI. How did you run those tests exactly and what errors are you getting (could you show the output of rustc?)

[dhardy]

I'm confused on this one — first time I ran with the std feature the only error was to do with rand (but didn't make much sense unless rand_distr was using the wrong version of rand, which should be impossible). Then I ran with --features=alloc, did cargo update, and ran with --features=std again. See here.

[sebcrozet]

@dhardy I've investigated this and this is because of those changes:

- #![cfg_attr(not(feature = "std"), no_std)]
- #![cfg_attr(all(feature = "alloc", not(feature = "std")), feature(alloc))]
+ #![no_std]
+ #[cfg(any(feature = "std", feature = "io", feature = "abomonation-serialize"))]
+ extern crate std;

Having #![no_std] always enabled will break the auto-import of the std prelude when doing a cargo build --features=std.

Replacing #![no_std] back to #![cfg_attr(not(feature = "std"), no_std)] should do the trick.

[newpavlov]

Replacing #![no_std] back to #![cfg_attr(not(feature = "std"), no_std)] should do the trick.

I think it's not a good approach, since it will break modules which import from core. You could use the weird use std as core; as was done previously, but arguably it will be better to import vector stuff from alloc and since we use 2018 edition you can write use std::format; for the macro. If you lazy, or there is too much stuff needed from std prelude in a module, you always can write use std::prelude::*;.

[sebcrozet]

@newpavlov I see, thank you for the suggestion. Would you like to make the necessary changes (without use std::prelude::*;) and check that running cargo test --features=std still works?

[dhardy]

I reworked this, revising @newpavlov's contributions (removing some and with cleaner commits). Hopefully it now passes tests and is in shape for merging. This also bumps the quickcheck version.

[dhardy]

I am not sure why you have removed the changes around core and alloc

Because it was another big changeset on top of my one, and ultimately the point of this PR is (1) rand 0.7 and (2) optional rand.

Also I don't understand the motivation behind rand_with_std feature.

Some things need e.g. thread_rng from rand, which is only available with rand's std feature. I don't think there's a way of enabling rand/std only when rand and std are enabled otherwise?

[dhardy]

There are still CI failures:

1. The lapack sub-crate fails to link (on the master branch it links but has run-time failures, so presumably caused by my changes)
2. The no_std build fails on Travis. Works for me so not sure what's going on here.

[newpavlov]

I don't think there's a way of enabling rand/std only when rand and std are enabled otherwise?

Ah, indeed. I thought that std = ["rand/std"] will not enable rand without listing it explicitly.

[newpavlov]

So right now rand_with_std is essentially used only for ThreadRng in new_random_generic. I guess a separate crate for ThreadRng could've made sense here, although we would simply replace the feature with an optional crate...

It may be worth to use something like thread_rng for the feature name instead of rand_with_std to convey intent a bit more clearly.

[Ralith]

Perhaps it would make the most sense to remove uses of thread_rng entirely? People who want it can reference it explicitly without much effort.

On Mon, Sep 16, 2019, 08:26 Artyom Pavlov [email protected] wrote:

So right now rand_with_std is essentially used only for ThreadRng in new_random_generic. I guess a separate crate for ThreadRng could've made sense here, although we would simply replaced the feature with an optional crate...

It may be worth to use something like thread_rng for the feature name instead of rand_with_std to convey intent a bit more clearly.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rustsim/nalgebra/pull/622?email_source=notifications&email_token=AAAZQ3RRANI55FNRJH7UWTLQJ6QRNA5CNFSM4IHEP2IKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6ZQ2VY#issuecomment-531828055, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAZQ3XNGCGKIP4WA7JT76DQJ6QRNANCNFSM4IHEP2IA .

[dhardy]

new_random_generic is used to implement new_random which appears to have over 200 uses, therefore removing it entirely does not appear a trivial change.

I'm hoping some day that Cargo will allow the rand_with_std feature to be dropped, but in the mean-time I think we need something like this, unless either std-without-rand or rand-without-std is dropped.

[vks]

This PR is partially obsolete, because nalgebra uses rand 0.8. Are there some parts left that are worth porting or should this be closed?

[newpavlov]

There is a bunch of changes which I've done in dhardy#1, which are still relevant in my opinion (mostly about making crates more idiomatic around handling of std/alloc/no_std). But in the near future I will not have time to port them.

[vks]

#864 was merged, which included some of the remaining changes in this PR. What still needs to be merged is the following:

• Rework features (std, alloc, no_std)
• Clean up extern crate statements