GeezerVison

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

GeezerVison is showing github.com — bpo-1635741: Port hashlib modules to multiphase init (PEP 489) by koubaa · Pull Request #21818 · python/cpython

Conversation

koubaa commented Aug 10, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

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

All reactions

koubaa requested a review from tiran as a code owner

August 10, 2020 21:12

the-knights-who-say-ni added the CLA signed label

Aug 10, 2020

bedevere-bot added the awaiting review label

Aug 10, 2020

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

Uh oh!

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

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'

All reactions

Sorry, something went wrong.

Uh oh!

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

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner reviewed

Aug 12, 2020

View reviewed changes

Comment thread

Modules/_blake2/blake2b_impl.c Resolved

Uh oh!

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

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

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/_blake2/blake2b_impl.c Resolved

Uh oh!

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

Comment thread

Modules/_blake2/blake2module.c Resolved

Uh oh!

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

Comment thread

Modules/_blake2/blake2module.c Resolved

Uh oh!

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

bedevere-bot added awaiting changes and removed awaiting review labels

Aug 13, 2020

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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

blake 2: bpo-1635741: port blake2 module to multi-phase init #21856
sha3: bpo-1635741: port sha3 module to multi-phase init #21855
everything else: I kept in this PR

Thanks for the review!

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner reviewed

Aug 13, 2020

View reviewed changes

Comment thread

Modules/md5module.c Resolved

Uh oh!

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

Comment thread

Modules/md5module.c Resolved

Uh oh!

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

Comment thread

Modules/md5module.c Resolved

Uh oh!

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

Comment thread

Modules/sha512module.c Resolved

Uh oh!

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

Comment thread

Modules/sha512module.c Resolved

Uh oh!

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

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

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

Uh oh!

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

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner reviewed

Aug 28, 2020

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

vstinner reviewed

Aug 29, 2020

View reviewed changes

Comment thread

Modules/md5module.c Resolved

Uh oh!

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

koubaa added 7 commits

August 31, 2020 16:15


          port sha1

4ae1a3f


          port sha512

b389566


          port md5

8add6b1


          blurb

3ff18f7


          address comments


          remove unneeded comment

e0c0feb


          fix use-after-free

6c7f435

koubaa added 4 commits

August 31, 2020 16:15


          add traverse/clear/free module methods

37e2282


          resolve comments

0384e6b


          pep7

2cfcb12


          fix dealloc function

4bcb57f

koubaa force-pushed the bpo-1635741-hashes branch from 02ffcf6 to 4bcb57f Compare

August 31, 2020 21:15

vstinner reviewed

Sep 1, 2020

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


          update based on PR review

3cd5d6e

vstinner reviewed

Sep 2, 2020

View reviewed changes

Comment thread

Modules/md5module.c Resolved

Uh oh!

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

koubaa added 2 commits

September 2, 2020 18:01


          use defining_class

0244bcf


          remove comment

b66a965

vstinner approved these changes

Sep 4, 2020

View reviewed changes

vstinner left a comment

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

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

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added awaiting merge and removed awaiting changes labels

Sep 4, 2020

vstinner approved these changes

Sep 4, 2020

View reviewed changes

vstinner left a comment

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.

LGTM, but please fix the NEWS entry.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Misc/NEWS.d/next/Core and Builtins/2020-08-10-16-11-32.bpo-1635741.O0d3ym.rst Resolved

Uh oh!

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


          fix readme

df69769

vstinner merged commit 63f102f into python:master

Sep 6, 2020

bedevere-bot removed the awaiting merge label

Sep 6, 2020

vstinner commented Sep 6, 2020

Copy link

Copy Markdown

Member

Merged, thanks.

All reactions

Sorry, something went wrong.

Uh oh!

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

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…

…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

Py_Finalize() doesn't clear all Python objects at exit #44470

Closed

Reviewers

vstinner

vstinner approved these changes

tiran

Awaiting requested review from tiran

Uh oh!

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

Assignees

tiran

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.

6 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.

GeezerVison

Conversation

koubaa commented Aug 10, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koubaa commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shihai1991 commented Aug 12, 2020

Uh oh!

shihai1991 commented Aug 12, 2020

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Aug 13, 2020

Uh oh!

koubaa commented Aug 13, 2020

Uh oh!

koubaa commented Aug 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

koubaa commented Aug 13, 2020

Uh oh!

koubaa commented Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Sep 6, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

koubaa commented Aug 10, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

koubaa commented Aug 12, 2020 •

edited

Loading

Uh oh!

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

koubaa commented Aug 22, 2020 •

edited

Loading

Uh oh!

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