Opened 20 months ago
Closed 17 months ago
#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: |
Description (last modified by )
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
- Cc embray dimpase added
comment:2 Changed 20 months ago by
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
I have a few questions:
- Should
MULTILIB
still be used? - 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 toSAGE_LOCAL/lib
. Was this change on purpose?
comment:4 Changed 20 months ago by
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
This ticket needs a branch
comment:6 Changed 20 months ago by
- Branch set to public/build/multiarch
- Commit set to 13216c1318cf1c05ef4b48b35a7b312fd27f6c2f
- Description modified (diff)
- Status changed from new to needs_review
comment:7 Changed 20 months ago by
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
- Status changed from needs_review to needs_work
comment:9 follow-up: ↓ 10 Changed 20 months ago by
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
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
- Commit changed from 13216c1318cf1c05ef4b48b35a7b312fd27f6c2f to d9f36dcd0a828fea90c1e2998d6d87352744bd0b
Branch pushed to git repo; I updated commit sha1. New commits:
d9f36dc | Don't use Python 3.8 syntax
|
comment:12 Changed 20 months ago by
- 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
- Dependencies set to #30901
comment:14 Changed 20 months ago by
- Status changed from needs_work to needs_review
comment:15 Changed 18 months ago by
- Milestone changed from sage-9.3 to sage-duplicate/invalid/wontfix
This is no longer needed.
comment:16 Changed 18 months ago by
- Reviewers set to Matthias Koeppe
- Status changed from needs_review to positive_review
comment:17 Changed 17 months ago by
- Resolution set to invalid
- Status changed from positive_review to closed
The code using
MULTILIB
was introduced in #27230.