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

upgrade to flint3 #35848

Merged
merged 9 commits into from
Dec 6, 2023
Merged

upgrade to flint3 #35848

merged 9 commits into from
Dec 6, 2023

Conversation

mezzarobba
Copy link
Member

@mezzarobba mezzarobba commented Jun 27, 2023

Upgrade to flint3.

Current Sage versions are not compatible with flint ≥ 3, and, though the diff is not huge, there are enough changes that versions including this PR will be incompatible with flint < 3.

Fixes #20003.
Closes #35993 as no longer relevant.

Related PRs in upstream projects:

Additional changes still needed for optional packages to work:

Planned follow-ups:

@mezzarobba mezzarobba force-pushed the flint3 branch 2 times, most recently from fc03812 to b7ca9e4 Compare June 29, 2023 15:27
@mezzarobba

This comment was marked as resolved.

@mezzarobba
Copy link
Member Author

mezzarobba commented Jul 3, 2023

Things look pretty good :-). Mostly a few benign output changes. I've reported the only dubious one at flintlib/flint#1410; let's wait for Fredrik's answer and the next alpha to decide how to update the tests. Hopefully it will also be possible to drop the patches at that point. We should also move the contents of libs/arb etc. to libs/flint and probably do some other cleanup of this kind. And update spkg-configure to check that flint is built with NTL support.

@mezzarobba

This comment was marked as resolved.

@mezzarobba

This comment was marked as resolved.

@mezzarobba

This comment was marked as resolved.

@mezzarobba
Copy link
Member Author

See also #35993.

@videlec
Copy link
Contributor

videlec commented Oct 10, 2023

(I can not push to your branch)

diff --git a/src/sage/libs/flint/fmpz_poly.pyx b/src/sage/libs/flint/fmpz_poly.pyx
index cfcbea9090..0034494247 100644
--- a/src/sage/libs/flint/fmpz_poly.pyx
+++ b/src/sage/libs/flint/fmpz_poly.pyx
@@ -23,12 +23,15 @@ from cpython.sequence cimport *
 
 from cysignals.memory cimport sig_free
 
+from .fmpz cimport fmpz_init_set_readonly, fmpz_clear_readonly, fmpz_init, fmpz_clear, fmpz_set_mpz, fmpz_get_mpz
+
 from sage.arith.long cimport pyobject_to_long
 from sage.cpython.string cimport char_to_str, str_to_bytes
 from sage.libs.flint.fmpz cimport *
 from sage.structure.sage_object cimport SageObject
 from sage.rings.integer cimport Integer
 
 cdef class Fmpz_poly(SageObject):
 
     def __cinit__(self):

@videlec
Copy link
Contributor

videlec commented Oct 10, 2023

At 6ef8bd2 flint fails to build on my machine (linking problem with NTL)

gcc -g -g -O2 -shared -Wl,-soname,libflint.so.18  build/generic_files_merged.lo build/thread_pool_merged.lo [...] build/ca_mat_merged.lo build/interfaces/NTL-interface.lo -o libflint.so.18.0.0 -Wl,-rpath-link,/home/doctorant/sage/local/lib -L/home/doctorant/sage/local/lib -Wl,-rpath,/home/doctorant/sage/local/lib -Wl,-rpath-link,/home/doctorant/sage/local/lib -L/home/doctorant/sage/local/lib -Wl,-rpath,/home/doctorant/sage/local/lib  -pthread -lmpfr -lgmp -lm  -lstdc++ -lntl
/usr/bin/ld: _ZN3NTL9zz_pEInfoE: TLS reference in build/interfaces/NTL-interface.lo mismatches non-TLS definition in /home/doctorant/sage/local/lib/libntl.so section .bss
/usr/bin/ld: /home/doctorant/sage/local/lib/libntl.so: error adding symbols: bad value
collect2: error: ld returned 1 exit status
make[5]: *** [Makefile:377: libflint.so.18.0.0] Error 1

@mezzarobba
Copy link
Member Author

