Opened 4 years ago

Closed 4 years ago

#20133 closed enhancement (fixed)

Use pkg-config for Sage setup

Reported by: vbraun Owned by:
Priority: major Milestone: sage-7.1
Component: build Keywords:
Cc: fbissey Merged in:
Authors: Volker Braun, François Bissey Reviewers: Volker Braun, François Bissey
Report Upstream: N/A Work issues:
Branch: 5d1cf57 (Commits) Commit: 5d1cf572adbda8742305e5c59711c46b5f96fcf9
Dependencies: Stopgaps:

Description (last modified by vbraun)

Use pkg-config for linking Sage extension modules where possible

Change History (63)

comment:1 Changed 4 years ago by vbraun

  • Branch set to u/vbraun/use_pkg_config_for_sage_setup

comment:2 Changed 4 years ago by git

  • Commit set to 066d7e8fd64c8466cfeb3407919d2781a3404c4e

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

066d7e8Use pkg-config for Sage linking

comment:3 Changed 4 years ago by vbraun

  • Authors set to Volker Braun
  • Component changed from PLEASE CHANGE to build
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to enhancement

comment:4 Changed 4 years ago by vbraun

  • Dependencies set to #20130

comment:5 Changed 4 years ago by git

  • Commit changed from 066d7e8fd64c8466cfeb3407919d2781a3404c4e to 33d50b200d54bcf827b99d54184b16062db0ea85

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

18d59bfMove third-party packages to pkg-config blas
de7efd2Fix typo: use lapack.pc not blas.pc
f1302b2Bump patchlevels
555b258Iml version/patchlevel fix
8f0e007Use pkg-config for Sage linking
33d50b2Fix typo

comment:6 Changed 4 years ago by git

  • Commit changed from 33d50b200d54bcf827b99d54184b16062db0ea85 to a7772cee4683aeb42ab7ecd26792850bcaa794e5

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

a7772ceDo not doctest that cblas has a particular name

comment:7 Changed 4 years ago by vbraun

  • Cc fbissey added
  • Status changed from new to needs_review

comment:8 Changed 4 years ago by fbissey

  • Status changed from needs_review to needs_work

I think there is more to do in cython.py. In sage-on-gentoo I added a libdir in the output of pyx_preparse to take into account potential blas/lapack outside of sage.

Which means I had also to add it in cython, the extra stuff look like this

@@ -208,8 +192,8 @@
         '.../sage/ext',
         '.../cysignals'],
         'c',
-        [], ['-w', '-O2'])
-        sage: s, libs, inc, lang, f, args = pyx_preparse("# clang c++\n #clib foo\n # cinclude bar\n")
+        [], ['-w', '-O2'],...)
+        sage: s, libs, inc, lang, f, args, libdirs = pyx_preparse("# clang c++\n #clib foo\n # cinclude bar\n")
         sage: lang
         'c++'
 
@@ -234,7 +218,7 @@
         '.../sage/ext',
         '.../cysignals']
 
-        sage: s, libs, inc, lang, f, args = pyx_preparse("# cargs -O3 -ggdb\n")
+        sage: s, libs, inc, lang, f, args, libdirs = pyx_preparse("# cargs -O3 -ggdb\n")
         sage: args
         ['-w', '-O2', '-O3', '-ggdb']
 
@@ -263,6 +247,7 @@
     s = """\ninclude "cdefs.pxi"\n""" + s
     s = """\ninclude "cysignals/signals.pxi"  # ctrl-c interrupt block support\ninclude "stdsage.pxi"\n""" + s
     args, s = parse_keywords('cargs', s)
+    libdirs = cblas_library_dirs()
     args = ['-w','-O2'] + args
 
     # Add cysignals directory to includes
@@ -271,7 +256,7 @@
         if os.path.isdir(cysignals_path):
             inc.append(cysignals_path)
 
-    return s, libs, inc, lang, additional_source_files, args
+    return s, libs, inc, lang, additional_source_files, args, libdirs
 
 ################################################################
 # If the user attaches a .spyx file and changes it, we have
@@ -413,7 +398,7 @@
 
     F = open(filename).read()
 
-    F, libs, includes, language, additional_source_files, extra_args = pyx_preparse(F)
+    F, libs, includes, language, additional_source_files, extra_args, libdirs = pyx_preparse(F)
 
     # add the working directory to the includes so custom headers etc. work
     includes.append(os.path.split(os.path.splitext(filename)[0])[0])
