Switch to `libbz2-rs-sys` and finish bz2 impl by coolreader18 · Pull Request #5709 · RustPython/RustPython
Conversation
I realized that libbz2-rs-sys exists, by the same folks who make libz-rs-sys - it's a Rust reimplementation of libbz2. This means we can do that same thing as in #5562, and avoid having to cross-compile C. I then realized after I removed the feature flag that we weren't actually running bz2 regrtests in CI, because we weren't passing it as a feature to cargo build, and so something like half of the tests in test_bz2.py were failing. So, I more or less finished up the impl, by making some code in zlib.rs generic over the specific Decompress struct.
This one is better than mine ... test_shutil is the only fail (due to 1 new exposed test) and there aren't too many expected failures comparatively. I'll close mine after this is merged.
please check #5605 and rebase on it or pick some idea if possible
Oh, shoot, sorry - I didn't see that that PR was open.
| object::{PyPayload, PyResult}, | ||
| types::Constructor, | ||
| }; | ||
| use crate::zlib::{ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is at-least 1 other decompression module that uses the same format (_lzma). I think this should be moved someplace common.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - I was planning on doing that in a followup, to reduce the amount of code movement in this pr.
| self.assertEqual(f.read(), "foobar") | ||
|
|
||
| # TODO: RUSTPYTHON | ||
| @unittest.expectedFailure |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A flag probably needs to be added to the decompressor state. I believe the issue is the wt and rt formats. Although I'm not to sure, I suppose looking at the cpython source might help.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this needed to be flagged now is because the whole test file wasn't running at all before - bz2 was not included in the --features=stdlib,threading,... flag in CI, so the module wasn't even getting compiled or tested at all.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, didn't noticed that. Thanks!
I checkout out of this pr for #5717 so try not to change the trait too much 😄 .
Co-authored-by: Ashwin Naren <arihant2math@gmail.com>
| } | ||
| impl<'a> Chunker<'a> { | ||
| fn new(data: &'a [u8]) -> Self { | ||
| pub(crate) fn new(data: &'a [u8]) -> Self { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub(crate) fn new(data: &'a [u8]) -> Self { | |
| pub fn new(data: &'a [u8]) -> Self { |
When the struct is pub(crate), only pub here is automatically pub(crate)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I think I'll change that in the follow-up.