#30901 closed enhancement (fixed)

sage.env._get_shared_lib_filename: Fix for MULTIARCH

Reported by: gh-tobiasdiez Owned by:
Priority: major Milestone: sage-9.3
Component: build Keywords: sd111
Cc: gh-tobiasdiez, fbissey, thansen, embray, dimpase Merged in:
Authors: Tobias Diez Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: fa4556a (Commits, GitHub, GitLab) Commit: fa4556ac2815bffa2d7866d94b3e061ba787d816
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-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)

Change History (27)

comment:1 Changed 13 months ago by gh-tobiasdiez

  • Status changed from new to needs_review

comment:2 Changed 13 months ago by git

  • Commit changed from 94e20c704da01082ace2da5a192ee158e091bc78 to 6dd6e5cc427b056efc3a4a65d59234493a95f04c

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

6dd6e5cFix compilation

comment:3 Changed 13 months ago by git

  • Commit changed from 6dd6e5cc427b056efc3a4a65d59234493a95f04c to d345bff9596c5e513aba8076a6c531fc9a366092

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

eceefb3Remove string wrap
d345bffFix test

comment:4 Changed 13 months ago by mkoeppe

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.

comment:5 Changed 13 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:6 Changed 13 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

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.

comment:7 Changed 13 months ago by git

  • Commit changed from d345bff9596c5e513aba8076a6c531fc9a366092 to c47c4bf7352b13764a0ec1aa497a6f24f3913ad9

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

c47c4bfCorrect indent

comment:8 Changed 13 months ago by mkoeppe

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.

comment:9 Changed 13 months ago by gh-tobiasdiez

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

comment:10 Changed 13 months ago by dimpase

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.

Last edited 13 months ago by dimpase (previous) (diff)

comment:11 Changed 12 months ago by git

  • Commit changed from c47c4bf7352b13764a0ec1aa497a6f24f3913ad9 to 3fcaf5f4408c946f5423d42facb5bb666635db35

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

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

comment:12 Changed 12 months ago by git

  • Commit changed from 3fcaf5f4408c946f5423d42facb5bb666635db35 to 090e6f10f1135062d1f65a1f1fa7a366e51ac388

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

090e6f1Simplify code

comment:13 Changed 12 months ago by mkoeppe

  • Keywords sd111 added

comment:14 follow-up: Changed 12 months ago by 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

comment:15 Changed 12 months ago by mkoeppe

  • Reviewers set to https://github.com/mkoeppe/sage/actions/runs/421393158, https://github.com/mkoeppe/sage/actions/runs/421393151

comment:16 Changed 12 months ago by mkoeppe

  • Description modified (diff)

comment:17 in reply to: ↑ 14 Changed 12 months ago by gh-tobiasdiez

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

Last edited 12 months ago by gh-tobiasdiez (previous) (diff)

comment:18 follow-up: Changed 12 months ago by gh-tobiasdiez

  • Description modified (diff)

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.

comment:19 in reply to: ↑ 18 ; follow-ups: Changed 12 months ago by mkoeppe

Replying to gh-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.

comment:20 Changed 12 months ago by mkoeppe

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.

comment:21 in reply to: ↑ 19 Changed 12 months ago by gh-tobiasdiez

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

comment:22 Changed 12 months ago by mkoeppe

OK, fine

comment:23 Changed 12 months ago by git

  • Commit changed from 090e6f10f1135062d1f65a1f1fa7a366e51ac388 to fa4556ac2815bffa2d7866d94b3e061ba787d816

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

fa4556aRemove _get_sage_local

comment:24 in reply to: ↑ 19 Changed 12 months ago by gh-tobiasdiez

Replying to mkoeppe:

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

Done!

comment:25 Changed 12 months ago by mkoeppe

  • Reviewers changed from https://github.com/mkoeppe/sage/actions/runs/421393158, https://github.com/mkoeppe/sage/actions/runs/421393151 to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:26 Changed 12 months ago by gh-tobiasdiez

Thanks!

comment:27 Changed 12 months ago by vbraun

  • Branch changed from public/build/multiarchsimple to fa4556ac2815bffa2d7866d94b3e061ba787d816
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.