Strange linking order for omalloc due to pkgconfig bug

[jpflori]

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

[jdemeyer]

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?

[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. """

[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.

[jpflori]

Is this upstream:

• ​https://github.com/matze/pkgconfig

?

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

[infinity0]

Debian also thinks that is the homepage, yes.

[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?

[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.

[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?

[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).

[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.

[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).

[fbissey]

Not sure when upstream will cut a release, should we patch in the meantime?

[jpflori]

I say we patch.

[fbissey]

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

[fbissey]

Now ready for review.

---

New commits:

​9d4de4a	Add https://github.com/matze/pkgconfig/pull/11
​65a1d2a	migrating numpy helper script to use list
​0accd59	migrate module_list.py to use list from pkgconfig

[jdemeyer]

For reference, please add a link to the upstream ticket in the patch file and in the ticket description.

[git]

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

​07b5632

Add upstream information to the patch

[fbissey]

Hope it is satisfactory.

[jdemeyer]

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

[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.

[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...

[jdemeyer]

This seems to break building scipy, log attached.

[jdemeyer]

Log of failed scipy build

[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).

[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.

[git]

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

​9dd50b6

Only query blas for numpy's and scipy's site.cfg

[fbissey]

You'll need to rebuild numpy before scipy for the fix to work.

[jdemeyer]

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

[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.

[jdemeyer]

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

[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.

[jdemeyer]

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.

[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.

[git]

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

​971c9d4	upgrade pkgconfig to 1.2.2
​b03524b	migrate to list as used by the new pkgconfig 1.2.2.

[fbissey]

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.

[jdemeyer]

33

[fbissey]

All right, I'll do that ASAP.

[git]

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

​bd61fc4

bump numpy's version to make sure it rebuilds

[fbissey]

Done.

[jdemeyer]

Breakage: #22047