+from .fmpz cimport fmpz_init_set_readonly, fmpz_clear_readonly, fmpz_init, fmpz_clear, fmpz_set_mpz, fmpz_get_mpz

This is with the version from yesterday afternoon, where I forgot the line

 from sage.libs.flint.fmpz cimport *

isn't it?

@mezzarobba
Copy link
Member Author

TLS reference in build/interfaces/NTL-interface.lo mismatches non-TLS definition

Did you build sage from scratch or incrementally? Could it be that there are somehow two different versions of NTL at play here?

@videlec
Copy link
Contributor

videlec commented Oct 10, 2023

TLS reference in build/interfaces/NTL-interface.lo mismatches non-TLS definition

Did you build sage from scratch or incrementally? Could it be that there are somehow two different versions of NTL at play here?

I obtained that after a make distclean and ./configure --with-system-flint=no --with-system-giac=no --with-system-ntl=no --with-system-singular=no --with-system-symengine=no --with-system-msolve=no.

@mezzarobba
Copy link
Member Author

Can you post the full build logs somewhere?

@videlec
Copy link
Contributor

videlec commented Oct 10, 2023

@mezzarobba

This comment was marked as outdated.

@mezzarobba
Copy link
Member Author

mezzarobba commented Oct 10, 2023

Ok, I can reproduce the issue...

(edit:) ...but with an incremental build. I see this error if I (1) first build sage with system NTL, (2) run ./configure --with-system-ntl=no (3) run make build. If however I then (4) run ./sage -f flint, then things appear to work again.

(edit:) ok, actually I see the issue when building from scratch too when I use --with-system-ntl=no.

@videlec
Copy link
Contributor

videlec commented Oct 10, 2023

Thanks. And could you see if you can reproduce the issue when building flint (with the ntl interface enabled!) separately from sage?

Flint standalone compiled. Configured with

$ ./configure --with-ntl --with-gmp --with-mpfr --with-blas --disable-static

@mezzarobba
Copy link
Member Author

@videlec could you test again with this commit?

@videlec
Copy link
Contributor

videlec commented Oct 10, 2023

I encouter the same kind of linking error against ntl but now with giac instead of flint. Probably unrelated to the PR.

