Turn the no_std feature into default-enabled std #74

kupiakos · 2023-06-10T03:47:45Z

This gets #68 over the finish line to being a functional #![no_std] library.

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 #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.

Cargo.toml

rmsyn · 2023-06-10T22:29:18Z

libflate_lz77/Cargo.toml


 [dev-dependencies]
-libflate = { path = "../", version = "1" }
+libflate = { path = "../", version = "1", default-features = false }


Suggested change

libflate = { path = "../", version = "1", default-features = false }

libflate = { path = "../", version = "2", default-features = false }

Or, just remove the version completely. Since we're using a path dependency here, the version isn't necessary.

IIUC it's necessary for crates.io and docs.rs to have version declarations in order to build docs and run crater tests correctly

That's usually correct, but with path dependencies you don't need to specify version. You can specify the version, but if you do, it has to match the Cargo.toml version (meaning you would need to make the above edit).

I will be keeping both version and path specifiers.

with path dependencies you don't need to specify version

The issue is with building tests using crates.io sources, since cargo publish packages the source of just that crate as a tarball, instead of having access to the full git repository. So, if a dependency is specified with just a path specifier, I doubt docs.rs or crates.io-side unit tests will be able to run correctly. Cargo calls out the distinction that crates.io has here in its documentation:

The git or path dependency will be used locally (in which case the version is checked against the local copy), and when published to a registry like crates.io, it will use the registry version.

For an example in the wild, see serde, which in its dev-dependencies imports serde_derive using both path and version specifiers.

For an example in the wild, see serde, which in its dev-dependencies imports serde_derive using both path and version specifiers.

Right on, then the suggested edit should work. Necessary with the major version bump.

Done. I went with the same style as writing 2.0 instead of 2 if that makes a difference to you.

src/deflate/mod.rs

src/deflate/decode.rs

src/bit.rs

src/deflate/encode.rs

src/deflate/mod.rs

src/gzip.rs

src/non_blocking/deflate/decode.rs

src/non_blocking/gzip.rs

src/non_blocking/zlib.rs

src/util.rs

src/zlib.rs

rmsyn · 2023-06-10T23:16:00Z

Some minor edits for imports to fix std/no_std test builds.

There is also a needed change in a file you didn't edit, src/lz77.rs:

    use crate::deflate::symbol::Symbol;
+   #[cfg(not(feature = "std"))]
+   use alloc::{vec, vec::Vec};

Otherwise, LGTM. Thanks for adding the example binary, and test runner!

.github/workflows/ci.yml

src/deflate/encode.rs

src/finish.rs

src/non_blocking/deflate/mod.rs

src/gzip.rs

rmsyn · 2023-06-13T00:45:47Z

@kupiakos Please run both std and no_std test builds, and make sure they complete successfully.

# std tests
cargo test --all

# no_std tests
cargo test --all --no-default-features

They complete successfully with the suggested edits in this PR.

src/deflate/decode.rs

src/gzip.rs

kupiakos · 2023-06-13T05:39:34Z

@rmsyn PTAL at the changes. Again, my apologies on missing the #![no_std] tests. In my defense, most of the embedded unit tests I work with don't have a std feature, they're default #![no_std] and so I forget cargo test --no-default-features is even an option 😄

@sile Can you check that the Github workflow works as intended? Not sure if @rmsyn can.

.github/workflows/ci.yml

rmsyn · 2023-06-14T01:42:45Z

PTAL at the changes.

LGTM, all the tests pass under std and no_std builds. Not sure about the CI runners, but they look like they should work now.

edit: one kind of hacky way to see if test runners work is to open a PR against your fork of the repository. It should run the CI. Not a local way to run CI, but could be useful.

sile · 2023-06-18T08:26:43Z

@kupiakos Some CI checks have failed (in case you haven't noticed yet).

kupiakos · 2023-07-08T01:27:49Z

Sorry for taking so long to get to this. It was a ~~mistyped flag~~ many things. I'm figuring out testing the CI on my PR as suggested, though feel free to approve your workflow when you see this.

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-07-08T02:04:58Z

@sile I've confirmed CI works on my end, and have squashed all the fixup commits for a cleaner history.

sile

LGTM, thank you!

sile · 2023-07-08T09:50:54Z

Thank you too, @rmsyn, for your review!

kupiakos mentioned this pull request Jun 10, 2023

lib: add support for no_std builds #68

Merged