@@ -453,19 +438,17 @@
 
 from sage.env import SAGE_LOCAL
 
-extra_link_args =  ['-L' + SAGE_LOCAL + '/lib']
 extra_compile_args = %s
 
 ext_modules = [Extension('%s', sources=['%s.%s', %s],
                      libraries=%s,
-                     library_dirs=[SAGE_LOCAL + '/lib/'],
+                     library_dirs=[SAGE_LOCAL + '/lib/', '%s'],
                      extra_compile_args = extra_compile_args,
-                     extra_link_args = extra_link_args,
                      language = '%s' )]
 
 setup(ext_modules = ext_modules,
       include_dirs = %s)
-    """%(extra_args, name, name, extension, additional_source_files, libs, language, includes)
+    """%(extra_args, name, name, extension, additional_source_files, libs, libdirs, language, includes)
     open('%s/setup.py'%build_dir,'w').write(setup)
     cython_include = ' '.join(["-I '%s'"%x for x in includes if len(x.strip()) > 0 ])
 

unless you have that sorted some other way I didn't notice, I would that part to be included.

comment:9 Changed 4 years ago by vbraun

Fine with me; Since you already have it as a patch can you just add a commit so I don't have to apply your diff by hand?

comment:10 Changed 4 years ago by fbissey

Will do as soon as my day job allows me, I'll put it in positive review afterwards.

comment:11 Changed 4 years ago by jdemeyer

I suggest to move the pkgconfig stuff you added to src/module_list.py to some place within the sage package. Then it can more easily be imported by other packages too, analogous to the sage_include_directories() function in src/sage/env.py.

Suppose I write a spkg which uses Sage and also links against BLAS, then it would be nice if I could reuse that code. That would also remove the need to duplicate that code in cython.py.

comment:12 Changed 4 years ago by fbissey

Admittedly, it sounds like a good idea. I guess it should be a new file otherwise env.py will end up being a dumping ground. But when you talk about blas in particular it is true that we have a straight duplication between cython.py and module_list.py (even in my original version in sage-on-gentoo).

On the other hand I am not sure why in your use case you couldn't use the .pc files directly.

Of course that could be more convenient but I have a feeling it is on the slippery slope of putting the kitchen sink into sage.

comment:13 Changed 4 years ago by fbissey

  • Branch changed from u/vbraun/use_pkg_config_for_sage_setup to u/fbissey/use_pkg_config_for_sage_setup
  • Commit changed from a7772cee4683aeb42ab7ecd26792850bcaa794e5 to 6d8e32f88985f4498c0e7e1f95c7aef0b8170781

OK pushing branch including changes to pyx_preparse note that cblas' include directory is not currently added. It hasn't proved necessary yet but could be in the future.


New commits:

6d8e32fAdd libdirs (including blas' libdir) pyx_preparse

comment:14 Changed 4 years ago by fbissey

  • Status changed from needs_work to positive_review

comment:15 Changed 4 years ago by git

  • Commit changed from 6d8e32f88985f4498c0e7e1f95c7aef0b8170781 to dd84287f8f793661387977e65e9761722809c759
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

dd84287remove extranauseous brackets

comment:16 Changed 4 years ago by fbissey

Sorry for the noise, I thought I had removed those brackets before the last commit.

comment:17 Changed 4 years ago by fbissey

  • Status changed from needs_review to needs_work

OK, Volker has done things differently from what I used to do and now I have found that the difference is fairly important.

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

comment:18 Changed 4 years ago by git

  • Commit changed from dd84287f8f793661387977e65e9761722809c759 to 12d54b5620e55af324a9b1dddefc2acda3142d88

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

12d54b5proper list building for library_dir

comment:19 Changed 4 years ago by fbissey

Not a difference between I have done and Volker has done. It looks like I had a bug for some time that happens not to be triggered in the common case in sage-on-gentoo. Let's see if the patchbot will come clean this time.

comment:20 Changed 4 years ago by vbraun

IMHO in an ideal world you'd just run pkg-config --libs blas gsl zlib. By telling pkg-config all dependencies you enable it to order the libraries, too. The current verbosity is just a symptom of not having pc files for all libraries. We should rather work on that than move the current workaround into the Sage library.

comment:21 Changed 4 years ago by jdemeyer

If you need to add flags like

+              library_dirs = gsl_library_dirs,
+              include_dirs = gsl_include_dirs),

