[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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.

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

bpo-33164: Blake2 dependency update #6286

Merged
merged 7 commits into from May 23, 2019
Merged

bpo-33164: Blake2 dependency update #6286

merged 7 commits into from May 23, 2019

Conversation

Copy link

ghost commented Mar 28, 2018

Updating the module following-up recent changes

BLAKE2/libb2@620681a

https://bugs.python.org/issue33164

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

ghost commented Jun 5, 2018

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

memset_s(v, n, 0, n);
#elif defined(HAVE_EXPLICIT_BZERO)
explicit_bzero(v, n);
#elif defined(HAVE_EXPLICIT_MEMSET)
Copy link
Member

methane Jun 6, 2018

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

ghost Jun 6, 2018

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.

Copy link
Member

methane commented Jun 6, 2018

Please revert aclocal.m4 unless it's mandatory.

methane requested a review from tiran Jun 6, 2018
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 added the type-security A security issue label Jun 6, 2018
Copy link
Author

ghost commented Aug 16, 2018

Any update ? :)

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 left a comment

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.

Copy link

bedevere-bot commented Aug 16, 2018

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 removed the type-security A security issue label Aug 16, 2018
Copy link
Author

ghost commented Aug 17, 2018

Module code updated.

csabella requested review from tiran and methane May 17, 2019
Copy link
Contributor

csabella commented May 17, 2019

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

Copy link

the-knights-who-say-ni commented May 20, 2019

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!

Copy link
Author

ghost commented May 20, 2019

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

Copy link
Contributor

csabella commented May 21, 2019

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

Copy link
Author

ghost commented May 21, 2019

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

Copy link
Contributor

csabella commented May 23, 2019

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

None yet

5 participants