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 + checking no_std builds in Travis CI #15

Merged
merged 2 commits into from
Jun 21, 2018
Merged

Conversation

fitzgen
Copy link
Collaborator

@fitzgen fitzgen commented May 9, 2018

Was going to rebase #10 to use feature(alloc) rather than feature(collections) but it was already done. Added a check that no_std builds in Travis CI while I was at it.

r? @SimonSapin

tonychain and others added 2 commits June 30, 2017 16:06
This adds an enabled-by-default "std" cargo feature which can be selectively
removed to enable no_std support.

When the "std" feature is absent, feature(alloc) will be enabled to
provide Vec support. This requires nightly, but since "std" is enabled by
default, this should have no impact on stable users.
@jasondavies
Copy link

It would be great to see this in a new release! Very useful for generating minimal wasm binaries (with no_std). Thanks.

@fitzgen fitzgen merged commit 40bfb2f into thomcc:master Jun 21, 2018
@fitzgen fitzgen deleted the no_std branch June 21, 2018 20:16
@cuviper
Copy link

cuviper commented Jun 22, 2018

When the "std" feature is absent, feature(alloc) will be enabled to provide Vec support. This requires nightly, but since "std" is enabled by default, this should have no impact on stable users.

FYI, this is a breaking change for any stable users who had default-features = false.

I'm not impacted here -- just wanted to point it out because the same sort of thing bit me when I first tried to add no_std to num, as seen in rust-num/num#297. Since there were no features here before anyway, it's unlikely anyone did this, so it's somewhat of a pedantic point. Hopefully you're in the clear. 😄

@SimonSapin
Copy link
Collaborator

Since the public API and functionality is not changing, it might be better to reverse the flag: have features = ["no_std"] enable the (unstable) “feature” of being compatible with targets that do not have a std crate.

@cuviper
Copy link

cuviper commented Jun 22, 2018

Though now that this is published, changing further risks breakage too. Now it's between my hypothetical stable default-features = false user versus any new folks who really do want no_std with alloc.

@fitzgen
Copy link
Collaborator Author

fitzgen commented Jun 22, 2018

Cargo features are intended to be additive, so I think keeping a "std" feature to add the dependency on std is the way to go. It is also what I've seen the most in the ecosystem.

The point about this being technically a breaking change is a good one: I hadn't thought of that. I think the best thing to do now is see if anyone was actually using default-features = false and if so, yank the new release and bump to version 2 to re-release. If not, then let the technically-breaking change stand.

@SimonSapin
Copy link
Collaborator

Yes cargo feature flags should be additive, but depending on std is not a feature in itself. (In some crates depending on std might enable some of the crate’s own features, but this is not the case here.) I’d say that not depending on std is a feature, since it adds support for some targets.

@SimonSapin
Copy link
Collaborator

Anyway, with rust-lang/rfcs#2480 this crate can hopefully become #![no_std] unconditionally.

@cuviper
Copy link

cuviper commented Jun 22, 2018

I’d say that not depending on std is a feature, since it adds support for some targets.

True, as long as you don't lose anything in the process, like an impl for std::error::Error. IOW enabling a feature should not disable any functionality -- which I think is fine here apart from stable compatibility.

@SimonSapin
Copy link
Collaborator

(Yes, this is what I meant.)

@fitzgen fitzgen mentioned this pull request Jun 29, 2018
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.

5 participants