Remove the FLINT include directory from module_list.py

[novoselt]

Upon the upgrade of FLINT from 1.5.2 to 2.3 (#12173), its include directory in $SAGE_LOCAL/include was changed from all upper to all lower case (FLINT/-> flint/), and Sage library files including stuff from there were changed accordingly.

But, as originally reported on #14600, $SAGE_SRC/module_list.py still contained references to the old name, which led to continual unnecessary rebuilds of 23 extension modules having depends = flint_depends, because the latter was still [SAGE_INC + '/FLINT/flint.h'], a now non-existing file.

---

It seems the many include_dirs = [SAGE_INC + '/FLINT'] in module_list.py were (and still are) superfluous anyway; i.e., apparently all files using FLINT already #include "flint/foo.h" rather than just foo.h, so we should remove those entries as well.

---

For backwards compatibility, it might be worth keeping a symbolic link $SAGE_LOCAL/include/FLINT -> flint, but that should be created in an updated FLINT spkg (more precisely, its spkg-install).

Depends on #12173

Component: build

Keywords: sage -b rebuilds

Author: Volker Braun

Reviewer: Nathann Cohen, Leif Leonhardy, Jeroen Demeyer

Merged: sage-5.10.beta4

Issue created by migration from https://trac.sagemath.org/ticket/14603

[vbraun]

Updated patch

[vbraun]

comment:1

Attachment: trac_14600_lowercase_FLINT.patch.gz

I've stripped out the wrong and unnecessary flint include_dir

[nexttime]

comment:6

In fact, we could just let all modules linking to FLINT automatically depend on flint.h, like we do for GMP/MPIR; no need to "manually" specifiy flint_depends. (See $SAGE_SRC/setup.py, lib_headers = ...).

[nathanncohen]

comment:7

In fact, we could just let all modules linking to FLINT automatically depend on flint.h,

So is this patch still waiting for a review ? It does the job and it looks good to me, but if you think that it should be done in a different way...

Nathann

[nexttime]

comment:8

Replying to @nathanncohen:

In fact, we could just let all modules linking to FLINT automatically depend on flint.h,

So is this patch still waiting for a review ? It does the job and it looks good to me, but if you think that it should be done in a different way...

Nathann

Patch "Looks good to me."^TM.

What I suggested is just one more step forward... (making it more modular and less error-prone by removing even more from module_list.py).

[nexttime]

comment:9

P.S.: The code in setup.py I was referring to is

# Do not put all, but only the most common libraries and their headers
# (that are likely to change on an upgrade) here:
# [At least at the moment. Make sure the headers aren't copied with "-p",
# or explicitly touch them in the respective spkg's spkg-install.]
lib_headers = { "gmp":     [ os.path.join(SAGE_INC,'gmp.h') ],   # cf. #8664, #9896
                "gmpxx":   [ os.path.join(SAGE_INC,'gmpxx.h') ]
              }

for m in ext_modules:

    for lib in lib_headers.keys():
        if lib in m.libraries:
            m.depends += lib_headers[lib]

    # FIMXE: Do NOT link the following libraries to each and
    #        every module (regardless of the language btw.):
    m.libraries = ['csage'] + m.libraries + ['stdc++', 'ntl']

    m.extra_compile_args += extra_compile_args
    m.extra_link_args += extra_link_args
    m.library_dirs += ['%s/lib' % SAGE_LOCAL]

We'd just have to add

                "flint":   [ os.path.join(SAGE_INC,"flint","flint.h") ]

there.

[nathanncohen]

comment:10

Well, if you feel like adding a reviewer's patch that does that then I will review both :-)

Nathann

[nexttime]

comment:11

Replying to @nathanncohen:

Well, if you feel like adding a reviewer's patch that does that then I will review both :-)

Nathann

Yes, later today.

[nexttime]

Reviewer: Nathann Cohen, Leif Leonhardy

[jdemeyer]

comment:12

Looks good to me.

[jdemeyer]

Changed reviewer from Nathann Cohen, Leif Leonhardy to Nathann Cohen, Leif Leonhardy, Jeroen Demeyer

[jdemeyer]

Merged: sage-5.10.beta4

[vbraun]

comment:13

Thanks. Leif's followup will be another ticket, then.