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 a default-on std feature #146

Merged
merged 1 commit into from Feb 11, 2016

Conversation

Projects
None yet
9 participants
@alexcrichton
Copy link
Member

alexcrichton commented Jan 21, 2016

This adds a std Cargo feature which disables #![no_std] builds of libc, but
is enabled by default. The library will currently continue to link to the
standard library to maintain backwards compatibility with the 0.2 series and
older Rust compilers for now, but this default can possible be changed in the
future.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jan 21, 2016

r? @brson

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 21, 2016

🙌🙌🙌🙌🙌🙌🙌🙌🙌

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Jan 21, 2016

cargo features are additive, so this does not work under the cargo scheme. If any dependency activates "no-std", the whole project will have it (for libc).

cargo features can't turn things off. Could this instead use a positive feature "use-std" that is enabled by default? The unioning of cargo features will work correctly then.

@alexcrichton alexcrichton force-pushed the alexcrichton:no-std branch from 3138084 to aa41fbe Jan 21, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jan 21, 2016

For this library specifically I think that this is an additive feature (because the API will not change based on it or not) as it's adding a feature, but for libraries in general I think that they're going to provide a more full suite of functionality with std than without.

Along those lines I'd be interested in starting to pioneer a convention for libraries with no_std support. An example of this is that the log crate now has a use_std feature. I wouldn't mind following suit there as well.

I also think that it makes more sense to "turn off the standard library" than "enable turning off the standard library", e.g. disable-features = false seems to jive better than features = ["no-std"].

cc @rust-lang/libs, this is at the very least a backwards compatibility hazard as once we've decided a name/interface for this it can't be changed. Thoughts?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jan 21, 2016

Ugh cross-org notifications, see my above message: ^^

cc @aturon, @brson, @sfackler, @huonw, @Kimundi, @BurntSushi, @Gankro

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Jan 21, 2016

