Opened 4 years ago
Last modified 4 years ago
#21625 closed enhancement
Strange linking order for omalloc — at Version 18
Reported by: | jpflori | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.5 |
Component: | packages: standard | Keywords: | |
Cc: | Merged in: | ||
Authors: | François Bissey | Reviewers: | |
Report Upstream: | Fixed upstream, in a later stable release. | Work issues: | |
Branch: | u/fbissey/pkgconfig (Commits) | Commit: | 0accd599126af79c272238689343bddcc45637c8 |
Dependencies: | Stopgaps: |
Description (last modified by )
This is a follow up to #17254 which upgraded to singular 4:
The linking order for Singular libraries is determined by pkgconfig
which gives
-lomalloc -lSingular -lpolys -lfactory -lresources
This does not look that right.
This is because pkgconfig
uses sets that don't preserve the order of libraries (and include directories or macros etc...).
A solution is to migrate pkgconfig
from sets to list.
https://github.com/matze/pkgconfig/issues/10
has been opened upstream to explain the rational, along with a pull request to implement it at
https://github.com/matze/pkgconfig/pull/11
This has now been accepted upstream but is not part of a release yet.
Change History (18)
comment:1 in reply to: ↑ description Changed 4 years ago by
- Summary changed from Starnge linking order for omalloc to Strange linking order for omalloc
comment:2 Changed 4 years ago by
See https://trac.sagemath.org/ticket/17254#comment:510 : """ During linking some Sage files I see -lomalloc -lSingular -lpolys -lfactory -lresources which does not look that right. """
comment:3 Changed 4 years ago by
- Description modified (diff)
- Milestone changed from sage-7.4 to sage-7.5
comment:4 Changed 4 years ago by
That's an annoying defect that I have noticed before in the python interface to pkg-config. The libraries are put in a set and of course it is not preserving the order. It was already evident when using ATLAS for blas/lapack.
$ pkg-config --libs Singular -lSingular -ldl -lpolys -lflint -lmpfr -lntl -lgmp -ldl -lfactory -lflint -lmpfr -lntl -lgmp -lomalloc -lresources
but
$ python Python 2.7.10 (default, Jun 12 2016, 19:30:46) [GCC 5.3.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import pkgconfig >>> singular_pc = pkgconfig.parse('Singular') >>> list(singular_pc['libraries']) [u'omalloc', u'dl', u'Singular', u'polys', u'factory', u'flint', u'ntl', u'gmp', u'resources', u'mpfr'] >>> singular_pc['libraries'] set([u'omalloc', u'dl', u'Singular', u'polys', u'factory', u'flint', u'ntl', u'gmp', u'resources', u'mpfr'])
which is what you are seeing. If you still have a sage with ATLAS somewhere look at what you get for lapack. This needs to be fixed upstream so that something preserving the order is used instead of a set.
comment:5 Changed 4 years ago by
Is this upstream:
?
It does not look very active, last release was three years ago.
comment:6 Changed 4 years ago by
Debian also thinks that is the homepage, yes.
comment:7 Changed 4 years ago by
Well pkg-config
is relatively stable. The nice thing about the set is of course that you don't have repeated elements in it. Is there an "ordered set" type in python?
comment:8 Changed 4 years ago by
After a bit of googling I think upstream should either depend from an "ordered set" package or switch to a dictionary of list instead of a dictionary of set and then remove duplication.
comment:9 Changed 4 years ago by
I have been working on making pkgconfig
use lists instead of sets and then doing some deduplication.
There are two issues with deduplication - which was originally achieved by using sets:
- Is it really necessary, that makes longer lines but
pkg-config
doesn't do it in the first place. Which brings us to second issue - Currently the first instance of everything is kept. Not a problem for macros, may be for include directories and library directories. Definitely a potential problem for libraries.
I don't think deduplication - potentially from the end of the list rather than the beginning of it, is worth it.
Opinion?
comment:10 Changed 4 years ago by
I agree with François: we shouldn't be smarter than necessary. There is nothing wrong with longer command lines (as long as it's not excessive).
comment:11 Changed 4 years ago by
OK, I definitely think it is not worth the bother so I'll probably go with this branch for my upstream pull request https://github.com/kiwifb/pkgconfig/tree/listify. If anyone wants to comment, now is the time.
comment:12 Changed 4 years ago by
Upstream accepted my change, hopefully that will come with a new release soon. I did some experiments, while we will be able to simplify sage's code to populate the various variables *_{libs,library_dirs,include_dir}
it should work out of the box without modification (haven't checked cflags actually).
comment:13 Changed 4 years ago by
- Report Upstream changed from N/A to Fixed upstream, but not in a stable release.
Not sure when upstream will cut a release, should we patch in the meantime?
comment:14 Changed 4 years ago by
I say we patch.
comment:15 Changed 4 years ago by
Working on this. Some adjustments are needed in modules_list.py
.
comment:16 Changed 4 years ago by
- Branch set to u/fbissey/pkgconfig
- Commit set to 0accd599126af79c272238689343bddcc45637c8
- Status changed from new to needs_review
comment:17 Changed 4 years ago by
- Status changed from needs_review to needs_info
For reference, please add a link to the upstream ticket in the patch file and in the ticket description.
comment:18 Changed 4 years ago by
- Description modified (diff)
Replying to jpflori:
You mean during the build of Singular itself or Sage modules using Singular?