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

Autogenerate flint headers #36449

Merged
merged 42 commits into from
Jan 14, 2024
Merged

Conversation

videlec
Copy link
Contributor

@videlec videlec commented Oct 11, 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

  • The title is concise, informative, and self-explanatory.
  • 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

#35848

Upstream issues and patches

@videlec videlec mentioned this pull request Oct 11, 2023
@videlec
Copy link
Contributor Author

videlec commented Oct 11, 2023

The linting error is ridiculous


╭─ Error: foreign_latex: foreign commands in LaTeX ────────────────────────────╮
│     666                                                                      │
│     667     void fmpz_bin_uiui(fmpz_t f, ulong n, ulong k)                   │
│  ❱  668     # Sets `f` to the binomial coefficient `{n \choose k}`.          │
│     669                                                                      │
│     670     void _fmpz_rfac_ui(fmpz_t r, const fmpz_t x, ulong a, ulong b)   │
│  ╭─ Hint: ────────────────────────────────────────────────────────────────╮  │
│  │  use equivalent LaTeX commands instead of plain TeX commands such as   │  │
│  │  \over, \choose, etc.                                                  │  │
│  ╰────────────────────────────────────────────────────────────────────────╯  │
╰─ sage/libs/flint/fmpz.pxd:668 ───────────────────────────────────────────────╯

@mezzarobba
Copy link
Member

mezzarobba commented Oct 11, 2023

  • Could we automatically or semi-automatically declare values as bint instead of int where relevant? E.g. int fmpq_is_zero(...) should ideally be bint fmpq_is_zero(...).
  • I wonder if we want to keep the comments extracted from the documentation. It is definitely convenient, but it means we really have to keep the pxd files up to date. Also, if this part stay, it should be mentioned somewhere (if only for LGPL compliance purpose) that the descriptions are extracted from the flint docs. (edit:) Also, if the comments stay, I think it would be more readable to put them before the corresponding declarations.

@videlec
Copy link
Contributor Author

videlec commented Oct 11, 2023

If you have a simple rule to change a int my_function(...) into bint my_function(...) I am happy to add it to the generator.

@mezzarobba
Copy link
Member

No, I think we would need a mechanism for specifying it (or rather, for specifying exceptions, because I think most occurrences of int actually correspond to booleans).

@mezzarobba
Copy link
Member

Joels says he has some heuristics for deciding if a function is a predicate (= if the return value is a boolean), but nothing in the case of boolean arguments.

@videlec videlec force-pushed the autogeneration-flint-headers branch from 05079b2 to 4799839 Compare October 12, 2023 07:30
@videlec
Copy link
Contributor Author

videlec commented Oct 12, 2023

@mezzarobba @fredrik-johansson The header flint/fft_small.h exists in flint but is not present in the sage source tree after installation (and there is a fft_small.rst)). Is there a reason why?

@mezzarobba
Copy link
Member

I think this is because it is only installed if flint is built with avx2 support. This needs to be enabled explicitly (no automatic detection at the moment), and sage does not do it.

@videlec
Copy link
Contributor Author

videlec commented Oct 12, 2023

All right, this is not too bad. With the autogeneration, I could just try to match headers in the installed directory rather than the flint sources.

@videlec
Copy link
Contributor Author

videlec commented Oct 12, 2023

Now

.../flint/machine_vectors.h:1543:2: error: #error machine_vector.h not implmented
     1543 | #error machine_vector.h not implmented

Beyond the missing e in implemented I do not quite understand the error.

@videlec
Copy link
Contributor Author

videlec commented Oct 12, 2023

The problem is that the header machine_vector.h is copied in the sage source tree but "broken". The latter is included in flint_wrap.h and my autogeneration scripts includes it in flint_wrap.h.

@videlec
Copy link
Contributor Author

videlec commented Oct 12, 2023

