search

sagemath / cysignals

cysignals: interrupt and signal handling for Cython. Source repository for https://pypi.org/project/cysignals/
GNU Lesser General Public License v3.0
44 stars 23 forks source link

Replace compile-time dependency on pari by run-time dispatch #125

Closed mkoeppe closed 3 months ago

mkoeppe commented 3 years ago

The current design is not compatible with modern Python packaging, in which PEP517/518 dictate that there is no such thing as an "optional build dependency". Also, if installing a wheel one can never be sure whether it has been built with the PARI connection or not.

This should be replaced by a hook into which PARI (via cypari/cypari2) can install itself.

embray commented 3 years ago

I was thinking about this just yesterday. I admit I don't 100% understand how the PARI integration is supposed to work or what it's for. But AFAICT it could be replaced with a more generic hook. Such a hook might be useful for other interfaces as well, such as for GAP.

embray commented 3 years ago

If nothing else, for easier pip-install, --without-pari should be the default, rather than --with-pari.

mkoeppe commented 3 years ago

Let's look into creating the hook sometime during in the Sage 9.4 development cycle.

dimpase commented 2 years ago

I'm able to build wheels (for manylinux and macOS) for a cython module dependent on an external C++ library in https://github.com/dimpase/primecountpy using https://github.com/pypa/cibuildwheel

OK, here we'd need to build libpari - should not make a difference.

Should I try?

mkoeppe commented 2 years ago

No, the best way forward is to merge #149

dimpase commented 2 years ago

Doesn't #149 need newer Cython?

dimpase commented 2 years ago

If I recall correctly, one reason Pari gets in here is that its threads implementation is a bit funny, and if it's not called early things go bad, one gets some weird TLS-related segfaults. (But maybe it's my PTSD speaks up :-))

dimpase commented 2 years ago

Anyhow, we just got a wheel builder in here, Pari or no Pari (the wheels we build do have Pari linked in).

kliem commented 2 years ago

If I recall correctly, one reason Pari gets in here is that its threads implementation is a bit funny, and if it's not called early things go bad, one gets some weird TLS-related segfaults. (But maybe it's my PTSD speaks up :-))

149 removes exactly this dependency. Pari headers provide PARI_SIGINT_block and PARI_SIGINT_pending. This is all we need. What we aim for is somehow getting this stuff on runtime, if available.

I don't know if this is even possible and less so, if it is possible to integrate such a mechanism in cysignals. I do have a workaround by just passing pointers on runtime. #149 can get those pointers, provided that cypari2 has https://github.com/sagemath/cypari2/pull/109 merged.

dimpase commented 2 years ago

On Wed, 1 Dec 2021, 19:35 Jonathan Kliem, @.***> wrote:

If I recall correctly, one reason Pari gets in here is that its threads implementation is a bit funny, and if it's not called early things go bad, one gets some weird TLS-related segfaults. (But maybe it's my PTSD speaks up :-))

149 https://github.com/sagemath/cysignals/pull/149 removes exactly

this dependency. Pari headers provide PARI_SIGINT_block and PARI_SIGINT_pending. This is all we need. What we aim for is somehow getting this stuff on runtime, if available.

I don't know if this is even possible and less so, if it is possible to integrate such a mechanism in cysignals. I do have a workaround by just passing pointers on runtime. #149 https://github.com/sagemath/cysignals/pull/149 can get those pointers, provided that cypari2 has sagemath/cypari2#109 https://github.com/sagemath/cypari2/pull/109 merged.

we can certainly merge the latter and see if all this plays together well.

I believe that in the linux multithreaded setting Pari has to be "served first", and this was accomplished by the hack you are removing here.

—

You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sagemath/cysignals/issues/125#issuecomment-983989751, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJXYHFS5PFH5IKR3NQ4PV3UOZ2HTANCNFSM4X7GMOVA .