Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#14603 closed defect (fixed)

Remove the FLINT include directory from module_list.py

Reported by: novoselt Owned by: vbraun
Priority: blocker Milestone: sage-5.10
Component: build Keywords: sage -b rebuilds
Cc: Merged in: sage-5.10.beta4
Authors: Volker Braun Reviewers: Nathann Cohen, Leif Leonhardy, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12173 Stopgaps:

Description

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).

Attachments (1)

trac_14600_lowercase_FLINT.patch (8.6 KB) - added by vbraun 8 years ago.
Updated patch

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by vbraun

Updated patch

comment:1 Changed 8 years ago by vbraun

I've stripped out the wrong and unnecessary flint include_dir

comment:2 Changed 8 years ago by vbraun

  • Summary changed from Rename the FLINT include directory consistently to Remove the FLINT include directory

comment:3 Changed 8 years ago by vbraun

  • Status changed from new to needs_review

comment:4 Changed 8 years ago by vbraun

  • Summary changed from Remove the FLINT include directory to Remove the FLINT include directory from module_list.py

comment:5 Changed 8 years ago by jdemeyer

  • Priority changed from critical to blocker

comment:6 follow-up: Changed 8 years ago by leif

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 = ...).

comment:7 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by ncohen

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

comment:8 in reply to: ↑ 7 Changed 8 years ago by leif

Replying to ncohen:

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).

comment:9 Changed 8 years ago by leif

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.

comment:10 follow-up: Changed 8 years ago by ncohen

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

Nathann

comment:11 in reply to: ↑ 10 Changed 8 years ago by leif

  • Reviewers set to Nathann Cohen, Leif Leonhardy

Replying to ncohen:

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

Nathann

Yes, later today.

comment:12 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.10.beta4
  • Resolution set to fixed
  • Reviewers changed from Nathann Cohen, Leif Leonhardy to Nathann Cohen, Leif Leonhardy, Jeroen Demeyer
  • Status changed from needs_review to closed

Looks good to me.

comment:13 Changed 8 years ago by vbraun

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

Note: See TracTickets for help on using tickets.