sagemath / cysignals

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

discover cypari2 on runtime instead of on configure #149

Closed kliem closed 2 years ago

kliem commented 3 years ago

This is intended to solve #148.

Usage, one must include PARI header now before cimporting from cysignals (if at all). If the PARI headers are included after cimporting cysignals, it fails intentionally (this could be removed, but that PARI's signal blocking is ignored.)

kliem commented 3 years ago

One needs to modify cypari/types.pxd to link to pari, as now every file that includes PARI headers needs to link it as well.

mkoeppe commented 3 years ago

I am not sure if this is the right solution, or even an improvement. With this change, now there can be several packages in the system that use cysignals but have an inconsistent opinion on whether they need to respect PARI's signal blocking status.

kliem commented 3 years ago

What do you mean by inconsistent opinion?

There is one problem I'm aware of: If you use a package that uses PARI, but doesn't include the headers, you wouldn't catch on that you are not paying attention to PARI's signal blocking status. However, in this case it is wrong usage (and should be mentioned somewhere, still don't know if anyone will read this). I don't know how to avoid this at all.

One thing that we could do is to rely on cypari2. In this case whenever cysignals is initialized it tries to import some function from cypari2 that passes pointers to PARI_SIGINT_block and PARI_SIGINT_pending (typecasted, because it is a python function). Maybe that is better. (This approach would only require a few changes to the current branch).

mkoeppe commented 3 years ago

If you use a package that uses PARI, but doesn't include the headers, you wouldn't catch on that you are not paying attention to PARI's signal blocking status.

Yes, exactly.

However, in this case it is wrong usage (and should be mentioned somewhere, still don't know if anyone will read this).

Basically you are moving the burden of depending on / conditionally depending on PARI from cysignals to all of cysignals's consumers. That's a non-solution.

One thing that we could do is to rely on cypari2. In this case whenever cysignals is initialized it tries to import some function from cypari2 that passes pointers to PARI_SIGINT_block and PARI_SIGINT_pending (typecasted, because it is a python function). Maybe that is better.

Yes, something like this is what I meant by "runtime dispatch" in #125.

kliem commented 3 years ago

Depending on cypari2 is also a responsibility for the user. However, I agree that this is much nicer to communicate. Probably something like the following:

This is transparent and manageable also for unexperienced users.

Thinking about this, I like this much better than my other approach (even if it involves some typecasting from volatile int* to python int and back to volatile int*).

I'll do the changes tomorrow.

mkoeppe commented 3 years ago

This is looking good to me; but shouldn't the CI be testing that it works without error if cypari2 is present in the runtime environment?

kliem commented 3 years ago

Yes, but that won't work until cypari2 has a new release. Of course, we could just install PARI and cypari2 for the CI and it will automatically change later.

mkoeppe commented 3 years ago

Yes, here on this PR you could just set up the CI to use a pinned version cypari2 @ URL

mkoeppe commented 3 years ago

Oh, the CI was still using Travis CI - this does not seem to run any more.

kliem commented 3 years ago

It was running, until before commit ad37280.