Skip to content

gh-91246: Implement PEP 775: Make zlib required on platforms other than wasi #130297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 14 commits into from
Closed

gh-91246: Implement PEP 775: Make zlib required on platforms other than wasi #130297

wants to merge 14 commits into from

Conversation

StanFromIreland
Copy link
Contributor

@StanFromIreland StanFromIreland commented Feb 19, 2025 •
edited by hugovk
Loading

According to Petr's plan:

You need zlib to install pip, as both the wheel and sdist are compressed. The problem is not 300 extra lines of code. It’s that practically, for most users, CPython without zlib is broken.
But,

  • zlib-less builds of CPython might still be useful in some special cases.
  • Requiring zlib for WASI is not (and should not be) a priority. I haven’t looked at Android and iOS; this discussion might predate their support.

Let’s make that official. I propose to change our messaging, not (much) code.

Here’s a concrete proposal. Where I say WASI, Android/iOS (or any other platform) can be added if needed.

The 3.14 What’s New should say:

zlib is now officially required to build CPython.
It is still technically possible to build CPython without it for special needs, like bootstrapping. Such builds are not supported, but we can accept pull requests to keep them working.
As an exception, zlib is not required on WASI.

Lib/test/test_zlib.py should add the following comment, and a zlib-less build should fail with an error message that points to it. This serves as a deprecation warning. (On WASI the tests should instead continue to be skipped.)

Building CPython without zlib is not supported.
Anyone who wants build CPython this way should be prepared to patch it, but the core team may help getting those patches to the main branch (as that’s the place where multiple third parties can cooperate).

For tests to pass without zlib, this file needs to be removed.

Existing tests skips (e.g. @requires_zlib()) stay in (except in test_zlib.py itself, as above).
binascii’s reimplementation of crc32 is removed; corresponding tests are skipped.
Code that only generates nice error messages in zlib-less builds is removed. Corresponding tests are adjusted.

PEP 11 gets a new entry:

  • Name: Systems without zlib, except WASI
  • Unsupported in: Python 3.14
  • Code removed in: Unknown (see Lib/test/test_zlib.py for details)

configure is changed to produce a warning on systems without zlib, except WASI.

Note that the WASI exception means that we’ll still test some zlib-less build. That’s a good thing.
Getting zlib there is the opposite of a blocker issue: I’d prefer that to happen well after this change.

However binascii's re-implementation of crc32 cannot be removed till WASI supports zlib.

📚 Documentation preview 📚: https://cpython-previews--130297.org.readthedocs.build/

@bedevere-app bedevere-app bot mentioned this pull request Feb 19, 2025
Closed
@StanFromIreland StanFromIreland marked this pull request as draft February 19, 2025 10:44
@StanFromIreland StanFromIreland mentioned this pull request Feb 19, 2025
Closed
4 tasks
@StanFromIreland StanFromIreland marked this pull request as ready for review February 19, 2025 11:37
@StanFromIreland StanFromIreland requested a review from a team as a code owner February 19, 2025 11:37
@StanFromIreland StanFromIreland mentioned this pull request Feb 19, 2025
Closed
@StanFromIreland StanFromIreland changed the title gh-91246: Make zlib required to build python gh-91246: Make zlib required to build python on platforms other than WASI Feb 19, 2025
@hugovk hugovk mentioned this pull request Feb 19, 2025
Closed
@StanFromIreland @gpshead
Remove fancy error messages for zlib import errors
57d3520 
Co-authored-by: Gregory P. Smith <greg@krypto.org>
encukou
encukou reviewed Feb 19, 2025
View reviewed changes
encukou
encukou reviewed Feb 20, 2025
View reviewed changes
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 20, 2025
encukou
encukou reviewed Feb 20, 2025
View reviewed changes
StanFromIreland and others added 2 commits February 20, 2025 13:08
@StanFromIreland @encukou
Apply suggestions from Petr
c62bf24 
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@StanFromIreland
Update configure
4300d9e
@encukou
Copy link
Member

encukou commented Feb 21, 2025 •
edited
Loading

Please add zlib to build requirements. The version notes from zlib should be added moved there.
ZLIB_LIBS should mention zlib, not gzip.

Please also adjust docs for shutil.get_archive_formats, shutil.make_archive, and any others you find. Add .. versionchanged:: next notes.

AA-Turner
AA-Turner reviewed Feb 23, 2025
View reviewed changes
AA-Turner
AA-Turner reviewed Feb 23, 2025
View reviewed changes
AA-Turner
AA-Turner reviewed Feb 23, 2025
View reviewed changes
AA-Turner
AA-Turner reviewed Feb 23, 2025
View reviewed changes
Lib/test/test_zlib.py
@@ -7,9 +7,22 @@
import random
import sys
from test.support import bigmemtest, _1G, _4G, is_s390x
import importlib
Copy link
Member

Choose a reason for hiding this comment

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

Keep imports sorted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are all technically "Standard library imports"...

Copy link
Member

Choose a reason for hiding this comment

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

The comment was about alphabetically sorted, your reply about grouped by type 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry but where in PEP 8 is that specified, I cannot seem to find any mention of it?

Copy link
Member

Choose a reason for hiding this comment

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

It’s a very common practice. But another one is to always import os and sys at the top.
Yet another is to sort by module length to get a nice pyramid drawing 📐

AA-Turner
AA-Turner reviewed Feb 23, 2025
View reviewed changes
vpoulailleau
vpoulailleau reviewed Feb 25, 2025
View reviewed changes
Doc/whatsnew/3.14.rst
@@ -1310,6 +1310,10 @@ Build changes
* GNU Autoconf 2.72 is now required to generate :file:`configure`.
(Contributed by Erlend Aasland in :gh:`115765`.)

* zlib is now required to build CPytho on all platforms except WASI.
Copy link

Choose a reason for hiding this comment

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

Suggested change
* zlib is now required to build CPytho on all platforms except WASI.
* zlib is now required to build CPython on all platforms except WASI.

@StanFromIreland
Copy link
Contributor Author

PEP 775 was withdrawn.

@StanFromIreland StanFromIreland changed the title gh-91246: Make zlib required to build python on platforms other than WASI gh-91246: Implement PEP 775 May 9, 2025
@merwok
Copy link
Member

merwok commented May 9, 2025

@StanFromIreland could you keep the previous title and add the mention of the PEP if you want?

@StanFromIreland StanFromIreland deleted the enfore-zlib branch May 9, 2025 19:39
@StanFromIreland StanFromIreland changed the title gh-91246: Implement PEP 775 gh-91246: Implement PEP 775: Make zlib required on platforms other than wasi May 9, 2025
@StanFromIreland
Copy link
Contributor Author

@merwok Is this sufficient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@encukou encukou encukou left review comments

@merwok merwok merwok left review comments

@AA-Turner AA-Turner AA-Turner left review comments

@vpoulailleau vpoulailleau vpoulailleau left review comments

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

@ethanfurman ethanfurman Awaiting requested review from ethanfurman ethanfurman is a code owner

@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner

@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

@pfmoore pfmoore Awaiting requested review from pfmoore

@pradyunsg pradyunsg Awaiting requested review from pradyunsg

@gaogaotiantian gaogaotiantian Awaiting requested review from gaogaotiantian

@pablogsal pablogsal Awaiting requested review from pablogsal

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

Assignees
No one assigned
Labels
awaiting review 🔨 test-with-buildbots Test PR w/ buildbots; report in status section
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@StanFromIreland @encukou @merwok @AA-Turner @vpoulailleau