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

pytoxbot · 2016-09-17T17:18:31Z

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 · 2016-09-17T17:18:32Z

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 · 2016-09-17T17:18:32Z

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 · 2016-09-17T17:18:32Z

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 · 2016-09-17T17:18:32Z

Original comment by romcheg

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

pytoxbot · 2016-09-17T17:18:32Z

Original comment by bossylobster

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

pytoxbot · 2016-09-17T17:18:32Z

Original comment by sfermigier

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

pytoxbot · 2016-09-17T17:18:32Z

Original comment by @maiksensi

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

pytoxbot · 2016-09-17T17:18:33Z

Original comment by @cedk

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

pytoxbot · 2016-09-17T17:18:33Z

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 · 2016-09-17T17:18:33Z

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 · 2016-09-17T17:18:33Z

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 · 2016-09-17T17:18:33Z

Original comment by @kozhukalov

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

pytoxbot · 2016-09-17T17:18:33Z

Original comment by @nelfin

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

pytoxbot · 2016-09-17T17:18:33Z

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 · 2016-09-17T17:18:34Z

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 · 2016-09-17T17:18:34Z

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 · 2016-09-17T17:18:34Z

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 · 2016-09-17T17:18:34Z

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 · 2016-09-17T17:18:34Z

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 · 2016-09-17T17:18:34Z

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 · 2016-09-17T17:18:34Z

Original comment by timmwagener

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

pytoxbot · 2016-09-17T17:18:35Z

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 · 2016-09-17T17:18:35Z

Original comment by waprin

Same issue:

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

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

obestwalter · 2017-03-10T18:20:45Z

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

nelfin · 2017-03-13T11:52:52Z

@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 · 2017-03-13T12:11:17Z

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 · 2017-03-13T12:41:32Z

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 · 2017-03-13T12:46:57Z

Don't bother including that test by the way, looks like it was covered by test_setenv_cross_section_subst_issue294 bd1d309

obestwalter · 2017-03-13T13:34:15Z

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

pytoxbot mentioned this issue Sep 17, 2016

Recursive replacement when getdict('setenv') #288

Closed

obestwalter added bug labels Nov 4, 2016

obestwalter closed this as completed Mar 13, 2017

obestwalter removed the help wanted label Mar 13, 2017

hugovk mentioned this issue Sep 19, 2017

Use latest Tox pylast/pylast#221

Merged

tox-dev locked and limited conversation to collaborators Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

obestwalter commented Mar 10, 2017

nelfin commented Mar 13, 2017

obestwalter commented Mar 13, 2017

nelfin commented Mar 13, 2017

nelfin commented Mar 13, 2017

obestwalter commented Mar 13, 2017

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

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

Comments

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

pytoxbot commented Sep 17, 2016

obestwalter commented Mar 10, 2017

nelfin commented Mar 13, 2017

obestwalter commented Mar 13, 2017

nelfin commented Mar 13, 2017

nelfin commented Mar 13, 2017

obestwalter commented Mar 13, 2017