Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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.

Tarball: https://pypi.python.org/packages/9d/ba/80910bbed2b4e646a6adab4474d2e506744c260c7002a0e6b41ef8750d8d/pkgconfig-1.2.2.tar.gz

Attachments (1)

scipy-0.17.1.p0.log (12.3 KB) - added by jdemeyer 3 years ago.
Log of failed scipy build

Download all attachments as: .zip

Change History (47)

comment:1 in reply to: ↑ description Changed 3 years ago by jdemeyer

  • Summary changed from Starnge linking order for omalloc to Strange linking order for omalloc

Replying to jpflori:

This is a follow up to #17254 which upgraded to singular 4:

  • strange linking order (omalloc comes before singular!)

You mean during the build of Singular itself or Sage modules using Singular?

comment:2 Changed 3 years ago by jpflori

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 3 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-7.4 to sage-7.5

comment:4 Changed 3 years ago by fbissey

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 3 years ago by jpflori

Is this upstream:

?

It does not look very active, last release was three years ago.

comment:6 Changed 3 years ago by infinity0

Debian also thinks that is the homepage, yes.

comment:7 Changed 3 years ago by fbissey

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 3 years ago by fbissey

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 3 years ago by fbissey

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?

Last edited 3 years ago by fbissey (previous) (diff)

comment:10 Changed 3 years ago by jdemeyer

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 3 years ago by fbissey

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 3 years ago by fbissey

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 3 years ago by fbissey

  • 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 3 years ago by jpflori

I say we patch.

comment:15 Changed 3 years ago by fbissey

Working on this. Some adjustments are needed in modules_list.py.

comment:16 Changed 3 years ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/pkgconfig
  • Commit set to 0accd599126af79c272238689343bddcc45637c8
  • Status changed from new to needs_review

Now ready for review.


New commits:

9d4de4aAdd https://github.com/matze/pkgconfig/pull/11
65a1d2amigrating numpy helper script to use list
0accd59migrate module_list.py to use list from pkgconfig

comment:17 Changed 3 years ago by jdemeyer

  • 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 3 years ago by fbissey

  • Description modified (diff)

comment:19 Changed 3 years ago by git

  • Commit changed from 0accd599126af79c272238689343bddcc45637c8 to 07b5632ab1be45ca816b1396105bfc98743460bf

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

07b5632Add upstream information to the patch

comment:20 Changed 3 years ago by fbissey

  • Status changed from needs_info to needs_review

Hope it is satisfactory.

comment:21 Changed 3 years ago by jdemeyer

Would it make sense to bump the Singular version to force a rebuild with the new pkgconfig?

comment:22 follow-up: Changed 3 years ago by 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.

comment:23 Changed 3 years ago by jdemeyer

  • 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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

This seems to break building scipy, log attached.

Changed 3 years ago by jdemeyer

Log of failed scipy build

comment:26 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:27 follow-up: Changed 3 years ago by fbissey

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 3 years ago by fbissey

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 3 years ago by git

  • Commit changed from 07b5632ab1be45ca816b1396105bfc98743460bf to 9dd50b6a72f15f173bac66720fc77e10c4ee087b

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

9dd50b6Only query blas for numpy's and scipy's site.cfg

comment:30 Changed 3 years ago by fbissey

  • 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 3 years ago by jdemeyer

I don't really like that fix. It should not be a problem to have -lopenblas -lopenblas in the libraries.

comment:32 Changed 3 years ago by fbissey

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 pkgconfigand 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 3 years ago by jdemeyer

Please bump the numpy version number since the change you make is non-trivial.

comment:34 Changed 3 years ago by fbissey

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 3 years ago by jdemeyer

  • 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 3 years ago by fbissey

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.

Last edited 3 years ago by fbissey (previous) (diff)

comment:37 Changed 3 years ago by git

  • Commit changed from 9dd50b6a72f15f173bac66720fc77e10c4ee087b to b03524b653cde297552f7f34c3b7522b4ccd013f

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

971c9d4upgrade pkgconfig to 1.2.2
b03524bmigrate to list as used by the new pkgconfig 1.2.2.

comment:38 Changed 3 years ago by fbissey

  • 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:39 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

33

comment:40 Changed 3 years ago by fbissey

All right, I'll do that ASAP.

comment:41 Changed 3 years ago by git

  • Commit changed from b03524b653cde297552f7f34c3b7522b4ccd013f to bd61fc49a33cb0c3c764ba15c0262d344b2e78a1

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

bd61fc4bump numpy's version to make sure it rebuilds

comment:42 Changed 3 years ago by fbissey

  • Status changed from needs_work to needs_review

Done.

comment:43 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:44 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:45 Changed 3 years ago by vbraun

  • Branch changed from u/fbissey/pkgconfig to bd61fc49a33cb0c3c764ba15c0262d344b2e78a1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:46 Changed 3 years ago by jdemeyer

  • Commit bd61fc49a33cb0c3c764ba15c0262d344b2e78a1 deleted

Breakage: #22047

Note: See TracTickets for help on using tickets.