Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to compile for core #108

Closed
wants to merge 5 commits into from
Closed

Conversation

jethrogb
Copy link

@jethrogb jethrogb commented Jul 9, 2016

This patch makes it possible to compile this crate with no_std enabled. This mostly just makes available the Rng trait and related types. Of course, in core, there is no default RNG.

Basically the patch just chanes all mentions of std to core and configs-out everything that doesn't work in core. It adds a dependency on my core_io crate which was made basically the same way.

To use the crate in core-mode, you have to do {use-default-features=false,features=["no_std"]}.

I'm happy to carry this patch as a separate core_rand crate but I think it'd be nicer if it lived upstream (getting closer to my ultimate goal of making most things available in no_std mode).

@jethrogb
Copy link
Author

jethrogb commented Jul 9, 2016

I fixed the 1.0.0 build in the latest commit. I don't think the nightly failure is due to this patch.

@jethrogb jethrogb mentioned this pull request Jul 9, 2016
@eefriedman
Copy link

eefriedman commented Jul 9, 2016

The feature checks seem backwards; "std" should be an enabled-by-default feature, as opposed to "no_std" being a disabled-by-default feature.

It would be a good idea to get rid of the dependencies on alloc, collections, and core_io; they're basically unnecessary, and it would be nice to allow using #![no_std] with rand on stable Rust.

@jethrogb
Copy link
Author

jethrogb commented Jul 9, 2016

The feature checks seem backwards; "std" should be an enabled-by-default feature, as opposed to "no_std" being a disabled-by-default feature.

Features are opt-in over the whole dependency tree. If your project is no_std, you want to be able to specify transitive dependencies to also be no_std. You can't do this with use-default-features=false because that won't be set on the dependencies. I hope it's clear what I'm trying to say, if not, I can try to clarify.

Also, I think the no_std ecosystem basically settled on having a no_std feature. For example bitflags had this before the feature was removed.

It would be a good idea to get rid of the dependencies on alloc, collections, and core_io; they're basically unnecessary, and it would be nice to allow using #![no_std] with rand on stable Rust.

I'll look into this. They are used in some places.

@jethrogb
Copy link
Author

jethrogb commented Jul 9, 2016

Added the core_io, box, and vec features, without them, compiles on stable with no_std enabled.

@eefriedman
Copy link

Features are opt-in over the whole dependency tree. If your project is no_std, you want to be able to specify transitive dependencies to also be no_std. You can't do this with use-default-features=false because that won't be set on the dependencies.

I'm not sure what you're getting at with this...

The reason you want a default-on feature is that it reduces the complexity of writing and using a no_std crate. Suppose I write a no_std crate cool_stuff that depends on rand... if rand has an "std" feature, I just write this:

[dependencies]
rand = { version = "0.3", default-features = false }

Then if someone uses my crate, they just write:

[dependencies]
cool_stuff = { version = "0.1" }

And everything just works whether or not the final program is no_std. (no_std was carefully designed to make this work.)

Now, suppose rand has a no_std feature instead. In this scenario, cool_stuff is forced to expose a completely useless no_std feature:

[dependencies.rand]
rand = { version = "0.3" }

[features]
no_std = ["rand/no_std"]

(Alternatively, if it doesn't expose a no_std feature, you'll get strange compile errors if another crate in the program uses the std variant of rand.)

Then if you want to use mycrate in a no_std scenario, you have to set this completely useless feature:

[dependencies]
mycrate = { version = "0.1", features = ["no_std"] }

This seems worse overall.

@jethrogb
Copy link
Author

I think you're making a good point. I suppose it's important to consider that when building a no_std final crate, all dependencies need to be no_std-aware.

(Alternatively, if it doesn't expose a no_std feature, you'll get strange compile errors if another crate in the program uses the std variant of rand.)

This is the most convincing point.

Jethro Beekman added 4 commits July 10, 2016 11:49
@alexcrichton
Copy link
Contributor

Thanks for the PR! Also cc @BurntSushi

This seems invasive enough to me that we probably don't want to merge without rethinking the design of the rand crate in general, as it may be very difficult to use without std yet remain on stable Rust.

@eefriedman
Copy link

This seems invasive enough to me that we probably don't want to merge without rethinking the design of the rand crate in general, as it may be very difficult to use without std yet remain on stable Rust.

I'm not sure what you mean by "difficult to use". I mean, without the std feature, the rand crate doesn't include thread_rng (so you have to choose an RNG and seed it yourself), but almost everything else works as-is.

@alexcrichton
Copy link
Contributor

I'm specifically worried about seemingly arbitrary APIs disappearing, such as the distributions, random, sample, weak_rng, etc. That and using a library like rand without std is quite difficult because it's so commonly depended on with std enabled. Going through and disabling std from all those other dependencies might be too onerous.

@nagisa
Copy link
Contributor

nagisa commented Jul 12, 2016

This mostly just makes available the Rng trait and related types.

This should be a separate crate which rand depends on, IMO, rather than the distinction between core-ful and core-less versions of the rand crate.

@jethrogb
Copy link
Author

I'm specifically worried about seemingly arbitrary APIs disappearing, such as the distributions, random, sample, weak_rng, etc.

I suppose this sounds like a vote for @nagisa's proposal below, but I think it's acceptable. People working in no_std are used to not having the full functionality available to them.

That and using a library like rand without std is quite difficult because it's so commonly depended on with std enabled. Going through and disabling std from all those other dependencies might be too onerous.

An important part of the Rust ecosystem is the existence of several core (pun intended) traits that people can depend on in their interfaces, Rand and Rng in this case. As such, I don't agree with this statement. In fact, making these traits available in the no_std world will make it easier, not harder, to port software to work in no_std. You have to start somewhere, and that is at the bottom of the dependency tree.

This should be a separate crate which rand depends on, IMO, rather than the distinction between core-ful and core-less versions of the rand crate.

This is an option. However, applying this paradigm everywhere would result in twice the number of crates. I'm not convinced that that is desirable.

@paholg
Copy link

paholg commented Jul 22, 2016

I'm curious where you are with this. It looks like the last point in the discussion was deciding whether to use @jethrogb's changes, or split rand into two crates.

FWIW, I agree with @jethrogb about

That and using a library like rand without std is quite difficult because it's so commonly depended on with std enabled. Going through and disabling std from all those other dependencies might be too onerous.

Allowing dependence on rand without std won't muck with any existing dependencies, it just means those of us working without access to std get to start using a part of rand. I see it solely as a win, with the only downside that it will add some overhead to maintaining rand.

If it would help, I would be happy to implement the alternative to this PR, the splitting into two crates, so it might be easier to compare the two solutions, but I think that this is the better one.

@bluss
Copy link
Contributor

bluss commented Nov 3, 2016

Cargo's crate feature system is not compatible with a setup like in the PR's current state.

Crate features must be additive, otherwise there can be conflicts when you compile two different rdeps of rand in the same project.

Scenario: Mother crate depends on A and B.

  • A depends on rand with no changes
  • B depends on rand using default-features = false, features = ["core_io"]

Cargo compiles the rand crate in the mother project with feature unioning. Features "default", "std", "core_io" and "libc" are thus selected. Crate B's use of core_io traits to interoperate with rand will not compile, if it even gets that far.

Edit: is this a concern that can be disregarded in some way? I was thinking of it as a fundamental incompatibility, but I guess opinions can differ on how the crate features system should be used.

@jethrogb jethrogb mentioned this pull request Nov 3, 2016
@jethrogb
Copy link
Author

jethrogb commented Nov 3, 2016

@bluss The idea is that if you're in the no_std world you're only using crates that are compatible with it. If you're not, thing will break.

With the current feature set, it is not intended for library crates to set the core_io feature. Library crates using rand just need to set default-features = false, and have a default-on std feature that also pulls in rand/std. This will keep everything working in the std case. In the no_std case, the final crate also needs to depend on rand and set features = [core_io].

@bluss
Copy link
Contributor

bluss commented Nov 3, 2016

I thought it should be possible to write crates that are compatible with both worlds simultaneously? Otherwise we need to rewrite the whole crate base to have special no-std compatible versions.

What I am worried about is things like rand::read::ReadRng where instead of enabling it for just std-using code, it switches its API between std not enabled and std enabled. This is not composable.

@jethrogb
Copy link
Author

jethrogb commented Nov 3, 2016

I thought it should be possible to write crates that are compatible with both worlds simultaneously?

Not if you have a crate that depends on another crate only when using no_std. Cargo's feature unioning prevents any kind of sane handling here. The compatibility only goes one way: crates that use no_std generally can be used in the std world.

Otherwise we need to rewrite the whole crate base to have special no-std compatible versions.

You need that anyway, because any crate that is not no_std will pull in std and now your final link artifact will be broken because you didn't want to link to std.

What I am worried about is things like rand::read::ReadRng where instead of enabling it for just std-using code, it switches its API between std not enabled and std enabled. This is not composable.

Are you saying that there should be a separate rand::core_read::ReagRng that uses the core_io traits? This puts a lot of strain on the ecosystem because now everywhere crates want to use ReadRng they need to have use #[cfg] to select a version.

@bluss
Copy link
Contributor

bluss commented Nov 5, 2016

You need that anyway, because any crate that is not no_std will pull in std and now your final link artifact will be broken because you didn't want to link to std.

We can't accept this situation, in the big picture I mean. We need a composable ecosystem where more and more basic parts are being made available to both core- and std-using projects. We need it to not be two worlds.

