FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-112346: Bugfix: Remove faster codepath from gzip.compress as it introduces behavioral inconsistencies by rhpvorderman · Pull Request #114116 · python/cpython · GitHub

/ cpython Public

gh-112346: Bugfix: Remove faster codepath from gzip.compress as it introduces behavioral inconsistencies - #114116

Closed
rhpvorderman wants to merge 8 commits into
python:mainfrom
rhpvorderman:gh-112346
Closed

gh-112346: Bugfix: Remove faster codepath from gzip.compress as it introduces behavioral inconsistencies#114116
rhpvorderman wants to merge 8 commits into
python:mainfrom
rhpvorderman:gh-112346

Conversation

rhpvorderman commented Jan 16, 2024
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

As mentioned in the linked issue, delegating to zlib.compress causes behavorial changes in the OS byte. As a result this change caused reproducible build failures. The separate codepath caused issues before as well:
#90425

Therefore the codepath should be removed. Speed ain't everything. The mention of zlib.compress in the documentation is maintained so users who not have specific requirements can use the speedier path.

This change needs to be backported to 3.11 and 3.12


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

rhpvorderman changed the title gh-112346: Remove faster codepath from gzip.compress as it introduces behavioral inconsistencies gh-112346: Bugfix: Remove faster codepath from gzip.compress as it introduces behavioral inconsistencies Jan 16, 2024

Copy link
Copy Markdown
Contributor Author

So is there any procedure for News entries for bugfixes that need to be backported to older versions of CPython?

serhiy-storchaka self-requested a review January 18, 2024 08:13

iii-i commented Feb 28, 2024

Copy link
Copy Markdown
Contributor

I would suggest the following fixup to make this work on BE (mtime is always LE, regardless of the host endianness):

--- a/Lib/test/test_gzip.py
+++ b/Lib/test/test_gzip.py
@@ -725,7 +725,7 @@ def test_issue112346(self):
         for mtime in (0, 42):
             with self.subTest(mtime=mtime):
                 compress = gzip.compress(data1, compresslevel=1, mtime=mtime)
-                assert struct.unpack("IxB", compress[4:10]) == (mtime, 255)
+                assert struct.unpack("<IxB", compress[4:10]) == (mtime, 255)
 
     def test_decompress(self):
         for data in (data1, data2):

Copy link
Copy Markdown
Contributor Author

Thanks for the suggestion @iii-i . That was an oversight on my part. I am going to pin in the hope this fix gets noticed.

Copy link
Copy Markdown
Contributor Author

ping

Copy link
Copy Markdown
Contributor Author

Ping @gpshead . Could you shine your light on this? It's only a very small change and it has remained under the radar for almost half a year now.

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, except using assert in the test.

Comment thread Lib/test/test_gzip.py Outdated
for mtime in (0, 42):
with self.subTest(mtime=mtime):
compress = gzip.compress(data1, compresslevel=1, mtime=mtime)
assert struct.unpack("<IxB", compress[4:10]) == (mtime, 255)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Use TestCase assertion methods instead of the assert statement.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done!

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing this so quickly @serhiy-storchaka . I added a news entry.

gpshead added needs backport to 3.13 bugs and security fixes needs backport to 3.12 only security fixes labels Jun 12, 2024

gpshead commented Jun 12, 2024

Copy link
Copy Markdown
Member

I suggest not making this change given how long it has been the behavior of released Pythons. We cannot backport this to 3.11 as it is not a security issue. And anyone picking a change like this up in a patch release is just as likely to run into problems with the behavior change between differing Python versions.

gpshead marked this pull request as draft June 12, 2024 21:59

Copy link
Copy Markdown
Contributor Author

@gpshead That makes sense. Sorry, I went into panic mode when code changes I made gave someone some reproducibility troubles.
I do hope that the two separate code paths do not cause any more in the future.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL