Switch to libbz2-rs-sys and finish bz2 impl - #5709
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
please check #5605 and rebase on it or pick some idea if possible |
Sorry, something went wrong.
|
Oh, shoot, sorry - I didn't see that that PR was open. |
Sorry, something went wrong.
| object::{PyPayload, PyResult}, | ||
| types::Constructor, | ||
| }; | ||
| use crate::zlib::{ |
There was a problem hiding this comment.
Since there is at-least 1 other decompression module that uses the same format (_lzma). I think this should be moved someplace common.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree - I was planning on doing that in a followup, to reduce the amount of code movement in this pr.
Sorry, something went wrong.
| self.assertEqual(f.read(), "foobar") | ||
|
|
||
| # TODO: RUSTPYTHON | ||
| @unittest.expectedFailure |
There was a problem hiding this comment.
@coolreader18 @arihant2math what cause this regression? could this be fixed in future?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, didn't noticed that. Thanks!
Sorry, something went wrong.
|
I checkout out of this pr for #5717 so try not to change the trait too much 😄 . |
Sorry, something went wrong.
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 { |
There was a problem hiding this comment.
| 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)
Sorry, something went wrong.
There was a problem hiding this comment.
True, I think I'll change that in the follow-up.
Sorry, something went wrong.
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.