Skip to content

gh-129296: Fix pyatomic.h include paths #129320

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 29, 2025
Merged

gh-129296: Fix pyatomic.h include paths #129320

merged 2 commits into from
Jan 29, 2025
+4 −4

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Jan 26, 2025 •
edited
Loading

As reported in the issue, these paths are relative to the base include directory instead of the header file. While this is fine in practice because the compiler falls back to the base include directory, it doesn't match the style of other includes in the project.

Originally added in

@bedevere-app bedevere-app bot mentioned this pull request Jan 26, 2025
Closed
@zanieb
Copy link
Contributor Author

zanieb commented Jan 26, 2025

I'm unsure if this is worth backporting to 3.13

colesbury
colesbury approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Thanks!

@colesbury colesbury added the needs backport to 3.13 bugs and security fixes label Jan 28, 2025
@colesbury
Copy link
Contributor

I will tentatively label this as "needs backport to 3.13", but will wait for feedback from @Yhg1s.

@raymondiiii

This comment was marked as off-topic.

Sign in to view
@colesbury colesbury requested a review from vstinner January 28, 2025 20:45
vstinner
vstinner approved these changes Jan 29, 2025
View reviewed changes
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

Would you mind to update also # include "cpython/pthread_stubs.h" in Include/cpython/pythread.h?

@vstinner vstinner enabled auto-merge (squash) January 29, 2025 14:54
@vstinner
Copy link
Member

Thanks for updating Include/cpython/pythread.h. I enabled automerge.

@vstinner vstinner removed the needs backport to 3.13 bugs and security fixes label Jan 29, 2025
@vstinner
Copy link
Member

I don't think that it's worth it to backport this change to 3.12 and 3.13. It's nice to have in the main branch, but it doesn't fix a build error, since it's not needed to backport it.

@vstinner vstinner merged commit 3a974e3 into python:main Jan 29, 2025
42 checks passed
@jacobm-splunk
Copy link

@vstinner

I've actually been seeing this error when running clang against code that's linking against CPython:

python313/include/python3.13/cpython/pyatomic.h:543:4: error: "no available pyatomic implementation for this platform/compiler" [clang-diagnostic-error]

It's fairly annoying to have to either:

  1. Somehow ignore the header files from the included cpython (probably with some cmake / clang options)
  2. Exclude this specific code from running against clang until we upgrade python to 3.14

I would really appreciate if there was either a good workaround, or this was back ported to 3.12 and 3.13.

@vstinner vstinner added the needs backport to 3.13 bugs and security fixes label Feb 28, 2025
@miss-islington-app
Copy link

Thanks @zanieb for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 28, 2025
@zanieb @miss-islington
pythongh-129296: Fix pyatomic.h include paths (pythonGH-129320)
cb32439 
Use relative includes in Include/cpython/pyatomic.h for
pyatomic_gcc.h, pyatomic_std.h and pyatomic_msc.h.

Do a similar change in Include/cpython/pythread.h for
pthread_stubs.h include.
(cherry picked from commit 3a974e3)

Co-authored-by: Zanie Blue <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Feb 28, 2025

GH-130667 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Feb 28, 2025
@vstinner vstinner added the needs backport to 3.12 only security fixes label Feb 28, 2025
@miss-islington-app
Copy link

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

@miss-islington-app
Copy link

Sorry, @zanieb and @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3a974e39d54902699f360bc4db2fd351a6baf3ef 3.12

@bedevere-app
Copy link

bedevere-app bot commented Feb 28, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Feb 28, 2025
@vstinner
Copy link
Member

I've actually been seeing this error when running clang against code that's linking against CPython

Ah. When #129296 was reported, it wasn't clear if it was a compiler error. If you're unable to build Python, I'm ok to backport the changes to 3.12 and 3.13.

Note that Python 3.12 only needs the pythread.h change (pyatomic.h doesn't exist in Python 3.12).

vstinner added a commit that referenced this pull request Feb 28, 2025
@vstinner @zanieb
[3.12] gh-129296: Fix pythread.h include paths (#129320) (#130668)
107e08d 
gh-129296: Fix `pythread.h` include paths (#129320)

Use relative includes in Include/cpython/pythread.h for
pthread_stubs.h.

(cherry picked from commit 3a974e3)

Co-authored-by: Zanie Blue <[email protected]>
vstinner pushed a commit that referenced this pull request Feb 28, 2025
@miss-islington @zanieb
[3.13] gh-129296: Fix pyatomic.h include paths (GH-129320) (#130667)
125ca02 
gh-129296: Fix `pyatomic.h` include paths (GH-129320)

Use relative includes in Include/cpython/pyatomic.h for
pyatomic_gcc.h, pyatomic_std.h and pyatomic_msc.h.

Do a similar change in Include/cpython/pythread.h for
pthread_stubs.h include.
(cherry picked from commit 3a974e3)

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

@vstinner vstinner vstinner approved these changes

@colesbury colesbury colesbury approved these changes

Assignees

@vstinner vstinner

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zanieb @colesbury @raymondiiii @vstinner @jacobm-splunk