Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upno_std support (using enabled-by-default "std" feature) #135
Conversation
tonychain
force-pushed the
chain:no_std
branch
from
9857d1b
to
e12c0b5
Jun 17, 2017
tonychain
force-pushed the
chain:no_std
branch
2 times, most recently
from
229772f
to
dd36b02
Jun 19, 2017
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jun 20, 2017
|
Sorry to bring up a bikeshedding topic, but I've been looking at @alexcrichton I chose See: rust-lang-nursery/api-guidelines#23 |
This comment has been minimized.
This comment has been minimized.
|
Long ago I remember we discussed creating a convention for the "enable std support" feature for the |
carllerche
reviewed
Jun 26, 2017
|
I left my comments inline. I believe that conditionally defining Hopefully @alexcrichton can review as he is more familiar w/ no std. |
| serde = { version = "1.0", optional = true } | ||
|
|
||
| [dependencies.iovec] |
This comment has been minimized.
This comment has been minimized.
carllerche
Jun 26, 2017
Owner
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.
| [features] | ||
| default = ["use_std"] | ||
| use_alloc = [] | ||
| use_std = ["iovec"] |
This comment has been minimized.
This comment has been minimized.
carllerche
Jun 26, 2017
Owner
It seems that use_std implies use_alloc. I'm not sure if there is a way to define this in Cargo.toml.
This comment has been minimized.
This comment has been minimized.
tonychain
Jun 26, 2017
Author
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; |
This comment has been minimized.
This comment has been minimized.
carllerche
Jun 26, 2017
Owner
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 :(
This comment has been minimized.
This comment has been minimized.
tonychain
Jun 26, 2017
Author
A couple options here:
- Gate this (and anything that depends on it) on
use_alloc - Stop using
std::io::Cursorentirely (i.e. fix #75)
This comment has been minimized.
This comment has been minimized.
carllerche
Jun 26, 2017
Owner
@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?
tonychain
force-pushed the
chain:no_std
branch
from
dd36b02
to
e0982c9
Jun 26, 2017
This comment has been minimized.
This comment has been minimized.
|
Ah yeah I agree with @carllerche in that the handling of I'm personally not a huge fan of using unstable crates like |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 3, 2017
|
The cleanest solution re: Regarding |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 3, 2017
This comment has been minimized.
This comment has been minimized.
|
Yeah I think with |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 5, 2017
|
All Personally I would much rather deal with some changes to nightly-only features of |
This comment has been minimized.
This comment has been minimized.
|
Oh sure that makes sense for |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 6, 2017
|
Who would be trying to swap |
This comment has been minimized.
This comment has been minimized.
|
There's use cases for no_std on stable, nightly is not the only consumer. |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 6, 2017
|
I think pretty much all practical |
This comment has been minimized.
This comment has been minimized.
|
Ok, well, my opinion is that this crate should not have a feature where it only uses the |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 7, 2017
•
|
@alexcrichton would it help if the feature were more clearly labeled to indicate its experimental/nightly nature, e.g. re: feature unioning, the crate builds with all combinations of |
This comment has been minimized.
This comment has been minimized.
|
I would still personally prefer to not add such a feature to the crate, but I'd again defer to @carllerche's thoughts. |
This comment has been minimized.
This comment has been minimized.
dtolnay
commented
Jul 9, 2017
|
I know this is mentioned in the threads that were linked above but to reiterate: I strongly prefer If some crate has an optional dependency on [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 |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 14, 2017
|
@alexcrichton perhaps it'd be better if I started with a less ambitious PR which gates all functionality which requires allocators on the We can then look at what functionality is missing from If it turns out that's a bad approach, we can circle back on handling Does that seem like a better way forward to you? |
This comment has been minimized.
This comment has been minimized.
|
@tarcieri seems reasonable to me! |
tonychain
force-pushed the
chain:no_std
branch
from
e0982c9
to
13f1a56
Jul 18, 2017
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
tonychain
force-pushed the
chain:no_std
branch
from
13f1a56
to
9da196d
Jul 18, 2017
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 18, 2017
|
Okay, I've updated the PR to massively reduce the scope and feature set for Per rust-lang-nursery/api-guidelines@a15e953 this uses "std" as the feature name (cc @dtolnay) |
This comment has been minimized.
This comment has been minimized.
|
I moved all of the allocator-dependent gating to a separate PR: |
tonychain
referenced this pull request
Jul 18, 2017
Closed
"allocator" and "nightly" features (for no_std environments) #153
tonychain
force-pushed the
chain:no_std
branch
from
9da196d
to
66998e5
Jul 18, 2017
tonychain
force-pushed the
chain:no_std
branch
from
66998e5
to
aaed035
Jul 24, 2017
This comment has been minimized.
This comment has been minimized.
|
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... |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Aug 17, 2017
|
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. |
alexcrichton
reviewed
Aug 17, 2017
| @@ -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)] | |||
This comment has been minimized.
This comment has been minimized.
alexcrichton
Aug 17, 2017
Contributor
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.
alexcrichton
reviewed
Aug 17, 2017
| use core::sync::atomic::{self, AtomicUsize, AtomicPtr}; | ||
| use core::sync::atomic::Ordering::{Relaxed, Acquire, Release, AcqRel}; | ||
|
|
||
| #[cfg(feature = "std")] |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Aug 17, 2017
Contributor
Did this file need to change? It looks like the whole module is only compiled when std is enabled?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
One thing I've found useful is to have a module just called |
carllerche
added this to the v0.5 milestone
Dec 14, 2017
This comment has been minimized.
This comment has been minimized.
|
Sorry this has been left to stale. I will try to get back to this soon. |
tarcieri
added a commit
to microcrates/bytes
that referenced
this pull request
Feb 9, 2018
tarcieri
added a commit
to microcrates/bytes
that referenced
this pull request
Feb 9, 2018
tarcieri
referenced this pull request
Feb 9, 2018
Merged
no_std support (making "std" an optional feature) #2
tarcieri
added a commit
to microcrates/bytes
that referenced
this pull request
Feb 9, 2018
This comment has been minimized.
This comment has been minimized.
Kixunil
commented
Mar 12, 2018
|
What's the status of this? |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Mar 12, 2018
|
This PR, in and of itself, is pretty useless: as @alexcrichton noted in the line notes none of Even if this PR and #153 were merged (and it looks like #153 won't ever be merged) there's a bigger problem though: For those reasons I've created a mostly-compatible fork of bytes here, which vendors https://github.com/microcrates/bytes My understanding is bytes 0.5 will redo the API so as not to need |
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
|
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
closed this
May 25, 2018
This comment has been minimized.
This comment has been minimized.
Kixunil
commented
May 26, 2018
|
@carllerche How would you feel about putting the traits into a separate |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 2, 2018
|
There's an RFC to stabilize |
This comment has been minimized.
This comment has been minimized.
|
@Kixunil I have no problem moving the traits, but they fundamentally depend on 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. |
This comment has been minimized.
This comment has been minimized.
tarcieri
commented
Jul 2, 2018
•
|
Yeah, fixing #75 is still a blocker on any EDIT: Actually there's some discussion on rust-lang/rfcs#2480 about moving |
tonychain commentedJun 16, 2017
•
edited
::stdto::corestdon the "std" feature