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-116116: Backport PR #42 to fix building with clang-cl on windows-i686 #116117

Merged
merged 4 commits into from Mar 4, 2024

Conversation

georgthegreat
Copy link
Contributor

@georgthegreat georgthegreat commented Feb 29, 2024

Backport PR #42 to work around the following compilation error:

In file included from $(SOURCE_ROOT)/python3/src/Modules/_blake2/blake2b_impl.c:30:
$(SOURCE_ROOT)/python3/src/Modules/_blake2/impl/blake2b.c(31,23): error: conflicting types for '_mm_set_epi64x'
static inline __m128i _mm_set_epi64x( const uint64_t u1, const uint64_t u0 )
                      ^
$(TOOL_ROOT)/clang/14.0.6/include/emmintrin.h(3613,1): note: previous definition is here
_mm_set_epi64x(long long __q1, long long __q0)
^
1 error generated.

@hugovk hugovk added OS-windows 3.12 bugs and security fixes 3.13 new features, bugs and security fixes and removed 3.12 bugs and security fixes 3.13 new features, bugs and security fixes labels Feb 29, 2024
@zooba
Copy link
Member

zooba commented Feb 29, 2024

Seems fine to me. The generated files are the SBOM containing the hashes of these files.

@sethmlarson What's the way we should handle this kind of in-repo patching? Is regenerating the SBOM enough? Or do we need to track the before/after state as well? (Apologies for the incorrect tag if you see this, other-Seth)

@sethmlarson
Copy link
Contributor

@zooba I think it's fine that we make minor in-source changes, we run the risk of losing them when the dependency is updated so we should try to upstream the changes too. In a perfect world we'd be marking our SBOM with precise patches/descendants relationships, but I'm not sure how important that is for our primary use-case which is vulnerability management/discovery.

@zooba
Copy link
Member

zooba commented Feb 29, 2024

Okay, so this one is a backport from upstream so we won't lose it. We just need the SBOM to be regenerated to be able to merge the PR. I believe there's an open PR adding that functionality to the Windows build, so we'll probably have to wait for that to be merged before this can be updated.

@georgthegreat
Copy link
Contributor Author

@zooba, I have updated sbom files as requested

@georgthegreat
Copy link
Contributor Author

@zooba, let's merge it.

@gpshead gpshead self-assigned this Mar 4, 2024
@gpshead gpshead added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Mar 4, 2024
@zooba zooba merged commit 9b9e819 into python:main Mar 4, 2024
38 checks passed
@miss-islington-app
Copy link

Thanks @georgthegreat for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 4, 2024
… on windows-i686 (pythonGH-116117)

(cherry picked from commit 9b9e819)

Co-authored-by: Yuriy Chernyshov <thegeorg@yandex-team.com>
@miss-islington-app
Copy link

Sorry, @georgthegreat and @zooba, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 9b9e819b5116302cb4e471763feb2764eb17dde8 3.11

@miss-islington-app miss-islington-app bot assigned zooba and unassigned gpshead Mar 4, 2024
@bedevere-app
Copy link

bedevere-app bot commented Mar 4, 2024

GH-116315 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Mar 4, 2024
zooba pushed a commit that referenced this pull request Mar 4, 2024
…ndows-i686 (GH-116117)

(cherry picked from commit 9b9e819)

Co-authored-by: Yuriy Chernyshov <thegeorg@yandex-team.com>
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@zooba
Copy link
Member

zooba commented Mar 5, 2024

@georgthegreat Is this needed for 3.11? Could you prepare a PR for that branch? I suspect it's the SBOM conflicting, but maybe it's not needed for an older version of blake2.

@georgthegreat
Copy link
Contributor Author

georgthegreat commented Mar 5, 2024

We do not need it in our codebase as we have switched to 3.12, so it is up to you to decide.
Do you consider this bug critical?

@georgthegreat georgthegreat deleted the clang-cl branch March 5, 2024 16:00
@zooba zooba removed the needs backport to 3.11 only security fixes label Mar 5, 2024
@zooba
Copy link
Member

zooba commented Mar 5, 2024

Do you consider this bug critical?

Nope :) But it wasn't critical to me in main either, so that's not a great bar for whether to backport. Happy to defer to your needs, and if someone else needs it for 3.11 then they can prepare a backport.

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
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.

None yet

5 participants