Are you saying that there should be a separate rand::core_read::ReagRng that uses the core_io traits? This puts a lot of strain on the ecosystem because now everywhere crates want to use ReadRng they need to have use #[cfg] to select a version.

Well something like it — I'll say what I wish for here, without deeper knowledge: A consumer should not need to support both. If the core-only API is good enough for them, then they use it in both core- and std-compatible mode.

@jethrogb
Copy link
Author

jethrogb commented Nov 5, 2016

We can't accept this situation, in the big picture I mean. We need a composable ecosystem where more and more basic parts are being made available to both core- and std-using projects. We need it to not be two worlds.

Agreed. The current situation is mostly a limitation of the way Cargo handles things. I think the situation could be improved if Cargo would let the final crate set cfgs throughout the dependency chain and cfg-dependent dependency/feature selection actually worked.

@Jascha-N
Copy link

Any progress on this?

@alexcrichton alexcrichton added the F-new-int Functionality: new, within Rand label Jun 14, 2017
@dhardy
Copy link
Member

dhardy commented Dec 8, 2017

Looking into this,

  • The alloc crate provides Vec, so one alloc feature can replace both box and vec (maybe this is a recent addition to alloc; I don't know)
  • Why bother with core_io? Sure, I guess some things like the Read trait could be in core. Maybe you should push for that to happen if you're interested. But I don't see much point in working around this here?
  • Anyone know why floating-point functions like ln, exp, sqrt, powf are not part of the core lib?

BTW I've already reimplemented this. Just posting here to see if I get any answers to the above.

@jethrogb
Copy link
Author

jethrogb commented Dec 8, 2017

@dhardy When coming back to a year-and-half old PR, I'd expect a little more research from your part into the history and rationale behind the design choices. Just because you don't immediately see why something is the way it is or whether it's useful, doesn't mean that there's no merit to it. In particular, the first two questions are fully understood and easily answered by anyone who has ever dealt with the core ecosystem:

The alloc crate provides Vec, so one alloc feature can replace both box and vec (maybe this is a recent addition to alloc; I don't know)

This is a recent change. Vec used to be in collections.

Why bother with core_io? Sure, I guess some things like the Read trait could be in core. Maybe you should push for that to happen if you're interested. But I don't see much point in working around this here?

From the core_io crate information: “This is a copy of libstd::io with all the parts that don't work in core removed. Most importantly, it provides the Read and Write traits.” std::io::Error depends heavily on other things in std so it's not possible to define the same traits in core proper in a way that's compatible with std until we get actually-useful type parameter defaults.

@pitdicker
Copy link
Contributor

@jethrogb I think there is a bit of trouble with the communication.

As part of the rand crate redesign there was an almost endless discussion about how to add error handling, with a focus on working also in the core ecosystem.
There was dhardy#11 to hopefully get to know a bit more about the needs of no_std.

https://github.com/dhardy/rand includes efforts to support no_std, especially with the rand_core split.

But you seem to know better than us what is needed for the core ecosystem.

core_io is only required for ReadRng right? I personally don't see all that much use for that part of Rand. Is it used more in the crates you care about? Then it maybe makes sense to add an optional dependency on core_io. An alternative is to provide a no_std implementation of ReadRng in a different crate.

@jethrogb
Copy link
Author

jethrogb commented Dec 12, 2017

People ask about using the io traits in core all the time, which is why the crate exists. Whether it's specifically used in combination with rand, I don't know, but it's non-use might just be a result of rand not being readily available in core. An optional feature sounds good to me, but it might be a bit cumbersome, because I think core_io doesn't compile on stable, so if you accidentally select it, it might result in issues.

@dhardy
Copy link
Member

dhardy commented Dec 12, 2017

Okay, that makes sense, I suppose. I'm less sure whether core_io is useful here since it only enables the Read RNG — I suppose it depends whether no_std platforms can have Readable entropy sources? We could always add a core_io option later so if in doubt leave out.

With vec and box replaced by a single alloc option (and some updates to the current code) I think this is ready for merging. It may also be worth adding a note to the README (e.g. see here and the mention under Testing).

It is a shame that OsRng is unavailable but nothing we can do about that. The new JitterRng is useable if a nanosecond-resolution timer is available, otherwise users have to supply their own entropy. Unfortunately I don't think there's any way to make thread_rng and random still work in this case.

The weird thing is those distributions not being usable due to missing FP operations; this almost seems like an oversight in the core crate? Never mind, I guess most core users won't be writing much FP code anyway.

(Yes, I'd like to get some no_std code merged and I don't see why we shouldn't use @jethrogb's PR as a starting point.)

@dhardy dhardy mentioned this pull request Dec 14, 2017
@dhardy
Copy link
Member

dhardy commented Dec 17, 2017

Reimplemented as #208

@dhardy dhardy closed this Dec 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-new-int Functionality: new, within Rand
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants