tox 2.2.0+ breaks some tox.ini config files due to recursion issue

[pytoxbot]

• Bitbucket: https://bitbucket.org/hpk42/tox/issue/285
• Originally reported by: @booxter
• Originally created at: 2015-11-11T18:28:26.726

tox 2.2.0+ breaks the following projects in OpenStack: http://codesearch.openstack.org/?q=%3D\{env%3A.*%3F%3A.*%3F\}&i=nope&files=tox.ini&repos=

networking-l2gw, networking-vsphere, neutron, neutron-lbaas, and python-fuelclient

The OpenStack bug for the failure is: https://bugs.launchpad.net/neutron/+bug/1515335

The logs of the failure can be found at: http://logs.openstack.org/22/244122/2/gate/gate-neutron-python27/414e071/console.html#_2015-11-11_16_10_42_148

The line in tox.ini that triggers it is: https://github.com/openstack/neutron/blob/master/tox.ini#L26

Copying the section that triggers the issue here just in case:

[testenv:api] basepython = python2.7 setenv = {[testenv]setenv} OS_TEST_PATH=./neutron/tests/api TEMPEST_CONFIG_DIR={env:TEMPEST_CONFIG_DIR:/opt/stack/tempest/etc} OS_TEST_API_WITH_REST=1

The problematic line is the one setting TEMPEST_CONFIG_DIR.

[pytoxbot]

Original comment by sfermigier

Current version is 2.3.1 which fixes the issue, at least to people who have tried it. So I guess the issue can be closed.

[pytoxbot]

Original comment by @booxter

It's two months since the bug was revealed. Isn't it time to fix it either by a new release with a fix or a revert of the offending patch?

[pytoxbot]

Original comment by @hugovk

• Tox 2.1.1 works: https://travis-ci.org/pylast/pylast/builds/95971287
• Tox 2.2.1 doesn't work: https://travis-ci.org/pylast/pylast/builds/79797764
• Tox 2.3.0.dev2 works: https://travis-ci.org/hugovk/pylast/builds/95973938

The actual tests are failing, but the point is Tox works and actually runs the tests with 2.1.1 and 2.3.0.dev2, whereas 2.2.1 gives RuntimeError: maximum recursion depth exceeded.

Thanks!

[pytoxbot]

Original comment by romcheg

Tested 2.3.0.dev3 with python-fuelclient. Seems to work.

[pytoxbot]

Original comment by bossylobster

https://github.com/google/oauth2client still broken

[pytoxbot]

Original comment by sfermigier

Works for the Abilian project (unlike version 2.2.1). Thanks !

[pytoxbot]

Original comment by @maiksensi

I also tested 2.3.0.dev3 - works great. Thank you!

[pytoxbot]

Original comment by @cedk

I tested the version 2.3.0.dev3 and it works for the Tryton project. Thanks.

[pytoxbot]

Original comment by @hpk42

i think i have fully fixed the issue now, please try it out with:

#!python

pip install --pre -i https://devpi.net/hpk/dev -U tox

which should give you tox==2.3.0.dev2 as least (e.g. through tox --version).

[pytoxbot]

Original comment by @cedk

@hpk42 Don't you think it will be better to apply your plan from https://bitbucket.org/hpk42/tox/issues/285/tox-220-breaks-some-toxini-config-files#comment-23275850 and publish a release that doesn't break the existing configuration files until this issue is fixed?

Thanks.

[pytoxbot]

Original comment by @hpk42

@nelfin if you could integrate my tests from the branch and they all pass that'd be helpful. Haven't had time yet to look into your PR in detail but it looks promising that it causes no regressions so far.

[pytoxbot]

Original comment by @kozhukalov

Issue #288 was marked as a duplicate of this issue.

[pytoxbot]

Original comment by @nelfin

@kozhukalov, this breaks non-env: substitutions in [setenv].

[pytoxbot]

Original comment by @kozhukalov

This patch fixes the issue

--- a/tox/config.py Wed Nov 11 15:58:33 2015 +0100
+++ b/tox/config.py Fri Nov 20 10:45:40 2015 +0300
@@ -835,8 +835,8 @@
             return []
         return [x.strip() for x in s.split(sep) if x.strip()]

-    def getdict(self, name, default=None, sep="\n"):
-        s = self.getstring(name, None)
+    def getdict(self, name, default=None, sep="\n", replace=True):
+        s = self.getstring(name, None, replace=replace)
         if s is None:
             return default or {}

@@ -910,7 +910,7 @@
         return '\n'.join(filter(None, map(factor_line, lines)))

     def _replace_env(self, match):
