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.

Sign up for GitHub

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

Jump to bottom

Support flint 3.1 in sagelib #37484

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Support flint 3.1 in sagelib #37484

merged 2 commits into from
Feb 29, 2024

Conversation

antonio-rojas
Copy link
Contributor

Drop uses of deprecated API

  • fmpq_get_mpz_frac/fmpq_init_set_mpz_frac_readonly, replace by verbatim code from the removed functions
  • arb_version was only used by the obsolete arb216 and arb218 doctest tags, remove it

@antonio-rojas
Copy link
Contributor Author

I have a feeling this auto-generated bindings of the entire flint API are going to cause packagers many headaches

@antonio-rojas antonio-rojas mentioned this pull request Feb 26, 2024
Merged
5 tasks
@kiwifb
Copy link
Member

kiwifb commented Feb 26, 2024

I am not in a great position to test this right now. But it does feel like your changes are backward compatible with flint 3.0 if you only remove deprecated stuff. Correct? Binding autogeneration is what cypari2 does. Maybe there should be a cyflint?

@antonio-rojas
Copy link
Contributor Author

antonio-rojas commented Feb 26, 2024 •
edited
Loading

I am not in a great position to test this right now. But it does feel like your changes are backward compatible with flint 3.0 if you only remove deprecated stuff. Correct? Binding autogeneration is what cypari2 does. Maybe there should be a cyflint?

cypari generates bindings at build time from the installed version of pari. This, OTOH, is a static copy which is manually generated (IIUC) https://github.com/sagemath/sage/blob/develop/src/sage_setup/autogen/flint/README.md. Quite a different story.

Yes, this should be backwards compatible with flint 3.0. I haven't tested, but CI will tell.

@kiwifb
Copy link
Member

kiwifb commented Feb 26, 2024

Oh, I get it. It is really bothersome.

@mkoeppe mkoeppe requested a review from videlec February 26, 2024 20:24
@grhkm21
Copy link
Contributor

grhkm21 commented Feb 26, 2024

Yes that's correct. My PR #37458 improves the README slightly, but the bindings for FLINT are indeed generated manually.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 26, 2024

@antonio-rojas I'd suggest to clarify the scope of the PR (only sagelib) in the title of the PR; this can help avoid bizarre and intense complaints by other developers later.

@antonio-rojas antonio-rojas changed the title Support flint 3.1 Support flint 3.1 in sagelib Feb 26, 2024
@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 6e904cf; changes) is ready! 🎉

@mezzarobba
Copy link
Member

cypari generates bindings at build time from the installed version of pari. This, OTOH, is a static copy which is manually generated (IIUC) https://github.com/sagemath/sage/blob/develop/src/sage_setup/autogen/flint/README.md. Quite a different story.

Can you explain what is bothering you? I view these scripts simply as tools to help Sage developers maintain the pxd files containing declarations of flint functions; there is no requirement that the pxd we ship be complete or correspond to any particular flint release or anything like that; and the output of the scripts is pretty similar to the bindings we used to maintain manually, only more consistent. So I don't really see what difference it makes if the bindings are (partially) auto-generated.

mezzarobba
mezzarobba approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@mezzarobba mezzarobba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@mezzarobba
Copy link
Member

@vbraun This PR is not technically a blocker, but I think it would be nice if it could be part of sage-10.3.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 27, 2024

I think it would be nice if it could be part of sage-10.3.

@mezzarobba This would only help if we can sort out the trouble with Singular - see #37203

@antonio-rojas
Copy link
Contributor Author

I think it would be nice if it could be part of sage-10.3.

@mezzarobba This would only help if we can sort out the trouble with Singular - see #37203

It would still be beneficial for distros

@antonio-rojas
Copy link
Contributor Author

cypari generates bindings at build time from the installed version of pari. This, OTOH, is a static copy which is manually generated (IIUC) https://github.com/sagemath/sage/blob/develop/src/sage_setup/autogen/flint/README.md. Quite a different story.

Can you explain what is bothering you? I view these scripts simply as tools to help Sage developers maintain the pxd files containing declarations of flint functions; there is no requirement that the pxd we ship be complete or correspond to any particular flint release or anything like that; and the output of the scripts is pretty similar to the bindings we used to maintain manually, only more consistent. So I don't really see what difference it makes if the bindings are (partially) auto-generated.

