Opened 5 years ago
Closed 5 years ago
#20133 closed enhancement (fixed)
Use pkgconfig for Sage setup
Reported by:  vbraun  Owned by:  

Priority:  major  Milestone:  sage7.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, GitHub, GitLab)  Commit:  5d1cf572adbda8742305e5c59711c46b5f96fcf9 
Dependencies:  Stopgaps: 
Description (last modified by )
Use pkgconfig for linking Sage extension modules where possible
Change History (63)
comment:1 Changed 5 years ago by
 Branch set to u/vbraun/use_pkg_config_for_sage_setup
comment:2 Changed 5 years ago by
 Commit set to 066d7e8fd64c8466cfeb3407919d2781a3404c4e
comment:3 Changed 5 years ago by
 Component changed from PLEASE CHANGE to build
 Description modified (diff)
 Type changed from PLEASE CHANGE to enhancement
comment:4 Changed 5 years ago by
 Dependencies set to #20130
comment:5 Changed 5 years ago by
 Commit changed from 066d7e8fd64c8466cfeb3407919d2781a3404c4e to 33d50b200d54bcf827b99d54184b16062db0ea85
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
18d59bf  Move thirdparty packages to pkgconfig blas

de7efd2  Fix typo: use lapack.pc not blas.pc

f1302b2  Bump patchlevels

555b258  Iml version/patchlevel fix

8f0e007  Use pkgconfig for Sage linking

33d50b2  Fix typo

comment:6 Changed 5 years ago by
 Commit changed from 33d50b200d54bcf827b99d54184b16062db0ea85 to a7772cee4683aeb42ab7ecd26792850bcaa794e5
Branch pushed to git repo; I updated commit sha1. New commits:
a7772ce  Do not doctest that cblas has a particular name

comment:7 Changed 5 years ago by
 Cc fbissey added
 Status changed from new to needs_review
comment:8 Changed 5 years ago by
 Status changed from needs_review to needs_work
I think there is more to do in cython.py
. In sageongentoo 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" # ctrlc 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 5 years ago by
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 5 years ago by
Will do as soon as my day job allows me, I'll put it in positive review afterwards.
comment:11 Changed 5 years ago by
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 5 years ago by
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 sageongentoo).
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 5 years ago by
 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:
