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

Issue #285 on hold
Ihar Hrachyshka
created an issue

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.

Comments (25)

  1. Holger Krekel repo owner

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

  2. Holger Krekel repo owner

    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.

  3. Holger Krekel repo owner

    @n 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::

    [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

  4. n

    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.

  5. Stefano

    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?

  6. Vladimir 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(
    
  7. Holger Krekel repo owner

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

  8. Holger Krekel repo owner

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

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

  9. Log in to comment