GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-99108: Make vectorized versions of Blake2 available on x86, too by msprotz · Pull Request #125244 · python/cpython

Conversation

@msprotz

@msprotz msprotz commented Oct 10, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Per #99108 (comment): trying a quick fix to see if this builds.

@gpshead can you buildbot this for Debian x86? Thanks.

If it works, I'll patch & fix upstream, then pull it in accordingly into CPython. Thank you!

  • Issue: Replace built-in hashlib with verified implementations from HACL* #99108

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@msprotz
Try for Debian x86
091fd62
@msprotz
msprotz requested a review from gpshead as a code owner October 10, 2024 10:00
@bedevere-app bedevere-app Bot mentioned this pull request Oct 10, 2024
Replace built-in hashlib with verified implementations from HACL* #99108
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Oct 10, 2024
@msprotz

msprotz commented Oct 10, 2024

Copy link
Copy Markdown
Contributor Author

(The SBOM build will fail for the time being.)
All reactions

Sorry, something went wrong.

Uh oh!

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

encukou commented Oct 10, 2024

Copy link
Copy Markdown
Member

!buildbot x86
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Oct 10, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @encukou for commit 091fd62 🤖

The command will test the builders whose names match following regular expression: x86

The builders matched are:

  • x86-64 MacOS Intel NoGIL PR
  • x86-64 macOS PR
  • x86 Debian Installed with X PR
  • x86-64 MacOS Intel ASAN NoGIL PR
  • x86 Debian Non-Debug with X PR
All reactions

Sorry, something went wrong.

Uh oh!

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

msprotz commented Oct 10, 2024

Copy link
Copy Markdown
Contributor Author

Looks like this compiles but gives an unrelated error in tests. @gpshead @encukou do you agree? If so, I'll fix it upstream
All reactions

Sorry, something went wrong.

Uh oh!

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

encukou commented Oct 10, 2024

Copy link
Copy Markdown
Member

Yup, I agree!
I'll need to fix that ctypes error now that it's uncovered :)
gpshead reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msprotz msprotz mentioned this pull request Oct 11, 2024
Make Intel intrinsincs available even on x86 hacl-star/hacl-star#991
Merged
@zware zware mentioned this pull request Oct 15, 2024
Python fails to build on x86 Debian Non-Debug with X 3.x: error on Hacl_Hash_Blake2s_Simd128.c #125535
Closed
@vstinner

vstinner commented Oct 15, 2024

Copy link
Copy Markdown
Member

buildbot/x86 Debian Installed with X PR — Build done.
buildbot/x86 Debian Non-Debug with X PR — Build done.

Tests failed on these two workers, but it's a known issue: #121938

This PR fix the build on these two workers: #125535
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner added the skip news label Oct 15, 2024
vstinner
vstinner approved these changes Oct 15, 2024
View reviewed changes

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Oct 15, 2024
@vstinner
Run make regen-sbom
53d2b8d
@vstinner
vstinner requested a review from sethmlarson as a code owner October 15, 2024 15:48
@vstinner

vstinner commented Oct 15, 2024

Copy link
Copy Markdown
Member

(The SBOM build will fail for the time being.)

I ran make regen-sbom.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner enabled auto-merge (squash) October 15, 2024 15:50
@vstinner

vstinner commented Oct 15, 2024

Copy link
Copy Markdown
Member

If it works, I'll patch & fix upstream, then pull it in accordingly into CPython. Thank you!

Yeah, please fix upstream ;-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou encukou added the DO-NOT-MERGE label Oct 15, 2024
@msprotz

msprotz commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

The fix just got merged a couple hours ago upstream, I need to run a build to refresh the correct folder, and then I'll push the refresh here (along with make regen-sbom). Thanks!
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead disabled auto-merge October 15, 2024 21:42
@vstinner

vstinner commented Oct 16, 2024

Copy link
Copy Markdown
Member

@gpshead @encukou: Why did you block merging this PR? Do you prefer to wait until upstream is fixed, and then update Hacl in Python?
All reactions

Sorry, something went wrong.

Uh oh!

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

encukou commented Oct 16, 2024

Copy link
Copy Markdown
Member

Yes.
vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
msprotz added 2 commits October 16, 2024 13:16
@msprotz
Refresh HACL*
a77f318
@msprotz
Regen SBOM
acd04e6
@msprotz

msprotz commented Oct 16, 2024

