← 返回首页
bpo-1635741: Port hashlib modules to multiphase init (PEP 489) by koubaa · Pull Request #21818 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-1635741: Port hashlib modules to multiphase init (PEP 489)#21818

Merged
vstinner merged 15 commits into
python:masterfrom
koubaa:bpo-1635741-hashes
Sep 6, 2020
Merged

bpo-1635741: Port hashlib modules to multiphase init (PEP 489)#21818
vstinner merged 15 commits into
python:masterfrom
koubaa:bpo-1635741-hashes

Conversation

koubaa commented Aug 10, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

koubaa requested a review from tiran as a code owner August 10, 2020 21:12
koubaa changed the title Bpo 1635741 hashes bpo-1635741: Port hashlib modules to multiphase init (PEP 489) Aug 10, 2020
tiran self-assigned this Aug 11, 2020

koubaa commented Aug 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@shihai1991 @vstinner @corona10 FYI. The macOS test failure looks to be due to the following error which seems unrelated to my change:

module 'test.support' has no attribute 'EnvironmentVarGuard'

shihai1991 commented Aug 12, 2020

Copy link
Copy Markdown
Member

@shihai1991 @vstinner @corona10 FYI. The macOS test failure looks to be due to the following error which seems unrelated to my change:

module 'test.support' has no attribute 'EnvironmentVarGuard'

Hm, this failure have no relation with your PR. It's weird. Some failed test cases have updated: https://github.com/python/cpython/blob/master/Lib/test/test_selectors.py#L540

shihai1991 commented Aug 12, 2020

Copy link
Copy Markdown
Member

The failed test cases have been fixed in 490c542
Can you do some git rebase operation? MAYBE this operation can solve your probleam.

vstinner reviewed Aug 12, 2020
View reviewed changes
Comment thread Modules/_blake2/blake2b_impl.c Outdated Show resolved Hide resolved
koubaa force-pushed the bpo-1635741-hashes branch from 28c3f7b to 8491100 Compare August 12, 2020 23:55
vstinner requested changes Aug 13, 2020
View reviewed changes

vstinner 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 Hide comment

This PR is a little bit too large. Would you mind to restrict it to blake2 which is already complex enough?

Once this PR is merged, you can write a PR for sha3 which is complex. Then we can finish with remaining modules like md5 and sha1. Or publish all PR are once.

Comment thread Modules/_blake2/blake2b_impl.c Outdated Show resolved Hide resolved
Comment thread Modules/_blake2/blake2module.c Outdated Show resolved Hide resolved
Comment thread Modules/_blake2/blake2module.c Outdated Show resolved Hide resolved

bedevere-bot commented Aug 13, 2020

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

koubaa commented Aug 13, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner that's not a problem - I put each file in its own commit so I can easily submit separate PRs. I'll submit one for sha3, one for blake2, and one with the remaining ones. After breaking up and creating PRs for each I'll address your comments

koubaa force-pushed the bpo-1635741-hashes branch from 5ddff05 to fddf3ce Compare August 13, 2020 12:22

koubaa commented Aug 13, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner

Thanks for the review!

vstinner reviewed Aug 13, 2020
View reviewed changes
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha512module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha512module.c Outdated Show resolved Hide resolved

koubaa commented Aug 13, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner I'm going on vacation so I'll come back to address you comment next week.

koubaa force-pushed the bpo-1635741-hashes branch from fddf3ce to 617660d Compare August 21, 2020 23:43

koubaa commented Aug 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The MacOS/Ubuntu CI has these test failures:

3 tests failed:
test_hashlib test_random test_statistics

Running locally (windows) those tests pass:

./PCBuild/amd64/python.exe -m test test_statistics test_hashlib test_random
0:00:00 Run tests sequentially
0:00:00 [1/3] test_statistics
0:00:08 load avg: 0.00 [2/3] test_hashlib
0:00:09 load avg: 0.00 [3/3] test_random
== Tests result: SUCCESS ==

I rebased on the latest master so I'm not sure what could be causing it.

**** EDIT ****
This was due to a use-after free which is now fixed.

