#21625 closed enhancement (fixed)
Strange linking order for omalloc due to pkgconfig bug
Reported by: | jpflori | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.5 |
Component: | packages: standard | Keywords: | |
Cc: | Merged in: | ||
Authors: | François Bissey | Reviewers: | Jeroen Demeyer |
Report Upstream: | Fixed upstream, in a later stable release. | Work issues: | |
Branch: | bd61fc4 (Commits, GitHub, GitLab) | Commit: | |
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 and is part of a new release with a few other fixes.
Attachments (1)
Change History (47)
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)
comment:19 Changed 4 years ago by
- Commit changed from 0accd599126af79c272238689343bddcc45637c8 to 07b5632ab1be45ca816b1396105bfc98743460bf
Branch pushed to git repo; I updated commit sha1. New commits:
07b5632 | Add upstream information to the patch
|
comment:20 Changed 4 years ago by
- Status changed from needs_info to needs_review
Hope it is satisfactory.
comment:21 Changed 4 years ago by
Would it make sense to bump the Singular version to force a rebuild with the new pkgconfig
?
comment:22 follow-up: ↓ 24 Changed 4 years ago by
I guess so, of course we need to bump singular
one way or another before 7.5 is released. But I can easily add a commit here and have the singular ticket depend on it.
comment:23 Changed 4 years ago by
- Reviewers set to Jeroen Demeyer
- Summary changed from Strange linking order for omalloc to Strange linking order for omalloc due to pkgconfig bug
comment:24 in reply to: ↑ 22 Changed 4 years ago by
Replying to fbissey:
I guess so, of course we need to bump
singular
one way or another before 7.5 is released. But I can easily add a commit here and have the singular ticket depend on it.
On second thought, the problem is really in pkgconfig
(the Python package) which simply reads the system .pc
files. So there is no need to regenerate the .pc
files. So no further action is needed I guess.
I am now going to do a complete build from scratch with this ticket and see if it works...
comment:25 Changed 4 years ago by
This seems to break building scipy, log attached.
comment:26 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:27 follow-up: ↓ 31 Changed 4 years ago by
Hum, I can reproduce it. It is triggered by the site.cfg
generated for numpy
(installed at local/lib/python2.7/site-packages/numpy/distutils/site.cfg
). There is now a repetition of openblas
for blas_libs
and that seems to be the issue strangely enough (it should be able to take more than one library).
comment:28 Changed 4 years ago by
Ok pkgconfig
(python) does another thing that's weird compared to pkg-config
. I can do this
(sage-sh) fbissey@QCD-nzi3:sage$ pkg-config --libs blas cblas -L/home/fbissey/sandbox/git-fork/sage/local/lib -lopenblas
but pkgconfig
(python) will do two calls to pkg-config
and add them together. Of course with sets that wasn't visible. In any case we should be able to survive with just blas
in numpy
's site.cfg
. I am not sure why Gentoo insists on both - possibly historical reasons.
comment:29 Changed 4 years ago by
- Commit changed from 07b5632ab1be45ca816b1396105bfc98743460bf to 9dd50b6a72f15f173bac66720fc77e10c4ee087b
Branch pushed to git repo; I updated commit sha1. New commits:
9dd50b6 | Only query blas for numpy's and scipy's site.cfg
|
comment:30 Changed 4 years ago by
- Status changed from needs_work to needs_review
You'll need to rebuild numpy
before scipy
for the fix to work.
comment:31 in reply to: ↑ 27 Changed 4 years ago by
I don't really like that fix. It should not be a problem to have -lopenblas -lopenblas
in the libraries.
comment:32 Changed 4 years ago by
I would have to dig inside numpy
. That's where the problem is, really. But it could need a mirror fix in scipy
. I think it would be better to go back to pkgconfig
and add a patch to make its output more consistent, both internally and with pkg-config
. That wouldn't do anything for numpy
/scipy
setup problem of course.
comment:33 Changed 4 years ago by
Please bump the numpy version number since the change you make is non-trivial.
comment:34 Changed 4 years ago by
Was thinking about that. I am not satisfied with the branch as it is anyway. I will hammer some more on pkgconfig
side with another pull upstream as soon as I can. I'll probably revert my last commit but even so it is probably best if numpy
is bumped as well.
comment:35 Changed 4 years ago by
- Status changed from needs_review to needs_work
If you cannot immediately fix the real issue, it's OK for me to merge this hack as-is.
I'm setting it back to needs_work because you still want to work on it.
comment:36 Changed 4 years ago by
I have pushed a few more things upstream and requested a release. The changes I sent upstream makes my last commit unnecessary, so at the very least I'll retract it. If upstream makes a new release, I'll update the package instead of adding more patches.
comment:37 Changed 4 years ago by
- Commit changed from 9dd50b6a72f15f173bac66720fc77e10c4ee087b to b03524b653cde297552f7f34c3b7522b4ccd013f
comment:38 Changed 4 years ago by
- Description modified (diff)
- Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.
- Status changed from needs_work to needs_review
OK, new release from upstream with all fixes included. Now the output is list, which keep the order, and is also strictly mirroring the output of the equivalent pkg-config
command. The extra 2 minor releases are for various fixes to the tests that we don't use in sage but are important for me in Gentoo since I do run nose
tests.
comment:40 Changed 4 years ago by
All right, I'll do that ASAP.
comment:41 Changed 4 years ago by
- Commit changed from b03524b653cde297552f7f34c3b7522b4ccd013f to bd61fc49a33cb0c3c764ba15c0262d344b2e78a1
Branch pushed to git repo; I updated commit sha1. New commits:
bd61fc4 | bump numpy's version to make sure it rebuilds
|
comment:43 Changed 4 years ago by
- Description modified (diff)
comment:44 Changed 4 years ago by
- Status changed from needs_review to positive_review
comment:45 Changed 4 years ago by
- Branch changed from u/fbissey/pkgconfig to bd61fc49a33cb0c3c764ba15c0262d344b2e78a1
- Resolution set to fixed
- Status changed from positive_review to closed
comment:46 Changed 4 years ago by
- Commit bd61fc49a33cb0c3c764ba15c0262d344b2e78a1 deleted
Breakage: #22047
Replying to jpflori:
You mean during the build of Singular itself or Sage modules using Singular?