#27230 closed enhancement (fixed)
Improve detection of library files in sage.env
Reported by:  embray  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  misc  Keywords:  
Cc:  dimpase, fbissey  Merged in:  
Authors:  Erik Bray  Reviewers:  François Bissey, Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  ab94f51 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
This is basically repackaging the existing code from sage.env
that is used to set the SINGULAR_SO
variable, but making it a bit more generic and flexible.
This will be useful as well for #26930 to get the correct libgap path. It also tries to be a bit more flexible than the old code, so as to reduce the need for patching in some distros, though this does not guarantee to eliminate that need altogether.
Change History (38)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
 Branch set to u/embray/misc/sageenvlibfilename
 Commit set to b3c86d4ef8aab430172fbbba66bf70baa3f869ae
 Status changed from new to needs_review
comment:3 Changed 3 years ago by
 Dependencies set to #27040
comment:4 Changed 3 years ago by
merge conflict, red branch...
comment:5 Changed 3 years ago by
 Commit changed from b3c86d4ef8aab430172fbbba66bf70baa3f869ae to 34b27b2c8068805c2f24adaa45740a80839cecbc
comment:6 Changed 3 years ago by
Rebased on latest develop again. It seems 8.7.beta2 was released since I was previously working on this yesterday, and already includes #27040.
comment:7 Changed 3 years ago by
 Status changed from needs_review to needs_work
I just realized with my second commit the test could fail now on Debian.
comment:8 Changed 3 years ago by
 Commit changed from 34b27b2c8068805c2f24adaa45740a80839cecbc to 917a21ba4b07dc86ed855d57bfba5cde7a110f8e
Branch pushed to git repo; I updated commit sha1. New commits:
917a21b  fixed the test to be a little more flexible

comment:9 Changed 3 years ago by
 Status changed from needs_work to needs_review
New commits:
917a21b  fixed the test to be a little more flexible

comment:10 Changed 3 years ago by
Does this stuff really belong to src/sage/env.py
?
IMHO a better fit would be src/sage/libs/  as this is really something to do with libs...
comment:11 Changed 3 years ago by
env.py
is a central repository of variables so it fits there. But I am open to discussion about splitting some of it in the future if there is a good rationale. But that will be in the future.
Now this will remove a bit of patch in sageongentoo in a nice generic way. The amount of patching of sage in sageongentoo has gone down considerably.
comment:12 Changed 3 years ago by
 Reviewers set to François Bissey
 Status changed from needs_review to positive_review
Patchbot has two failures unrelated to this ticket and it looks good to me. So I am giving this a positive review.
comment:13 Changed 3 years ago by
 Status changed from positive_review to needs_work
OK, so please apply to your branch
 a/src/sage/env.py +++ b/src/sage/env.py @@ 267,6 +267,10 @@ def _get_shared_lib_filename(libname, *additional_libnames): SINGULAR_SO = _get_shared_lib_filename('Singular', 'singularSingular') var('SINGULAR_SO', SINGULAR_SO) +# locate libgap shared object +GAP_SO= _get_shared_lib_filename('gap','') +var('GAP_SO', GAP_SO) + # post process if ' ' in DOT_SAGE: if UNAME[:6] == 'CYGWIN':
and then I'll use it on #26930
comment:14 Changed 3 years ago by
 Reviewers changed from François Bissey to François Bissey, Dima Pasechnik
 Status changed from needs_work to positive_review
I just added the stuff I asked for in comment 13 to the branch of #26930, so no worried about it, positive review.
comment:15 followup: ↓ 17 Changed 3 years ago by
 Status changed from positive_review to needs_work
pyflakes
reports:
src/sage/env.py:37: redefinition of unused 'sys' from line 32 src/sage/env.py:235: undefined name 'libdir' src/sage/env.py:235: undefined name 'libdir'
The latter is a genuine bug as far as I can see.
comment:16 Changed 3 years ago by
Just a minor thing, but I find this remark more confusing than helping and suggest to remove it:
(technically this is superfluous but clearer when made explicit)
comment:17 in reply to: ↑ 15 Changed 3 years ago by
Replying to jdemeyer:
pyflakes
reports:src/sage/env.py:37: redefinition of unused 'sys' from line 32 src/sage/env.py:235: undefined name 'libdir' src/sage/env.py:235: undefined name 'libdir'The latter is a genuine bug as far as I can see.
I am guessing Erik meant libname
from the context.
comment:18 followup: ↓ 19 Changed 3 years ago by
Another detail: why this signature
def _get_shared_lib_filename(libname, *additional_libnames)
when all the arguments play the same role?
Wouldn't it be clearer to write it as
def _get_shared_lib_filename(*libnames)
comment:19 in reply to: ↑ 18 Changed 3 years ago by
Replying to jdemeyer:
Another detail: why this signature
def _get_shared_lib_filename(libname, *additional_libnames)when all the arguments play the same role?
Wouldn't it be clearer to write it as
def _get_shared_lib_filename(*libnames)
I pinged back and forth on that, but ultimately did it that way so that it would require at least one argument. I suppose it could allow zero arguments and then just return None but why bother?
comment:20 Changed 3 years ago by
 Commit changed from 917a21ba4b07dc86ed855d57bfba5cde7a110f8e to d137bc4f18cb77b0e67c3cb7beab8ea2a7f5efb2
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
11a8a89  trac #26822: pep8 in centrality_betweenness and centrality_closeness

