sage.env._get_shared_lib_filename: Fix for MULTIARCH

[tobiasdiez]

On distributions that use the multiarch installation scheme, python provides the MULTIARCH sysconfig variable.

We modify sage.env._get_shared_lib_filename to use it.
(Currently it tries to use MULTILIB which does not exist.)
Moreover, I took the opportunity to refactor the code in the _get_shared_lib_filename method to use pathlib.

(extracted from #30371)

CC: @tobiasdiez @kiwifb @tobihan @embray @dimpase

Component: build

Keywords: sd111

Author: Tobias Diez

Branch/Commit: fa4556a

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/30901

[sagetrac-git]

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

6dd6e5c

Fix compilation

[sagetrac-git]

Changed commit from 94e20c7 to 6dd6e5c

[sagetrac-git]

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

eceefb3	Remove string wrap
d345bff	Fix test

[sagetrac-git]

Changed commit from 6dd6e5c to d345bff

[mkoeppe]

comment:4

Why does it change the code for Cygwin? Beyond the scope of the ticket.

And why does it change the doctest to pattern = "*/lib*Singular-*.so"? My guess is that this is untested.

[tobiasdiez]

comment:6

Thanks for the review!

I had to change the Cygwin code as it previously took the last match of search_directories, but on the other OS it was the first match. Thus, in order to unify the code I had to change the order. Please view this as part of "Moreover, I took the opportunity to refactor the code in the _get_shared_lib_filename method to use pathlib.".

For the doctest change, the method returns the following path on my system: /usr/lib/x86_64-linux-gnu/libsingular-Singular-4.1.1.so. This is because of the resolve() statement, which follows symlinks. I had to add resolve() since otherwise the singular library doesn't initialize correctly.

[sagetrac-git]

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

c47c4bf

Correct indent

[sagetrac-git]

Changed commit from d345bff to c47c4bf

[mkoeppe]

comment:8

If you want to make changes to the Cygwin code, then you'll have to test it on Cygwin as well. We have a severe shortage of Cygwin developers/testers and we cannot just replace tested code by untested code.

[tobiasdiez]

comment:9

That make sense, I was under the impression that you also have CI for cygwin.

I have tested the code manually during development. After sudo touch /usr/bin/cygSingular-test.dll the following script

import sysconfig
import sys
from sage.env import _get_shared_lib_path

print(sysconfig.get_config_var('BINDIR'))

sys.platform = 'cygwin'
print(_get_shared_lib_path("Singular"))

returns, as expected,

/usr/bin
/usr/bin/cygSingular-test.dll

[dimpase]

comment:10

GH Actions cygwin build is very flaky, as it has to be done in stages due to time limits.
We need a dedicated Cygwin runner (GH Actions supports selfhosted runners) to improve it, so that it can be done in one go, I think.

[sagetrac-git]

Changed commit from c47c4bf to 3fcaf5f

[sagetrac-git]

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

3fcaf5f

Merge branch 'develop' of git://github.com/sagemath/sage into public/build/multiarchsimple

[sagetrac-git]

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

090e6f1

Simplify code

[sagetrac-git]

Changed commit from 3fcaf5f to 090e6f1

[mkoeppe]

Changed keywords from none to sd111

[mkoeppe]

comment:14

I think the error handling in src/sage/libs/singular/singular.pyx should not be removed because SINGULAR_SO can be set via sage_conf and hence in this case it is not guaranteed by the new code in sage.env that the file exists

[mkoeppe]

Reviewer: https://github.com/mkoeppe/sage/actions/runs/421393158, https://github.com/mkoeppe/sage/actions/runs/421393151

[tobiasdiez]

comment:17

Replying to @mkoeppe:

I think the error handling in src/sage/libs/singular/singular.pyx should not be removed because SINGULAR_SO can be set via sage_conf and hence in this case it is not guaranteed by the new code in sage.env that the file exists

There were two existence checks in singular.pyx. The current version still contains if not SINGULAR_SO or not os.path.exists(SINGULAR_SO).

[tobiasdiez]

comment:18

By the way, I didn't change the variables to Path. That was the case in a previous version, but for the sage of keeping the changes confined I've removed it again.

[mkoeppe]

comment:19

Replying to @tobiasdiez:

By the way, I didn't change the variables to Path. That was the case in a previous version, but for the sage of keeping the changes confined I've removed it again.

Thanks for pointing this out. But then I think the renaming of _get_shared_lib_filename to _get_shared_lib_path also should not be done.

Also, please let's get rid of the helper function _get_sage_local.

[mkoeppe]

comment:20

The Cygwin tests are now a bit more robust and although it did not run through completely because of an unrelated problem, it seems OK.

[tobiasdiez]

comment:21

Replying to @mkoeppe:

But then I think the renaming of _get_shared_lib_filename to _get_shared_lib_path also should not be done.

But it's the complete path (as a string) that is returned, and not just the name of the file (relative to some folder).

[mkoeppe]

comment:22

OK, fine

[sagetrac-git]

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

fa4556a

Remove _get_sage_local

[sagetrac-git]

Changed commit from 090e6f1 to fa4556a

[tobiasdiez]

comment:24

Replying to @mkoeppe:

Also, please let's get rid of the helper function _get_sage_local.

Done!

[mkoeppe]

Changed reviewer from https://github.com/mkoeppe/sage/actions/runs/421393158, https://github.com/mkoeppe/sage/actions/runs/421393151 to Matthias Koeppe

[tobiasdiez]

comment:26

Thanks!

[vbraun]

Changed branch from public/build/multiarchsimple to fa4556a