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

discover cypari2 on runtime instead of on configure #149

Closed
wants to merge 11 commits into from

Conversation

kliem
Copy link

@kliem kliem commented Aug 31, 2021

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
Copy link
Author

kliem commented Aug 31, 2021

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
Copy link

mkoeppe commented Sep 1, 2021

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
Copy link
Author

kliem commented Sep 1, 2021

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
Copy link

mkoeppe commented Sep 1, 2021

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
Copy link
Author

kliem commented Sep 1, 2021

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:

  • In order for cysignals to respect PARI's blocking state, cypari2 >= 2.1.3 must be available on runtime.

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.

@kliem kliem changed the title discover pari in macro.h instead of on configure discover cypari2 on runtime instead of on configure Sep 2, 2021
@mkoeppe
Copy link

mkoeppe commented Sep 7, 2021

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
Copy link
Author

kliem commented Sep 7, 2021

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
Copy link

mkoeppe commented Sep 7, 2021

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

@mkoeppe
Copy link

mkoeppe commented Sep 8, 2021

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

@kliem
Copy link
Author

kliem commented Sep 8, 2021

It was running, until before commit ad37280.

@kliem kliem deleted the branch sagemath:master December 10, 2021 20:00
@kliem kliem closed this Dec 10, 2021
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.

2 participants