Opened 4 years ago

Closed 2 years ago

#23762 closed enhancement (duplicate)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords:
Cc: jhpalmieri Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/jhpalmieri/env (Commits, GitHub, GitLab) Commit: c7cff80c2eb7c62e09917ac95f44305d3884ebd2
Dependencies: #23758, #21535 Stopgaps:

Status badges

Description

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.

Change History (7)

comment:1 Changed 4 years ago by jhpalmieri

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.

comment:2 Changed 4 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/env

comment:3 Changed 4 years ago by jhpalmieri

  • Commit set to c7cff80c2eb7c62e09917ac95f44305d3884ebd2
  • Dependencies set to #23758

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 Changed 4 years ago by jhpalmieri

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 follow-up: Changed 4 years ago by jdemeyer

  • Dependencies changed from #23758 to #23758, #21535

Potential conflict with #21535.

comment:6 in reply to: ↑ 5 Changed 4 years ago by jhpalmieri

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 Changed 2 years ago by embray

  • Milestone changed from sage-8.2 to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.