My bad! I did actually assume it would change its functionality depending on std / no std (this is the use case I've been thinking about). Very interested in that we have a common way to do this across crates.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 21, 2016

If I understand @bluss's concern, it 'no-std' is a convention, then it will be impossible to selectively turn off for specific crates; activating the cargo 'no-std' feature anywhere will turn it off for all crates. Is that right?

I also think that it makes more sense to "turn off the standard library" than "enable turning off the standard library", e.g. disable-features = false seems to jive better than features = ["no-std"].

I don't understand what this comment means. What does 'disable-features = false' do?

Could this instead use a positive feature "use-std" that is enabled by default? The unioning of cargo features will work correctly then.

Can you spell out why this is? Also, what do you do to disable a feature, either project-wide or per-crate?

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Jan 21, 2016

You have a library D version "1" which supports core and/or std. D has the feature flag "use-std".

We have application A, with dependencies B and C.

  • B depends on D version "1" with "use-std"
  • C depends on D version "1" with no features

To build A, cargo has to build B, C, D. To build D, it unions all the features needed, so it builds D with "use-std". For this reason we can't use features to subtract, we must only add, since cargo treats them as additive (adding a feature flag must not break those that depend on the crate as it was without the feature flag).

Having default features is like having all deps specifying they depend on the default features automatically.

Specifying features is something you do in the dependencies section. For example.

[dependencies]
D = { version = "1", features = ["use-std"] }
E = { version = "1", default-features = false }

Edit: Note that since libc neither "adds" nor "subtracts", I don't think it matters for this package..

An example of adding would be to implement the trait "Read" when possible (when std is available).

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jan 21, 2016

it will be impossible to selectively turn off for specific crates; activating the cargo 'no-std' feature anywhere will turn it off for all crates

Yeah this is correct

What does 'disable-features = false' do?

Oops, that actually does nothing, I meant default-features = false which is actually a Cargo thing!


I that I would want to concretely propose that std is the feature named used to activate linking to the standard library (although I'm also fine with bikeshedding the name). Although libc is not really additive or subtractive on this, it seems like a much more plausible convention for external crates to have this behavior (enabling std enables more features).

@alexcrichton alexcrichton force-pushed the alexcrichton:no-std branch 2 times, most recently from 639e60a to fd755bb Jan 22, 2016

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Jan 22, 2016

In this case the only reason we need a Cargo feature is to allow libc to work on older Rust versions that don't support no_std. If we didn't have to support older Rust version then we could just unconditionally make libc no_std.

README.md Outdated
[dependencies]
libc = { version = "0.2", features = ["no-std"] }
```

This comment has been minimized.

@Stebalien

Stebalien Jan 22, 2016

Contributor

Needs to be updated to yes-std.

@alexcrichton alexcrichton changed the title Add a no-std feature Add a std feature Jan 22, 2016

@alexcrichton alexcrichton changed the title Add a std feature Add a default-on std feature Jan 22, 2016

@alexcrichton alexcrichton force-pushed the alexcrichton:no-std branch from fd755bb to b1a6f45 Jan 22, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jan 22, 2016

@Amanieu yes that's correct, it's currently expected that this crate works with versions like Rust 1.5 and below, so switching the defaults would be a bit of a breaking change for libc.

@alexcrichton alexcrichton force-pushed the alexcrichton:no-std branch from b1a6f45 to 4e5b983 Jan 23, 2016

@tbu-

This comment has been minimized.

Copy link
Contributor

tbu- commented Jan 23, 2016

Instead of having default-features = false, it would be better to selectively turn off default features, because otherwise, adding more default features would be a breaking change. Is that possible with Cargo or is there an issue for it?

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Jan 24, 2016

That's a troubling thought. It seems cargo needs a new feature for this.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jan 24, 2016

@tbu- ah yes that's a good point, but it also only applies in the case where existing APIs are hidden behind a feature. If a default-on feature is used to gate just-newly-added APIs then it wouldn't cause breakage.

There is currently not a Cargo issue for disabling a feature (or just disabling a default feature), but it seems like it may be interesting to add.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 27, 2016

It's not clear to me we've arrived at the solution. This is a big convention to set. Is everybody comfortable with this: 'std' is the feature that enables linking to std, enabled by default, and you turn on no_std by disabling all default features?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jan 27, 2016

Yeah I agree, I would like to talk about this during libs triage.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 27, 2016

I'm having trouble following the thrust of this thread. On the one hand, we seem to be talking about a hack in libc to get backwards-compatibility with older versions of rustc. On the other hand, we seem to be talking about a general convention. Is the latter also just about backcompat?

I feel like we continually run into issues like this and we should seriously consider a first-class way to straddle versions via cfg.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jan 27, 2016

Yes, there are two issues here, but the question for the libs team (in my mind) is about the feature naming/semantics itself.

To recap, the first issue is that it would be a breaking change right now to make libc #![no_std] by default because older compilers will not accept this.

The second issue is that there are many use cases throughout our crates of having a "libcore mode". For example the log crate recently picked this up. The idea is that you can provide what is a likely stripped down set of functionality without std, but you can still provide the core set of utilities. I'm thinking that we want to have features called std which are enabled by default to express this ability because:

  • @bluss is right in that features are additive in Cargo, so having a feature to remove functionality could cause complications when compiling
  • This provides a uniform interface of default-features = false to give the bare bones functionality of a crate.

As @tbu- mentioned, however, there is a backwards-compatibility hazard. Once the set of default features have been decided, then the existing API of the crate must always be available with default-features = false. Which is to say that over time we can add more default-on features for new APIs, but we cannot take existing functionality and move it behind a default feature.


@aturon I agree that straddling versions with some sort of cfg to just always support #![no_std] would be best, but... we don't have it :(

@Gankro

This comment has been minimized.

Copy link

Gankro commented Feb 2, 2016

@alexcrichton just have the build script detect the rustc version, and branch on that!

cargo/1.2 rustc/1.5 (llvm/3.8, like clang)

@bluss bluss referenced this pull request Feb 6, 2016

Merged

Support no_std in nodrop #25

@alexcrichton alexcrichton force-pushed the alexcrichton:no-std branch from 4e5b983 to d55f631 Feb 11, 2016

Add a default-on "use_std" feature
This adds a `use_std` Cargo feature which disables `#![no_std]` builds of libc,
but is enabled by default. The library will currently continue to link to the
standard library to maintain backwards compatibility with the 0.2 series and
older Rust compilers for now, but this default can possible be changed in the
future.

@alexcrichton alexcrichton force-pushed the alexcrichton:no-std branch from d55f631 to 6d46b6f Feb 11, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Feb 11, 2016

The libs team discussed this during triage today and the conclusion was to take this strategy with the name "use_std" (to mirror the log crate). I've pushed a commit with these changes and I'll merge when CI is green!

alexcrichton added a commit that referenced this pull request Feb 11, 2016

Merge pull request #146 from alexcrichton/no-std
Add a default-on std feature

@alexcrichton alexcrichton merged commit 61eddaf into rust-lang:master Feb 11, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexcrichton alexcrichton deleted the alexcrichton:no-std branch Feb 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.