OK, I was under the (wrong) impression that things would break if the pxd contains bindings for API that no longer exists, which would force distro patching for every API change in flint even if it wasn't directly used in sage. Still, header removals (such as mpf_mat.h and mpf_vec.h in 3.1) will require patching, even if nothing from those headers is used in sage. But I expect these to be less frequent than general API changes.

@mezzarobba
Copy link
Member

OK, I was under the (wrong) impression that things would break if the pxd contains bindings for API that no longer exists, which would force distro patching for every API change in flint even if it wasn't directly used in sage. Still, header removals (such as mpf_mat.h and mpf_vec.h in 3.1) will require patching, even if nothing from those headers is used in sage. But I expect these to be less frequent than general API changes.

Yes, there's a tradeoff here: we are shipping bindings for things that are not used, but it is convenient to have them already available when you are writing new code using flint, be it in sagelib (and I hope sagelib will make use of more of flint in the future!) or in sage packages... That being said, I hoped that the flint developers had removed everything they wanted to remove with the 3.0 release, and that does not seem to be the case. We'll see how things develop with the next few releases!

@videlec
Copy link
Contributor

videlec commented Feb 27, 2024

A .pxd by itself is mostly harmless : these are just informations for the cython compiler. As long as you do not compile a cythonized file, API mismatches would remain unnoticed.

@tornaria
Copy link
Contributor

tornaria commented Feb 27, 2024 •
edited
Loading

Works for me.

Singular 4.3.2p10 works fine.

To build singular 4.3.2p10 with flint 3.1, I use Singular/Singular@b5605a7 + https://gitlab.archlinux.org/archlinux/packaging/packages/singular/-/blob/main/flint-3.1.patch

@tornaria
Copy link
Contributor

@vbraun This PR is not technically a blocker, but I think it would be nice if it could be part of sage-10.3.

It's a blocker for sagelib.

@antonio-rojas
Copy link
Contributor Author

To build singular 4.3.2p10 with flint 3.1, I use Singular/Singular@b5605a7 + https://gitlab.archlinux.org/archlinux/packaging/packages/singular/-/blob/main/flint-3.1.patch

This is upstreamed now Singular/Singular@772cf3b

@tornaria
Copy link
Contributor

To build singular 4.3.2p10 with flint 3.1, I use Singular/Singular@b5605a7 + https://gitlab.archlinux.org/archlinux/packaging/packages/singular/-/blob/main/flint-3.1.patch

This is upstreamed now Singular/Singular@772cf3b

Thanks for the pointer. OTOH, singular 4.3.2p15 is not released in the same way as p10, and it breaks sagelib in other ways (I know you have a patch but that one is less clear to me so I'll stick to p10).

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 27, 2024

I've also been tracking Singular spielwiese, and there's still a bunch of build problems - see Singular/Singular#1205

dimpase
dimpase approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@dimpase dimpase left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good with Flint 3.0.1 from the system

@vbraun vbraun merged commit 686e933 into sagemath:develop Feb 29, 2024
18 checks passed
@antonio-rojas antonio-rojas deleted the flint-3.1 branch February 29, 2024 21:40
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 2024
sagemathgh-37203: build/pkgs/flint 3.1.3
4a6edc3 
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
For this upgrade we tighten the check for GMP so that 6.2.1 is accepted
but 6.2.0 is rejected. For example `ubuntu-focal-standard` brings 6.2.0,
we accept it, and then FLINT complains:
https://github.com/mkoeppe/flint2/actions/runs/7718792281/job/2104079370
6#step:11:5395

We also change the upper bound on system `flint` accepted by
`configure`. Closes sagemath#37838.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#37484
- Depends on sagemath#37495 (merged and reverted here)
- Depends on sagemath#37570 (merged here)
- Depends on sagemath#37351 (merged here)
- Depends on sagemath#37726 (merged here to resolve merge conflict)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#37203
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mezzarobba mezzarobba mezzarobba approved these changes

@tornaria tornaria tornaria approved these changes

@dimpase dimpase dimpase approved these changes

@kiwifb kiwifb Awaiting requested review from kiwifb

@videlec videlec Awaiting requested review from videlec

Assignees
No one assigned
Labels
p: blocker / 1
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.
9 participants
@antonio-rojas @kiwifb @grhkm21 @mkoeppe @mezzarobba @videlec @tornaria @dimpase @vbraun