Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change some string variables in env.py to methods #30833

Closed
mkoeppe opened this issue Oct 26, 2020 · 21 comments
Closed

Change some string variables in env.py to methods #30833

mkoeppe opened this issue Oct 26, 2020 · 21 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Oct 26, 2020

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

Depends on #30901

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

Component: build

Branch/Commit: public/build/multiarch @ d9f36dc

Reviewer: Matthias Koeppe

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Oct 26, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 26, 2020

comment:1

The code using MULTILIB was introduced in #27230.

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 26, 2020

comment:2

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.

@tobiasdiez
Copy link
Contributor

comment:3

I have a few questions:

  1. Should MULTILIB still be used?
  2. If yes, what's the correct way to use it? In edfb14d 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?

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 27, 2020

comment:4

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 3, 2020

comment:5

This ticket needs a branch

@tobiasdiez

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

Author: Tobias Diez

@tobiasdiez
Copy link
Contributor

Branch: public/build/multiarch

@tobiasdiez
Copy link
Contributor

comment:6

Ready for review.


New commits:

13216c1Fix multiarch for shared libraries

@tobiasdiez
Copy link
Contributor

Commit: 13216c1

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 9, 2020

comment:7

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

@tobiasdiez
Copy link
Contributor

comment:9

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 9, 2020

comment:10

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

Changed commit from 13216c1 to d9f36dc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

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

d9f36dcDon't use Python 3.8 syntax

@tobiasdiez

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

comment:12

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.

@tobiasdiez tobiasdiez changed the title sage.env._get_shared_lib_filename: Fix for MULTIARCH Change some string variables in env.py to methods Nov 12, 2020
@tobiasdiez
Copy link
Contributor

Dependencies: #30901

@tobiasdiez
Copy link
Contributor

comment:15

This is no longer needed.

@tobiasdiez tobiasdiez removed this from the sage-9.3 milestone Jan 6, 2021
@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 6, 2021

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 6, 2021

Changed author from Tobias Diez to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants