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 alloc feature #42

Merged
merged 5 commits into from
Dec 23, 2020
Merged

Add alloc feature #42

merged 5 commits into from
Dec 23, 2020

Conversation

Luro02
Copy link
Contributor

@Luro02 Luro02 commented Mar 4, 2020

Fixes #40

@Luro02
Copy link
Contributor Author

Luro02 commented Mar 4, 2020

I change the -D flag to -W for clippy, because of https://www.reddit.com/r/rust/comments/f5xpib/psa_denywarnings_is_actively_harmful/

@KokaKiwi KokaKiwi self-assigned this Mar 5, 2020
Cargo.toml Outdated
@@ -16,14 +16,15 @@ travis-ci = { repository = "KokaKiwi/rust-hex", branch = "master" }

[features]
default = ["std"]
std = []
alloc = ["serde/alloc"]
std = ["alloc", "serde/std"]
Copy link
Owner

@KokaKiwi KokaKiwi Mar 5, 2020

Choose a reason for hiding this comment

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

I tried that on my local dev env and i ran into a feature-resolutions problem: Enabling the std feature implicitly enables the serde feature because of the dependency on serde/std feature...
I think it's kinda a Cargo-level issue and i don't really know what to do about that.

Letting it be would make hex compiling implicitly serde by default even if it's should be an optional feature, even if not actually used, so shouldn't be a problem by itself apart from the fact it actually compiles something unused...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the relevant issue is rust-lang/cargo#3494

Copy link
Contributor Author

@Luro02 Luro02 Mar 5, 2020

Choose a reason for hiding this comment

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

I would tend toward merging this PR, because it fixes some outstanding issues.

The bug does not affect the run time performance and only increases the compile-time, which is kinda neglectable.

Though for now I think waiting is the best option, especially because this would bump the dependencies from zero to one. (some people specifically use this crate, because it has zero dependencies)

Copy link

Choose a reason for hiding this comment

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

Unfortunately, waiting also means that users of this crate in environments without alloc depend on a git branch rather than a released version until then. (No problem for the application I'm writing currently, though, as it's just a demo that sits in git and is not aim for to crates.io.)

Would introducing a feature "serde-std" that is to be enabled by users who want to use the crate with both serde and std resolve the issue? (Or a "serde-nostd" that only depends on serde and not on serde/std).

Copy link
Owner

@KokaKiwi KokaKiwi Nov 30, 2020

Choose a reason for hiding this comment

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

I was thinking, what about simply removing the dependencies over serde/std and serde/alloc, just having dependencies over "nostd-serde"?
hex does not really need the std version of serde as it only requires the de/serializing traits
Also, it would let the lib user being the only one setting the deps as they require it or not

@chrysn
Copy link

chrysn commented Nov 28, 2020

@Luro02, could you try to update the branch to the latest developments in this crate? Right now, it merge-conflicts on half the affected files.

In the meantime, I've uploaded the branch under a different name and with a README that indicates that it's just a temporary fork -- this allows me to publish other crates that depend on the no-alloc feature on crates.io.

@Luro02
Copy link
Contributor Author

Luro02 commented Nov 29, 2020

@chrysn done

@Luro02 Luro02 requested a review from KokaKiwi November 29, 2020 09:33
@Luro02
Copy link
Contributor Author

Luro02 commented Nov 29, 2020

Blocking issue has been closed (rust-lang/cargo#3494):

The issue has been resolved by introducing a new syntax for "weak dependencies", through which it is possible to specify feature flags, without enabling them.

Sadly this is an unstable feature (Tracking Issue: rust-lang/cargo#8832) and it seems like it might take quite a while, until it is stabilized.

Copy link
Owner

@KokaKiwi KokaKiwi left a comment

Choose a reason for hiding this comment

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

@KokaKiwi KokaKiwi merged commit a354af2 into KokaKiwi:master Dec 23, 2020
@KokaKiwi KokaKiwi mentioned this pull request Dec 23, 2020
chrysn added a commit to chrysn-pull-requests/coap-rs that referenced this pull request Feb 8, 2021
They are not used, so let's not build them.

(Also, some crates still cause trouble when serde is active with std
when the expect it not to be, so let's not cause any of these troubles
needlessly).

See-Also: https://gitlab.com/chrysn/coap-handler/-/issues/1
See-Also: KokaKiwi/rust-hex#42
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.

implicit dependency of "std" for "serde" feature in published 0.4.2
3 participants