vstinner reviewed Aug 28, 2020
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2020-08-10-16-11-32.bpo-1635741.O0d3ym.rst Outdated Show resolved Hide resolved
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha1module.c Outdated Show resolved Hide resolved
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha512module.c Outdated Show resolved Hide resolved
vstinner reviewed Aug 29, 2020
View reviewed changes
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
25 hidden items Load more…
koubaa force-pushed the bpo-1635741-hashes branch from 02ffcf6 to 4bcb57f Compare August 31, 2020 21:15
vstinner reviewed Sep 1, 2020
View reviewed changes
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
Comment thread Misc/NEWS.d/next/Core and Builtins/2020-08-10-16-11-32.bpo-1635741.O0d3ym.rst Outdated Show resolved Hide resolved
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha1module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha1module.c Outdated Show resolved Hide resolved
1 hidden conversation Load more…
Comment thread Modules/sha512module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha512module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha512module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha512module.c Outdated Show resolved Hide resolved
Comment thread Modules/sha512module.c Outdated Show resolved Hide resolved
vstinner reviewed Sep 2, 2020
View reviewed changes
Comment thread Modules/md5module.c Outdated Show resolved Hide resolved
vstinner approved these changes Sep 4, 2020
View reviewed changes

vstinner 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 Hide comment

LGTM. Thanks for the multiple updates. These PR wasn't easy!

Comment thread Modules/sha512module.c

if (Py_IS_TYPE((PyObject*)self, &SHA512type)) {
if ( (newobj = newSHA512object())==NULL)
if (Py_IS_TYPE((PyObject*)self, st->sha512_type)) {

vstinner Sep 4, 2020

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 Hide comment

Without defining_class, this test was wrong. So thanks for using defining_class :-)

vstinner approved these changes Sep 4, 2020
View reviewed changes

vstinner 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 Hide comment

LGTM, but please fix the NEWS entry.

Comment thread Misc/NEWS.d/next/Core and Builtins/2020-08-10-16-11-32.bpo-1635741.O0d3ym.rst Outdated Show resolved Hide resolved
vstinner merged commit 63f102f into python:master Sep 6, 2020

vstinner commented Sep 6, 2020

Copy link
Copy Markdown
Member

Merged, thanks.

shihai1991 added a commit to shihai1991/cpython that referenced this pull request Sep 10, 2020
Merge remote-tracking branch 'origin/master' into bpo_1635741_1 …
00b300c
* origin/master: (1373 commits) bpo-1635741: Port mashal module to multi-phase init (python#22149) bpo-1635741: Port _string module to multi-phase init (pythonGH-22148) bpo-1635741: Convert _sha256 types to heap types (pythonGH-22134) bpo-1635741: Port the termios to multi-phase init (PEP 489) (pythonGH-22139) bpo-41732: add iterator to memoryview (pythonGH-22119) bpo-40744: Drop support for SQLite pre 3.7.3 (pythonGH-20909) bpo-41316: Make tarfile follow specs for FNAME (pythonGH-21511) bpo-41720: Add "return NotImplemented" in turtle.Vec2D.__rmul__(). (pythonGH-22092) bpo-1635741 port _curses_panel to multi-phase init (PEP 489) (pythonGH-21986) bpo-1635741: Port _overlapped module to multi-phase init (pythonGH-22051) bpo-1635741: Port _opcode module to multi-phase init (PEP 489) (pythonGH-22050) bpo-1635741 port zlib module to multi-phase init (pythonGH-21995) [doc] Add link to Generic in typing (pythonGH-22125) bpo-41513: Expand comments and add references for a better understanding (pythonGH-22123) bpo-1635741: Port _sha1, _sha512, _md5 to multiphase init (pythonGH-21818) closes bpo-41723: Fix an error in the py_compile documentation. (pythonGH-22110) [doc] Fix padding in some typing definitions (pythonGH-22114) Fix documented Python version for venv --upgrade-deps (pythonGH-22113) bpo-40318: Migrate to SQLite3 trace v2 API (pythonGH-19581) bpo-41687: Fix sendfile implementation to work with Solaris (python#22040) ...
koubaa deleted the bpo-1635741-hashes branch September 11, 2020 02:24
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
bpo-1635741: Port _sha1, _sha512, _md5 to multiphase init (pythonGH-2… …
5644536
…1818) Port the _sha1, _sha512, and _md5 extension modules to multi-phase initialization API (PEP 489).
kylotan mannequin mentioned this pull request Sep 19, 2022
Closed
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes
tiran Awaiting requested review from tiran

Assignees

tiran

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Footer

© 2026 GitHub, Inc.