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

Fix PyPy compilation #120

Merged
merged 3 commits into from
Oct 19, 2023
Merged

Fix PyPy compilation #120

merged 3 commits into from
Oct 19, 2023

Conversation

astrofrog
Copy link
Contributor

@astrofrog astrofrog commented Oct 18, 2023

When compiling with PyPy, it looks like _typeobject is already defined by PyPy itself - this causes the conda-forge build to fail since it includes PyPy builds: https://github.com/conda-forge/pyerfa-feedstock/pull/31/checks?check_run_id=17819660419

This PR does the simple fix of making sure we don't redefine _typeobject if we are using PyPy which should be harmless for CPython.

I'm also adding PyPy wheels here to see if they are fast enough to build as this would be a good regression test in itself and would also be nice to provide PyPy wheels.

@astrofrog
Copy link
Contributor Author

I've reverted the fix to check if the wheel builds fail for PyPy.

@astrofrog
Copy link
Contributor Author

The PyPy wheel builds fail without the patch here, let's make sure they pass now.

@astrofrog
Copy link
Contributor Author

For now I've made it so that we only build wheels for PyPy versions where there are Numpy wheels as I think that is a reasonable compromise (otherwise we have to build Numpy from source on PyPy and we need to add a bit of complexity to get that to work). Then we match what Numpy does and we also ensure that PyERFA does build with PyPy without going over the top.

@astrofrog astrofrog marked this pull request as ready for review October 18, 2023 13:34
@astrofrog
Copy link
Contributor Author

Ok I think this is fine now and ready for review.

Once/if this is merged, I would suggest tagging a new bugfix release so that we can fix the conda build (conda-forge/pyerfa-feedstock#31)

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

I hope at some point in the not too distant future we can just require newer gcc and be done with it, but for now this is very nice!

@mhvk
Copy link
Contributor

mhvk commented Oct 18, 2023

p.s. I'm happy to have a pyerfa 2.0.1.1 with as main CHANGES.rst entry that we ensured things build for PyPy.

Copy link
Member

@avalentino avalentino left a comment

Choose a reason for hiding this comment

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

LGTM

@mhvk
Copy link
Contributor

mhvk commented Oct 19, 2023

Great! With two approvals, I'll go ahead and merge this, and make a new release that also includes a fix for #123 (which is quite serious)

@mhvk mhvk merged commit 0079812 into liberfa:master Oct 19, 2023
23 checks passed
lpsinger added a commit to lpsinger/ligo.skymap that referenced this pull request Nov 21, 2023
- Define `Py_LIMITED_API` and `NPY_TARGET_VERSION` macros in
  setup.py rather than in the C source code.
- Don't define Numpy's `struct _typeobject` if we are building for
  PyPy because PyPy doesn't implement Python's limited C API
  (see liberfa/pyerfa#120).
- Define `struct _typeobject` workaround after including Python.h,
  not before, because on PyPy, Python.h defines the macros that we
  depend on to detect whether we are building under PyPy.
- Use `NPY_TARGET_VERSION` for builds that are backwards-compatible
  with old versions of Numpy.
- Build with any version of Numpy >=1.25 and <2
  (see liberfa/pyerfa#121).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants