#30833 closed enhancement (invalid)

Change some string variables in env.py to methods

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords:
Cc: gh-tobiasdiez, fbissey, thansen, embray, dimpase Merged in:
Authors: Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: public/build/multiarch (Commits, GitHub, GitLab) Commit: d9f36dcd0a828fea90c1e2998d6d87352744bd0b
Dependencies: #30901 Stopgaps:

Status badges

Description (last modified by gh-tobiasdiez)

This ticket changes some of the string variables in src/env.py pointing to paths to methods returning a Path.

Change History (17)

comment:1 Changed 20 months ago by mkoeppe

  • Cc embray dimpase added

The code using MULTILIB was introduced in #27230.

comment:2 Changed 20 months ago by mkoeppe

As noted in #27230, another version of this code is in find_library in src/sage/misc/compat.py, which claims to provide better platform compatibility compared to ctypes.util.find_library (on Cygwin and macOS). It should be checked whether this code (introduced in 2017, for Python 2.7) is still necessary.

comment:3 Changed 20 months ago by gh-tobiasdiez

I have a few questions:

  1. Should MULTILIB still be used?
  2. If yes, what's the correct way to use it? In https://github.com/sagemath/sage/commit/edfb14de609040b990546bd07331add3bb0711fc the behavior was changed: before it was appended to usr/lib but after this commit it is appended to SAGE_LOCAL/lib. Was this change on purpose?

comment:4 Changed 20 months ago by mkoeppe

On a quick search, I have not found any evidence for the existence of the MULTILIB variable, and on the ticket where it was introduced there is no discussion of testing on any specific multiarch platforms. My guess is that this was never working. Let's get rid of it and then just test it on our supported platforms.

comment:5 Changed 20 months ago by mkoeppe

This ticket needs a branch

comment:6 Changed 20 months ago by gh-tobiasdiez

  • Authors set to Tobias Diez
  • Branch set to public/build/multiarch
  • Commit set to 13216c1318cf1c05ef4b48b35a7b312fd27f6c2f
  • Description modified (diff)
  • Status changed from new to needs_review

Ready for review.


New commits:

13216c1Fix multiarch for shared libraries

comment:7 Changed 20 months ago by mkoeppe

Too many changes on one ticket - interface, implementation, ...

This places an unnecessary burden on reviewers.

Also, it seems that this ticket tries to establish a new convention get_... for things in sage.env, including a new function get_sage_local.

As I said on another ticket - we already have a place for more structured access to system information - that's sage.feature. Please don't create another place in sage.env

comment:8 Changed 20 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:9 follow-up: Changed 20 months ago by gh-tobiasdiez

I'm sorry that the changes are bigger. However, I mostly rewrote the _get_shared_lib_filename method, and the other changes are only a consequence of these changes. For example, the new implementation returns only a path that exists, so the additional check in singular.pyx is no longer necessary. I honestly don't know how I can split this ticket into smaller pieces.

For the get functions, do you suggest to go back to the variable approach? My reason for the get functions was that this access the file path, and thus having getter functions potentially leads to better startup time (e.g. if you don't initialize singular). If you prefer I can of course revert to using variables. For get_sage_local, that was only a minor refactoring since I used it in multiple places. I can make it a private function if desired.

comment:10 in reply to: ↑ 9 Changed 20 months ago by mkoeppe

Replying to gh-tobiasdiez:

My reason for the get functions was that this access the file path, and thus having getter functions potentially leads to better startup time (e.g. if you don't initialize singular).

That's a concern that is orthogonal to this ticket, so please don't do it on this ticket.

comment:11 Changed 20 months ago by git

  • Commit changed from 13216c1318cf1c05ef4b48b35a7b312fd27f6c2f to d9f36dcd0a828fea90c1e2998d6d87352744bd0b

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

d9f36dcDon't use Python 3.8 syntax

comment:12 Changed 20 months ago by gh-tobiasdiez

  • Description modified (diff)
  • Summary changed from sage.env._get_shared_lib_filename: Fix for MULTIARCH to Change some string variables in env.py to methods

I've now extracted some of the changes to #30901, so that this ticket is now only about the change of interface from variables to methods.

comment:13 Changed 20 months ago by gh-tobiasdiez

  • Dependencies set to #30901

comment:14 Changed 20 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

comment:15 Changed 18 months ago by gh-tobiasdiez

  • Milestone changed from sage-9.3 to sage-duplicate/invalid/wontfix

This is no longer needed.

comment:16 Changed 18 months ago by mkoeppe

  • Authors Tobias Diez deleted
  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:17 Changed 17 months ago by chapoton

  • Resolution set to invalid
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.