fix: building in zkvm guest #457
Conversation
CodSpeed Performance ReportMerging #457 will not alter performanceComparing Summary
|
|
|
||
| use zstd::Decoder; | ||
| #[cfg(feature = "zstd")] | ||
| use ruzstd as _; |
There was a problem hiding this comment.
ruzstd is only used for decoding? I guess it should be fully compatible with the C impl, right?
There was a problem hiding this comment.
also curious about this?
dogeos using codec crate in guest, zstd won't compile on such target.
don't get this, why don't put decompress_blob_data at dogeos side
There was a problem hiding this comment.
they also uses other things provided by scroll-codec, so two options:
- provide option to use rust zstd
- gate v2::zstd, and make zstd as optional dep.
There was a problem hiding this comment.
- gate v2::zstd, and make zstd as optional dep.
Does that mean that they don't actually need decompress_blob_data? Otherwise, without zstd, how would it work?
| let mut decoder = StreamingDecoder::new(header_data.as_slice()).unwrap(); | ||
| // heuristic: use data length as the allocated output capacity. | ||
| let mut output = Vec::with_capacity(header_data.len()); | ||
| decoder.read_to_end(&mut output).unwrap(); |
There was a problem hiding this comment.
I don't see why it's safe to use unwrap here. Are you sure this cannot fail? If not, we should return Result instead
There was a problem hiding this comment.
ok, but previous zstd call also unwrap and ignore errors, is there a such assumption that data is always valid?
There was a problem hiding this comment.
returning Result will change the API, it's a breaking change, ok for me, but ok for others?
There was a problem hiding this comment.
I believe this function is only used inside rollup-node, no?
There was a problem hiding this comment.
I think it's fine to update the API.
dogeos using codec crate in guest, zstd won't compile on such target.