Got passed the trouble of fft_small.h and machine_vectors.h. Now I got

   /sage/local/include/flint/qqbar.h: In function ‘qqbar_struct* _qqbar_vec_init(mp_limb_signed_t)’:
    /sage/local/include/flint/qqbar.h:57:33: error: invalid conversion from ‘void*’ to ‘qqbar_ptr’ {aka ‘qqbar_struct*’} [-fpermissive]
       57 |     qqbar_ptr vec = flint_malloc(len * sizeof(qqbar_struct));
          |                     ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
          |                                 |
          |                                 void*
    In file included from /sage/local/include/flint/fexpr.h:26,
                     from /sage/local/include/flint/ca.h:26,
                     from /sage/local/include/flint/ca_vec.h:21,
                     from sage/libs/flint/flint_wrap.h:48,
                     from sage/algebras/quatalg/quaternion_algebra_cython.cpp:1203:
    /sage/local/include/flint/calcium.h: In function ‘void calcium_stream_init_str(calcium_stream_struct*)’:
    /sage/local/include/flint/calcium.h:61:26: error: invalid conversion from ‘void*’ to ‘char*’ [-fpermissive]
       61 |     out->s = flint_malloc(16);
          |              ~~~~~~~~~~~~^~~~
          |                          |
          |                          void*
    In file included from /sage/local/include/flint/ca.h:26,
                     from /sage/local/include/flint/ca_vec.h:21,
                     from sage/libs/flint/flint_wrap.h:48,
                     from sage/algebras/quatalg/quaternion_algebra_cython.cpp:1203:
    /sage/local/include/flint/fexpr.h: In function ‘void fexpr_init(fexpr_struct*)’:
    /sage/local/include/flint/fexpr.h:82:30: error: invalid conversion from ‘void*’ to ‘mp_limb_t*’ {aka ‘long unsigned int*’} [-fpermissive]
       82 |     expr->data = flint_malloc(sizeof(ulong));
          |                  ~~~~~~~~~~~~^~~~~~~~~~~~~~~
          |                              |
          |                              void*
    /sage/local/include/flint/fexpr.h: In function ‘fexpr_struct* _fexpr_vec_init(mp_limb_signed_t)’:
    /sage/local/include/flint/fexpr.h:97:33: error: invalid conversion from ‘void*’ to ‘fexpr_ptr’ {aka ‘fexpr_struct*’} [-fpermissive]
       97 |     fexpr_ptr vec = flint_malloc(sizeof(fexpr_struct) * len);
          |                     ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
          |                                 |
          |                                 void*
    /sage/local/include/flint/fexpr.h: In function ‘void fexpr_fit_size(fexpr_struct*, mp_limb_signed_t)’:
    /sage/local/include/flint/fexpr.h:118:35: error: invalid conversion from ‘void*’ to ‘mp_limb_t*’ {aka ‘long unsigned int*’} [-fpermissive]
      118 |         expr->data = flint_realloc(expr->data, size * sizeof(ulong));
          |                      ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          |                                   |
          |                                   void*
    /sage/local/include/flint/fexpr.h: In function ‘int fexpr_is_builtin_symbol(const fexpr_struct*, mp_limb_signed_t)’:
    /sage/local/include/flint/fexpr.h:313:116: warning: comparison of integer expressions of different signedness: ‘mp_limb_t’ {aka ‘long unsigned int’} and ‘mp_limb_signed_t’ {aka ‘long int’} [-Wsign-compare]
      313 |     return (FEXPR_TYPE(head) == FEXPR_TYPE_SMALL_SYMBOL) && (((head >> 8) & 0xff) == 0) && (FEXPR_BUILTIN_ID(head) == i);
          |                                                                                                                    ^
    /sage/local/include/flint/fexpr.h: In function ‘void fexpr_vec_init(fexpr_vec_struct*, mp_limb_signed_t)’:
    /sage/local/include/flint/fexpr.h:459:36: error: invalid conversion from ‘void*’ to ‘fexpr_struct*’ [-fpermissive]
      459 |         vec->entries = flint_malloc(sizeof(fexpr_struct) * len);
          |                        ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~

@mezzarobba
Copy link
Member

All right, this is not too bad. With the autogeneration, I could just try to match headers in the installed directory rather than the flint sources.