libtool: link: g++ -std=gnu++11  -fPIC -DPIC -shared -nostdlib /usr/lib/gcc/x86_64-linux-gnu/11/../../../x86_64-linux-gnu/crti.o /usr/lib/gcc/x86_64-linux-gnu/11/crtbeginS.o  .libs/input_lexer.o .libs/sym2poly.o .libs/gausspol.o .libs/threaded.o .libs/moyal.o .libs/maple.o .libs/ti89.o .libs/mathml.o .libs/misc.o .libs/permu.o .libs/quater.o .libs/desolve.o .libs/input_parser.o .libs/symbolic.o .libs/index.o .libs/modpoly.o .libs/modfactor.o .libs/ezgcd.o .libs/derive.o .libs/solve.o .libs/intg.o .libs/intgab.o .libs/risch.o .libs/lin.o .libs/series.o .libs/subst.o .libs/vecteur.o .libs/sparse.o .libs/csturm.o .libs/tex.o .libs/global.o .libs/ifactor.o .libs/alg_ext.o .libs/gauss.o .libs/isom.o .libs/plot.o .libs/plot3d.o .libs/rpn.o .libs/prog.o .libs/pari.o .libs/cocoa.o .libs/unary.o .libs/usual.o .libs/identificateur.o .libs/gen.o .libs/tinymt32.o .libs/first.o .libs/TmpLESystemSolver.o .libs/TmpFGLM.o .libs/help.o .libs/lpsolve.o .libs/optimization.o .libs/signalprocessing.o .libs/graphe.o .libs/graphtheory.o .libs/nautywrapper.o .libs/markup.o .libs/kdisplay.o .libs/kadd.o .libs/caseval.o .libs/cutils.o .libs/graphic.o .libs/libbf.o .libs/libregexp.o .libs/libunicode.o .libs/qjsgiac.o .libs/quickjs.o .libs/quickjs-libc.o .libs/js.o   -L/home/doctorant/sage/local/lib -lntl -lpari -L/usr/lib/x86_64-linux-gnu/openblas-pthread/ -lgsl -lopenblas -lrt -lpthread -lnauty -lcliquer -lusb-1.0 -lcurl -lglpk -ldl -lecm -lmpfi -lmpfr -lgmp -L/home/doctorant/sage/local/lib/../lib -L/usr/lib/gcc/x86_64-linux-gnu/11 -L/usr/lib/gcc/x86_64-linux-gnu/11/../../../x86_64-linux-gnu -L/usr/lib/gcc/x86_64-linux-gnu/11/../../../../lib -L/lib/x86_64-linux-gnu -L/lib/../lib -L/usr/lib/x86_64-linux-gnu -L/usr/lib/../lib -L/usr/lib/gcc/x86_64-linux-gnu/11/../../.. -lstdc++ -lm -lc -lgcc_s /usr/lib/gcc/x86_64-linux-gnu/11/crtendS.o /usr/lib/gcc/x86_64-linux-gnu/11/../../../x86_64-linux-gnu/crtn.o  -g -O2 -g -O2 -Wl,-rpath-link -Wl,/home/doctorant/sage/local/lib -Wl,-rpath -Wl,/home/doctorant/sage/local/lib -Wl,-rpath-link -Wl,/home/doctorant/sage/local/lib -Wl,-rpath -Wl,/home/doctorant/sage/local/lib   -Wl,-soname -Wl,libgiac.so.0 -o .libs/libgiac.so.0.0.0
/usr/bin/ld: _ZN3NTL8ZZ_pInfoE: TLS reference in .libs/modpoly.o mismatches non-TLS definition in /home/doctorant/sage/local/lib/libntl.so section .bss
/usr/bin/ld: /home/doctorant/sage/local/lib/libntl.so: error adding symbols: bad value

@dkwo
Copy link

dkwo commented Nov 21, 2023

@mkoeppe Thanks.
@dimpase Sure, the thing is that Void linux currently does not have native builders for aarch64.
For the record, currently the blockers to cross-compilation are ntl (hence flintlib, eclib); fflas-ffpack, givaro, linbox; sympow; and maxima.

@dimpase
Copy link
Member

dimpase commented Nov 21, 2023

you can build a native aarch64 SoC builder for under $100 - won't be fast, but will do the job.

@dkwo
Copy link

dkwo commented Nov 21, 2023

@dimpase Once again, I can of course build natively on my aarch64 machine; the point is, their build infrastructure is cross-build for that arch, so it won't be in the repositories..

@mezzarobba
Copy link
Member Author

mezzarobba commented Nov 21, 2023 via email

@dkwo
Copy link

dkwo commented Nov 21, 2023

@mezzarobba Thanks a lot, this is what I thought.
What about eclib? can it also be built without ntl, as far as sage is concerned?
I guess I was a bit puzzled by this comment from @JohnCremona

for one thing it will mean that building Sage will not also involve building eclib which has flint as a dependency.

@JohnCremona
Copy link
Member

The functionality in eclib which Sage uses does need NTL. Flint is optional. I'll look more carefully to see how vital for Sage the NTL dependency is.

@dimpase
Copy link
Member

dimpase commented Nov 21, 2023

@dimpase Once again, I can of course build natively on my aarch64 machine; the point is, their build infrastructure is cross-build for that arch, so it won't be in the repositories..

maxima/ecl are not going to get optional any time soon. So trying to remove NTL seems to be a bit in vain.

@dkwo
Copy link

dkwo commented Nov 21, 2023

@JohnCremona Got it, thanks.

So trying to remove NTL seems to be a bit in vain.

Well, it would shorten the time needed to build sage in the above mentioned case.
Of course, the best outcome would be to make it cross buildable.

@jhpalmieri
Copy link
Member

