Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#18450 closed enhancement (fixed)

Define library dependencies in .pxd files

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-6.8
Component: cython Keywords:
Cc: Jean-Pierre Flori, Sebastien 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:

Status badges

Description (last modified by Jeroen Demeyer)

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: added gmp
  • sage/rings/real_arb (optional): added gmp

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: added pari
  • sage/rings/padics/common_conversion: added pari

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: added pari
  • sage/rings/padics/padic_capped_relative_element: added pari
  • sage/rings/padics/padic_fixed_mod_element: added pari
  • sage/rings/real_double: added pari

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: added gmp
  • sage/combinat/words/word_char: added gmp
  • sage/graphs/base/static_sparse_backend: added gmp
  • sage/graphs/weakly_chordal: added gmp

Unused libraries are removed:

  • sage/graphs/graph_generators_pyx: removed gmp
  • sage/groups/semimonomial_transformations/semimonomial_transformation: removed gmp
  • sage/modular/modsym/p1list: removed gmp
  • sage/modules/vector_mod2_dense: removed gmp

We need 1 patch to upstream Cython to fix dependency handling:

Change History (47)

comment:1 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 8 years ago by Jeroen Demeyer

Cc: Jean-Pierre Flori added

comment:3 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:4 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:5 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:6 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:7 Changed 8 years ago by Jeroen Demeyer

Dependencies: #18455

comment:8 Changed 8 years ago by Jeroen Demeyer

Branch: u/jdemeyer/define_library_dependencies_in__pxd_files

comment:9 Changed 8 years ago by Jeroen Demeyer

Commit: 49029fd4db2d1a2874fab1011c8683ecd7a4bb02
Description: modified (diff)
Report Upstream: Fixed upstream, but not in a stable release.Reported upstream. No feedback yet.

New commits:

2ed346bRemove unneeded inclusions of cdefs.pxi
c1eb2b8Add Cython patches to improve handling of dependencies
49029fdDefine some library dependencies in .pxd/.pxi files

comment:10 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:11 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:12 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:13 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:14 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:15 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:16 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:17 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:18 Changed 8 years ago by git

Commit: 49029fd4db2d1a2874fab1011c8683ecd7a4bb02a1ad51c116ae506ff08e50100b6f83f7c0faec4d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

57e9278More removals of cdefs.pxi
370106dEven more removals of cdefs
ca87ba3Add Cython patches to improve handling of dependencies
a1ad51cDefine some library dependencies in .pxd/.pxi files

comment:19 Changed 8 years ago by Jeroen Demeyer

Status: newneeds_review

comment:20 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:21 Changed 8 years ago by Sebastien Gouezel

Cc: Sebastien Gouezel added

comment:22 Changed 8 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work
Work issues: rebase

comment:23 Changed 8 years ago by git

Commit: a1ad51c116ae506ff08e50100b6f83f7c0faec4d941fa7420f1942114f88bb3a03bd84bb1997d896

Branch pushed to git repo; I updated commit sha1. New commits:

941fa74Merge tag '6.8.beta0' into t/18450/define_library_dependencies_in__pxd_files

comment:24 Changed 8 years ago by Jeroen Demeyer

Milestone: sage-6.7sage-6.8
Status: needs_workneeds_review
Work issues: rebase

comment:25 Changed 8 years ago by Jeroen Demeyer

Report Upstream: Reported upstream. No feedback yet.Reported upstream. Developers deny it's a bug.

comment:26 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:27 Changed 8 years ago by Jeroen Demeyer

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 8 years ago by Jean-Pierre Flori

Please do!

I had a look at the upstream ticket and did not get any strong feeling yet.

comment:29 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)
Report Upstream: Reported upstream. Developers deny it's a bug.Fixed upstream, but not in a stable release.
Status: needs_reviewneeds_work

comment:30 Changed 8 years ago by git

Commit: 941fa7420f1942114f88bb3a03bd84bb1997d8968b83481163b70d58d3d2f3cf036e55bbb80297fb

Branch pushed to git repo; I updated commit sha1. New commits:

ae16699Merge tag '6.8.beta2' into HEAD
8b83481Move pari/decl.pxi to pari/paridecl.pxd

comment:31 Changed 8 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:32 Changed 8 years ago by Sebastien Gouezel

Status: needs_reviewneeds_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 8 years ago by Jeroen Demeyer

Right, it's partitions_c.cc which needs those libraries, not the Cython code...

comment:34 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:35 Changed 8 years ago by git

Commit: 8b83481163b70d58d3d2f3cf036e55bbb80297fb1c72dd60c5daddd9f3fae41692bb11d4e029f70c

Branch pushed to git repo; I updated commit sha1. New commits:

1c72dd6partitions_c.cc needs gmp and mpfr libraries

comment:36 Changed 8 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:37 Changed 8 years ago by git

Commit: 1c72dd60c5daddd9f3fae41692bb11d4e029f70c87be62bd136cf6951c8761a4ba5742778202bda0

Branch pushed to git repo; I updated commit sha1. New commits:

87be62bCorrect Jonathan Bober's name

comment:38 Changed 8 years ago by Jean-Pierre Flori

Coule you please open a follow up ticket for the Pari linking issue?

comment:39 Changed 8 years ago by Jeroen Demeyer

You mean the fact that pari gets added as library when it's not needed? That's now #18583.

comment:40 Changed 8 years ago by Sebastien Gouezel

Status: needs_reviewneeds_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 8 years ago by Jean-Pierre Flori

There is a lgmp but before lecm and the linker is picky.

comment:42 Changed 8 years ago by git

Commit: 87be62bd136cf6951c8761a4ba5742778202bda00be796e608b4e876e01d2e17a30fc5a00867e795

Branch pushed to git repo; I updated commit sha1. New commits:

0be796eAdd ecm library before gmp

comment:43 Changed 8 years ago by Jeroen Demeyer

Status: needs_workneeds_review

Right, I forgot to add ecm.

comment:44 Changed 8 years ago by Sebastien Gouezel

Reviewers: Sebastien Gouezel
Status: needs_reviewpositive_review

Good to go.

comment:45 Changed 8 years ago by Jeroen Demeyer

Thanks for the review!

comment:46 Changed 8 years ago by Volker Braun

Branch: u/jdemeyer/define_library_dependencies_in__pxd_files0be796e608b4e876e01d2e17a30fc5a00867e795
Resolution: fixed
Status: positive_reviewclosed

comment:47 Changed 6 years ago by Leif Leonhardy

Commit: 0be796e608b4e876e01d2e17a30fc5a00867e795

Just out of curiosity: Are there follow-ups moving further things into *.pxd?

Version 0, edited 6 years ago by Leif Leonhardy (next)
Note: See TracTickets for help on using tickets.