Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#27230 closed enhancement (fixed)

Improve detection of library files in sage.env

Reported by: embray Owned by:
Priority: major Milestone: sage-8.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:

Status badges

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 4 years ago by embray

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

comment:2 Changed 4 years ago by embray

  • Branch set to u/embray/misc/sage-env-lib-filename
  • Commit set to b3c86d4ef8aab430172fbbba66bf70baa3f869ae
  • Status changed from new to needs_review

Oh, it's working again....


New commits:

32940a6Remove deprecated stuff related to Cython
d6581fdClean up sage.env
84eb61brepackages the code for finding the libSingular.so as a somewhat
b3c86d4rework _get_shared_lib_filename to cover more system-specific cases,

comment:3 Changed 4 years ago by embray

  • Dependencies set to #27040

This is built on top of #27040, with which it would otherwise conflict.

For #26930 the idea then would be to add

LIBGAP_SO = _get_shared_lib_filename('gap')

comment:4 Changed 4 years ago by dimpase

merge conflict, red branch...

comment:5 Changed 4 years ago by git

  • Commit changed from b3c86d4ef8aab430172fbbba66bf70baa3f869ae to 34b27b2c8068805c2f24adaa45740a80839cecbc

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

51aa946repackages the code for finding the libSingular.so as a somewhat
34b27b2rework _get_shared_lib_filename to cover more system-specific cases,

comment:6 Changed 4 years ago by embray

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 4 years ago by embray

  • 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 4 years ago by git

  • Commit changed from 34b27b2c8068805c2f24adaa45740a80839cecbc to 917a21ba4b07dc86ed855d57bfba5cde7a110f8e

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

917a21bfixed the test to be a little more flexible

comment:9 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

New commits:

917a21bfixed the test to be a little more flexible
Last edited 4 years ago by embray (previous) (diff)

comment:10 Changed 4 years ago by dimpase

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 4 years ago by fbissey

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 sage-on-gentoo in a nice generic way. The amount of patching of sage in sage-on-gentoo has gone down considerably.

comment:12 Changed 4 years ago by fbissey

  • 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 4 years ago by dimpase

  • 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', 'singular-Singular')
 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

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

comment:14 Changed 4 years ago by dimpase

  • 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 follow-up: Changed 4 years ago by jdemeyer

  • 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 4 years ago by jdemeyer

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 4 years ago by fbissey

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 follow-up: Changed 4 years ago by 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)

comment:19 in reply to: ↑ 18 Changed 4 years ago by embray

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 4 years ago by git

  • Commit changed from 917a21ba4b07dc86ed855d57bfba5cde7a110f8e to d137bc4f18cb77b0e67c3cb7beab8ea2a7f5efb2

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

11a8a89trac #26822: pep8 in centrality_betweenness and centrality_closeness
7f780bbtrac #26822: fix doctest error in centrality_closeness
0bb77cdUpdated SageMath version to 8.7.beta2
5001b21repackages the code for finding the libSingular.so as a somewhat
db8a725rework _get_shared_lib_filename to cover more system-specific cases,
d137bc4fixed the test to be a little more flexible

comment:21 Changed 4 years ago by git

  • Commit changed from d137bc4f18cb77b0e67c3cb7beab8ea2a7f5efb2 to cc2a024b7474a6a5e5cb3b11f0797a22d395672a

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

05f3dd9rework _get_shared_lib_filename to cover more system-specific cases,
cc2a024fixed the test to be a little more flexible

comment:22 Changed 4 years ago by embray

  • 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 right-enough).

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 sub-packages in Sage (at least anything that does anything less trivial that define a few pure-Python classes and functions).


New commits:

11a8a89trac #26822: pep8 in centrality_betweenness and centrality_closeness
7f780bbtrac #26822: fix doctest error in centrality_closeness
0bb77cdUpdated SageMath version to 8.7.beta2
5001b21repackages the code for finding the libSingular.so as a somewhat
05f3dd9rework _get_shared_lib_filename to cover more system-specific cases,
cc2a024fixed the test to be a little more flexible

comment:23 Changed 4 years ago by dimpase

  • Status changed from needs_review to needs_work

an import is missing, it seems:

[sagelib-8.7.beta2] && sage-python23 -u setup.py --no-user-cfg build install
[sagelib-8.7.beta2] ************************************************************************
[sagelib-8.7.beta2] Traceback (most recent call last):
[sagelib-8.7.beta2]   File "setup.py", line 61, in <module>
[sagelib-8.7.beta2]     import sage.env
[sagelib-8.7.beta2]   File "/mnt/opt/Sage/sage-dev/src/sage/env.py", line 264, in <module>
[sagelib-8.7.beta2]     SINGULAR_SO = _get_shared_lib_filename('Singular', 'singular-Singular')
[sagelib-8.7.beta2]   File "/mnt/opt/Sage/sage-dev/src/sage/env.py", line 247, in _get_shared_lib_filename
[sagelib-8.7.beta2]     libdirs = [sysconfig.get_config_var('LIBDIR')]
[sagelib-8.7.beta2] NameError: global name 'sysconfig' is not defined
[sagelib-8.7.beta2] ************************************************************************
[sagelib-8.7.beta2] Error building the Sage library

comment:24 Changed 4 years ago by embray

That's weird--I thought an import sysconfig was already added in #27040, but apparently not?

comment:25 Changed 4 years ago by jdemeyer

  • Dependencies #27040 deleted

comment:26 Changed 4 years ago by git

  • Commit changed from cc2a024b7474a6a5e5cb3b11f0797a22d395672a to 8d62ac50fb8c679e512ced730a5555271c2b1c65

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

346eb67rework _get_shared_lib_filename to cover more system-specific cases,
8d62ac5fixed the test to be a little more flexible

comment:27 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

comment:28 Changed 4 years ago by jdemeyer

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

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

comment:29 Changed 4 years ago by embray

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 4 years ago by git

  • Commit changed from 8d62ac50fb8c679e512ced730a5555271c2b1c65 to ab94f51c0c45221d92969721be0a26dc832c1330

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

461ae1drepackages the code for finding the libSingular.so as a somewhat
3b7df99rework _get_shared_lib_filename to cover more system-specific cases,
ab94f51fixed the test to be a little more flexible

comment:31 Changed 4 years ago by embray

  • Status changed from needs_info to needs_review

That's better.

comment:32 Changed 4 years ago by dimpase

  • Status changed from needs_review to positive_review

ok, tests out well.

comment:33 Changed 4 years ago by vbraun

  • Branch changed from u/embray/misc/sage-env-lib-filename to ab94f51c0c45221d92969721be0a26dc832c1330
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:34 Changed 3 years ago by embray

  • Commit ab94f51c0c45221d92969721be0a26dc832c1330 deleted

Ugh, this is still completely broken on Cygwin.

comment:35 Changed 3 years ago by gh-timokau

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 dimpase

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 gh-timokau

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 embray

New ticket please.

Note: See TracTickets for help on using tickets.