Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Mar 28, 2018

Updating the module following-up recent changes

BLAKE2/libb2@620681a

https://bugs.python.org/issue33164

@ghost ghost changed the title Blake2 dependency update bpo33164: Blake2 dependency update Mar 28, 2018
@ghost ghost changed the title bpo33164: Blake2 dependency update bpo-33164: Blake2 dependency update Mar 28, 2018
@ghost
Copy link
Author

ghost commented Jun 5, 2018

Had been fixed the windows build issue (had been pushed upstream too).

Copy link
Member

Choose a reason for hiding this comment

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

Where these HAVE_* macros come from?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I regenerated configure.ac/configure in the last commit.

@methane
Copy link
Member

methane commented Jun 6, 2018

Please revert aclocal.m4 unless it's mandatory.

@methane methane requested a review from tiran June 6, 2018 23:02
@methane
Copy link
Member

methane commented Jun 6, 2018

I want more verbose NEWS entry (e.g. "Updated blake2 implementation which uses secure memset implementation provided by platform.")

@methane methane added the type-security A security issue label Jun 6, 2018
@ghost
Copy link
Author

ghost commented Aug 16, 2018

Any update ? :)

@tiran
Copy link
Member

tiran commented Aug 16, 2018

One builder is failing. Please rebase your PR and push again. This should trigger a rebuild.

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

The code in Modules/_blake2/impl is a copy of the reference implementation from https://github.com/BLAKE2. Please get your patch into the reference implementation first.

@bedevere-bot
Copy link

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.

@tiran tiran removed the type-security A security issue label Aug 16, 2018
@ghost
Copy link
Author

ghost commented Aug 17, 2018

Module code updated.

@csabella csabella requested review from methane and tiran May 17, 2019 23:48
@csabella
Copy link
Contributor

@dcarlier-afilias, please rebase to fix merge conflicts. Thanks!

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@ghost
Copy link
Author

ghost commented May 20, 2019

Any chance it lands for 3.8 or will it be 3.9 (at best) ? thx.

@csabella
Copy link
Contributor

@dcarlier-afilias, this is a massive change, so my guess is that it would be difficult to land it in 3.8. I had flagged @tiran and @methane for review and asked for the rebase just to begin the process.

The code in Modules/_blake2/impl is a copy of the reference implementation from https://github.com/BLAKE2. Please get your patch into the reference implementation first.

I assume you took care of @tiran's original review comment? My guess is that you did based on your followup comment, but I just want to make sure.

@ghost
Copy link
Author

ghost commented May 21, 2019

I pushed upstream what I deemed necessary within a year indeed. I hope it is ok.

@csabella
Copy link
Contributor

@dcarlier-afilias, looks like I was wrong about it landing in 3.8. 🙂 Thank you for the contribution! @methane, thank you for the review and the merge! 🎉 🎉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants