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

Backport fixes for PyPy (GH-5429) #5465

Merged
merged 2 commits into from Jun 11, 2023
Merged

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Jun 4, 2023

Backported from #5429

  • Use _PyEval_GetAsyncGenFinalizer, _PyEval_GetAsyncGenFirstiter on PyPy instead of probing inside the PyThreadState struct

  • Require a nightly PyPy build for Py3.9 to support async iteration and finalisation.

  • Avoid PyIter_Next() and call tp_iternext() instead where possible because PyIter_Next() swallows StopIteration exceptions and thus looses the return value.

See https://foss.heptapod.net/pypy/pypy/-/issues/3280
See https://foss.heptapod.net/pypy/pypy/-/issues/3935

Currently require a nightly PyPy build for Py3.9 to support async iteration and finalisation.

Avoid PyIter_Next() and call tp_iternext() instead because PyIter_Next() swallows StopIteration exceptions and thus looses the return value.

See https://foss.heptapod.net/pypy/pypy/-/issues/3280
See https://foss.heptapod.net/pypy/pypy/-/issues/3935
@mattip
Copy link
Contributor Author

mattip commented Jun 4, 2023

It seems skbuild is failing on macOS 2.7.

File "<path>/python2.7/site-packages/skbuild/constants.py", line 41, in _default_skbuild_plat_name
       major_macos, minor_macos = release.split(".")[:2]
   ValueError: need more than 1 value to unpack

This is the old scikit-build as required by line-profiler v3.1.0 (which tag? hmm, maybe the fix_310_wheel branch), which is the last version to support python2.7, in its requirements/build.txt and not the newer scikit-build-core.

They released 4 days ago, maybe something is broken? But when I look at that file line 41 is different and this code appears on line 66. @henryiii any thoughts?

@henryiii
Copy link

henryiii commented Jun 4, 2023

It's also an old scikit-build, 0.15, as 0.16 and 0.17 don't support 2.7.

How does release not have a .? macOS should always report at least the minor version. Maybe it's empty? I'd love to see what it actually is.

@henryiii
Copy link

henryiii commented Jun 4, 2023

There's no line profiler with that number on PyPI. I think they deleted that release? I'm going to guess it never had wheels. Maybe try again?

I can release fixes to 0.15.x if I know what needs fixing.

@mattip
Copy link
Contributor Author

mattip commented Jun 4, 2023

There's no line profiler with that number on PyPI

I see line-proviler 3.1.0 here. It has wheels only for manylinux1 and manylinux2010.

But back to the problem at hand: at scikit-build version 1.5, consts.py:41 has this code:

release = os.environ.get("MACOSX_DEPLOYMENT_TARGET", release)

And indeed, in the CI actions file, there is

MACOSX_DEPLOYMENT_TARGET: "11"

On master it is "10.15"

@mattip
Copy link
Contributor Author

mattip commented Jun 4, 2023

This was changed in ba82c22 for both 3.0.0b3 and 0.29.x. I will try 11.0

@mattip
Copy link
Contributor Author

mattip commented Jun 4, 2023

CI is passing now (there are some timeouts after 40 minutes).

@henryiii
Copy link

henryiii commented Jun 4, 2023

I think we should probably allow settings like "11", I think that's supported when compiling; from (rather old) macOS source code, it looks like default values were injected if they were missing.

Not sure why 11 is being targeted, though, 10.14 is enough for full C++ support, otherwise 10.9 or 10.10 is fine. Seeing it set to 11 when moving to macOS 11 looks suspicious. Edit, no, it was 10.14 -> 11 while macOS being compiled was always 11.

@scoder
Copy link
Contributor

scoder commented Jun 4, 2023

I used target 11 (ok, probably should have used 11.0) to support Arm64. Isn't that needed?

@henryiii
Copy link

henryiii commented Jun 4, 2023

Oh, is it only setting 11 targeting ARM? Yes, 11 is the first supporting ARM, though IIRC it ignores a lower setting so that you can make Universal binaries that support 10.x too. Could be wrong there, but I know we don't have built in fuse support for cibuildwheel yet, so it has support something like this.

Is this distributing binaries or just for testing?

@scoder
Copy link
Contributor

scoder commented Jun 4, 2023

This is in the CI workflow, not the wheel build. The macOS CI build in 0.29.x has generally been unhappy for a while now, but 0.29.x is in boring legacy maintenance mode, so the macOS build setup isn't hugely important any more (to me, that is). If it's easy to fix, fine. If not, that's probably fine, too.

@henryiii
Copy link

henryiii commented Jun 4, 2023

"11.0" should be just fine then. I'll see if I can match the behavior for single digit values better (more permissive or better error message, whichever is closer).

@mattip
Copy link
Contributor Author

mattip commented Jun 7, 2023

"11.0" should be just fine then.

Cool. That is what I did in this PR.

@mattip
Copy link
Contributor Author

mattip commented Jun 10, 2023

Does this need anything more?

@da-woods da-woods merged commit 51078b3 into cython:0.29.x Jun 11, 2023
37 of 47 checks passed
@da-woods
Copy link
Contributor

Thank @mattip

@da-woods da-woods added this to the 0.29.36 milestone Jun 11, 2023
@da-woods da-woods added the PyPy label Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants