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

lib: enable no-std compatibility #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ byteorder = "1.4.3"
crc = "3.0.0"
log = { version = "0.4.17", optional = true }
env_logger = { version = "0.9.0", optional = true }
core2 = { version = "^0.4", optional = true }

[dev-dependencies]
rust-lzma = "0.5"
Expand All @@ -25,6 +26,7 @@ seq-macro = "0.3"
enable_logging = ["env_logger", "log"]
stream = []
raw_decoder = []
no_std = ["core2"]

Choose a reason for hiding this comment

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

This is 100% opposite of what is suggested in the cargo book:

"features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features. A feature should not introduce a SemVer-incompatible change.

For example, if you want to optionally support no_std environments, do not use a no_std feature. Instead, use a std feature that enables std."

In my experience, people are definitely following the cargo book's recommendation. So I think you had it right the first time.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review.

Instead, use a std feature that enables std

I think this advice is for the general case, when optional dependencies aren't involved. It's not possible to use optional dependencies in a no_std build using the std feature approach.

This is one of the main reasons @gendx suggested using the no_std feature approach.

Reverting to the original PR isn't much work, but the changes do have their drawbacks.

A feature should not introduce a SemVer-incompatible change.

This PR (both versions) do not break SemVer, users would still get std builds by default. Compiled code should literally be identical to code without the changes, unless the opt-in feature is used.

In the original, the user opts-in with --default-features false. In the current version, the user opts-in with --features no_std. Otherwise, the build should use all the std dependencies, and be identical to the current mainline.

Choose a reason for hiding this comment

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

std should be part of your defaults, people who want to use it with no_std will use --default-features false (or default-features = false in the Cargo.toml). This is the pattern that exists across all the crates I work with in the embedded space.

Copy link
Author

Choose a reason for hiding this comment

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

std should be part of your defaults, people who want to use it with no_std will use --default-features false (or default-features = false in the Cargo.toml). This is the pattern that exists across all the crates I work with in the embedded space.

I agree with you, that's why I did it that way in the original PR. The (now-)optional dependency gets dropped from compilation when std feature is enabled, since it's not used anywhere else. That essentially turns a required dependency into an optional dependency, with the exception that the crate (and its dependencies) are always downloaded.

I'm fine with either version, honestly. I can revert to the original changes, or keep the PR as it is now. It's up to what @gendx wants to do.

Still no movement on the core2 PR.


[package.metadata.docs.rs]
features = ["stream", "raw_decoder"]
Expand Down
3 changes: 3 additions & 0 deletions src/decode/lzbuffer.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use crate::error;
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

pub trait LzBuffer<W>
Expand Down
11 changes: 8 additions & 3 deletions src/decode/lzma.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ use crate::decode::rangecoder::{BitTree, LenDecoder, RangeDecoder};
use crate::decompress::{Options, UnpackedSize};
use crate::error;
use crate::util::vec2d::Vec2D;
#[cfg(feature = "no_std")]
use alloc::string::String;
use byteorder::{LittleEndian, ReadBytesExt};
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

