8000 MNT: try updating pythoncapi-compat by tacaswell · Pull Request #26213 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MNT: try updating pythoncapi-compat #26213

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
Apr 8, 2024
Merged

MNT: try updating pythoncapi-compat #26213

merged 2 commits into from
Apr 8, 2024

Conversation

tacaswell
Copy link
Contributor
@tacaswell tacaswell commented Apr 4, 2024 •
edited
Loading

xref to python/pythoncapi-compat#89

It looks like python/cpython#116883 and python/pythoncapi-compat#87 are collaborating to break the numpy build.

This was fixed upstream so update instead of revert.

@seberg seberg requested a review from ngoldbaum April 4, 2024 18:08
@ngoldbaum
Copy link
Member

The build failure is only for python 3.13, right?

@tacaswell
Copy link
Contributor Author

I'm not sure this failure is even in a tagged pre-release.

@vstinner
Copy link
Contributor
vstinner commented Apr 4, 2024

I fixed the issue with the current Python main branch in pythoncapi-compat: python/pythoncapi-compat#89 (comment)

@tacaswell
MNT: update pythoncapi-compat
93fd0a5 
Needed for current cpython main branch
@tacaswell tacaswell force-pushed the mnt/revert_pycomapt branch from cf25c00 to 93fd0a5 Compare April 4, 2024 22:17
@vstinner
Copy link
Contributor
vstinner commented Apr 4, 2024

Does a CI fail with pythoncapi-compat.h and Python 3.13? Which CI?

@ngoldbaum
Copy link
Member

It looks like a linux CI that has -Werror set is now failing with:

n file included from ../numpy/_core/src/common/npy_pycompat.h:5,
                 from ../numpy/_core/src/multiarray/arraytypes.c.src:15:
../numpy/_core/src/common/pythoncapi-compat/pythoncapi_compat.h:1304:1: error: ‘PyDict_SetDefaultRef’ defined but not used [-Werror=unused-function]
 1304 | PyDict_SetDefaultRef(PyObject *d, PyObject *key, PyObject *default_value,
      | ^~~~~~~~~~~~~~~~~~~~

not sure offhand why the compiler considers this one unused but not all the others

@vstinner
Copy link
Contributor
vstinner commented Apr 5, 2024

not sure offhand why the compiler considers this one unused but not all the others

It's a stupid mistake: I forgot "inline" in "static inline". Fixed in pythoncapi-compat by python/pythoncapi-compat@01341ac

@tacaswell tacaswell changed the title MNT: try to revert changes to pycompat shim MNT: try updating pythoncapi-compat Apr 5, 2024
@tacaswell
Copy link
Contributor Author
tacaswell commented Apr 5, 2024 •
edited
Loading

I can confirm that with this PR to numpy + cpython main numpy builds for me locally.

@ngoldbaum
Copy link
Member

@tacaswell can you update the submodule once more to fix the compiler warning failing CI?

@tacaswell
Copy link
Contributor Author

Ah, sorry did not get to this.

@ngoldbaum
Copy link
Member

No worries, figured I'd just push on top of this to finish it up.

@tacaswell
Copy link
Contributor Author

I can also confirm that this PR builds with cpython main still.

@tacaswell tacaswell marked this pull request as ready for review April 8, 2024 15:17
@ngoldbaum
Copy link
Member

Pulling this in, thanks for getting the ball rolling on this @tacaswell!

@ngoldbaum ngoldbaum merged commit 8e799a7 into numpy:main Apr 8, 2024
@tacaswell tacaswell deleted the mnt/revert_pycomapt branch April 8, 2024 15:57
@vstinner
Copy link
Contributor
vstinner commented Apr 8, 2024

Thanks for the bug reports, it helped me to discover bugs and enhance pythoncapi-compat :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngoldbaum ngoldbaum Awaiting requested review from ngoldbaum

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tacaswell @ngoldbaum @vstinner
0