Copy link
Copy Markdown
Contributor Author

Ok this is pushed, let's see what CI says. @vstinner FYI, this would be an SBOM violation to push a local fix to an external vendored library, so this is why we have to propagate the fix upstream first... thanks!
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

vstinner commented Oct 16, 2024

Copy link
Copy Markdown
Member

FYI, this would be an SBOM violation to push a local fix to an external vendored library

I didn't know that. We made many changes in mimalloc vendored copy.
All reactions

Sorry, something went wrong.

Uh oh!

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

msprotz commented Oct 16, 2024

Copy link
Copy Markdown
Contributor Author

Maybe mimalloc gets a different treatment? The SBOM CI target fails if the vendored copy of HACL* is not exactly identical to upstream...
All reactions

Sorry, something went wrong.

Uh oh!

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

gpshead commented Oct 16, 2024

Copy link
Copy Markdown
Member

For this one we set it up to be upstream-first and just track specific upstream versions. it works since we've got active maintainers. :)
vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware zware removed the DO-NOT-MERGE label Oct 16, 2024
@zware

zware commented Oct 16, 2024

Copy link
Copy Markdown
Member

!buildbot x86 Deb
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Oct 16, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @zware for commit acd04e6 🤖

The command will test the builders whose names match following regular expression: x86 Deb

The builders matched are:

  • x86 Debian Non-Debug with X PR
  • x86 Debian Installed with X PR
All reactions

Sorry, something went wrong.

Uh oh!

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

zware commented Oct 17, 2024

Copy link
Copy Markdown
Member

This fixes the build issue on x86 Debian. Not sure what's up with the CI job checking the SBOM, though. I can't reproduce that failure locally (cc @sethmlarson for sbom insight).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware
Merge branch 'main' into protz_hacl_refresh
1fc3671
zware
zware reviewed Oct 17, 2024
View reviewed changes
Comment thread Misc/sbom.spdx.json Resolved

Uh oh!

There was an error while loading. Please reload this page.
befeleme pushed a commit to fedora-python/cpython that referenced this pull request Oct 17, 2024
@msprotz @befeleme
00441: Make vectorized versions of Blake2 available on x86, too
6c1be08 
Source: python#125244
@vstinner

vstinner commented Oct 17, 2024

Copy link
Copy Markdown
Member

@msprotz: Can you re-run make regen-sbom?
All reactions

Sorry, something went wrong.

Uh oh!

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

msprotz commented Oct 17, 2024

Copy link
Copy Markdown
Contributor Author

I thought I had run it. Let me run it again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msprotz @zware
Update Misc/sbom.spdx.json
aedc1df 
Co-authored-by: Zachary Ware <[email protected]>
@msprotz

msprotz commented Oct 17, 2024

Copy link
Copy Markdown
Contributor Author

I committed the suggested change, but make regen-sbom generates no diff on my machine still. Let's see if this fixes CI
All reactions

Sorry, something went wrong.

Uh oh!

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

zware commented Oct 17, 2024

Copy link
Copy Markdown
Member

I could have left a clearer note, sorry. It turned out that the generated files check was failing on regen-sbom because Tools/build/generate_sbom.py behaves differently if CI is defined in the environment. In that case, it actually does the download and regenerates the checksum.
All reactions

Sorry, something went wrong.

Uh oh!

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

msprotz commented Oct 17, 2024

Copy link
Copy Markdown
Contributor Author

Ah ok! Good to know for next time, thank you.
zware reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware
zware enabled auto-merge (squash) October 17, 2024 14:52
@zware
zware merged commit 528bbab into python:main Oct 17, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 17, 2024
@tianon tianon mentioned this pull request Oct 21, 2024
Remove i386 from 3.14 (for now) docker-library/python#985
Merged
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@msprotz @vstinner
@zware @ebonnal
pythonGH-99108: Make vectorized versions of Blake2 available on x86, …
006b0c9 
…too (python#125244)

Accomplished by updating HACL* vendored code from hacl-star/hacl-star@a6a0949 to hacl-star/hacl-star@315a9e4

Co-authored-by: Victor Stinner <[email protected]>
Co-authored-by: Zachary Ware <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zware zware zware left review comments
@vstinner vstinner vstinner approved these changes
@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner
@sethmlarson sethmlarson Awaiting requested review from sethmlarson sethmlarson is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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

@msprotz @encukou @bedevere-bot @vstinner @gpshead @zware
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.