Skip to content
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

GH-99108: Make vectorized versions of Blake2 available on x86, too #125244

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

msprotz
Copy link
Contributor

@msprotz msprotz commented Oct 10, 2024

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!

@msprotz
Copy link
Contributor Author

msprotz commented Oct 10, 2024

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

@encukou
Copy link
Member

encukou commented Oct 10, 2024

!buildbot x86

@bedevere-bot
Copy link

🤖 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

@msprotz
Copy link
Contributor Author

msprotz commented Oct 10, 2024

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

@encukou
Copy link
Member

encukou commented Oct 10, 2024

Yup, I agree!
I'll need to fix that ctypes error now that it's uncovered :)

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

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner
Copy link
Member

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

I ran make regen-sbom.

@vstinner vstinner enabled auto-merge (squash) October 15, 2024 15:50
@vstinner
Copy link
Member

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

Yeah, please fix upstream ;-)

@msprotz
Copy link
Contributor Author

msprotz commented Oct 15, 2024

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 gpshead disabled auto-merge October 15, 2024 21:42
@vstinner
Copy link
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?

@encukou
Copy link
Member

encukou commented Oct 16, 2024

Yes.

@msprotz
Copy link
Contributor Author

msprotz commented Oct 16, 2024

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!

@vstinner
Copy link
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.

@msprotz
Copy link
Contributor Author

msprotz commented Oct 16, 2024

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

@gpshead
Copy link
Member

gpshead commented Oct 16, 2024

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. :)

@zware
Copy link
Member

zware commented Oct 16, 2024

!buildbot x86 Deb

@bedevere-bot
Copy link

🤖 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

@zware
Copy link
Member

zware commented Oct 17, 2024

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

Misc/sbom.spdx.json Outdated Show resolved Hide resolved
befeleme pushed a commit to fedora-python/cpython that referenced this pull request Oct 17, 2024
@vstinner
Copy link
Member

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

@msprotz
Copy link
Contributor Author

msprotz commented Oct 17, 2024

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

Co-authored-by: Zachary Ware <[email protected]>
@msprotz
Copy link
Contributor Author

msprotz commented Oct 17, 2024

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

@zware
Copy link
Member

zware commented Oct 17, 2024

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.

@msprotz
Copy link
Contributor Author

msprotz commented Oct 17, 2024

Ah ok! Good to know for next time, thank you.

@zware zware enabled auto-merge (squash) October 17, 2024 14:52
@zware zware merged commit 528bbab into python:main Oct 17, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants