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

no_std support (using enabled-by-default "std" feature) #135

Closed
wants to merge 1 commit into from

Conversation

tonychain
Copy link

@tonychain tonychain commented Jun 16, 2017

  • Changes all references to libcore features from ::std to ::core
  • Feature gates anything dependent on std on the "std" feature

@tarcieri
Copy link

Sorry to bring up a bikeshedding topic, but I've been looking at use_std vs std as a feature name.

@alexcrichton I chose use_std to match futures-rs, however per some various bikeshedding issues, I think people might be aligning around std.

See:

rust-lang/api-guidelines#23
rust-lang/log#176

@alexcrichton
Copy link
Contributor

Long ago I remember we discussed creating a convention for the "enable std support" feature for the log crate and we settled on use_std. I have no opinion on this so long as we are consistent. Unfotunately, nothing is consistent right now.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left my comments inline. I believe that conditionally defining Cursor isn't a valid use of features. The next breaking bytes change will remove Cursor usage though.

Hopefully @alexcrichton can review as he is more familiar w/ no std.

serde = { version = "1.0", optional = true }

[dependencies.iovec]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this means that the vectored IO fns on the traits aren't available, which doesn't seem great (long term).

That said, they could be brought back incrementally w/o a breaking change.

Cargo.toml Outdated
[features]
default = ["use_std"]
use_alloc = []
use_std = ["iovec"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that use_std implies use_alloc. I'm not sure if there is a way to define this in Cargo.toml.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, other way around: use_alloc and use_std are orthogonal. use_alloc is intended for no_std use, i.e. --no-default-features --features=use_alloc

bytes builds without it per this PR, but with so many things disabled it isn't particularly useful

use alloc::vec::Vec;

#[cfg(not(feature = "use_std"))]
use buf::Cursor;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, given that features must be additive, I don't think this conditionally vendoring cursor is something that can be done.

This, of course, means that IntoBuf can't really be implemented without std :(

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple options here:

  1. Gate this (and anything that depends on it) on use_alloc
  2. Stop using std::io::Cursor entirely (i.e. fix io::Cursor is a bit out of place #75)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcrichton if buf::Cursor is defined as a re-export of std::io::Cursor when use_std is set, is that ok for "additive" features?

@alexcrichton
Copy link
Contributor

Ah yeah I agree with @carllerche in that the handling of Cursor here, while unfortunate, I don't think is a valid usage of features. Features must be composable in that they purely add APIs rather than change existing ones.

I'm personally not a huge fan of using unstable crates like alloc because that also means that this library sometimes requires nightly and other times not. That may just be my own personal preference though.

@tarcieri
Copy link

tarcieri commented Jul 3, 2017

The cleanest solution re: Cursor is probably to just wait for #75 to get fixed (which I think targets bytes 0.5), which I think probably wouldn't be too far off from what I've already done, but using that 100% of the time as opposed to relying on std::io::Cursor at all.

Regarding alloc and nightly: to my knowledge nightly is required for any sort of no_std usage (since lang_items is needed to use no_std, AFAICT). The crate still builds on its own with --no-default-features, but alloc (or std) is really necessary to get a lot of the useful functionality.

@tarcieri
Copy link

tarcieri commented Jul 3, 2017

All right, took a stab at #75 here: #146

That should get past having two different implementations of buf::Cursor depending on whether or not we have use_std enabled.

@alexcrichton
Copy link
Contributor

Yeah I think with Cursor there's not much recourse but waiting for the next major version (unfortunately). For alloc you're right, but the current state of affairs is that allocation without the standard library is unstable right now. This may change in the near future with the pending stabilization of allocators, but I'd personally prefer that a mixture of features for a library doesn't require it oscillate between requiring nightly or not.

@tarcieri
Copy link

tarcieri commented Jul 5, 2017

All no_std users are presently stuck on nightly. It'd be great to get the many things keeping no_std users on nightly stabilized, but in the meantime I'm willing to tolerate a little instability, and have some pressing needs for crates that depend on bytes.

Personally I would much rather deal with some changes to nightly-only features of bytes targeted at no_std users than be locked out of the ecosystem of crates that depend on bytes, with my only recourse to maintain a fork of bytes and forks of the crates that depend on bytes.

@alexcrichton
Copy link
Contributor

Oh sure that makes sense for no_std users but this is otherwise a stable crate, and I'd consider it a little too surprising for this crate to sometimes require nightly and sometimes not, depending on activate features.

@tarcieri
Copy link

tarcieri commented Jul 6, 2017

Who would be trying to swap use_std for use_alloc except for nightly-only no_std users?

@alexcrichton
Copy link
Contributor

There's use cases for no_std on stable, nightly is not the only consumer.

@tarcieri
Copy link

tarcieri commented Jul 6, 2017

I think pretty much all practical no_std uses need lang_items at the very least

@alexcrichton
Copy link
Contributor

Ok, well, my opinion is that this crate should not have a feature where it only uses the alloc crate. That's an unstable requirement in an otherwise stable crate, and can cause surprises when Cargo unions features together. My recommendation is that, if we really want it, the no_std feature of this crate should just provide the traits. I'll leave it up to @carllerche to merge if he'd like.

@tarcieri
Copy link

tarcieri commented Jul 7, 2017

@alexcrichton would it help if the feature were more clearly labeled to indicate its experimental/nightly nature, e.g. nightly_alloc or experimental_alloc?

re: feature unioning, the crate builds with all combinations of use_alloc and use_std, and I can test for that if you'd like. However, if you use use_alloc and use_std in conjunction, it will require nightly for aforementioned reasons.

@alexcrichton
Copy link
Contributor

I would still personally prefer to not add such a feature to the crate, but I'd again defer to @carllerche's thoughts.

@dtolnay
Copy link

dtolnay commented Jul 9, 2017

I know this is mentioned in the threads that were linked above but to reiterate: I strongly prefer std over use_std and I plan to push that as the one we standardize on.

If some crate has an optional dependency on bytes:

[package]
name = "tarcieri"

[dependencies]
bytes = { version = "0.4", optional = true }

then I depend on it like this:

[package]
name = "dtolnay"

[dependencies]
tarcieri = { version = "0.1", features = ["bytes"] }

Why should it be any different for an optional dependency on the std or alloc crates? If a crate has an optional dependency on std, I should be able to enable that optional dependency with features = ["std"] and same for alloc.

@tarcieri
Copy link

@alexcrichton perhaps it'd be better if I started with a less ambitious PR which gates all functionality which requires allocators on the std feature (which, per @dtolnay, should probably be called std)

We can then look at what functionality is missing from bytes that's actually needed in the downstream crates which are consuming it (IntoBuf was the main one of interest for me), and whether we can handle the allocations in the downstream crates rather than in bytes itself.

If it turns out that's a bad approach, we can circle back on handling no_std allocations via liballoc in a separate PR.

Does that seem like a better way forward to you?

@alexcrichton
Copy link
Contributor

@tarcieri seems reasonable to me!

@tonychain tonychain changed the title Support no_std w\ enabled-by-default "use_std" feature no_std support (using enabled-by-default "std" feature) Jul 18, 2017
@tarcieri
Copy link

Okay, I've updated the PR to massively reduce the scope and feature set for no_std usage. This should be dramatically simpler and we can circle back on allocation.

Per rust-lang/api-guidelines@a15e953 this uses "std" as the feature name (cc @dtolnay)

@tonychain
Copy link
Author

I moved all of the allocator-dependent gating to a separate PR:

#153

- Changes all references to libcore features from ::std to ::core
- Feature gates anything dependent on std on the "std" feature
@carllerche
Copy link
Member

Sorry for the delay.

At a high level, this looks good to me. This simply feature flags everything that requires std. I think @alexcrichton has some thoughts on how to organize it though to reduce the actual number of cfg lines needed...

@tarcieri
Copy link

Definitely interested in ideas regarding how to simplify this sort of gating, as I've done in in over a dozen crates at this point, and would agree it doesn't feel particularly good doing this way.

@@ -70,8 +70,13 @@

#![deny(warnings, missing_docs, missing_debug_implementations)]
#![doc(html_root_url = "https://docs.rs/bytes/0.4")]
#![cfg_attr(not(feature = "std"), no_std)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one of the easiest things to do to ease development of no_std libraries is to unconditionally use the #![no_std] attribute.

Only if the std feature is enabled should this have extern crate std, and then in that case the prelude can be imported to specific modules if necessary.

use core::sync::atomic::{self, AtomicUsize, AtomicPtr};
use core::sync::atomic::Ordering::{Relaxed, Acquire, Release, AcqRel};

#[cfg(feature = "std")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this file need to change? It looks like the whole module is only compiled when std is enabled?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm pretty sure changes to this file could be reverted.

@alexcrichton
Copy link
Contributor

One thing I've found useful is to have a module just called std which contains all std-relevant implementations of traits and such which is #[cfg]'d off by default. Would that be possible here to cut down on #[cfg] traffic?

@carllerche carllerche added this to the v0.5 milestone Dec 14, 2017
@carllerche
Copy link
Member

Sorry this has been left to stale. I will try to get back to this soon.

@Kixunil
Copy link

Kixunil commented Mar 12, 2018

What's the status of this?

@tarcieri
Copy link

This PR, in and of itself, is pretty useless: as @alexcrichton noted in the line notes none of bytes.rs is available with this. My changes there were left over from me splitting an original PR into two parts, the other being #153, where I had parts of bytes.rs working when obtaining heap-allocated data structures through unstable, nightly-only allocator APIs. Without an allocator none of bytes.rs can be used.

Even if this PR and #153 were merged (and it looks like #153 won't ever be merged) there's a bigger problem though: std::io::Cursor is a pretty essential part of how bytes's API works presently.

For those reasons I've created a mostly-compatible fork of bytes here, which vendors std::io::Cursor:

https://github.com/microcrates/bytes

My understanding is bytes 0.5 will redo the API so as not to need std::io::Cursor, and future work on std should allow stable access to allocators in what are presently considered "no_std" environments (i.e. no_std-with-a-heap environments). When these things happen bytes should hopefully just work in these environments.

@Kixunil
Copy link

Kixunil commented Mar 13, 2018

To clarify this, I'm really only interested in the traits and their most basic methods. I'd find it more elegant if they were split, but I doubt I could convince enough people.

@carllerche
Copy link
Member

This has been inactive. Unless there is interest to champion this over the finish line, it probably won't get more attention.

As such, I'm going to close this. A new PR can be opened once there is time to work on this.

@carllerche carllerche closed this May 25, 2018
@Kixunil
Copy link

Kixunil commented May 26, 2018

@carllerche How would you feel about putting the traits into a separate no_std- friendly crate? I might find some time for that...

@tarcieri
Copy link

tarcieri commented Jul 2, 2018

There's an RFC to stabilize alloc here, which would make this actually possible:

rust-lang/rfcs#2480

@carllerche
Copy link
Member

@Kixunil I have no problem moving the traits, but they fundamentally depend on io::Cursor etc... for now. I do not think that progress can be made w/o a breaking change, and there is no plan to issue one immediately.

It might be possible to make progress w/o a huge breaking change, but doing so would be pretty complicated and not something that I have time for currently.

@tarcieri
Copy link

tarcieri commented Jul 2, 2018

Yeah, fixing #75 is still a blocker on any no_std support.

EDIT: Actually there's some discussion on rust-lang/rfcs#2480 about moving io::Cursor into alloc as well, which would be another way to address this if alloc were subsequently stabilized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants