GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Switch to libz-rs-sys for zlib implementation by coolreader18 · Pull Request #5562 · RustPython/RustPython

Conversation

@coolreader18

@coolreader18 coolreader18 commented Feb 26, 2025

Copy link
Copy Markdown
Member

This is a rust-only reimplementation of the libz-sys api, which I discovered from this blog post. It can build without any C toolchain, which makes it a good pick for us when we're targeting windows and wasm, and the performance isn't anything to scoff at (see blog post). Also, the zlib feature now enables the entire zlib module, which I feel like just makes more sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@youknowone

youknowone commented Feb 26, 2025

Copy link
Copy Markdown
Member

if it is a pure rust implementation, do we still need a zlib feature?
All reactions

Sorry, something went wrong.

Uh oh!

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

coolreader18 commented Feb 26, 2025

Copy link
Copy Markdown
Member Author

It's a reasonably heavy dependency (~500KB), so I figured it might be nice to still have the option to turn it off.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@coolreader18
coolreader18 force-pushed the zlib-rs branch from f69087f to 62bf9ae Compare February 26, 2025 02:45
@youknowone

youknowone commented Feb 26, 2025

Copy link
Copy Markdown
Member

We don't suggest feature for every library which can be a redundant dependencies. If the binary size matters, turning off the entire stdlib feature and consist their own well-selected (or we can suggest a few pre-selected) set of libraries will make more sense.
All reactions

Sorry, something went wrong.

Uh oh!

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

coolreader18 commented Feb 26, 2025

Copy link
Copy Markdown
Member Author

Fair enough.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@coolreader18
coolreader18 force-pushed the zlib-rs branch 2 times, most recently from 6e740d9 to 3887f7c Compare February 26, 2025 02:55
@coolreader18
Switch to libz-rs-sys for zlib implementation
00b8559
@coolreader18
coolreader18 force-pushed the zlib-rs branch from 3887f7c to 00b8559 Compare February 26, 2025 02:57
@youknowone

youknowone commented Feb 26, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Probably we can remove zlib feature from rustpython crate but add a lot of features for each library in rustpython-stdlib as a user interface to support easy library on/off. (not a scope of this pr)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
youknowone
youknowone approved these changes Feb 26, 2025
View reviewed changes
@coolreader18
coolreader18 merged commit 4468dcb into RustPython:main Feb 26, 2025
@coolreader18
coolreader18 deleted the zlib-rs branch February 26, 2025 05:19
@arihant2math arihant2math mentioned this pull request Mar 13, 2025
Fix bz2 module #5605
Closed
@youknowone youknowone mentioned this pull request Mar 31, 2025
zlib not found after install with --features ssl #5157
Closed
@coolreader18 coolreader18 mentioned this pull request Apr 17, 2025
Switch to libbz2-rs-sys and finish bz2 impl #5709
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone 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.

2 participants

@coolreader18 @youknowone
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 01402520 — 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.