-        env_list = self.getdict('setenv')
+        env_list = self.getdict('setenv', replace=False)
         match_value = match.group('substitution_value')
         if not match_value:
             raise tox.exception.ConfigError(

[pytoxbot]

Original comment by @stefano-m

I am having the same problem with tox 2.2.1 where I reference {[testenv]setenv} in a platform-dependent section that contains itself an environment substitution, namely APP_TEMP_DIR = {env:TEMP:c:\\temp}.

I have the feeling that one substitution bounces back to the other.

PDB tells me that the recursion fails in

tox/config.py(995)_replace()

where the following is called

RE_ITEM_REF.sub(self._replace_match, x)

Maybe it has something to do with the

SectionReader._replace_match

method?

[pytoxbot]

Original comment by @nelfin

Actually no regression tests fail in my pull request. It was only the initial changeset that introduced a failure. I'll look to adding another test case for {envsitepackagesdir} and/or integrating your changes on /issue285 where applicable this weekend.

[pytoxbot]

Original comment by @hpk42

@nelfin i didn't look at it yet. Will check it out next week -- even if just one regression test fails (like also in my branch) it can hint at a severe regression that will break people's tox.ini file. If i am not mistaken we also need to be lazy wrt to subtitutions like {envsitepackagesdir} or (currently not lazy) {envbindir} etc. . Here is quick unverified pseudo-example::

#!python

[testenv]
setenv = 
    COMPUTE = {envbindir}/something
envdir = /somedir 
basepython = {envbindir}/somepython
commands = 
     {env:COMPUTE} ...

This means we can not just compute setenv completely up-front.

could you mark the tests that fail with your branch pytest.mark.xfail(reason='issue285-fixing broke this')"? have a nice weekend, holger

[pytoxbot]

Original comment by @nelfin

@hpk42 have you seen my pull request from a little while ago? https://bitbucket.org/hpk42/tox/pull-requests/176/compute-closure-of-osenviron-and

[pytoxbot]

Original comment by @hpk42

I just spend 4 hours trying to resolve this issue according to my plan. See https://bitbucket.org/hpk42/tox/branch/issue285#diff for the current work. It fixes all setenv issues but there remains one more ordering-issue which i couldn't quickly resolve. Am running out of time for this week, probably will try on tuesday or wednesday to continue if nobody else can fix the remaining bug (which is a hairy ordering issue if i am not mistaken). I also need to review what i did this morning.

Folks, one more thing: my company (http://merlinux.eu) offers support contracts for fixing bugs, implementing features for tox, devpi, pytest: you might consider getting one for your company if you heavily depend on things. Usually we then fix things within two days. We have three companies who signed up and their issues usually take precedence FYI.

[pytoxbot]

Original comment by @hpk42

Seems we need to work harder for env-substitution. The problem was a side effect from me merging https://bitbucket.org/hpk42/tox/pull-requests/169/tries-to-fix-99/diff trying to fix #99 where i failed to see in the review that env-substitution breaks for setenv itself. Maybe @itxaka can also take a look.

I guess we need to get more lazy about parsing the setenv NAME=VALUE settings and that's not just a few lines of change if i am not mistaken. We probably need to introduce a mode where we don't do env-subsitutions on VALUE so that we can know about all NAMEs in setenv. And in a second pass more intelligently resolve setenv-set variables (noting recursions etc.)

Not sure when i get to it. If no fix is done neither by any of you nor by mid next week i'll probably revert the PR that caused the problem (it's valid to want to have #99 resolved of course but on balance it's more important to keep existing configs working).

[pytoxbot]

Original comment by timmwagener

Same issue here. Not sure if I'm allowed to show tox.ini files...

[pytoxbot]

Original comment by @nelfin

I have a partial fix for this here: https://bitbucket.org/nelfin/tox/branches/compare/nelfin/tox:37ee634%0Dhpk42/tox:default#diff but this introduces a regression and I haven't yet fixed self-referencing keys like this:

[testenv]
setenv = 
  TEST={env:TEST:default}

[pytoxbot]

Original comment by waprin

Same issue:

https://github.com/googlecloudplatform/python-docs-samples

https://travis-ci.org/GoogleCloudPlatform/python-docs-samples/builds/90595623

[obestwalter]

This one flies far over my head. Can anyone who is into this give a bit of feedback about this?

[nelfin]

@obestwalter from memory @hpk42's work on lazy evaluation of setenv substitutions solved 99.9% of people's use cases for this: only mutually recursive setenv clauses weren't fixed, but they never worked previously:

def test_setenv_mutual_reference_issue285(self, monkeypatch, newconfig):
    """Prevent setenv variables from producing mutual infinite recursion."""
    monkeypatch.delenv('TEST1', raising=False)
    monkeypatch.delenv('TEST2', raising=False)
    config = newconfig("""
        [testenv]
        setenv =
          TEST1={env:TEST2}
          TEST2={env:TEST1}
    """)
    reader = SectionReader('testenv', config._cfg)
    assert reader is not None
    with pytest.raises(tox.exception.ConfigError):
        reader.getargvlist('setenv')

[obestwalter]

Hi @nelfin thanks, but where is that test from? I can't find it in the current incarnation and the changelog refers to this issue as fixed (with your help :)).

I just adapted your test to the current version and it passes, which means that a config error is thrown correctly to prevent the recursion. I wonder why that test is not in the code base anymore though.

Shall we add this test to document this behaviour?

We can't close the bug 99.9% so I would say, we close it 100% and if that specific problem pops up again a new issue needs to be filed.

[nelfin]

I don't know if it ever was in the codebase, that test came from https://bitbucket.org/hpk42/tox/pull-requests/176/compute-closure-of-osenviron-and/diff. I agree with your sentiment and say close this as it appears to be fixed. Specifically, https://github.com/tox-dev/tox/blob/master/tox/config.py#L282 seems to check for setenv cycles.

[nelfin]

Don't bother including that test by the way, looks like it was covered by test_setenv_cross_section_subst_issue294 https://github.com/tox-dev/tox/commit/bd1d309fe0464e5c2e43de5c90197a5149532abe

[obestwalter]

o.k. great let's close this then - thanks for bringing me up to speed!