you must put them in the .pxd files, not in module_list.py. The reason is that there are a lot more extensions than the ones in sage/gsl/*.pyx which use the gsl library. If you add those dirs and includes to the gsl .pxd files, then all extensions using gsl will automatically have the correct flags added.

This currently applies only to gsl, since all other libraries that you changed are listed directly in module_list.py.

comment:22 Changed 4 years ago by jdemeyer

Given that you add the BLAS libs to gsl_libs, I guess you should also add the BLAS library and include dirs to the GSL library and include dirs.

comment:23 follow-up: Changed 4 years ago by vbraun

Correctly treating library and include dirs is only relevant for non-Sage, non-system GSL installs. IMHO there is no point in worrying about that at the moment as we can't test it so it isn't going to work no matter what. First we need a way to configure Sage to not build our own GSL...

comment:24 in reply to: ↑ 23 Changed 4 years ago by jdemeyer

Replying to vbraun:

IMHO there is no point in worrying about that at the moment

In that case, just leave GSL alone in this branch: either do it right or don't do it at all.

I am very much against adding known-broken code with the excuse that you cannot test it, so you cannot see that it is in fact broken.

comment:25 Changed 4 years ago by fbissey

I must say that I was surprised by Volker carpet bombing of all things that have a .pc file when we could have limited ourselves to blas and lapack (for coinor) where we want to be able to use something installed outside of sage.

I don't think it would be needed for moving to hashstack as you would build everything in same prefix (I think). Now if you want to package for something like spack that becomes more interesting. But some other stuff will have to happen before you get anywhere there.

comment:26 Changed 4 years ago by vbraun

IMHO not having a pc file for a shared library is a bug in an of itself. We are just reinventing the wheel if we try to write our own shared library configuration system. Pkgconfig a proven tool, just use it. E.g. look how elegantly xorg and Gnome handle >~100 shared libraries whereas we are to the hilt in a shit creek with just a handful.

comment:27 follow-up: Changed 4 years ago by vbraun

Correction: Both not having a pc file and not using an existing pc file are a bug in an of itself.

comment:28 Changed 4 years ago by git

  • Commit changed from 12d54b5620e55af324a9b1dddefc2acda3142d88 to d4525c0860db5ea8657959ebcf33afb33c5b2b03

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

d4525c0Add extra output to pyx_preparse doctest

comment:29 in reply to: ↑ 27 Changed 4 years ago by fbissey

Replying to vbraun:

Correction: Both not having a pc file and not using an existing pc file are a bug in an of itself.

Fine it is a bug. But if we are drinking that kool-aid, we'll have to also drink Jeroen's kool-aid about .pxd files. Otherwise it would be like "having your cake and eat it too" as the English would say.

And apologies for missing one last bit of my old patch to that doctest.


New commits:

d4525c0Add extra output to pyx_preparse doctest

comment:30 follow-up: Changed 4 years ago by fbissey

Migrating building directives to .pxd files is an ongoing process. I suggest we merge this ticket as is but start to migrate more aggressively. We have one migration to blas/lapack .pc to do on the top of my head and that is numpy and scipy. Once that is done, use of alternative blas/lapack in vanilla sage will have come a long way.

comment:31 in reply to: ↑ 30 ; follow-up: Changed 4 years ago by jdemeyer

Replying to fbissey:

Migrating building directives to .pxd files is an ongoing process.

In the case of GSL, the library declarations are already migrated to .pxd files. So, if you want to fine-tune these declarations (by adding library_dirs for example), those fine-tuned declarations should also be in .pxd files. Otherwise it's a mess and we're worse off than before.

Concretely, this should be reverted:

-    Extension('*', ['sage/gsl/*.pyx']),
+    Extension('*', ['sage/gsl/*.pyx'],
+              library_dirs = gsl_library_dirs,
+              include_dirs = gsl_include_dirs),

comment:32 Changed 4 years ago by fbissey

  • Work issues set to fix gsl build instructions

OK I will take care of that but that may be in the morning in my time zone.

comment:33 Changed 4 years ago by jdemeyer

Note that this ticket conflicts badly with #17635.

comment:34 in reply to: ↑ 31 ; follow-up: Changed 4 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

Migrating building directives to .pxd files is an ongoing process.

In the case of GSL, the library declarations are already migrated to .pxd files. So, if you want to fine-tune these declarations (by adding library_dirs for example), those fine-tuned declarations should also be in .pxd files. Otherwise it's a mess and we're worse off than before.

Concretely, this should be reverted:

-    Extension('*', ['sage/gsl/*.pyx']),
+    Extension('*', ['sage/gsl/*.pyx'],
+              library_dirs = gsl_library_dirs,
+              include_dirs = gsl_include_dirs),

Jeroen, I can see that some distutils stuff is indeed already present for sage/libs/gsl/*.pxd files, but there is nothing for sage/gsl/*.pxd should I regularise that as well or is there something I am missing (like why is there sage/libs/gsl/ and sage/gsl/)?

comment:35 in reply to: ↑ 34 Changed 4 years ago by jdemeyer

Replying to fbissey:

but there is nothing for sage/gsl/*.pxd

Indeed and it should be that way. The # distutils stuff should be put where the library interface is declared and only there.

comment:36 Changed 4 years ago by git

  • Commit changed from d4525c0860db5ea8657959ebcf33afb33c5b2b03 to 6203f778428cd2af89a24633262fb9e441d19a43

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

6203f77Use the information of gsl.pc directly in sage/libs/gsl/*.pxd files as necessary

comment:37 Changed 4 years ago by fbissey

OK, hope I did it all correctly. Three .pxd files didn't have any distutils instructions. For those I only added stuff as I was thinking it could be necessary (include directory for two of them)..

comment:38 Changed 4 years ago by fbissey

Anyone has a clue why the librae patchbot has an apparently unrelated doctest failure

sage -t --long src/sage/all.py
**********************************************************************
File "src/sage/all.py", line 305, in sage.all._write_started_file
Failed example:
    os.path.isfile(started_file)
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of   3 in sage.all._write_started_file
    [20 tests, 1 failure, 0.57 s]
----------------------------------------------------------------------
sage -t --long src/sage/all.py  # 1 doctest failed
----------------------------------------------------------------------

All other doctests passing.

comment:39 Changed 4 years ago by jdemeyer

  • Work issues changed from fix gsl build instructions to fix gsl build instructionsl pkgconfig import
  1. You need to define the GSL aliases, note the command line:

gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -Igsl_include_dirs -I/home/worker/sage-patchbot/local/lib/python2.7/site-packages/cysignals -I/home/worker/sage-patchbot/local/include -I/home/worker/sage-patchbot/local/include/python2.7 -I/home/worker/sage-patchbot/local/lib/python2.7/site-packages/numpy/core/include -I/home/worker/sage-patchbot/src -I/home/worker/sage-patchbot/src/sage/ext -I/home/worker/sage-patchbot/src/build/cythonized -I/home/worker/sage-patchbot/src/build/cythonized/sage/ext -I/home/worker/sage-patchbot/local/include/python2.7 -c /home/worker/sage-patchbot/src/build/cythonized/sage/gsl/gsl_array.c -o build/temp.linux-x86_64-2.7/home/worker/sage-patchbot/src/build/cythonized/sage/gsl/gsl_array.o -DGSL_DISABLE_DEPRECATED -fno-strict-aliasing -w

  1. Can you try to not import pkgconfig every time that Sage starts?

comment:40 follow-up: Changed 4 years ago by fbissey

I should have known. I supposed pkgconfig is called from cython.py somehow at startup. Would putting the import pkgconfig in a method stop that problem?

comment:41 in reply to: ↑ 40 Changed 4 years ago by jdemeyer

Replying to fbissey:

Would putting the import pkgconfig in a method stop that problem?

Yes. Even better, import the whole cython.py lazily.

comment:42 Changed 4 years ago by jdemeyer

This should work if you add import sage.misc.cython to some doctests:

  • src/sage/misc/all.py

    diff --git a/src/sage/misc/all.py b/src/sage/misc/all.py
    index 25838c1..071c6d5 100644
    a b from sage_eval import sage_eval, sageobj 
    7676
    7777from sage_input import sage_input
    7878
    79 from cython import cython_lambda, cython_create_local_so
    80 from cython_c import cython_compile as cython
     79lazy_import("sage.misc.cython", ["cython_lambda", "cython_create_local_so"])
     80lazy_import("sage.misc.cython_c", "cython_compile", "cython")
    8181lazy_import("sage.misc.cython_c", "cython_compile", "pyrex", deprecation=9552)
    8282lazy_import("sage.misc.cython_c", "cython_compile", "sagex", deprecation=9552)
    8383
Last edited 4 years ago by jdemeyer (previous) (diff)

comment:43 Changed 4 years ago by fbissey

Re-building and running doctests to find any needed import sage.misc.cython.

comment:44 Changed 4 years ago by git

  • Commit changed from 6203f778428cd2af89a24633262fb9e441d19a43 to 44c1ac5d8d60dcbe990159157f4b34ff8334dac7

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

37ba00aUsing aliases properly, and lazy import from cython
44c1ac5Fix doctests after lazy importing of cython.py

comment:45 follow-up: Changed 4 years ago by fbissey

  • Authors changed from Volker Braun to Volker Braun, François Bissey
  • Status changed from needs_work to needs_review

If you would like to have a look and mark it positive Jeroen.

comment:46 in reply to: ↑ 45 Changed 4 years ago by jdemeyer

Replying to fbissey:

If you would like to have a look and mark it positive Jeroen.

Can it wait until the next beta? There are already a lot of tickets closed-but-not-yet-in-develop involving the build system, let's not push things...

comment:47 Changed 4 years ago by fbissey

Sure it is turning to be a very busy beta cycle. Not sure where Volker will want to cut things out.

comment:48 Changed 4 years ago by jdemeyer

  • Work issues fix gsl build instructionsl pkgconfig import deleted

comment:49 Changed 4 years ago by jdemeyer

  • Dependencies #20130 deleted

comment:50 Changed 4 years ago by jdemeyer

Is this ticket supposed to make it possible to use openblas instead of ATLAS?

comment:51 Changed 4 years ago by jdemeyer

  • Branch changed from u/fbissey/use_pkg_config_for_sage_setup to u/jdemeyer/use_pkg_config_for_sage_setup

comment:52 Changed 4 years ago by jdemeyer

  • Commit changed from 44c1ac5d8d60dcbe990159157f4b34ff8334dac7 to 252f00206b00580d015486506feb9285588658db

New commits:

252f002Merge tag '7.1.beta6' into t/20133/use_pkg_config_for_sage_setup

comment:53 Changed 4 years ago by fbissey

  • Branch changed from u/jdemeyer/use_pkg_config_for_sage_setup to u/fbissey/use_pkg_config_for_sage_setup
  • Commit changed from 252f00206b00580d015486506feb9285588658db to 44c1ac5d8d60dcbe990159157f4b34ff8334dac7

Not quite. We still haven't converted numpy/scipy to use .pc files - I don't know there is a ticket yet for that. And there is a ticket to select your blas somewhere. But once we deal with numpy/scipy you could get there with a bit of hand holding.

comment:54 follow-up: Changed 4 years ago by vbraun

There is no way to NOT build ATLAS afaik so thats still missing....

comment:55 Changed 4 years ago by fbissey

Someone determined could link build/pkgs/atlas to build/pkgs/openblas but I have to agree that's not really something I can recommend.

comment:56 in reply to: ↑ 54 Changed 4 years ago by jdemeyer

Replying to vbraun:

There is no way to NOT build ATLAS afaik so thats still missing....

Right, but I meant the pkgconfig part...

comment:57 Changed 4 years ago by fbissey

I am looking into getting numpy and scipy on board. I'll create a ticket when I am satisfied.

comment:58 Changed 4 years ago by fbissey

I have started #20157 for getting numpy and scipy to use .pc files. But to come back to Jeroen's technical question and after working on numpy for a bit. If you were to build openblas instead atlas then yes it should all build and work (but there is at least one doctest producing numerical noise). Note that numpy tries openblas before atlas. So if you have both installed openblas will be favored by default.

comment:59 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The Sage library should depend on pkgconfig.

comment:60 Changed 4 years ago by git

  • Commit changed from 44c1ac5d8d60dcbe990159157f4b34ff8334dac7 to 5d1cf572adbda8742305e5c59711c46b5f96fcf9

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

7fa6239Merge branch 'develop' into use_pkg_config_for_sage_setup
5d1cf57Add pkgconfig to sagelib's dependencies

comment:61 Changed 4 years ago by fbissey

  • Status changed from needs_work to needs_review

comment:62 Changed 4 years ago by vbraun

  • Reviewers set to Volker Braun, François Bissey
  • Status changed from needs_review to positive_review

comment:63 Changed 4 years ago by vbraun

  • Branch changed from u/fbissey/use_pkg_config_for_sage_setup to 5d1cf572adbda8742305e5c59711c46b5f96fcf9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.