sagemath/sage

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

mkoeppe opened this issue · 10 comments

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

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.

Dependencies: #23758

Commit: c7cff80

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

comment:5

Potential conflict with #21535.

Changed dependencies from #23758 to #23758, #21535

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

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