GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Switch to `libbz2-rs-sys` and finish bz2 impl by coolreader18 · Pull Request #5709 · RustPython/RustPython

Conversation

coolreader18 commented Apr 17, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

fanninpm commented Apr 17, 2025

Copy link

Copy Markdown

Contributor

Uh oh!

There was an error while loading. Please reload this page.

arihant2math commented Apr 17, 2025

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone commented Apr 18, 2025

Copy link

Copy Markdown

Member

please check #5605 and rebase on it or pick some idea if possible

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

coolreader18 commented Apr 18, 2025

Copy link

Copy Markdown

Member Author

Oh, shoot, sorry - I didn't see that that PR was open.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arihant2math requested changes

Apr 18, 2025

View reviewed changes

Comment thread

stdlib/src/bz2.rs

		object::{PyPayload, PyResult},
		types::Constructor,
		};
		use crate::zlib::{

arihant2math Apr 18, 2025

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coolreader18 Apr 19, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arihant2math reacted with thumbs up emoji

All reactions

👍 1 reaction

coolreader18 force-pushed the bz2 branch from f9863e5 to 165743c Compare

April 19, 2025 03:58

arihant2math approved these changes

Apr 19, 2025

View reviewed changes

youknowone reviewed

Apr 19, 2025

View reviewed changes

Comment thread

Lib/test/test_bz2.py

		self.assertEqual(f.read(), "foobar")

		# TODO: RUSTPYTHON
		@unittest.expectedFailure

youknowone Apr 19, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coolreader18 @arihant2math what cause this regression? could this be fixed in future?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

arihant2math Apr 19, 2025

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coolreader18 Apr 20, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone reacted with thumbs up emoji

All reactions

👍 1 reaction

youknowone Apr 20, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

arihant2math mentioned this pull request

Apr 19, 2025

lzma implementation #5717

Merged

arihant2math commented Apr 20, 2025

Copy link

Copy Markdown

Contributor

I checkout out of this pr for #5717 so try not to change the trait too much 😄 .

coolreader18 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

coolreader18 and others added 3 commits

April 19, 2025 23:13


          Use libbz2-rs-sys for bzip2 implementation

e640487

Co-authored-by: Ashwin Naren <[email protected]>


          Remove bz2 feature

e377e43


          Finish _bz2 implementation

f0d46bf

coolreader18 force-pushed the bz2 branch from 165743c to f0d46bf Compare

April 20, 2025 04:13

youknowone approved these changes

Apr 20, 2025

View reviewed changes

Comment thread

stdlib/src/zlib.rs

		}
		impl<'a> Chunker<'a> {
		fn new(data: &'a [u8]) -> Self {
		pub(crate) fn new(data: &'a [u8]) -> Self {

youknowone Apr 20, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

	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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coolreader18 Apr 20, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coolreader18 merged commit fbaeecc into RustPython:main

Apr 20, 2025

coolreader18 deleted the bz2 branch

April 20, 2025 22:53

coolreader18 mentioned this pull request

Apr 21, 2025

Split out common compression routines into separate file #5728

Merged

Reviewers

arihant2math

arihant2math approved these changes

youknowone

youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

coolreader18 commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanninpm commented Apr 17, 2025

Uh oh!

arihant2math commented Apr 17, 2025

Uh oh!

youknowone commented Apr 18, 2025

Uh oh!

coolreader18 commented Apr 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arihant2math commented Apr 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coolreader18 commented Apr 17, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.