Do you mean that cython complains if you try to cdef extern something from a .h file that is not there? In any case, I think I agree that it can be annoying to have different headers installed depending how the library was built. I asked Fredrik and Albin what they thought of always installing everything.

@fredrik-johansson
Copy link
Contributor

Got passed the trouble of fft_small.h and machine_vectors.h. Now I got

I will try to fix the missing C++ handrails on FLINT's side.

@videlec videlec changed the title Autogeneration flint headers Autogenerate flint headers Oct 12, 2023
@@ -130,19 +130,16 @@ def has_rational_point(self, point=False, algorithm='default',
sage: C = Conic(K, [t^2 - 2, 2*t^3, -2*t^3 - 13*t^2 - 2*t + 18])
sage: C.has_rational_point(point=True)
(True, (-3 : (t + 1)/t : 1))

sage: R.<t> = FiniteField(23)[]
sage: C = Conic([2, t^2 + 1, t^2 + 5])
Copy link
Member

Choose a reason for hiding this comment

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

the # optional is not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mezzarobba also coming from at #35848


sage: # needs sage.rings.number_field
sage: F.<i> = QuadraticField(-1)
sage: R.<t> = F[]
Copy link
Member

Choose a reason for hiding this comment

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

the version with the block # needs is the correct one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mezzarobba also coming from at #35848

@videlec videlec marked this pull request as draft October 12, 2023 20:24
@mezzarobba mezzarobba mentioned this pull request Oct 13, 2023
@videlec
Copy link
Contributor Author

videlec commented Oct 13, 2023

I also wrote a script for auto-generating the deprecated arb headers. It turns out that the current flint is missing 11 function declarations present in sage.libs.arb

acb.pxd: void acb_rising_ui_bs(acb_t z, const acb_t x, unsigned long n, long prec)
acb.pxd: void acb_rising_ui_rs(acb_t z, const acb_t x, unsigned long n, unsigned long step, long prec)
acb.pxd: void acb_rising_ui_rec(acb_t z, const acb_t x, unsigned long n, long prec)
arb_hypgeom.pxd: void arb_hypgeom_legendre_p_rec(arb_t res, arb_t res_prime, unsigned long n, const arb_t x, long prec)
arb.pxd: char *arb_dump_str(const arb_t x)
arb.pxd: void arb_rising_ui_bs(arb_t z, const arb_t x, unsigned long n, long prec)
arb.pxd: void arb_rising_ui_rs(arb_t z, const arb_t x, unsigned long n, unsigned long step, long prec)
arb.pxd: void arb_rising_ui_rec(arb_t z, const arb_t x, unsigned long n, long prec)
arb.pxd: void arb_rising2_ui_bs(arb_t u, arb_t v, const arb_t x, unsigned long n, long prec)
arb.pxd: void arb_rising2_ui_rs(arb_t u, arb_t v, const arb_t x, unsigned long n, unsigned long step, long prec)
arb.pxd: void arb_rising2_ui(arb_t u, arb_t v, const arb_t x, unsigned long n, long prec)

@videlec videlec force-pushed the autogeneration-flint-headers branch from e03d500 to ba9f134 Compare October 13, 2023 07:39
@mezzarobba
Copy link
Member

I also wrote a script for auto-generating the deprecated arb headers. It turns out that the current flint is missing 11 function declarations present in sage.libs.arb

acb.pxd: void acb_rising_ui_bs(acb_t z, const acb_t x, unsigned long n, long prec)
acb.pxd: void acb_rising_ui_rs(acb_t z, const acb_t x, unsigned long n, unsigned long step, long prec)
acb.pxd: void acb_rising_ui_rec(acb_t z, const acb_t x, unsigned long n, long prec)
arb_hypgeom.pxd: void arb_hypgeom_legendre_p_rec(arb_t res, arb_t res_prime, unsigned long n, const arb_t x, long prec)
arb.pxd: char *arb_dump_str(const arb_t x)
arb.pxd: void arb_rising_ui_bs(arb_t z, const arb_t x, unsigned long n, long prec)
arb.pxd: void arb_rising_ui_rs(arb_t z, const arb_t x, unsigned long n, unsigned long step, long prec)
arb.pxd: void arb_rising_ui_rec(arb_t z, const arb_t x, unsigned long n, long prec)
arb.pxd: void arb_rising2_ui_bs(arb_t u, arb_t v, const arb_t x, unsigned long n, long prec)
arb.pxd: void arb_rising2_ui_rs(arb_t u, arb_t v, const arb_t x, unsigned long n, unsigned long step, long prec)
arb.pxd: void arb_rising2_ui(arb_t u, arb_t v, const arb_t x, unsigned long n, long prec)

The *rising* have been removed, arb_dum_str does exist, and arb_hypgeom_legendre_p_rec used to appear in the documentation but actually never existed.

@videlec
Copy link
Contributor Author

videlec commented Oct 14, 2023

@mezzarobba At 8dbaac4 everything compiles but remains three files with doctest errors

  • rings/complex_arb.pyx
  • rings/real_arb.pyx
  • tests/cmdline.py

The two first one looks related to the flint3 update. Don't you have them at #35848?

sage -t --long --random-seed=197884604164061967531234625559645336413 src/sage/rings/complex_arb.pyx
**********************************************************************
File "src/sage/rings/complex_arb.pyx", line 1363, in sage.rings.complex_arb.ComplexBall.__init__
Failed example:
    ComplexBall(CBF100, 10^100)
Expected:
    1.000000000000000000000000000000e+100
Got:
    [1.000000000000000000000000000000e+100 +/- 1e+65]
**********************************************************************
File "src/sage/rings/complex_arb.pyx", line 3011, in sage.rings.complex_arb.ComplexBall.rising_factorial
Failed example:
    ComplexBallField(128)(1).rising_factorial(2**64)
Expected:
    [2.343691126796861348e+347382171305201285713 +/- ...e+347382171305201285694]
Got:
    [2.3436911267968613484246968809169202443e+347382171305201285713 +/- 5.15e+347382171305201285675]
**********************************************************************
2 items had failures:
   1 of  25 in sage.rings.complex_arb.ComplexBall.__init__
   1 of   7 in sage.rings.complex_arb.ComplexBall.rising_factorial
    [664 tests, 2 failures, 3.93 s]
sage -t --long --random-seed=197884604164061967531234625559645336413 src/sage/rings/real_arb.pyx
**********************************************************************
File "src/sage/rings/real_arb.pyx", line 900, in sage.rings.real_arb.RealBallField.gamma
Failed example:
    RBF.gamma(10**20)
Expected:
    [+/- ...e+1956570552410610660600]
Got:
    [1.932849514310098e+1956570551809674817225 +/- 8.97e+1956570551809674817209]
**********************************************************************
File "src/sage/rings/real_arb.pyx", line 1104, in sage.rings.real_arb.RealBallField.double_factorial
Failed example:
    RBF.double_factorial(2**20)
Expected:
    [1.4483729903e+2928836 +/- ...e+2928825]
Got:
    [1.448372990348437e+2928836 +/- 7.97e+2928820]
**********************************************************************
2 items had failures:
   1 of   5 in sage.rings.real_arb.RealBallField.double_factorial
   1 of   6 in sage.rings.real_arb.RealBallField.gamma
    [576 tests, 2 failures, 0.53 s]
sage -t --long --random-seed=197884604164061967531234625559645336413 src/sage/tests/cmdline.py
**********************************************************************
File "src/sage/tests/cmdline.py", line 563, in sage.tests.cmdline.test_executable
Failed example:
    out.startswith("3.")
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/tests/cmdline.py", line 565, in sage.tests.cmdline.test_executable
Failed example:
    err
Expected:
    ''
Got:
    '/home/doctorant/sage/src/bin/sage: ligne 653 : exec: sqlite3 : non trouvé\n'
**********************************************************************
File "src/sage/tests/cmdline.py", line 567, in sage.tests.cmdline.test_executable
Failed example:
    ret
Expected:
    0
Got:
    127
**********************************************************************
1 item had failures:
   3 of 188 in sage.tests.cmdline.test_executable
    [187 tests, 3 failures, 33.75 s]

@mezzarobba
Copy link
Member

At 8dbaac4 everything compiles but remains three files with doctest errors

* `rings/complex_arb.pyx`

* `rings/real_arb.pyx`

* `tests/cmdline.py`

The two first one looks related to the flint3 update. Don't you have them at #35848?

I do. One is flintlib/flint#1410, which Fredrik has no plans to change back before flint-3.0, so I will update the test. The other ones are functions which have become more accurate; I am not sure why I haven't already updated the corresponding tests. (Perhaps the changes come from improvements made between 3.0-alpha1 and 3.0-rc1 ?)

@videlec videlec force-pushed the autogeneration-flint-headers branch 2 times, most recently from b46ef9d to 8e1238b Compare October 15, 2023 15:56
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 5, 2024
…e relative by absolute imports

    
<!-- ^^^^^
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 -->
We restructure the `all.py` files for modularization.

Guided by the technical constraints outlined in
https://groups.google.com/g/sage-devel/c/ozh-7ZZ848s, sagemath#35095 defines
distribution packages (pip-installable packages) **sagemath-{brial,
combinat, eclib, flint, gap, giac, glpk, graphs, groups, homfly, lcalc,
libbraiding, libecm, linbox, modules, mpmath, ntl, pari, plot,
polyhedra, schemes, singular, standard-no-symbolics, symbolics}**.

When a namespace package such as `sage.misc` is filled by modules from
several distribution packages, we create modules named:
- `src/sage/misc/all__sagemath_environment.py`
- `src/sage/misc/all__sagemath_objects.py`
- `src/sage/misc/all__sagemath_repl.py`

Import statements are moved from `src/sage/misc/all.py` to these files
as appropriate, and `src/sage/misc/all.py` imports `*` from there.

Also some imports are replaced by lazy imports.

The new files provide the top level namespaces for the modularized
distribution packages, thus [enabling modularized testing](https://doc.s
agemath.org/html/en/developer/packaging_sage_library.html#testing-
distribution-packages).

This design was introduced in sagemath#29865 (merged in Jan 2022, early in the
Sage 9.6 development cycle).

