#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)
Change History (14)
Changed 8 years ago by
comment:1 Changed 8 years ago by
I've stripped out the wrong and unnecessary flint include_dir
comment:2 Changed 8 years ago by
- Summary changed from Rename the FLINT include directory consistently to Remove the FLINT include directory
comment:3 Changed 8 years ago by
- Status changed from new to needs_review
comment:4 Changed 8 years ago by
- 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
- Priority changed from critical to blocker
comment:6 follow-up: ↓ 7 Changed 8 years ago by
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: ↓ 8 Changed 8 years ago by
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
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
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: ↓ 11 Changed 8 years ago by
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
- 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
- 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
Thanks. Leif's followup will be another ticket, then.
Updated patch