Skip to content

Isolate environment variable substitutions #521

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 11, 2017
Merged

Isolate environment variable substitutions #521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
10b0d6c
Isolate environment variable substitutions
May 18, 2017
89848ac
Merge branch 'master' into envsubs-independence
AndreLouisCaron Jun 30, 2017
16529d2
Merge branch 'master' into envsubs-independence
obestwalter Jul 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ Not released yet
- #517: Forward NUMBER_OF_PROCESSORS by default on Windows to fix
`multiprocessor.cpu_count()`.
- #518: Forward `USERPROFILE` by default on Windows.
- #515: Don't require environment variables in test environments
where they are not used.

2.7.0
-----
Expand Down
21 changes: 21 additions & 0 deletions tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,27 @@ def test_substitution_notfound_issue246(tmpdir, newconfig):
assert 'FOO' in env
assert 'BAR' in env

def test_substitution_notfound_issue515(tmpdir, newconfig):
config = newconfig("""
[tox]
envlist = standard-greeting

[testenv:standard-greeting]
commands =
python -c 'print("Hello, world!")'

[testenv:custom-greeting]
passenv =
NAME
commands =
python -c 'print("Hello, {env:NAME}!")'
""")
conf = config.envconfigs['standard-greeting']
assert conf.commands == [
['python', '-c', 'print("Hello, world!")'],
]

@pytest.mark.xfail(raises=AssertionError, reason="issue #301")
def test_substitution_nested_env_defaults_issue301(tmpdir, newconfig, monkeypatch):
monkeypatch.setenv("IGNORE_STATIC_DEFAULT", "env")
monkeypatch.setenv("IGNORE_DYNAMIC_DEFAULT", "env")
Expand Down
68 changes: 36 additions & 32 deletions tox/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,8 @@ def __init__(self, config, inipath):
factors = set(name.split('-'))
if section in self._cfg or factors <= known_factors:
config.envconfigs[name] = \
self.make_envconfig(name, section, reader._subs, config)
self.make_envconfig(name, section, reader._subs, config,
replace=name in config.envlist)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: this is the core of the change I wanted to make. Don't replace environment substitutions unless the environment is in envlist.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, my intent is to use replace=False when the environment is not scheduled in the current run. I'm not 100% sure config.envlist is what I want here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, my intent is to use replace=False when the environment is not scheduled in the current run.

Do you mean: all the runs scheduled in this invocation? e.g. tox -e py27,py34 would then only do substitutions for these two environments when parsing the ini?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant.


all_develop = all(name in config.envconfigs
and config.envconfigs[name].usedevelop
Expand All @@ -808,7 +809,7 @@ def _list_section_factors(self, section):
factors.update(*mapcat(_split_factor_expr, exprs))
return factors

def make_envconfig(self, name, section, subs, config):
def make_envconfig(self, name, section, subs, config, replace=True):
factors = set(name.split('-'))
reader = SectionReader(section, self._cfg, fallbacksections=["testenv"],
factors=factors)
Expand All @@ -823,7 +824,7 @@ def make_envconfig(self, name, section, subs, config):
atype = env_attr.type
if atype in ("bool", "path", "string", "dict", "dict_setenv", "argv", "argvlist"):
meth = getattr(reader, "get" + atype)
res = meth(env_attr.name, env_attr.default)
res = meth(env_attr.name, env_attr.default, replace=replace)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: adding this here is why I needed to add a replace argument all over the place.

elif atype == "space-separated-list":
res = reader.getlist(env_attr.name, sep=" ")
elif atype == "line-list":
Expand Down Expand Up @@ -942,9 +943,9 @@ def addsubstitutions(self, _posargs=None, **kw):
if _posargs:
self.posargs = _posargs

def getpath(self, name, defaultpath):
def getpath(self, name, defaultpath, replace=True):
toxinidir = self._subs['toxinidir']
path = self.getstring(name, defaultpath)
path = self.getstring(name, defaultpath, replace=replace)
if path is not None:
return toxinidir.join(path, abs=True)

Expand All @@ -954,12 +955,12 @@ def getlist(self, name, sep="\n"):
return []
return [x.strip() for x in s.split(sep) if x.strip()]

def getdict(self, name, default=None, sep="\n"):
def getdict(self, name, default=None, sep="\n", replace=True):
value = self.getstring(name, None)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: may have forgotten the replace=replace here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so as well, should also be passed through to getstring.

return self._getdict(value, default=default, sep=sep)

def getdict_setenv(self, name, default=None, sep="\n"):
value = self.getstring(name, None, replace=True, crossonly=True)
def getdict_setenv(self, name, default=None, sep="\n", replace=True):
value = self.getstring(name, None, replace=replace, crossonly=True)
definitions = self._getdict(value, default=default, sep=sep)
self._setenv = SetenvDict(definitions, reader=self)
return self._setenv
Expand All @@ -976,8 +977,8 @@ def _getdict(self, value, default, sep):

return d

def getbool(self, name, default=None):
s = self.getstring(name, default)
def getbool(self, name, default=None, replace=True):
s = self.getstring(name, default, replace=replace)
if not s:
s = default
if s is None:
Expand All @@ -994,12 +995,12 @@ def getbool(self, name, default=None):
"boolean value %r needs to be 'True' or 'False'")
return s