<!-- 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". -->
- Copied from sagemath#35095.
- Part of sagemath#29705

Moreover, applied a one-line command to replace relative by absolute
imports, thus complementing sagemath#36666, which does not touch `.all*` files.

The changes to other files in `sage.modules` etc. come from the PRs
sagemath#36597, sagemath#36572, sagemath#36588, sagemath#36589 merged here and do not need review.

<!-- 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.
- [x] 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#36597 (merged here to resolve merge conflict)
- Depends on sagemath#36572 (merged here to
resolve merge conflict)
- Depends on sagemath#36588 (merged here to
resolve merge conflict)
- Depends on sagemath#36589 (merged here to
resolve merge conflict)
- Depends on sagemath#36449 (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#36676
Reported by: Matthias Köppe
Reviewer(s): David Coudert, John H. Palmieri, Matthias Köppe, Tobias Diez
@vbraun vbraun merged commit d30ac1c into sagemath:develop Jan 14, 2024
11 of 17 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Jan 14, 2024
@antonio-rojas
Copy link
Contributor

This breaks build for me. Sagelib-only build against system libraries on Arch

In file included from sage/libs/flint/flint_wrap.h:83,
                 from build/cythonized/sage/libs/linbox/linbox_flint_interface.cpp:1253:
/usr/include/fplll/defs.h:133:11: error: expected unqualified-id before numeric constant
  133 | const int CPU_SIZE_1       = 53;
      |           ^~~~~~~~~~
