sage.env: _add_variable_or_fallback depends on the order of a dict

Question

sage.env: _add_variable_or_fallback depends on the order of a dict

jhpalmieri opened this issue 8 years ago · 23 comments

In sage/env.py, the function _add_variable_or_fallback does some variable substitution: if a string contains $VAR, it substitutes the value of VAR in the dictionary SAGE_ENV. It does this by iterating through the items of SAGE_ENV, which means that if both VAR and VAR_OTHER are in SAGE_ENV, it's sort of a coin toss as to what happens when it comes across $VAR_OTHER.

(This arose when I was working on #21469: the string $SAGE_SRC_ROOT was being replaced by the value of SAGE_SRC, followed by the string _ROOT.)

CC: @mkoeppe

Component: build

Author: John Palmieri

Branch: 343d685

Reviewer: Matthias Koeppe

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

Answer 1 · 2017-08-30T18:51:07.000Z

Branch: u/jhpalmieri/variable_replacement

Answer 2 · 2017-08-30T18:51:21.000Z

New commits:

`60927e5`	`trac 23758: in env.py, _add_variable_or_fallback should depend less`

Answer 3 · 2017-08-30T18:51:21.000Z

Commit: 60927e5

Answer 4 · 2017-08-30T20:28:38.000Z

Changed commit from 60927e5 to c23cc1c

Answer 5 · 2017-08-30T20:28:38.000Z

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

`c23cc1c`	`trac 23758: restore "import os"`

Answer 6 · 2017-08-30T23:26:25.000Z

comment:4

I don't know, maybe we can just get rid of this whole variable substitution business, by using the Python variable SAGE_ROOT instead of "$SAGE_ROOT"?

Answer 7 · 2017-08-30T23:41:40.000Z

comment:5

(Could be a follow-up ticket.)

Answer 8 · 2017-08-30T23:42:05.000Z

comment:6

Perhaps add a doctest?

Answer 9 · 2017-08-31T01:18:23.000Z

Changed commit from c23cc1c to 343d685

Answer 10 · 2017-08-31T01:18:23.000Z

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

`343d685`	`trac 23758: add a doctest`

Answer 11 · 2017-08-31T01:19:47.000Z

comment:8

Here is a doctest. The same test fails for me in the develop branch, but I suppose that failure may not be repeatable on all platforms, since dictionaries are not ordered.

Answer 12 · 2017-08-31T01:22:05.000Z

comment:9

Replying to @mkoeppe:

I don't know, maybe we can just get rid of this whole variable substitution business, by using the Python variable SAGE_ROOT instead of "$SAGE_ROOT"?

I agree, a followup ticket if we want to address this. I'm not sure what the point of the substitution is, compared to using shell variables or some shortcut for accessing SAGE_ENV['foo'].

Answer 13 · 2017-08-31T04:54:44.000Z

Author: John H. Palmieri

Answer 14 · 2017-08-31T04:54:44.000Z

Reviewer: Matthias Koeppe

Answer 15 · 2017-08-31T05:01:47.000Z

comment:11

Follow up ticket is #23762.

Answer 16 · 2017-08-31T22:32:16.000Z

Changed commit from 343d685 to c7cff80

Answer 17 · 2017-08-31T22:32:16.000Z

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

`eb4137b`	`trac 23762: in sage/env.py, get rid of the variable substitution`
`c7cff80`	`Merge branch 't/23758/variable_replacement' into env`

Answer 18 · 2017-08-31T22:33:48.000Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

Answer 19 · 2017-08-31T22:33:48.000Z

Changed commit from c7cff80 to 343d685

Answer 20 · 2017-08-31T22:35:04.000Z

comment:14

Sorry, I accidentally pushed a branch here instead of to the appropriate ticket. The positively reviewed branch has been restored.

Answer 21 · 2017-09-04T06:11:11.000Z

Changed branch from u/jhpalmieri/variable_replacement to 343d685

Answer 22 · 2017-12-08T14:51:11.000Z

Changed commit from 343d685 to none

Answer 23 · 2017-12-08T14:51:11.000Z

Changed author from John H. Palmieri to John Palmieri