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

sage.env._add_variable_or_fallback: Get rid of $variable substitution feature #23762

Closed
mkoeppe opened this issue Aug 31, 2017 · 10 comments
Closed

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Aug 31, 2017

As discussed in #23758, the feature of _add_variable_or_fallback to do variable substitution seems unnecessarily complex.

All uses later in that module such as

_add_variable_or_fallback('SAGE_ETC',        opj('$SAGE_LOCAL', 'etc'))

can be replaced by simple use of Python variables (which all exist in the sage.env module)

_add_variable_or_fallback('SAGE_ETC',        opj(SAGE_LOCAL, 'etc'))

This also will have a clearer failure mode -- instead of leaving an unsubstituted $SAGE_LOCAL in a string if that variable is not set for some reason, an error will be raised.

Depends on #23758
Depends on #21535

CC: @jhpalmieri

Component: build

Branch/Commit: u/jhpalmieri/env @ c7cff80

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

@mkoeppe mkoeppe added this to the sage-8.2 milestone Aug 31, 2017
@jhpalmieri
Copy link
Member

comment:1

I think the variable substitution scheme was introduced in #13432. I've asked there about its rationale: I don't want to remove this without trying to understand why it was introduced in the first place.

In the mean time, I have a branch which makes the change from strings to variables: '$VAR' --> VAR.

@jhpalmieri
Copy link
Member

Branch: u/jhpalmieri/env

@jhpalmieri
Copy link
Member

Dependencies: #23758

@jhpalmieri
Copy link
Member

Commit: c7cff80

@jhpalmieri
Copy link
Member

New commits:

eb4137btrac 23762: in sage/env.py, get rid of the variable substitution
60927e5trac 23758: in env.py, _add_variable_or_fallback should depend less
c23cc1ctrac 23758: restore "import os"
343d685trac 23758: add a doctest
c7cff80Merge branch 't/23758/variable_replacement' into env

@jhpalmieri
Copy link
Member

comment:4

This branch passes doctests for me, but I want to wait to set it to "needs_review" for some feedback from the participants at #13432.

@jdemeyer
Copy link

jdemeyer commented Sep 1, 2017

comment:5

Potential conflict with #21535.

@jdemeyer
Copy link

jdemeyer commented Sep 1, 2017

Changed dependencies from #23758 to #23758, #21535

@jhpalmieri
Copy link
Member

comment:6

Replying to @jdemeyer:

Potential conflict with #21535.

Okay, it should be easy to rebase. I will wait until #21535 has settled into a stable state (and also to see if there are objections to this ticket as a whole).

@embray
Copy link
Contributor

embray commented Jan 18, 2019

comment:7

I don't think I ever saw the original version of this, but it would be superseded now by #27040 (which accomplishes the same goal).

@embray embray closed this as completed Jan 18, 2019
@embray embray removed this from the sage-8.2 milestone Jan 18, 2019
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

4 participants