6d8e32f  Add libdirs (including blas' libdir) pyx_preparse

comment:14 Changed 5 years ago by
 Status changed from needs_work to positive_review
comment:15 Changed 5 years ago by
 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:
dd84287  remove extranauseous brackets

comment:16 Changed 5 years ago by
Sorry for the noise, I thought I had removed those brackets before the last commit.
comment:17 Changed 5 years ago by
 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.
comment:18 Changed 5 years ago by
 Commit changed from dd84287f8f793661387977e65e9761722809c759 to 12d54b5620e55af324a9b1dddefc2acda3142d88
Branch pushed to git repo; I updated commit sha1. New commits:
12d54b5  proper list building for library_dir

comment:19 Changed 5 years ago by
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 sageongentoo. Let's see if the patchbot will come clean this time.
comment:20 Changed 5 years ago by
IMHO in an ideal world you'd just run pkgconfig libs blas gsl zlib
. By telling pkgconfig 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 5 years ago by
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 5 years ago by
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 followup: ↓ 24 Changed 5 years ago by
Correctly treating library and include dirs is only relevant for nonSage, nonsystem 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 5 years ago by
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 knownbroken code with the excuse that you cannot test it, so you cannot see that it is in fact broken.
comment:25 Changed 5 years ago by
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 5 years ago by
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 followup: ↓ 29 Changed 5 years ago by
Correction: Both not having a pc file and not using an existing pc file are a bug in an of itself.
comment:28 Changed 5 years ago by
 Commit changed from 12d54b5620e55af324a9b1dddefc2acda3142d88 to d4525c0860db5ea8657959ebcf33afb33c5b2b03
Branch pushed to git repo; I updated commit sha1. New commits:
d4525c0  Add extra output to pyx_preparse doctest

comment:29 in reply to: ↑ 27 Changed 5 years ago by
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 koolaid, we'll have to also drink Jeroen's koolaid 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:
d4525c0  Add extra output to pyx_preparse doctest

comment:30 followup: ↓ 31 Changed 5 years ago by
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 ; followup: ↓ 34 Changed 5 years ago by
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 finetune these declarations (by adding library_dirs
for example), those finetuned 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 5 years ago by
 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 5 years ago by
Note that this ticket conflicts badly with #17635.
comment:34 in reply to: ↑ 31 ; followup: ↓ 35 Changed 5 years ago by
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 finetune these declarations (by addinglibrary_dirs
for example), those finetuned 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 5 years ago by
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 5 years ago by
 Commit changed from d4525c0860db5ea8657959ebcf33afb33c5b2b03 to 6203f778428cd2af89a24633262fb9e441d19a43
Branch pushed to git repo; I updated commit sha1. New commits:
6203f77  Use the information of gsl.pc directly in sage/libs/gsl/*.pxd files as necessary

comment:37 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
 Work issues changed from fix gsl build instructions to fix gsl build instructionsl pkgconfig import
 You need to define the GSL aliases, note the command line:
gcc fnostrictaliasing g O2 DNDEBUG g fwrapv O3 Wall fPIC Igsl_include_dirs I/home/worker/sagepatchbot/local/lib/python2.7/sitepackages/cysignals I/home/worker/sagepatchbot/local/include I/home/worker/sagepatchbot/local/include/python2.7 I/home/worker/sagepatchbot/local/lib/python2.7/sitepackages/numpy/core/include I/home/worker/sagepatchbot/src I/home/worker/sagepatchbot/src/sage/ext I/home/worker/sagepatchbot/src/build/cythonized I/home/worker/sagepatchbot/src/build/cythonized/sage/ext I/home/worker/sagepatchbot/local/include/python2.7 c /home/worker/sagepatchbot/src/build/cythonized/sage/gsl/gsl_array.c o build/temp.linuxx86_642.7/home/worker/sagepatchbot/src/build/cythonized/sage/gsl/gsl_array.o DGSL_DISABLE_DEPRECATED fnostrictaliasing w
 Can you try to not import
pkgconfig
every time that Sage starts?
comment:40 followup: ↓ 41 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
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 76 76 77 77 from sage_input import sage_input 78 78 79 from cython import cython_lambda, cython_create_local_so 80 from cython_c import cython_compile as cython 79 lazy_import("sage.misc.cython", ["cython_lambda", "cython_create_local_so"]) 80 lazy_import("sage.misc.cython_c", "cython_compile", "cython") 81 81 lazy_import("sage.misc.cython_c", "cython_compile", "pyrex", deprecation=9552) 82 82 lazy_import("sage.misc.cython_c", "cython_compile", "sagex", deprecation=9552) 83 83
comment:43 Changed 5 years ago by
Rebuilding and running doctests to find any needed import sage.misc.cython
.
comment:44 Changed 5 years ago by
 Commit changed from 6203f778428cd2af89a24633262fb9e441d19a43 to 44c1ac5d8d60dcbe990159157f4b34ff8334dac7
comment:45 followup: ↓ 46 Changed 5 years ago by
 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 5 years ago by
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 closedbutnotyetindevelop involving the build system, let's not push things...
comment:47 Changed 5 years ago by
Sure it is turning to be a very busy beta cycle. Not sure where Volker will want to cut things out.
comment:48 Changed 5 years ago by
 Work issues fix gsl build instructionsl pkgconfig import deleted
comment:49 Changed 5 years ago by
 Dependencies #20130 deleted
comment:50 Changed 5 years ago by
Is this ticket supposed to make it possible to use openblas instead of ATLAS?
comment:51 Changed 5 years ago by
 Branch changed from u/fbissey/use_pkg_config_for_sage_setup to u/jdemeyer/use_pkg_config_for_sage_setup
comment:52 Changed 5 years ago by
 Commit changed from 44c1ac5d8d60dcbe990159157f4b34ff8334dac7 to 252f00206b00580d015486506feb9285588658db
New commits:
252f002  Merge tag '7.1.beta6' into t/20133/use_pkg_config_for_sage_setup

comment:53 Changed 5 years ago by
 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 followup: ↓ 56 Changed 5 years ago by
There is no way to NOT build ATLAS afaik so thats still missing....
comment:55 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
I am looking into getting numpy
and scipy
on board. I'll create a ticket when I am satisfied.
comment:58 Changed 5 years ago by
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 5 years ago by
 Status changed from needs_review to needs_work
The Sage library should depend on pkgconfig
.
comment:60 Changed 5 years ago by
 Commit changed from 44c1ac5d8d60dcbe990159157f4b34ff8334dac7 to 5d1cf572adbda8742305e5c59711c46b5f96fcf9
comment:61 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:62 Changed 5 years ago by
 Reviewers set to Volker Braun, François Bissey
 Status changed from needs_review to positive_review
comment:63 Changed 5 years ago by
 Branch changed from u/fbissey/use_pkg_config_for_sage_setup to 5d1cf572adbda8742305e5c59711c46b5f96fcf9
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Use pkgconfig for Sage linking