Isolate environment variable substitutions #521

AndreLouisCaron · 2017-05-18T18:00:01Z

Perform {env:X} replacement only in environments where X is used. This
enables different environments to use different subsets of the environment
(e.g. don't require union of all passenv sections in all environments).

Fixes #515.

Note to reviewers: this is a tentative fix. I'm definitely not sure how to best proceed with this fix. Feel free to recommend an alternate approach!

Perform `{env:X}` replacement only in environments where `X` is used. This enables different environments to use different subsets of the environment (e.g. don't require union of all passenv sections in all environments).

The-Compiler · 2017-05-18T20:39:02Z

Can't speak for the code, but I've had this problem before too.

AndreLouisCaron · 2017-06-01T13:38:26Z

Ping?

obestwalter · 2017-06-30T11:02:41Z

Hi @AndreLouisCaron thanks for the fix. Can you quickly tell me why you are not confident that the fix is the best way to go forward? I did not check the code yet, just wanted to get more insight, before I review it.

AndreLouisCaron · 2017-06-30T13:15:28Z

@obestwalter Some methods already had a replace argument, but not all of them. Also, it seemed (to me, without context) to be inconsistently applied. I couldn't figure out why it was applied in some places and not others, but maybe it was written like that for a good reason that is unknown to me :-)

obestwalter · 2017-06-30T13:22:50Z

o.k. thanks - I will take that into account and dig a bit through history to check whether there is some background to it.

AndreLouisCaron · 2017-06-30T14:40:24Z

tox/config.py

@@ -818,7 +819,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)


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

AndreLouisCaron · 2017-06-30T14:41:05Z

tox/config.py

@@ -787,7 +787,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)


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.

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.

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?

Yes, that's what I meant.

AndreLouisCaron · 2017-06-30T14:42:26Z

tox/config.py

+                new_arg += new_word
+                newcommand += new_arg
+        else:
+            newcommand = command


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

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

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.

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.

AndreLouisCaron · 2017-06-30T14:43:29Z

@obestwalter I added a few notes in the review to cue in on my reasoning. I hope it helps without skewing your independent review :-)

obestwalter · 2017-06-30T14:44:16Z

Thanks, very appreciated :)

AndreLouisCaron · 2017-06-30T16:45:06Z

tox/config.py

@@ -953,12 +954,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)


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

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

obestwalter · 2017-07-13T11:20:34Z

I'll review and test this at the EuroPython tox sprint weekend and then it will likely be in 2.8.

AndreLouisCaron · 2017-07-13T14:01:51Z

Thanks a bunch!

obestwalter

A few questions are open - see comments

obestwalter · 2017-07-16T11:51:05Z

tox/config.py

@@ -787,7 +787,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)


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?

obestwalter · 2017-07-16T11:51:58Z

tox/config.py

@@ -953,12 +954,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)


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

obestwalter · 2017-07-16T11:57:19Z

tox/config.py

+                new_arg += new_word
+                newcommand += new_arg
+        else:
+            newcommand = command


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

gaborbernat · 2017-07-18T07:05:37Z

tox/config.py

-            elif word.startswith("{posargs:") and word.endswith("}"):
-                if posargs:
+        if replace:
+            newcommand = ""


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

AndreLouisCaron · 2017-07-25T15:43:01Z

@obestwalter Can't reply directly to your comment.

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?

Yes.

AndreLouisCaron · 2017-07-25T15:45:00Z

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

Like I said, I'd like to answer, but I'm not sure I understand what your question is.

obestwalter · 2017-08-11T10:44:18Z

o.k. - I'll merge this now and fix up anything that is not quite there yet (e.g. one missing passing of replace). Will test this today and cut a release candidate from it at some point at the weekend. @AndreLouisCaron it would be great if you test the rc then yourself a bit and check if everything is as it should be.

AndreLouisCaron · 2017-08-11T13:17:54Z

Sorry, was on vacation the past 2 weeks and did not follow up. Thanks for proceeding with this! Let me know when the RC is up, I will gladly try it out :-)

fix up #521: add missing pass of arg to underlying function.

obestwalter · 2017-08-26T14:53:56Z

@AndreLouisCaron - hope you had a nice holiday :) rc1 with your changes is out already.

Isolate environment variable substitutions

10b0d6c

Perform `{env:X}` replacement only in environments where `X` is used. This enables different environments to use different subsets of the environment (e.g. don't require union of all passenv sections in all environments).

AndreLouisCaron commented Jun 30, 2017

View reviewed changes

Merge branch 'master' into envsubs-independence

89848ac

AndreLouisCaron commented Jun 30, 2017

View reviewed changes

Merge branch 'master' into envsubs-independence

16529d2

obestwalter reviewed Jul 17, 2017

View reviewed changes

gaborbernat reviewed Jul 18, 2017

View reviewed changes

obestwalter merged commit 67e88f6 into tox-dev:master Aug 11, 2017

obestwalter added a commit that referenced this pull request Aug 13, 2017

Merge pull request #574 from Avira/master

712c681

fix up #521: add missing pass of arg to underlying function.

cboylan mentioned this pull request Sep 1, 2017

2.8.0: {[testenv]setenv} substitution broken #595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isolate environment variable substitutions #521

Isolate environment variable substitutions #521

AndreLouisCaron commented May 18, 2017

The-Compiler commented May 18, 2017

AndreLouisCaron commented Jun 1, 2017

obestwalter commented Jun 30, 2017

AndreLouisCaron commented Jun 30, 2017

obestwalter commented Jun 30, 2017

AndreLouisCaron Jun 30, 2017

AndreLouisCaron Jun 30, 2017

AndreLouisCaron Jun 30, 2017

obestwalter Jul 16, 2017

AndreLouisCaron Jul 18, 2017

AndreLouisCaron Jun 30, 2017

obestwalter Jul 16, 2017

AndreLouisCaron Jul 18, 2017

obestwalter Jul 27, 2017

AndreLouisCaron commented Jun 30, 2017

obestwalter commented Jun 30, 2017

AndreLouisCaron Jun 30, 2017

obestwalter Jul 16, 2017

obestwalter commented Jul 13, 2017

AndreLouisCaron commented Jul 13, 2017

obestwalter left a comment

obestwalter Jul 16, 2017

obestwalter Jul 16, 2017

obestwalter Jul 16, 2017

gaborbernat Jul 18, 2017

AndreLouisCaron commented Jul 25, 2017

AndreLouisCaron commented Jul 25, 2017

obestwalter commented Aug 11, 2017

AndreLouisCaron commented Aug 11, 2017

obestwalter commented Aug 26, 2017

Isolate environment variable substitutions #521

Isolate environment variable substitutions #521

Conversation

AndreLouisCaron commented May 18, 2017

The-Compiler commented May 18, 2017

AndreLouisCaron commented Jun 1, 2017

obestwalter commented Jun 30, 2017

AndreLouisCaron commented Jun 30, 2017

obestwalter commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreLouisCaron commented Jun 30, 2017

obestwalter commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obestwalter commented Jul 13, 2017

AndreLouisCaron commented Jul 13, 2017

obestwalter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreLouisCaron commented Jul 25, 2017

AndreLouisCaron commented Jul 25, 2017

obestwalter commented Aug 11, 2017

AndreLouisCaron commented Aug 11, 2017

obestwalter commented Aug 26, 2017