Are there further work issues here?

@mezzarobba
Copy link
Member Author

Are there further work issues here?

None that I am aware of (at least if we are okay with temporarily breaking the e-antic and symengine optional packages).

@tornaria
Copy link
Contributor

tornaria commented Dec 1, 2023

I've been testing and using this for a while, and I will be shipping 10.2 in void linux with this patch, because I cannot hold back flintlib any longer.

I'm giving this positive review so it can be merged at the start of the 10.3 cycle.

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
    
Upgrade to flint3.

Current Sage versions are not compatible with flint ≥ 3, and, though the
diff is not huge, there are enough changes that versions including this
PR will be incompatible with flint < 3.

Fixes sagemath#20003.
Closes sagemath#35993 as no longer relevant.

Related PRs in upstream projects:
* Singular/Singular#1177
* flintlib/flint#1408
* flintlib/flint#1489
* flintlib/flint#1492
* flintlib/flint#1611
* algebraic-solving/msolve#76
* flatsurf/e-antic#264

Additional changes still needed for optional packages to work:
* sagemath#36677
* upgrade e-antic
* possibly more

Planned follow-ups:
* sagemath#36449
* sagemath#36433
    
URL: sagemath#35848
Reported by: Marc Mezzarobba
Reviewer(s): Vincent Delecroix
@vbraun
Copy link
Member

vbraun commented Dec 5, 2023

There is a buildbot that tests the e-antic optional package, do you have a plan for that? Or is it just going to be silently broken?

Note that normaliz depends on it

@mkoeppe
Copy link

mkoeppe commented Dec 5, 2023

I think the plan is to break it and fix it soon.

@vbraun vbraun merged commit a7b9ebc into sagemath:develop Dec 6, 2023
27 of 54 checks passed
@mezzarobba mezzarobba deleted the flint3 branch December 6, 2023 11:01
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 24, 2023
    