def getargvlist(self, name, default=""):
def getargvlist(self, name, default="", replace=True):
s = self.getstring(name, default, replace=False)
return _ArgvlistReader.getargvlist(self, s)
return _ArgvlistReader.getargvlist(self, s, replace=replace)

def getargv(self, name, default=""):
return self.getargvlist(name, default)[0]
def getargv(self, name, default="", replace=True):
return self.getargvlist(name, default, replace=replace)[0]

def getstring(self, name, default=None, replace=True, crossonly=False):
x = None
Expand Down Expand Up @@ -1153,7 +1154,7 @@ def _replace_substitution(self, match):

class _ArgvlistReader:
@classmethod
def getargvlist(cls, reader, value):
def getargvlist(cls, reader, value, replace=True):
"""Parse ``commands`` argvlist multiline string.

:param str name: Key name in a section.
Expand All @@ -1178,7 +1179,7 @@ def getargvlist(cls, reader, value):
replaced = reader._replace(current_command, crossonly=True)
commands.extend(cls.getargvlist(reader, replaced))
else:
commands.append(cls.processcommand(reader, current_command))
commands.append(cls.processcommand(reader, current_command, replace))
current_command = ""
else:
if current_command:
Expand All @@ -1188,31 +1189,34 @@ def getargvlist(cls, reader, value):
return commands

@classmethod
def processcommand(cls, reader, command):
def processcommand(cls, reader, command, replace=True):
posargs = getattr(reader, "posargs", "")
posargs_string = list2cmdline([x for x in posargs if x])

# Iterate through each word of the command substituting as
# appropriate to construct the new command string. This
# string is then broken up into exec argv components using
# shlex.
newcommand = ""
for word in CommandParser(command).words():
if word == "{posargs}" or word == "[]":
newcommand += posargs_string
continue
elif word.startswith("{posargs:") and word.endswith("}"):
if posargs:
if replace:
newcommand = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be new_command to be consistent with other variable names used around it

for word in CommandParser(command).words():
if word == "{posargs}" or word == "[]":
newcommand += posargs_string
continue
else:
word = word[9:-1]
new_arg = ""
new_word = reader._replace(word)
new_word = reader._replace(new_word)
new_word = new_word.replace('\\{', '{').replace('\\}', '}')
new_arg += new_word
newcommand += new_arg
elif word.startswith("{posargs:") and word.endswith("}"):
if posargs:
newcommand += posargs_string
continue
else:
word = word[9:-1]
new_arg = ""
new_word = reader._replace(word)
new_word = reader._replace(new_word)
new_word = new_word.replace('\\{', '{').replace('\\}', '}')
new_arg += new_word
newcommand += new_arg
else:
newcommand = command
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: the new else clause here is essential to the change. When replace=False, we should leave the command uninterpreted (we can't evaluate it since we're potentially missing some of the variables).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In combination with the current way of determining if replacements should happen that would mean though that it is possible that an environment that should run does not get any replacements if one of the variables is not set instead of rightfully crashing because there is a variable missing. Correct? I think I have create some example tox.ini and play this through to get my head around it though ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand your question. IMO, if you request to run an environment and we can't perform substitutions because something is missing, I would keep the existing behaviour (e.g. faily loudly) for the moment.

I agree that as a user, I would expect something else (e.g. run other envs, skip/fail the one(s) with the missing vars), but I would do that in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh .. I was not notified that you answered here :)

Actually I also prefer failing, but I am not sure if this is happening. I have to look closer at the test and do some exploring myself. Thanks.


# Construct shlex object that will not escape any values,
# use all values as is in argv.
Expand Down