/usr/include/fplll/defs.h:146:11: error: expected unqualified-id before numeric constant
  146 | const int SIZE_RED_FAILURE_THRESH = 5;
      |           ^~~~~~~~~~~~~~~~~~~~~~~

and a few other similar failures

@antonio-rojas
Copy link
Contributor

The problem is that CPU_SIZE_1 and SIZE_RED_FAILURE_THRESH are defined in flint/fmpz_lll.h and then redefined in fplll/defs.h, which is pulled via linbox. In sage-the-distro the issue doesn't happen because fplll integration is disabled in linbox.

@videlec
Copy link
Contributor Author

videlec commented Jan 15, 2024

Is it it a problem with flint itself then?

@antonio-rojas
Copy link
Contributor

Is it it a problem with flint itself then?

It's not a problem of flint or fplll per se. They just have conflicting definitions of two macros, so they can't be simultaneously included.

@videlec
Copy link
Contributor Author

videlec commented Jan 15, 2024

Did you contact upstream maintainers (@fredrik-johansson @ClementPernet)?

@antonio-rojas
Copy link
Contributor

antonio-rojas commented Jan 15, 2024

Regardless of whether upstreams do something about this conflict in the future, this needs to be fixed in Sage as it's currently broken on systems where linbox is built with fplll. Proposed fix at #37064

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 21, 2024
    