7f780bb  trac #26822: fix doctest error in centrality_closeness

0bb77cd  Updated SageMath version to 8.7.beta2

5001b21  repackages the code for finding the libSingular.so as a somewhat

db8a725  rework _get_shared_lib_filename to cover more systemspecific cases,

d137bc4  fixed the test to be a little more flexible

comment:21 Changed 3 years ago by
 Commit changed from d137bc4f18cb77b0e67c3cb7beab8ea2a7f5efb2 to cc2a024b7474a6a5e5cb3b11f0797a22d395672a
comment:22 Changed 3 years ago by
 Status changed from needs_work to needs_review
Addressed comment:16 and obvious bugs from comment:15 (in the latter case, it previously worked on Cygwin but I did not test on Cygwin after the refactoring I did; I'm confident now that it's rightenough).
Happy to shuffle things around to different locations as well, though keep in mind everything in sage.env
should work easily with minimal, if any, imports from other subpackages in Sage (at least anything that does anything less trivial that define a few purePython classes and functions).
New commits:
11a8a89  trac #26822: pep8 in centrality_betweenness and centrality_closeness

7f780bb  trac #26822: fix doctest error in centrality_closeness

0bb77cd  Updated SageMath version to 8.7.beta2

5001b21  repackages the code for finding the libSingular.so as a somewhat

05f3dd9  rework _get_shared_lib_filename to cover more systemspecific cases,

cc2a024  fixed the test to be a little more flexible

comment:23 Changed 3 years ago by
 Status changed from needs_review to needs_work
an import is missing, it seems:
[sagelib8.7.beta2] && sagepython23 u setup.py nousercfg build install [sagelib8.7.beta2] ************************************************************************ [sagelib8.7.beta2] Traceback (most recent call last): [sagelib8.7.beta2] File "setup.py", line 61, in <module> [sagelib8.7.beta2] import sage.env [sagelib8.7.beta2] File "/mnt/opt/Sage/sagedev/src/sage/env.py", line 264, in <module> [sagelib8.7.beta2] SINGULAR_SO = _get_shared_lib_filename('Singular', 'singularSingular') [sagelib8.7.beta2] File "/mnt/opt/Sage/sagedev/src/sage/env.py", line 247, in _get_shared_lib_filename [sagelib8.7.beta2] libdirs = [sysconfig.get_config_var('LIBDIR')] [sagelib8.7.beta2] NameError: global name 'sysconfig' is not defined [sagelib8.7.beta2] ************************************************************************ [sagelib8.7.beta2] Error building the Sage library
comment:24 Changed 3 years ago by
That's weirdI thought an import sysconfig
was already added in #27040, but apparently not?
comment:25 Changed 3 years ago by
 Dependencies #27040 deleted
comment:26 Changed 3 years ago by
 Commit changed from cc2a024b7474a6a5e5cb3b11f0797a22d395672a to 8d62ac50fb8c679e512ced730a5555271c2b1c65
comment:27 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:28 Changed 3 years ago by
 Status changed from needs_review to needs_info
There is something weird going on with the commits... Why am I seeing Updated SageMath version to 8.7.beta2
in the list of commits on this ticket (when clicking on "commits" in the box at the top of this page)? Either it's something wrong with Trac or with this branch, but it would be good to clear that up.
comment:29 Changed 3 years ago by
I clearly must have screwed something up while rebasing, because it also says I'm the committer on that commit. I suspect what I did was at some point I did a git rebase i
and selected too many commits. Still surprising that it would modify some commits and set me as the committer even if I didn't change them at all.
comment:30 Changed 3 years ago by
 Commit changed from 8d62ac50fb8c679e512ced730a5555271c2b1c65 to ab94f51c0c45221d92969721be0a26dc832c1330
comment:32 Changed 3 years ago by
 Status changed from needs_review to positive_review
ok, tests out well.
comment:33 Changed 3 years ago by
 Branch changed from u/embray/misc/sageenvlibfilename to ab94f51c0c45221d92969721be0a26dc832c1330
 Resolution set to fixed
 Status changed from positive_review to closed
comment:34 Changed 3 years ago by
 Commit ab94f51c0c45221d92969721be0a26dc832c1330 deleted
Ugh, this is still completely broken on Cygwin.
comment:35 Changed 3 years ago by
This fails to find any libraries on nix, since they won't be in the same libdir as python. For that reason I'm already setting the SINGULAR_SO
and GAP_SO
env vars, but now this test fails anyways.
Thoughts on how best to resolve this? I'm not sure the function should even be tested (instead maybe test that SINGULAR_SO
and GAP_SO
have proper values after being assigned. If we really do want to test that function, maybe ad the SINGULAR_SO
and GAP_SO
locations to the libdirs
list?
comment:36 Changed 3 years ago by
this seems to be duplucating in an ad hoc way what the system linker/loader knows. Is there a way to query the cache of ldconfig from Python? This would be a much more robust thing, for system libraries, IMHO.
comment:37 Changed 3 years ago by
Come to think of it, why do we need this at all? Isn't the functionality covered (better) by find_library
in src/sage/misc/compat.py
?
comment:38 Changed 3 years ago by
New ticket please.
I have a branch for this, but the cloud hosting provider for the VM I was working in seems to be monkeying around with something and now my VM lacks working DNS...