From ab79efb1283b00c9c138e891695729a3b7e09b1b Mon Sep 17 00:00:00 2001 From: Jiangeng Dong Date: Fri, 6 Oct 2023 16:02:50 -0700 Subject: [PATCH 1/5] feat: make mcap-rs wasm compatible --- .github/workflows/ci.yml | 3 ++ rust/Cargo.toml | 18 +++++++---- .../common/conformance_writer_spec.rs | 20 ++++++------- rust/examples/conformance_writer.rs | 8 ++--- rust/src/io_utils.rs | 4 +++ rust/src/lib.rs | 1 + rust/src/read.rs | 8 ++++- rust/src/records.rs | 1 + rust/src/write.rs | 30 ++++++++++--------- rust/tests/compression.rs | 1 + 10 files changed, 59 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4e792d602b..07f846c3cb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -475,6 +475,9 @@ jobs: components: "rustfmt, clippy" - run: cargo fmt --all -- --check - run: cargo clippy -- --no-deps + - run: cargo clippy --no-default-features -- --no-deps + - run: cargo clippy --no-default-features --features lz4 -- --no-deps + - run: cargo clippy --no-default-features --features zstd -- --no-deps - run: cargo build - run: cargo test - name: "publish to crates.io" diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 46b2f0fde5..a3c7386a4f 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -2,8 +2,8 @@ name = "mcap" description = "A library for reading and writing MCAP files" homepage = "https://mcap.dev" -keywords = [ "foxglove", "mcap" ] -categories = [ "science::robotics", "compression" ] +keywords = ["foxglove", "mcap"] +categories = ["science::robotics", "compression"] repository = "https://github.com/foxglove/mcap" documentation = "https://docs.rs/mcap" readme = "README.md" @@ -19,21 +19,29 @@ byteorder = "1.4" crc32fast = "1.3" enumset = "1.0.11" log = "0.4" -lz4 = "1.0" num_cpus = "1.13" paste = "1.0" thiserror = "1.0" +lz4_flex = { version = "0.11.1", optional = true } + +[target.'cfg(target_arch = "wasm32")'.dependencies] +zstd = { version = "0.11", default-features = false, features = [ + "wasm", +], optional = true } + +[target.'cfg(not(target_arch = "wasm32"))'.dependencies] zstd = { version = "0.11", features = ["zstdmt"], optional = true } [features] -default = ["zstd"] +default = ["zstd", "lz4"] zstd = ["dep:zstd"] +lz4 = ["dep:lz4_flex"] [dev-dependencies] anyhow = "1" atty = "0.2" camino = "1.0" -clap = { version = "3.2", features = ["derive"]} +clap = { version = "3.2", features = ["derive"] } itertools = "0.10" memmap = "0.7" rayon = "1.5" diff --git a/rust/examples/common/conformance_writer_spec.rs b/rust/examples/common/conformance_writer_spec.rs index 1d03309f52..f9afe22314 100644 --- a/rust/examples/common/conformance_writer_spec.rs +++ b/rust/examples/common/conformance_writer_spec.rs @@ -20,7 +20,7 @@ pub struct Record { } impl Record { - pub fn get_field(self: &Self, name: &str) -> &Value { + pub fn get_field(&self, name: &str) -> &Value { return &self .fields .iter() @@ -29,19 +29,19 @@ impl Record { .1; } - pub fn get_field_data(self: &Self, name: &str) -> Vec { + pub fn get_field_data(&self, name: &str) -> Vec { let data: Vec = self .get_field(name) .as_array() .unwrap_or_else(|| panic!("Invalid: {}", name)) - .into_iter() + .iter() .filter_map(|v| v.as_u64()) .filter_map(|n| u8::try_from(n).ok()) .collect(); - return data; + data } - pub fn get_field_meta(self: &Self, name: &str) -> BTreeMap { + pub fn get_field_meta(&self, name: &str) -> BTreeMap { let data = self .get_field(name) .as_object() @@ -50,17 +50,17 @@ impl Record { for (key, value) in data.iter() { result.insert(key.to_string(), value.as_str().unwrap().to_string()); } - return result; + result } - pub fn get_field_str(self: &Self, name: &str) -> &str { + pub fn get_field_str(&self, name: &str) -> &str { return self .get_field(name) .as_str() .unwrap_or_else(|| panic!("Invalid: {}", name)); } - pub fn get_field_u16(self: &Self, name: &str) -> u16 { + pub fn get_field_u16(&self, name: &str) -> u16 { return self .get_field(name) .as_str() @@ -68,7 +68,7 @@ impl Record { .unwrap_or_else(|| panic!("Invalid: {}", name)); } - pub fn get_field_u32(self: &Self, name: &str) -> u32 { + pub fn get_field_u32(&self, name: &str) -> u32 { return self .get_field(name) .as_str() @@ -76,7 +76,7 @@ impl Record { .unwrap_or_else(|| panic!("Invalid: {}", name)); } - pub fn get_field_u64(self: &Self, name: &str) -> u64 { + pub fn get_field_u64(&self, name: &str) -> u64 { return self .get_field(name) .as_str() diff --git a/rust/examples/conformance_writer.rs b/rust/examples/conformance_writer.rs index 9716f64dd5..39bffe8ae3 100644 --- a/rust/examples/conformance_writer.rs +++ b/rust/examples/conformance_writer.rs @@ -55,9 +55,7 @@ fn write_file(spec: &conformance_writer_spec::WriterSpec) { } "DataEnd" => { let data_section_crc = record.get_field_u32("data_section_crc"); - let _data_end = mcap::records::DataEnd { - data_section_crc: data_section_crc, - }; + let _data_end = mcap::records::DataEnd { data_section_crc }; // write data end } "Footer" => { @@ -105,7 +103,7 @@ fn write_file(spec: &conformance_writer_spec::WriterSpec) { let name = record.get_field_str("name"); let encoding = record.get_field_str("encoding"); let id = record.get_field_u64("id"); - let data: Vec = record.get_field_data(&"data"); + let data: Vec = record.get_field_data("data"); let schema = mcap::Schema { name: name.to_owned(), encoding: encoding.to_owned(), @@ -128,7 +126,7 @@ fn write_file(spec: &conformance_writer_spec::WriterSpec) { let contents = std::fs::read(tmp_path).expect("Couldn't read output"); std::io::stdout() - .write(&contents) + .write_all(&contents) .expect("Couldn't write output"); } diff --git a/rust/src/io_utils.rs b/rust/src/io_utils.rs index 4453af26fe..5bb6fcaf15 100644 --- a/rust/src/io_utils.rs +++ b/rust/src/io_utils.rs @@ -10,6 +10,10 @@ pub struct CountingCrcReader { } impl CountingCrcReader { + /// Creates a new `CountingCrcReader` with the given reader. + /// + /// This is not used when both `lz4` and `zstd` features are enabled. + #[allow(dead_code)] pub fn new(inner: R) -> Self { Self { inner, diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 37cb5a8ef4..9b2eb1b1c7 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -139,6 +139,7 @@ pub const MAGIC: &[u8] = &[0x89, b'M', b'C', b'A', b'P', 0x30, b'\r', b'\n']; pub enum Compression { #[cfg(feature = "zstd")] Zstd, + #[cfg(feature = "lz4")] Lz4, } diff --git a/rust/src/read.rs b/rust/src/read.rs index 689375e5a3..414c062b21 100644 --- a/rust/src/read.rs +++ b/rust/src/read.rs @@ -254,6 +254,8 @@ fn read_record(op: u8, body: &[u8]) -> McapResult> { enum ChunkDecompressor<'a> { Null(LinearReader<'a>), + /// This is not used when both `zstd` and `lz4` features are disabled. + #[allow(dead_code)] Compressed(Option>>), } @@ -274,10 +276,14 @@ impl<'a> ChunkReader<'a> { #[cfg(not(feature = "zstd"))] "zstd" => panic!("Unsupported compression format: zstd"), + #[cfg(feature = "lz4")] "lz4" => ChunkDecompressor::Compressed(Some(CountingCrcReader::new(Box::new( - lz4::Decoder::new(data)?, + lz4_flex::frame::FrameDecoder::new(data), )))), + #[cfg(not(feature = "lz4"))] + "lz4" => panic!("Unsupported compression format: lz4"), + "" => { if header.uncompressed_size != header.compressed_size { warn!( diff --git a/rust/src/records.rs b/rust/src/records.rs index 6520d8ffbc..eeb842c495 100644 --- a/rust/src/records.rs +++ b/rust/src/records.rs @@ -136,6 +136,7 @@ fn parse_vec = ()>>() -> BinResult> { Ok(parsed) } +#[allow(clippy::ptr_arg)] #[binrw::writer(writer, endian)] fn write_vec = ()>>(v: &Vec) -> BinResult<()> { use std::io::SeekFrom; diff --git a/rust/src/write.rs b/rust/src/write.rs index 67eaba4ac0..886c0ae953 100644 --- a/rust/src/write.rs +++ b/rust/src/write.rs @@ -151,10 +151,7 @@ impl WriteOptions { /// If `None`, chunks will not be automatically closed and the user must call `flush()` to /// begin a new chunk. pub fn chunk_size(self, chunk_size: Option) -> Self { - Self { - chunk_size: chunk_size, - ..self - } + Self { chunk_size, ..self } } /// Creates a [`Writer`] whch writes to `w` using the given options @@ -649,7 +646,8 @@ enum Compressor { Null(W), #[cfg(feature = "zstd")] Zstd(zstd::Encoder<'static, W>), - Lz4(lz4::Encoder), + #[cfg(feature = "lz4")] + Lz4(lz4_flex::frame::FrameEncoder), } impl Compressor { @@ -658,11 +656,8 @@ impl Compressor { Compressor::Null(w) => w, #[cfg(feature = "zstd")] Compressor::Zstd(w) => w.finish()?, - Compressor::Lz4(w) => { - let (w, err) = w.finish(); - err?; - w - } + #[cfg(feature = "lz4")] + Compressor::Lz4(w) => w.finish()?, }) } } @@ -673,6 +668,7 @@ impl Write for Compressor { Compressor::Null(w) => w.write(buf), #[cfg(feature = "zstd")] Compressor::Zstd(w) => w.write(buf), + #[cfg(feature = "lz4")] Compressor::Lz4(w) => w.write(buf), } } @@ -682,6 +678,7 @@ impl Write for Compressor { Compressor::Null(w) => w.flush(), #[cfg(feature = "zstd")] Compressor::Zstd(w) => w.flush(), + #[cfg(feature = "lz4")] Compressor::Lz4(w) => w.flush(), } } @@ -706,7 +703,10 @@ impl ChunkWriter { let compression_name = match compression { #[cfg(feature = "zstd")] Some(Compression::Zstd) => "zstd", + #[cfg(feature = "lz4")] Some(Compression::Lz4) => "lz4", + #[cfg(not(any(feature = "zstd", feature = "lz4")))] + Some(_) => unreachable!("`Compression` is an empty enum that cannot be instantiated"), None => "", }; @@ -726,14 +726,16 @@ impl ChunkWriter { let compressor = match compression { #[cfg(feature = "zstd")] Some(Compression::Zstd) => { + #[allow(unused_mut)] let mut enc = zstd::Encoder::new(writer, 0)?; + #[cfg(not(target_arch = "wasm32"))] enc.multithread(num_cpus::get_physical() as u32)?; Compressor::Zstd(enc) } - Some(Compression::Lz4) => { - let b = lz4::EncoderBuilder::new(); - Compressor::Lz4(b.build(writer)?) - } + #[cfg(feature = "lz4")] + Some(Compression::Lz4) => Compressor::Lz4(lz4_flex::frame::FrameEncoder::new(writer)), + #[cfg(not(any(feature = "zstd", feature = "lz4")))] + Some(_) => unreachable!("`Compression` is an empty enum that cannot be instantiated"), None => Compressor::Null(writer), }; let compressor = CountingCrcWriter::new(compressor); diff --git a/rust/tests/compression.rs b/rust/tests/compression.rs index 204da75b31..a0f66a6df6 100644 --- a/rust/tests/compression.rs +++ b/rust/tests/compression.rs @@ -49,6 +49,7 @@ fn zstd_round_trip() -> Result<()> { round_trip(Some(mcap::Compression::Zstd)) } +#[cfg(feature = "lz4")] #[test] fn lz4_round_trip() -> Result<()> { round_trip(Some(mcap::Compression::Lz4)) From 753023f354b47f6a0ae13a7c641fa491fee2f34d Mon Sep 17 00:00:00 2001 From: Jiangeng Dong Date: Fri, 6 Oct 2023 16:29:53 -0700 Subject: [PATCH 2/5] fix: typo --- rust/src/io_utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/io_utils.rs b/rust/src/io_utils.rs index 5bb6fcaf15..ab874718e5 100644 --- a/rust/src/io_utils.rs +++ b/rust/src/io_utils.rs @@ -12,7 +12,7 @@ pub struct CountingCrcReader { impl CountingCrcReader { /// Creates a new `CountingCrcReader` with the given reader. /// - /// This is not used when both `lz4` and `zstd` features are enabled. + /// This is not used when both `lz4` and `zstd` features are disabled. #[allow(dead_code)] pub fn new(inner: R) -> Self { Self { From b35ce27f1d982287c0347022f1c3ab1d5000b680 Mon Sep 17 00:00:00 2001 From: JiangengDong Date: Thu, 26 Oct 2023 21:16:36 -0700 Subject: [PATCH 3/5] wip: no need to turn off default feautures --- rust/Cargo.toml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rust/Cargo.toml b/rust/Cargo.toml index a3c7386a4f..76fdd354fb 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -25,9 +25,7 @@ thiserror = "1.0" lz4_flex = { version = "0.11.1", optional = true } [target.'cfg(target_arch = "wasm32")'.dependencies] -zstd = { version = "0.11", default-features = false, features = [ - "wasm", -], optional = true } +zstd = { version = "0.11", features = ["wasm"], optional = true } [target.'cfg(not(target_arch = "wasm32"))'.dependencies] zstd = { version = "0.11", features = ["zstdmt"], optional = true } From 8ae2c0b90b3d3a02a02bdd8f94da4882cf91d8ba Mon Sep 17 00:00:00 2001 From: JiangengDong Date: Mon, 30 Oct 2023 21:24:15 -0700 Subject: [PATCH 4/5] wip: revert irrelevant change --- rust/Cargo.toml | 6 +++--- rust/examples/common/conformance_writer_spec.rs | 16 ++++++++-------- rust/examples/conformance_writer.rs | 8 +++++--- rust/src/write.rs | 5 ++++- 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 76fdd354fb..bfe81be99e 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -2,8 +2,8 @@ name = "mcap" description = "A library for reading and writing MCAP files" homepage = "https://mcap.dev" -keywords = ["foxglove", "mcap"] -categories = ["science::robotics", "compression"] +keywords = [ "foxglove", "mcap" ] +categories = [ "science::robotics", "compression" ] repository = "https://github.com/foxglove/mcap" documentation = "https://docs.rs/mcap" readme = "README.md" @@ -39,7 +39,7 @@ lz4 = ["dep:lz4_flex"] anyhow = "1" atty = "0.2" camino = "1.0" -clap = { version = "3.2", features = ["derive"] } +clap = { version = "3.2", features = ["derive"]} itertools = "0.10" memmap = "0.7" rayon = "1.5" diff --git a/rust/examples/common/conformance_writer_spec.rs b/rust/examples/common/conformance_writer_spec.rs index f9afe22314..a984ba625d 100644 --- a/rust/examples/common/conformance_writer_spec.rs +++ b/rust/examples/common/conformance_writer_spec.rs @@ -20,7 +20,7 @@ pub struct Record { } impl Record { - pub fn get_field(&self, name: &str) -> &Value { + pub fn get_field(self: &Self, name: &str) -> &Value { return &self .fields .iter() @@ -29,19 +29,19 @@ impl Record { .1; } - pub fn get_field_data(&self, name: &str) -> Vec { + pub fn get_field_data(self: &Self, name: &str) -> Vec { let data: Vec = self .get_field(name) .as_array() .unwrap_or_else(|| panic!("Invalid: {}", name)) - .iter() + .into_iter() .filter_map(|v| v.as_u64()) .filter_map(|n| u8::try_from(n).ok()) .collect(); - data + return data; } - pub fn get_field_meta(&self, name: &str) -> BTreeMap { + pub fn get_field_meta(self: &Self, name: &str) -> BTreeMap { let data = self .get_field(name) .as_object() @@ -53,14 +53,14 @@ impl Record { result } - pub fn get_field_str(&self, name: &str) -> &str { + pub fn get_field_str(self: &Self, name: &str) -> &str { return self .get_field(name) .as_str() .unwrap_or_else(|| panic!("Invalid: {}", name)); } - pub fn get_field_u16(&self, name: &str) -> u16 { + pub fn get_field_u16(self: &Self, name: &str) -> u16 { return self .get_field(name) .as_str() @@ -68,7 +68,7 @@ impl Record { .unwrap_or_else(|| panic!("Invalid: {}", name)); } - pub fn get_field_u32(&self, name: &str) -> u32 { + pub fn get_field_u32(self: &Self, name: &str) -> u32 { return self .get_field(name) .as_str() diff --git a/rust/examples/conformance_writer.rs b/rust/examples/conformance_writer.rs index 39bffe8ae3..9716f64dd5 100644 --- a/rust/examples/conformance_writer.rs +++ b/rust/examples/conformance_writer.rs @@ -55,7 +55,9 @@ fn write_file(spec: &conformance_writer_spec::WriterSpec) { } "DataEnd" => { let data_section_crc = record.get_field_u32("data_section_crc"); - let _data_end = mcap::records::DataEnd { data_section_crc }; + let _data_end = mcap::records::DataEnd { + data_section_crc: data_section_crc, + }; // write data end } "Footer" => { @@ -103,7 +105,7 @@ fn write_file(spec: &conformance_writer_spec::WriterSpec) { let name = record.get_field_str("name"); let encoding = record.get_field_str("encoding"); let id = record.get_field_u64("id"); - let data: Vec = record.get_field_data("data"); + let data: Vec = record.get_field_data(&"data"); let schema = mcap::Schema { name: name.to_owned(), encoding: encoding.to_owned(), @@ -126,7 +128,7 @@ fn write_file(spec: &conformance_writer_spec::WriterSpec) { let contents = std::fs::read(tmp_path).expect("Couldn't read output"); std::io::stdout() - .write_all(&contents) + .write(&contents) .expect("Couldn't write output"); } diff --git a/rust/src/write.rs b/rust/src/write.rs index 886c0ae953..4d3c44d33b 100644 --- a/rust/src/write.rs +++ b/rust/src/write.rs @@ -151,7 +151,10 @@ impl WriteOptions { /// If `None`, chunks will not be automatically closed and the user must call `flush()` to /// begin a new chunk. pub fn chunk_size(self, chunk_size: Option) -> Self { - Self { chunk_size, ..self } + Self { + chunk_size: chunk_size, + ..self + } } /// Creates a [`Writer`] whch writes to `w` using the given options From c9caff63c08f16e0839592ed574483537f21bde9 Mon Sep 17 00:00:00 2001 From: JiangengDong Date: Mon, 30 Oct 2023 21:25:54 -0700 Subject: [PATCH 5/5] wip: revert more irrelevant change --- rust/examples/common/conformance_writer_spec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/examples/common/conformance_writer_spec.rs b/rust/examples/common/conformance_writer_spec.rs index a984ba625d..1d03309f52 100644 --- a/rust/examples/common/conformance_writer_spec.rs +++ b/rust/examples/common/conformance_writer_spec.rs @@ -50,7 +50,7 @@ impl Record { for (key, value) in data.iter() { result.insert(key.to_string(), value.as_str().unwrap().to_string()); } - result + return result; } pub fn get_field_str(self: &Self, name: &str) -> &str { @@ -76,7 +76,7 @@ impl Record { .unwrap_or_else(|| panic!("Invalid: {}", name)); } - pub fn get_field_u64(&self, name: &str) -> u64 { + pub fn get_field_u64(self: &Self, name: &str) -> u64 { return self .get_field(name) .as_str()