Undefine flint macros that conflict with fplll variables and break build
when both are included

See sagemath#36449 (comment)
    
URL: sagemath#37064
Reported by: Antonio Rojas
Reviewer(s): Tobias Diez, Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 1, 2024
…e relative by absolute imports

    
<!-- ^^^^^
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 -->
We restructure the `all.py` files for modularization.

Guided by the technical constraints outlined in
https://groups.google.com/g/sage-devel/c/ozh-7ZZ848s, sagemath#35095 defines
distribution packages (pip-installable packages) **sagemath-{brial,
combinat, eclib, flint, gap, giac, glpk, graphs, groups, homfly, lcalc,
libbraiding, libecm, linbox, modules, mpmath, ntl, pari, plot,
polyhedra, schemes, singular, standard-no-symbolics, symbolics}**.

When a namespace package such as `sage.misc` is filled by modules from
several distribution packages, we create modules named:
- `src/sage/misc/all__sagemath_environment.py`
- `src/sage/misc/all__sagemath_objects.py`
- `src/sage/misc/all__sagemath_repl.py`

Import statements are moved from `src/sage/misc/all.py` to these files
as appropriate, and `src/sage/misc/all.py` imports `*` from there.

Also some imports are replaced by lazy imports.

The new files provide the top level namespaces for the modularized
distribution packages, thus [enabling modularized testing](https://doc.s
agemath.org/html/en/developer/packaging_sage_library.html#testing-
distribution-packages).

This design was introduced in sagemath#29865 (merged in Jan 2022, early in the
Sage 9.6 development cycle).

<!-- 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". -->
- Copied from sagemath#35095.
- Part of sagemath#29705

Moreover, applied a one-line command to replace relative by absolute
imports, thus complementing sagemath#36666, which does not touch `.all*` files.

The changes to other files in `sage.modules` etc. come from the PRs
sagemath#36597, sagemath#36572, sagemath#36588, sagemath#36589 merged here and do not need review.

<!-- 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.
- [x] 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#36597 (merged here to resolve merge conflict)
- Depends on sagemath#36572 (merged here to
resolve merge conflict)
- Depends on sagemath#36588 (merged here to
resolve merge conflict)
- Depends on sagemath#36589 (merged here to
resolve merge conflict)
- Depends on sagemath#36449 (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#36676
Reported by: Matthias Köppe
Reviewer(s): David Coudert, John H. Palmieri, Kwankyu Lee, Matthias Köppe, Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 2024
    
The FLINT headers are generated manually using a script from sagemath#36449.
However, the documentations are not quite clear, and neither is the
commit that the current headers are generated from. This commit adds
these two details.

@videlec do you mind reviewing? I am not sure if the `flint-commit.txt`
is an acceptable way to put this information, or where I should put it.
Also I'm aware that this script might be moved in the future (since it
shouldn't belong in `sage_setup`?) but that shouldn't conflict with this
PR.
    
URL: sagemath#37458
Reported by: grhkm21
Reviewer(s): Vincent Delecroix
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.

None yet

6 participants