sile / libflate

A Rust implementation of DEFLATE algorithm and related formats (ZLIB, GZIP)
https://docs.rs/libflate
MIT License
178 stars 35 forks source link

Turn the no_std feature into default-enabled std #74

Closed kupiakos closed 1 year ago

kupiakos commented 1 year ago

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

This also:

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.

rmsyn commented 1 year ago

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!

rmsyn commented 1 year ago

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

kupiakos commented 1 year ago

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

rmsyn commented 1 year ago

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 commented 1 year ago

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

kupiakos commented 1 year ago

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.

kupiakos commented 1 year ago

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

sile commented 1 year ago

Thank you too, @rmsyn, for your review!