We
- write down a script to auto-generate flint header files (currently at
https://github.com/videlec/flint-header-sage-autogen)
- replace most of the files in `src/sage/libs/flint` by auto-generated
files. Doing so, the custom sage code in `src/sage/libs/flint` is always
contained in files suffixed by `_sage.pxd`/`_sage.pyx`.
- deprecate the files in `src/sage/libs/arb/`

### 📝 Checklist

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

### ⌛ Dependencies

sagemath#35848

#### Upstream issues and patches

- flintlib/flint#1523
- flintlib/flint#1529
- flintlib/flint#1532
- flintlib/flint#1535
- flintlib/flint#1536
- cython/cython#5779
- flintlib/flint#1653
- flintlib/flint#1655
    
URL: sagemath#36449
Reported by: Vincent Delecroix
Reviewer(s): Marc Mezzarobba, Matthias Köppe, Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 25, 2023
We
- write down a script to auto-generate flint header files (currently at
https://github.com/videlec/flint-header-sage-autogen)
- replace most of the files in `src/sage/libs/flint` by auto-generated
files. Doing so, the custom sage code in `src/sage/libs/flint` is always
contained in files suffixed by `_sage.pxd`/`_sage.pyx`.
- deprecate the files in `src/sage/libs/arb/`

### 📝 Checklist

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

### ⌛ Dependencies

sagemath#35848

#### Upstream issues and patches

- flintlib/flint#1523
- flintlib/flint#1529
- flintlib/flint#1532
- flintlib/flint#1535
- flintlib/flint#1536
- cython/cython#5779
- flintlib/flint#1653
- flintlib/flint#1655

URL: sagemath#36449
Reported by: Vincent Delecroix
Reviewer(s): Marc Mezzarobba, Matthias Köppe, Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 27, 2023
    
We
- write down a script to auto-generate flint header files (currently at
https://github.com/videlec/flint-header-sage-autogen)
- replace most of the files in `src/sage/libs/flint` by auto-generated
files. Doing so, the custom sage code in `src/sage/libs/flint` is always
contained in files suffixed by `_sage.pxd`/`_sage.pyx`.
- deprecate the files in `src/sage/libs/arb/`

### 📝 Checklist

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

### ⌛ Dependencies

sagemath#35848

#### Upstream issues and patches

- flintlib/flint#1523
- flintlib/flint#1529
- flintlib/flint#1532
- flintlib/flint#1535
- flintlib/flint#1536
- cython/cython#5779
- flintlib/flint#1653
- flintlib/flint#1655
    
URL: sagemath#36449
Reported by: Vincent Delecroix
Reviewer(s): Marc Mezzarobba, Matthias Köppe, Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 2, 2024
    
We
- write down a script to auto-generate flint header files (currently at
https://github.com/videlec/flint-header-sage-autogen)
- replace most of the files in `src/sage/libs/flint` by auto-generated
files. Doing so, the custom sage code in `src/sage/libs/flint` is always
contained in files suffixed by `_sage.pxd`/`_sage.pyx`.
- deprecate the files in `src/sage/libs/arb/`

### 📝 Checklist

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

### ⌛ Dependencies

sagemath#35848

#### Upstream issues and patches

- flintlib/flint#1523
- flintlib/flint#1529
- flintlib/flint#1532
- flintlib/flint#1535
- flintlib/flint#1536
- cython/cython#5779
- flintlib/flint#1653
- flintlib/flint#1655
    
URL: sagemath#36449
Reported by: Vincent Delecroix
Reviewer(s): Marc Mezzarobba, Matthias Köppe, Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 5, 2024
    
We
- write down a script to auto-generate flint header files (currently at
https://github.com/videlec/flint-header-sage-autogen)
- replace most of the files in `src/sage/libs/flint` by auto-generated
files. Doing so, the custom sage code in `src/sage/libs/flint` is always
contained in files suffixed by `_sage.pxd`/`_sage.pyx`.
- deprecate the files in `src/sage/libs/arb/`

### 📝 Checklist

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

### ⌛ Dependencies

sagemath#35848

#### Upstream issues and patches

- flintlib/flint#1523
- flintlib/flint#1529
- flintlib/flint#1532
- flintlib/flint#1535
- flintlib/flint#1536
- cython/cython#5779
- flintlib/flint#1653
- flintlib/flint#1655
    
URL: sagemath#36449
Reported by: Vincent Delecroix
Reviewer(s): Marc Mezzarobba, Matthias Köppe, Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 13, 2024
    
We
- write down a script to auto-generate flint header files (currently at
https://github.com/videlec/flint-header-sage-autogen)
- replace most of the files in `src/sage/libs/flint` by auto-generated
files. Doing so, the custom sage code in `src/sage/libs/flint` is always
contained in files suffixed by `_sage.pxd`/`_sage.pyx`.
- deprecate the files in `src/sage/libs/arb/`

### 📝 Checklist

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

### ⌛ Dependencies

sagemath#35848

#### Upstream issues and patches

- flintlib/flint#1523
- flintlib/flint#1529
- flintlib/flint#1532
- flintlib/flint#1535
- flintlib/flint#1536
- cython/cython#5779
- flintlib/flint#1653
- flintlib/flint#1655
    
URL: sagemath#36449
Reported by: Vincent Delecroix
Reviewer(s): Marc Mezzarobba, Matthias Köppe, Vincent Delecroix
albinahlback pushed a commit to flintlib/flint that referenced this pull request Jan 23, 2024
Change the definition of $BINDIR & co so that, in the case of staged
installations, they contain the final (binary) installation directory,
not the packaging directory.

The only tangible effect should be that, on MacOS, install_name_tool is
passed the correct library path. See

sagemath/sage#35848 (comment)
math-gout pushed a commit to math-gout/flint that referenced this pull request Feb 23, 2024
Change the definition of $BINDIR & co so that, in the case of staged
installations, they contain the final (binary) installation directory,
not the packaging directory.

The only tangible effect should be that, on MacOS, install_name_tool is
passed the correct library path. See

sagemath/sage#35848 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet