#18450 closed enhancement (fixed)
Define library dependencies in .pxd files
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | cython | Keywords: | |
Cc: | jpflori, gouezel | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Sebastien Gouezel |
Report Upstream: | Fixed upstream, but not in a stable release. | Work issues: | |
Branch: | 0be796e (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | #18455 | Stopgaps: |
Description (last modified by )
Rather than defining libraries in module_list.py
or in .pyx
files, we should add them to the .pxd
files where the declarations are.
In this branch, we do this as proof-of-concept for pari
and gmp
. Other tickets can be opened later for other libraries. We still leave the libraries explicitly in module_list.py
when needed as dependency of another library, for example mpfr
depends on gmp
.
One annoying part is that the order of libraries is not defined, so we need to manually re-order them using one global library order. Since the order of libraries currently specified in src/module_list.py
is far from a DAG, many extensions will be compiled with a different library order than before.
To clean up, we also remove the explicit mention of stdc++
for c++
code (which is automatically added anyway).
These are all modules where the list of libraries was actually changed:
These were probably underlinked before:
sage/rings/polynomial/pbori
: addedgmp
sage/rings/real_arb
(optional): addedgmp
These use inline functions or macros of libraries but don't use exported functions. So they don't need to link to the library, but it's added anyway:
sage/rings/rational
: addedpari
sage/rings/padics/common_conversion
: addedpari
Modules using the Sage/PARI interface but not directly using PARI will now link against pari
. Strictly speaking, this is a bug but it's hard to avoid:
sage/rings/padics/padic_capped_absolute_element
: addedpari
sage/rings/padics/padic_capped_relative_element
: addedpari
sage/rings/padics/padic_fixed_mod_element
: addedpari
sage/rings/real_double
: addedpari
This branch adds gmp
for all modules where bitsets are used. Some bitset functions require GMP, others do not. These modules were only using the ones not implemented using GMP:
sage/combinat/debruijn_sequence
: addedgmp
sage/combinat/words/word_char
: addedgmp
sage/graphs/base/static_sparse_backend
: addedgmp
sage/graphs/weakly_chordal
: addedgmp
Unused libraries are removed:
sage/graphs/graph_generators_pyx
: removedgmp
sage/groups/semimonomial_transformations/semimonomial_transformation
: removedgmp
sage/modular/modsym/p1list
: removedgmp
sage/modules/vector_mod2_dense
: removedgmp
We need 1 patch to upstream Cython to fix dependency handling:
- https://github.com/cython/cython/pull/381 (accepted)
Change History (47)
comment:1 Changed 7 years ago by
- Description modified (diff)
comment:2 Changed 7 years ago by
- Cc jpflori added
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
- Description modified (diff)
comment:5 Changed 7 years ago by
- Description modified (diff)
comment:6 Changed 7 years ago by
- Description modified (diff)
comment:7 Changed 7 years ago by
- Dependencies set to #18455
comment:8 Changed 7 years ago by
- Branch set to u/jdemeyer/define_library_dependencies_in__pxd_files
comment:9 Changed 7 years ago by
- Commit set to 49029fd4db2d1a2874fab1011c8683ecd7a4bb02
- Description modified (diff)
- Report Upstream changed from Fixed upstream, but not in a stable release. to Reported upstream. No feedback yet.
comment:10 Changed 7 years ago by
- Description modified (diff)
comment:11 Changed 7 years ago by
- Description modified (diff)
comment:12 Changed 7 years ago by
- Description modified (diff)
comment:13 Changed 7 years ago by
- Description modified (diff)
comment:14 Changed 7 years ago by
- Description modified (diff)
comment:15 Changed 7 years ago by
- Description modified (diff)
comment:16 Changed 7 years ago by
- Description modified (diff)
comment:17 Changed 7 years ago by
- Description modified (diff)
comment:18 Changed 7 years ago by
- Commit changed from 49029fd4db2d1a2874fab1011c8683ecd7a4bb02 to a1ad51c116ae506ff08e50100b6f83f7c0faec4d
comment:19 Changed 7 years ago by
- Status changed from new to needs_review
comment:20 Changed 7 years ago by
- Description modified (diff)
comment:21 Changed 7 years ago by
- Cc gouezel added
comment:22 Changed 7 years ago by
- Status changed from needs_review to needs_work
- Work issues set to rebase
comment:23 Changed 7 years ago by
- Commit changed from a1ad51c116ae506ff08e50100b6f83f7c0faec4d to 941fa7420f1942114f88bb3a03bd84bb1997d896
Branch pushed to git repo; I updated commit sha1. New commits:
941fa74 | Merge tag '6.8.beta0' into t/18450/define_library_dependencies_in__pxd_files
|
comment:24 Changed 7 years ago by
- Milestone changed from sage-6.7 to sage-6.8
- Status changed from needs_work to needs_review
- Work issues rebase deleted
comment:25 Changed 7 years ago by
- Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers deny it's a bug.
comment:26 Changed 7 years ago by
- Description modified (diff)
comment:27 Changed 7 years ago by
To potential reviewers: if the upstream status of the second Cython patch is what prevents you from reviewing this ticket, I can add a quick work-around for that. Just let me know.
comment:28 Changed 7 years ago by
Please do!
I had a look at the upstream ticket and did not get any strong feeling yet.
comment:29 Changed 7 years ago by
- Description modified (diff)
- Report Upstream changed from Reported upstream. Developers deny it's a bug. to Fixed upstream, but not in a stable release.
- Status changed from needs_review to needs_work
comment:30 Changed 7 years ago by
- Commit changed from 941fa7420f1942114f88bb3a03bd84bb1997d896 to 8b83481163b70d58d3d2f3cf036e55bbb80297fb
comment:31 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:32 Changed 7 years ago by
- Status changed from needs_review to needs_work
With the patch, mpfr
is not picked when compiling partitions_c
(on cygwin)
Part of the log:
build/temp.cygwin-1.7.35-x86_64-2.7/sage/combinat/partitions_c.o: dans la fonction « compute_current_precision »: /home/Sebastien/sage13/sage/src/sage/combinat/partitions_c.cc:448: référence indéfinie vers « __imp_mpfr_init2 » /home/Sebastien/sage13/sage/src/sage/combinat/partitions_c.cc:448:(.text+0x16): relocalisation tronquée pour concorder avec la taille: R_X86_64_PC32 vers le symbole indéfini __imp_mpfr_init2 /home/Sebastien/sage13/sage/src/sage/combinat/partitions_c.cc:452: référence indéfinie vers « __imp_mpfr_set_d »
Full log on request.
comment:33 Changed 7 years ago by
Right, it's partitions_c.cc
which needs those libraries, not the Cython code...
comment:34 Changed 7 years ago by
- Description modified (diff)
comment:35 Changed 7 years ago by
- Commit changed from 8b83481163b70d58d3d2f3cf036e55bbb80297fb to 1c72dd60c5daddd9f3fae41692bb11d4e029f70c
Branch pushed to git repo; I updated commit sha1. New commits:
1c72dd6 | partitions_c.cc needs gmp and mpfr libraries
|
comment:36 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:37 Changed 7 years ago by
- Commit changed from 1c72dd60c5daddd9f3fae41692bb11d4e029f70c to 87be62bd136cf6951c8761a4ba5742778202bda0
Branch pushed to git repo; I updated commit sha1. New commits:
87be62b | Correct Jonathan Bober's name
|
comment:38 Changed 7 years ago by
Coule you please open a follow up ticket for the Pari linking issue?
comment:39 Changed 7 years ago by
You mean the fact that pari
gets added as library when it's not needed? That's now #18583.
comment:40 Changed 7 years ago by
- Status changed from needs_review to needs_work
With the patch, linking problem of 'libecm' with 'gmp' Part of log:
gcc -shared -Wl,--enable-auto-image-base -L/home/Sebastien/sage13/sage/local/lib build/temp.cygwin-1.7.35-x86_64-2.7/build/cythonized/sage/libs/libecm.o -L/home/Sebastien/sage13/sage/local/lib -L/home/Sebastien/sage13/sage/local/lib/python2.7/config -L/home/Sebastien/sage13/sage/local/lib -lcsage -lgmp -lecm -lpython2.7 -o build/lib.cygwin-1.7.35-x86_64-2.7/sage/libs/libecm.dll /home/Sebastien/sage13/sage/local/lib/libecm.a(libecm_la-factor.o): dans la fonction « ecm_init »: /home/Sebastien/sage13/sage/local/var/tmp/sage/build/ecm-6.4.4/src/factor.c:32: référence indéfinie vers « __imp___gmpz_init_set_ui »
comment:41 Changed 7 years ago by
There is a lgmp but before lecm and the linker is picky.
comment:42 Changed 7 years ago by
- Commit changed from 87be62bd136cf6951c8761a4ba5742778202bda0 to 0be796e608b4e876e01d2e17a30fc5a00867e795
Branch pushed to git repo; I updated commit sha1. New commits:
0be796e | Add ecm library before gmp
|
comment:43 Changed 7 years ago by
- Status changed from needs_work to needs_review
Right, I forgot to add ecm
.
comment:44 Changed 7 years ago by
- Reviewers set to Sebastien Gouezel
- Status changed from needs_review to positive_review
Good to go.
comment:45 Changed 7 years ago by
Thanks for the review!
comment:46 Changed 7 years ago by
- Branch changed from u/jdemeyer/define_library_dependencies_in__pxd_files to 0be796e608b4e876e01d2e17a30fc5a00867e795
- Resolution set to fixed
- Status changed from positive_review to closed
comment:47 Changed 6 years ago by
- Commit 0be796e608b4e876e01d2e17a30fc5a00867e795 deleted
Just out of curiosity: Are there follow-ups moving further things into *.pxd
?
(More precisely, into # distutils: ...
declarations.)
New commits:
Remove unneeded inclusions of cdefs.pxi
Add Cython patches to improve handling of dependencies
Define some library dependencies in .pxd/.pxi files