/// Maximum input data that can be processed in one iteration.
Expand Down Expand Up @@ -165,7 +170,7 @@ impl LzmaParams {
pub(crate) struct DecoderState {
// Buffer input data here if we need more for decompression. Up to
// MAX_REQUIRED_INPUT bytes can be consumed during one iteration.
partial_input_buf: std::io::Cursor<[u8; MAX_REQUIRED_INPUT]>,
partial_input_buf: io::Cursor<[u8; MAX_REQUIRED_INPUT]>,
pub(crate) lzma_props: LzmaProperties,
unpacked_size: Option<u64>,
literal_probs: Vec2D<u16>,
Expand All @@ -188,7 +193,7 @@ impl DecoderState {
pub fn new(lzma_props: LzmaProperties, unpacked_size: Option<u64>) -> Self {
lzma_props.validate();
DecoderState {
partial_input_buf: std::io::Cursor::new([0; MAX_REQUIRED_INPUT]),
partial_input_buf: io::Cursor::new([0; MAX_REQUIRED_INPUT]),
lzma_props,
unpacked_size,
literal_probs: Vec2D::init(0x400, (1 << (lzma_props.lc + lzma_props.lp), 0x300)),
Expand Down Expand Up @@ -412,7 +417,7 @@ impl DecoderState {
range: u32,
code: u32,
) -> error::Result<()> {
let mut temp = std::io::Cursor::new(buf);
let mut temp = io::Cursor::new(buf);
let mut rangecoder = RangeDecoder::from_parts(&mut temp, range, code);
let _ = self.process_next_inner(output, &mut rangecoder, false)?;
Ok(())
Expand Down
6 changes: 4 additions & 2 deletions src/decode/lzma2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ use crate::decode::lzma::{DecoderState, LzmaProperties};
use crate::decode::{lzbuffer, rangecoder};
use crate::error;
use byteorder::{BigEndian, ReadBytesExt};
use std::io;
use std::io::Read;
#[cfg(feature = "no_std")]
use core2::io::{self, Read};
#[cfg(not(feature = "no_std"))]
use std::io::{self, Read};

#[derive(Debug)]
/// Raw decoder for LZMA2.
Expand Down
3 changes: 3 additions & 0 deletions src/decode/rangecoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ use crate::decode::util;
use crate::error;
use crate::util::const_assert;
use byteorder::{BigEndian, ReadBytesExt};
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

pub struct RangeDecoder<'a, R>
Expand Down
20 changes: 16 additions & 4 deletions src/decode/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,18 @@ use crate::decode::lzma::{DecoderState, LzmaParams};
use crate::decode::rangecoder::RangeDecoder;
use crate::decompress::Options;
use crate::error::Error;
use std::fmt::Debug;
#[cfg(feature = "no_std")]
use core::fmt::{self, Debug};
#[cfg(feature = "no_std")]
use core::u64::MAX as U64_MAX;
#[cfg(feature = "no_std")]
use core2::io::{self, BufRead, Cursor, Read, Write};
#[cfg(not(feature = "no_std"))]
use std::fmt::{self, Debug};
#[cfg(not(feature = "no_std"))]
use std::io::{self, BufRead, Cursor, Read, Write};
#[cfg(not(feature = "no_std"))]
use std::u64::MAX as U64_MAX;

/// Minimum header length to be read.
/// - props: u8 (1 byte)
Expand Down Expand Up @@ -52,7 +62,7 @@ impl<W> Debug for RunState<W>
where
W: Write,
{
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
fmt.debug_struct("RunState")
.field("range", &self.range)
.field("code", &self.code)
Expand Down Expand Up @@ -211,7 +221,7 @@ impl<W> Debug for Stream<W>
where
W: Write + Debug,
{
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
fmt.debug_struct("Stream")
.field("tmp", &self.tmp.position())
.field("state", &self.state)
Expand Down Expand Up @@ -277,7 +287,7 @@ where
// reset the cursor because we may have partial reads
input.set_position(0);
let bytes_read = input.read(&mut self.tmp.get_mut()[..])?;
let bytes_read = if bytes_read < std::u64::MAX as usize {
let bytes_read = if bytes_read < U64_MAX as usize {
bytes_read as u64
} else {
return Err(io::Error::new(
Expand Down Expand Up @@ -348,6 +358,8 @@ impl From<Error> for io::Error {
#[cfg(test)]
mod test {
use super::*;
#[cfg(feature = "no_std")]
use alloc::vec::Vec;

/// Test an empty stream
#[test]
Expand Down
3 changes: 3 additions & 0 deletions src/decode/util.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

pub fn read_tag<R: io::BufRead>(input: &mut R, tag: &[u8]) -> io::Result<bool> {
Expand Down
8 changes: 6 additions & 2 deletions src/decode/xz.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@ use crate::decode::util;
use crate::error;
use crate::xz::crc::{CRC32, CRC64};
use crate::xz::{footer, header, CheckMethod, StreamFlags};
#[cfg(feature = "no_std")]
use alloc::vec::Vec;
use byteorder::{BigEndian, LittleEndian, ReadBytesExt};
use std::io;
use std::io::Read;
#[cfg(feature = "no_std")]
use core2::io::{self, Read};
#[cfg(not(feature = "no_std"))]
use std::io::{self, Read};

#[derive(Debug)]
struct Record {
Expand Down
3 changes: 3 additions & 0 deletions src/encode/dumbencoder.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use crate::compress::{Options, UnpackedSize};
use crate::encode::rangecoder;
use byteorder::{LittleEndian, WriteBytesExt};
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

pub struct Encoder<'a, W>
Expand Down
5 changes: 5 additions & 0 deletions src/encode/lzma2.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
#[cfg(feature = "no_std")]
use alloc::vec;
use byteorder::{BigEndian, WriteBytesExt};
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

pub fn encode_stream<R, W>(input: &mut R, output: &mut W) -> io::Result<()>
Expand Down
5 changes: 5 additions & 0 deletions src/encode/rangecoder.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
#[cfg(all(test, feature = "no_std"))]
use alloc::vec::Vec;
use byteorder::WriteBytesExt;
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

#[cfg(test)]
Expand Down
3 changes: 3 additions & 0 deletions src/encode/util.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

/// An [`io::Write`] computing a digest on the bytes written.
Expand Down
6 changes: 4 additions & 2 deletions src/encode/xz.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ use crate::encode::{lzma2, util};
use crate::xz::crc::CRC32;
use crate::xz::{footer, header, CheckMethod, StreamFlags};
use byteorder::{LittleEndian, WriteBytesExt};
use std::io;
use std::io::Write;
#[cfg(feature = "no_std")]
use core2::io::{self, Write};
#[cfg(not(feature = "no_std"))]
use std::io::{self, Write};

pub fn encode_stream<R, W>(input: &mut R, output: &mut W) -> io::Result<()>
where
Expand Down
30 changes: 20 additions & 10 deletions src/error.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,17 @@
//! Error handling.

use std::fmt::Display;
use std::{io, result};
#[cfg(feature = "no_std")]
use alloc::string::String;
#[cfg(feature = "no_std")]
use core::fmt::{self, Display};
#[cfg(feature = "no_std")]
use core::result;
#[cfg(feature = "no_std")]
use core2::{error, io};
#[cfg(not(feature = "no_std"))]
use std::fmt::{self, Display};
#[cfg(not(feature = "no_std"))]
use std::{error, io, result};

/// Library errors.
#[derive(Debug)]
Expand All @@ -26,7 +36,7 @@ impl From<io::Error> for Error {
}

impl Display for Error {
fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Error::IoError(e) => write!(fmt, "io error: {}", e),
Error::HeaderTooShort(e) => write!(fmt, "header too short: {}", e),
Expand All @@ -36,8 +46,8 @@ impl Display for Error {
}
}

impl std::error::Error for Error {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
impl error::Error for Error {
fn source(&self) -> Option<&(dyn error::Error + 'static)> {
match self {
Error::IoError(e) | Error::HeaderTooShort(e) => Some(e),
Error::LzmaError(_) | Error::XzError(_) => None,
Expand All @@ -48,15 +58,15 @@ impl std::error::Error for Error {
#[cfg(test)]
mod test {
use super::Error;
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

#[test]
fn test_display() {
assert_eq!(
Error::IoError(std::io::Error::new(
std::io::ErrorKind::Other,
"this is an error"
))
.to_string(),
Error::IoError(io::Error::new(io::ErrorKind::Other, "this is an error")).to_string(),
"io error: this is an error"
);
assert_eq!(
Expand Down
7 changes: 7 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
//! Pure-Rust codecs for LZMA, LZMA2, and XZ.
#![cfg_attr(docsrs, feature(doc_cfg, doc_cfg_hide))]
#![cfg_attr(no_std, feature(no_std))]
#![deny(missing_docs)]
#![deny(missing_debug_implementations)]
#![forbid(unsafe_code)]

#[cfg(feature = "no_std")]
extern crate alloc;

#[macro_use]
mod macros;

Expand All @@ -15,6 +19,9 @@ pub mod error;
mod util;
mod xz;

#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

/// Compression helpers.
Expand Down
5 changes: 5 additions & 0 deletions src/util/vec2d.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
#[cfg(feature = "no_std")]
use alloc::boxed::Box;
#[cfg(feature = "no_std")]
use core::ops::{Index, IndexMut};
#[cfg(not(feature = "no_std"))]
use std::ops::{Index, IndexMut};

/// A 2 dimensional matrix in row-major order backed by a contiguous slice.
Expand Down
6 changes: 5 additions & 1 deletion src/xz/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ use crate::error;
use crate::xz::crc::CRC32;
use crate::xz::StreamFlags;
use byteorder::{BigEndian, LittleEndian, ReadBytesExt};
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

/// File format magic header signature, see sect. 2.1.1.1.
pub(crate) const XZ_MAGIC: &[u8] = &[0xFD, 0x37, 0x7A, 0x58, 0x5A, 0x00];
Expand All @@ -19,7 +23,7 @@ impl StreamHeader {
/// Parse a Stream Header from a buffered reader.
pub(crate) fn parse<BR>(input: &mut BR) -> error::Result<Self>
where
BR: std::io::BufRead,
BR: io::BufRead,
{
if !util::read_tag(input, XZ_MAGIC)? {
return Err(error::Error::XzError(format!(
Expand Down
16 changes: 13 additions & 3 deletions src/xz/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
//! [spec]: https://tukaani.org/xz/xz-file-format.txt

use crate::error;
#[cfg(feature = "no_std")]
use core2::io;
#[cfg(not(feature = "no_std"))]
use std::io;

pub(crate) mod crc;
Expand Down Expand Up @@ -85,12 +88,19 @@ impl From<CheckMethod> for u8 {
mod test {
use super::*;
use byteorder::{BigEndian, ReadBytesExt};
use std::io::{Seek, SeekFrom};
#[cfg(feature = "no_std")]
use core::u8::MAX as U8_MAX;
#[cfg(feature = "no_std")]
use core2::io::{self, Seek, SeekFrom};
#[cfg(not(feature = "no_std"))]
use std::io::{self, Seek, SeekFrom};
#[cfg(not(feature = "no_std"))]
use std::u8::MAX as U8_MAX;

#[test]
fn test_checkmethod_roundtrip() {
let mut count_valid = 0;
for input in 0..std::u8::MAX {
for input in 0..U8_MAX {
if let Ok(check) = CheckMethod::try_from(input) {
let output: u8 = check.into();
assert_eq!(input, output);
Expand All @@ -106,7 +116,7 @@ mod test {
check_method: CheckMethod::Crc32,
};

let mut cursor = std::io::Cursor::new(vec![0u8; 2]);
let mut cursor = io::Cursor::new(vec![0u8; 2]);
let len = input.serialize(&mut cursor).unwrap();
assert_eq!(len, 2);

Expand Down