A real-world problem with Tox 2+ environment variable isolation

Issue #263 on hold
saaj created an issue

Another v2 problem that took me several hours of wondering why.

Briefly, it was a CI service drone.io and a code coverage service codecov.io. Here's the relevant snippet from its client that can explain the problem without me talking much about it:

elif os.getenv('CI') == "true" and os.getenv('DRONE') == "true":
    # http://docs.drone.io/env.html
    defaults.update(dict(branch=os.getenv('DRONE_BRANCH'),
                         service='drone.io',
                         build=os.getenv('DRONE_BUILD_NUMBER'),
                         build_url=os.getenv('DRONE_BUILD_URL'),
                         commit=os.getenv('DRONE_COMMIT')))

And an QA env that may look something like this obviously no longer sends coverage data because it can't get information about the environment.

[testenv:qa]
deps = coverage
commands = 
  coverage run --branch --source=lib setup.py test
  codecov

I bet most of much QA services and their clients work the same way to identify service, commit, branches, etc. And it's quite reasonable to do such things from within a Tox env or even have dedicated Tox envs. So there's a serious doubt that this idea of environment isolation (which already leaks because of LANG exception) turned on by default is compatible with real-world Tox usage. At very least there should be an option to restore outer environment at once.

Of course one can look at the env spec of a CI service, then examine QA service's client and put bunch of passenv in tox.ini. But have doubts than many developer will be happy about it.

As a side note, because it's already third v2 problem (others were missing LANG with broken py3, missing {envsitepackagesdir}) I tend to think current v2 series is not production ready and is beta at most. Reverted to v1 again.

Comments (14)

  1. Holger Krekel repo owner

    Are you aware that you can set an env-var TOX_TESTENV_PASSENV=* which will pass along all environment variables for all tox invocations on a machine?

  2. saaj reporter

    I wasn't aware of this. Though, the issue is more about the default isolation and its consequences.

  3. Holger Krekel repo owner

    yes, i know that the introduction of default isolation caused some doubtful troubles. I had asked on the testing-in-python mailing list before and people were in favor. In retrospect i would not do it again, though.

  4. Holger Krekel repo owner

    just checked and i think we could change the default such that if no passenv setting is done, everything is copied from the invocation environment. if passenv is set it would work just as now.

    Just not sure it's at this point a good idea to change something again.

  5. Florian Bruhin

    So no passenv would be passenv = *, and passenv = (does that work) would be the current behaviour?

    IMHO the isolation is a good thing, it helped me find real bugs e.g. because of LC_ALL.

  6. Stefano

    FWIW, I am in favor of isolation, but I would make it easy to explicitly turn it off in tox.ini.

    So, users who wanted to remove isolation would have to set passenv = * and maybe tox could even log this.

    What is not clear to me right now is whether there are already exceptions to passenv so that even if I explicitly did passenv = some variables would be passed anyway (LANG? LD_LIBRARY_PATH?).

  7. Florian Bruhin

    So, users who wanted to remove isolation would have to set passenv = * and maybe tox could even log this.

    That's already the current behaviour, no?

    What is not clear to me right now is whether there are already exceptions to passenv so that even if I explicitly did passenv = some variables would be passed anyway

    It seems like that's indeed the case:

    tox.ini:

    [tox]
    skipsdist = true
    
    [testenv]
    # passenv =
    # passenv = *
    commands = {envpython} env.py
    

    env.py:

    import os
    from pprint import pprint
    
    pprint(dict(os.environ))
    

    Without passenv:

    {'LANG': 'en_US.UTF-8',
     'PATH': '/home/florian/tmp/passenv/.tox/python/bin:/usr/lib/ccache/bin/:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:/home/florian/bin',
     'PYTHONHASHSEED': '3274241618',
     'VIRTUAL_ENV': '/home/florian/tmp/passenv/.tox/python'}
    

    With passenv =:

    {'LANG': 'en_US.UTF-8',
     'PATH': '/home/florian/tmp/passenv/.tox/python/bin:/usr/lib/ccache/bin/:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:/home/florian/bin',
     'PYTHONHASHSEED': '4069066776',
     'VIRTUAL_ENV': '/home/florian/tmp/passenv/.tox/python'}
    

    With passenv = *:

    {'ACK_COLOR_MATCH': 'bold red',
     [...]
     'vcs_info_msg_1_': ''}
    

    I'd prefer passenv = to mean "don't pass any variables at all" as well.

  8. saaj reporter

    @The-Compiler to me it also sounds like "fuzzing", not only "isolation" (tough I also found out that my packages couldn't be installed in casual Docker container with bare locale). Some environment variables are set to sane values most of the time, like LANG, and resetting it to C makes the environment a feel under-fuzzing-test (and breaks some expectation, like in case of Python3). Tox isn't really a locale fuzzing testing tool. It's a Python virtual environment automation tool. As a matter of best practice it should have been better placed in Tox documentation as a recommendation.

    I think in Tox v3 the decision should be taken (or revised) more deliberately than it was in v2.

  9. Stefano

    @The-Compiler I was not aware that passenv = * was a valid configuration option (I must have missed that in the documentation).

    @saaj I guess that there are some cases where one might always pass a minimal set of "sane" env vars, but often we do not know for sure what those env vars look like.

    Personally, I would be OK with a small set of well documented env vars that are always passed, but that would also be the gateway to add exceptions after exceptions.

    I am convincing myself that tox should probably not pass any env var unless passenv = * is specifically set, and even in that case a message should be logged in the output.

  10. Holger Krekel repo owner

    For the record, https://testrun.org/tox/latest/config.html#confval-passenv=SPACE-SEPARATED-GLOBNAMES documents what names are passed by default. It also mentions TOX_TESTENV_PASSENV environment variable with which you can influence all tox runs (e.g. for passing travis specific environment varaibles down to the tools). At this point we can not change the meaning of "passenv = ..." to mean "only pass down those env vars" because it would break existing test suites.

  11. Log in to comment