Opened 3 years ago

Last modified 3 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 fbissey)

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 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)
Note: See TracTickets for help on using tickets.