lib: add support for no_std builds #68

rmsyn · 2023-02-05T19:46:28Z

Introduce a new, default std feature to the library, and conditionally use structures and traits from no_std compatible libraries.

Existing std users can continue using the library with no changes. no_std users can import the library using default_features = false.

Changes were testing by running the test suite using the std and no_std configurations:

# std build/test
cargo build
cargo test

# no_std build/test
cargo build --no-default-features
cargo test --no-default-features

sile · 2023-02-06T09:07:48Z

Thank you for your PR!
I will review this PR within a week.

sile · 2023-02-06T09:23:30Z

BTW, I fixed clippy warnings in #69. So, please merge the master branch to fix CI lint failures.

rmsyn · 2023-02-08T04:16:04Z

BTW, I fixed clippy warnings in #69

nice

sile · 2023-02-11T00:39:59Z

This PR basically seems good. However, this change always adds extra dependencies core2 and hashbrown (those haven't reached the v1 release) even when users, who want to use the "std" feature, don't actually need that.
I want to be cautious to add mandatory dependencies. So how about adding "no_std" feature and making core2 and hashbrown optional dependencies instead?

rmsyn · 2023-02-11T06:28:00Z

Thanks for the review.

So how about adding "no_std" feature and making core2 and hashbrown optional dependencies instead?

That makes sense to me. Wasn't sure if following the default std feature pattern from other crates was better, or the optional dependency + no_std feature.

The two fixup commits implement the changes you requested, and remove the default std feature. Leaving in the std feature resulted in the slightly awkward cargo build/test --no-default-features --features no_std for no_std builds (default-features = false, features = ["no_std"] for library usage). So, I removed the std feature.

If you like the changes, I'll squash them into the original commit.

sile · 2023-02-13T08:43:55Z

Thank you for the changes! LGTM 👍

I'll squash them into the original commit.

I will merge this PR once you do that.

Introduce a new, default `std` feature to the library, and conditionally use structures and traits from `no_std` compatible libraries. Existing `std` users can continue using the library with no changes. `no_std` users can import the library using `default_features = false`.

sile

Thanks!

kupiakos · 2023-05-31T19:07:31Z

That makes sense to me. Wasn't sure if following the default std feature pattern from other crates was better, or the optional dependency + no_std feature.

That is definitely the better pattern to follow - cargo features are intended to be additive, since if multiple libraries depend on different features in a library, both features are enabled . A no_std feature is the opposite of this.

Please consider redoing this PR the way most of the community follows and Cargo intends.

This PR also doesn't fix #![no_std] builds, since the crc32fast and adler32 dependencies define default-enabled std features that this repo doesn't default-features = false. There's not really an easy way to make a no_std feature work with these dependencies; the best way to fix this is to switch to an optional std feature.

Leaving in the std feature resulted in the slightly awkward cargo build/test --no-default-features --features no_std for no_std builds (default-features = false, features = ["no_std"] for library usage). So, I removed the std feature.

The way to alleviate this is to make std a non-default feature and require users that want std features to include the optional std feature. std code works with #![no_std] code, but not the other way around.

sile · 2023-05-31T20:03:32Z

@kupiakos Thank you for your comment.

cargo features are intended to be additive, since if multiple libraries depend on different features in a library

Make sense.
I'm willing to remove the no_std feature and introduce a std feature instead (and release the next major version as it would be a breaking change).

Are you interested in creating a PR for this change?

kupiakos · 2023-05-31T21:11:31Z

I'm willing to remove the no_std feature and introduce a std feature instead (and release the next major version as it would be a breaking change).

I can take a look sometime this next week. Which option of these would you prefer @sile ?

Make std a non-default feature and require users that want std features to include the optional std feature.
Make std a default feature and require #![no_std] users to default-features = false.

sile · 2023-05-31T21:25:23Z

Great!

Which option of these would you prefer?

I don't have a strong opinion on which one is the better choice at this point.
Please choose the one you think is most appropriate and let's discuss it in the PR if need.

rmsyn · 2023-06-01T01:29:43Z

That is definitely the better pattern to follow - cargo features are intended to be additive, since if multiple libraries depend on different features in a library, both features are enabled . A no_std feature is the opposite of this.

Please consider redoing this PR the way most of the community follows and Cargo intends.

Yeah, this is how the original PR was. The conversion isn't too involved (basically a few global sed invocations).

Let me know if you would like me to make the PR, or if you want to. I'm good with either, and appreciate your review.

This PR also doesn't fix #![no_std] builds, since the crc32fast and adler32 dependencies define default-enabled std features that this repo doesn't default-features = false. There's not really an easy way to make a no_std feature work with these dependencies; the best way to fix this is to switch to an optional std feature.

The library still technically builds, but yes, I missed these dependencies bringing in std by default. In the follow-up to this PR, I agree with you that the indirect dependency on std in no_std builds should be eliminated.

The way to alleviate this is to make std a non-default feature and require users that want std features to include the optional std feature. std code works with #![no_std] code, but not the other way around.

Right, I missed those dependencies you mentioned. Good catch 👍

This also: - fixes the `no_std` not being enabled due to a swapped `cfg_attr`. - tests this builds with a `#![no_std]` binary. - A lot of the cfg's have been removed due to being unneeded. - adds a new dependency - updates the version to 2.0 - adds a ci command to to use cargo-no-std to check no-std Some notes on changes from sile#68: Everything in `core` is a subset of things in `std`. `core` is available in `std` environments, so if you're building a `no_std`-compatible library, it's best to import things that don't _require_ `std` from `core. core2 is also meant to be used as a `no_std` polyfill with its `std` feature. That is, you can reference `core2::io`, and if the `std` feature is enabled, it actually references `std::io`.

kupiakos · 2023-06-10T01:49:46Z

libflate_lz77/src/lib.rs

@@ -2,8 +2,19 @@
 //!
 //! LZ77 is a compression algorithm used in [DEFLATE](https://tools.ietf.org/html/rfc1951).
 #![warn(missing_docs)]
+#![cfg_attr(no_std, feature = "no_std")]


This should be #![cfg_attr(feature = "no_std", no_std)] - I don't believe the library was actually building as no_std.

kupiakos · 2023-06-10T03:49:26Z

@rmsyn, @sile, please take a look at #74

This also: - fixes the `no_std` not being enabled due to a swapped `cfg_attr`. - tests this builds with a `#![no_std]` binary. - A lot of the cfg's have been removed due to being unneeded. - adds a new dependency - updates the version to 2.0 - adds a ci command to to use cargo-no-std to check no-std Some notes on changes from sile#68: Everything in `core` is a subset of things in `std`. `core` is available in `std` environments, so if you're building a `no_std`-compatible library, it's best to import things that don't _require_ `std` from `core. core2 is also meant to be used as a `no_std` polyfill with its `std` feature. That is, you can reference `core2::io`, and if the `std` feature is enabled, it actually references `std::io`.

rmsyn force-pushed the no_std branch from 02e65e5 to ac02741 Compare February 8, 2023 04:07

rmsyn force-pushed the no_std branch from 02f4010 to b10f88a Compare February 14, 2023 02:21

sile approved these changes Feb 16, 2023

View reviewed changes

sile merged commit 8ba4e67 into sile:master Feb 16, 2023

kupiakos mentioned this pull request Jun 10, 2023

Turn the no_std feature into default-enabled std #74

Merged

kupiakos reviewed Jun 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: add support for no_std builds #68

lib: add support for no_std builds #68

rmsyn commented Feb 5, 2023

sile commented Feb 6, 2023

sile commented Feb 6, 2023

rmsyn commented Feb 8, 2023

sile commented Feb 11, 2023

rmsyn commented Feb 11, 2023

sile commented Feb 13, 2023

sile left a comment

kupiakos commented May 31, 2023 •

edited

Loading

sile commented May 31, 2023

kupiakos commented May 31, 2023

sile commented May 31, 2023

rmsyn commented Jun 1, 2023

kupiakos Jun 10, 2023 •

edited

Loading

kupiakos commented Jun 10, 2023

lib: add support for no_std builds #68

lib: add support for no_std builds #68

Conversation

rmsyn commented Feb 5, 2023

sile commented Feb 6, 2023

sile commented Feb 6, 2023

rmsyn commented Feb 8, 2023

sile commented Feb 11, 2023

rmsyn commented Feb 11, 2023

sile commented Feb 13, 2023

sile left a comment

Choose a reason for hiding this comment

kupiakos commented May 31, 2023 • edited Loading

sile commented May 31, 2023

kupiakos commented May 31, 2023

sile commented May 31, 2023

rmsyn commented Jun 1, 2023

kupiakos Jun 10, 2023 • edited Loading

Choose a reason for hiding this comment

kupiakos commented Jun 10, 2023

kupiakos commented May 31, 2023 •

edited

Loading

kupiakos Jun 10, 2023 •

edited

Loading