Enable most writer conformance tests for Rust #1313
Conversation
Muon
requested review from
james-rms,
bennetthardwick and
jtbandes
as code owners
| schemas: Default::default(), | ||
| channels: Default::default(), | ||
| next_channel_id: 0, | ||
| next_channel_id: 1, |
There was a problem hiding this comment.
why is this change neccessary?
There was a problem hiding this comment.
Good question. I was going to say something about the other writers starting from 1, but they don't. Actually, it's because the Rust interface tries to be clever and doesn't let you pick your own channel IDs, but the tests start from 1. If the tests started from a different number things would break.
Changing add_channel() to let you pick your own would be a breaking change. Or we could add a new method?
There was a problem hiding this comment.
You can choose your own channel IDs if you call write(Message{}) and set the channel ID in the Message.channel.id field.
There was a problem hiding this comment.
The problem is not writing the Message record, but the Channel record. We can't currently choose the ID when creating a Channel, and Channels may be created even if there are no messages.
There was a problem hiding this comment.
you don't need to call add_channel before calling Write(Message{channel: Some(Channel{...})). This will write the channel record with the ID you specify, then the message record.
There was a problem hiding this comment.
You can avoid writing a channel record when you see a Channel object in the spec JSON, instead storing it in a map somewhere. Then you call Write(Message) when the message comes up in the spec.
| @@ -672,7 +793,7 @@ impl<W: Write + Seek> Writer<W> { | |||
| const WHERE_WRITER: &'static str = "Trying to write a record on a finished MCAP"; | |||
|
|
|||
| /// Starts a new chunk if we haven't done so already. | |||
| fn chunkin_time(&mut self) -> McapResult<&mut ChunkWriter<W>> { | |||
| fn start_chunk(&mut self) -> McapResult<&mut ChunkWriter<W>> { | |||
There was a problem hiding this comment.
I am a killjoy who prioritizes code readability.
There was a problem hiding this comment.
I felt the itch to remove but ultimately felt the love when I saw this last time 🥹. Had to be done though.
rust/src/write.rs
Outdated
| @@ -175,6 +205,87 @@ impl WriteOptions { | |||
| self | |||
| } | |||
|
|
|||
| /// Specifies whether the writer should output statistics automatically. | |||
There was a problem hiding this comment.
I don't understand the use of "automatically" - there's not a manual alternative. Maybe:
| /// Specifies whether the writer should output statistics automatically. | |
| /// Specifies whether the writer should output a statistics record in the summary section. |
rust/src/write.rs
Outdated
| self | ||
| } | ||
|
|
||
| /// Specifies whether the writer should output a summary record automatically. |
There was a problem hiding this comment.
There is no summary record, there is a summary section (the records between the data end record and the footer record) which may be empty. https://mcap.dev/spec#summary-section
rust/src/write.rs
Outdated
| self | ||
| } | ||
|
|
||
| /// Specifies whether the writer should output summary offsets automatically. |
There was a problem hiding this comment.
The use of "automatically" here and throughout confuses me. the caller is not choosing between automatic and manual, they're choosing between present and absent.
|
I'll fix up the docstrings. |
Emit is always a verb, whereas output can be a noun or a verb.
Changelog
The DataEnd CRC calculation and missing writer options have been added to allow the Rust writer to pass the conformance tests.
Docs
https://linear.app/foxglove/issue/FG-9567/rust-mcap-writer-doesnt-pass-or-run-conformance-tests
Description
Previously, the Rust writer could not pass the conformance tests because it was not possible to control which summary record types would be included and because it would not compute the CRC for the DataEnd record.
Now, all current writer conformance tests pass, except the ones using the "pad" option. This brings the Rust crate up to parity with the C++ library (and possibly others).
No writer conformance tests run or pass
All writer conformance tests except those with "pad" pass
Fixes: FG-9567