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

Use cysignals hook #117

Merged
merged 5 commits into from
Nov 2, 2022
Merged

Use cysignals hook #117

merged 5 commits into from
Nov 2, 2022

Conversation

kliem
Copy link
Collaborator

@kliem kliem commented May 30, 2022

@mkoeppe @videlec

This branch inserts the hooks as discussed in #109.
What do you think?

It's based upon sagemath/cysignals#166.

@kliem
Copy link
Collaborator Author

kliem commented May 30, 2022

According to https://github.com/kliem/cypari2/runs/6660982099?check_suite_focus=true
this approach works at least for the test suite (usually there are quite a lot time outs, as the current github workflow does not build cysignals linked to pari).

@videlec
Copy link
Collaborator

videlec commented May 31, 2022

Great! Thanks for working on it. It will simplify a lot the distribution of cysignals.

@mkoeppe
Copy link

mkoeppe commented May 31, 2022

Looking great!

@videlec
Copy link
Collaborator

videlec commented Jun 2, 2022

Why did you bump the version here? Just for testing purposes? I do not see why a version bump would be inside a merge request.

@kliem
Copy link
Collaborator Author

kliem commented Jun 2, 2022

I don't know how this usually works. I just think it's irritating when the version number does not reflect the current status.
Anyway the version bump is mostly needed for testing purposes, so that I can make https://github.com/kliem/cypari2/releases/tag/v2.2.0a0 and use it on https://trac.sagemath.org/ticket/33878, which in return uses it for the test in sagemath/cysignals#166.

@kliem
Copy link
Collaborator Author

kliem commented Jun 2, 2022

This pull request isn't ready yet anyway, as it depends on a cysignals version, which isn't even released yet.

@dimpase
Copy link
Member

dimpase commented Oct 31, 2022

What's needed for this to make it? Is sagemath/cysignals ready for a new release?

@dimpase
Copy link
Member

dimpase commented Oct 31, 2022

It would be nice to have a new release here soon, to let Pari 2.15 upgrade into Sage

@dimpase
Copy link
Member

dimpase commented Oct 31, 2022

I don't see how VERSION is needed anywhere, apart from new releases. After all, our GH Actions checks are triggered by new pushes/PRs, not by version changes.

@kliem
Copy link
Collaborator Author

kliem commented Nov 1, 2022 via email

@mkoeppe
Copy link

mkoeppe commented Nov 1, 2022

Some projects solve this problem by making a commit that changes the version to some ".dev0" version right after a release

@dimpase
Copy link
Member

dimpase commented Nov 1, 2022

bumping up VERSION with every merged PR isn't feasible, IMHO.

@dimpase
Copy link
Member

dimpase commented Nov 2, 2022

OK, I've resolved this merge to be as supplied by the PR

@dimpase dimpase merged commit 8474c9c into master Nov 2, 2022
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.

4 participants