Change preservation behaviour for PYTHONPATH (backwards compatible)

[jarshwah]

I help to maintain https://github.com/django/django-box which is a vagrant machine setup with all of the dependencies needed to test Django. Django provides a tox file with explicit configuration to support the django-box vagrant machine. See: https://github.com/django/django/blob/3d14cbc86781ea1051af7f0c421bee3ecf2f9842/tox.ini

# relevant section
[testenv]
usedevelop = true
passenv = DJANGO_SETTINGS_MODULE PYTHONPATH HOME DISPLAY
setenv =
    PYTHONDONTWRITEBYTECODE=1
deps =
    py{3,34,35,36}: -rtests/requirements/py3.txt
    postgres: -rtests/requirements/postgres.txt
    mysql: -rtests/requirements/mysql.txt
    oracle: -rtests/requirements/oracle.txt
changedir = tests
commands =
    {envpython} runtests.py {posargs}

Various aliases are configured to run the test suite such as:

alias runtests36-postgres='PYTHONPATH=/home/vagrant/djangodata/ tox -c /django/tox.ini -e py36-postgres -- --settings=test_postgres'

The /home/vagrant/djangodata/ contains a bunch of python configuration files that define database connections. This directory is added to PYTHONPATH to make the configuration files available to the --settings= argument. Note the passenv in the tox configuration.

This worked perfectly fine in tox 2.3.0. Unfortunately, (after much debugging), I discovered that 2.4.0 removed support for PYTHONPATH entirely, even when explicitly defined in passenv.

remove PYTHONPATH from environment during the install phase because a tox-run should not have hidden dependencies and the test commands will also not see a PYTHONPATH. If this causes unforeseen problems it may be reverted in a bugfix release.

Our goals:

1. Avoid poluting django test directory with unrelated settings files. The settings files are related to the system doing the testing, and are passed to django.

2. Avoid hardcoding paths in the tox file, which isn't designed to only be used with this virtual machine.

If there's a way forward to make the settings directory available to the environment without PYTHONPATH, I'm all ears.

For me though, if I've explicitly configured PYTHONPATH in the passev configuration, then please, let me pass it through. Tox shouldn't be blessing or blocking environment variables with no work arounds.

I'd like to see this change reverted, but once again, I'm open to discussions on how best to fix this problem.

[nicoddemus]

As an aside, tox should emit a warning if the user is passing PYTHONPATH in passenv but tox is removing it implicitly.

[jarshwah]

#367 was what removed the ability to pass PYTHONPATH. I understand that this was supposed to fix a number of issues. To work around this, what about adding an option to the tox command line in the vein of --preserve-pythonpath? That would make it explicitly opt-in.

[obestwalter]

Quote from 2.4.0 changelog:

remove PYTHONPATH from environment during the install phase because a
tox-run should not have hidden dependencies and the test commands will also
not see a PYTHONPATH. If this causes unforeseen problems it may be
reverted in a bugfix release. Thanks Jason R. Coombs.

This makes sense to me and I don't think we should revert this as it did not cause unforeseen issues as such.

I guess we would merge a PR that would make it opt-in as suggested by @jarshwah?

[obestwalter]

As an aside, tox should emit a warning if the user is passing PYTHONPATH in passenv but tox is removing it implicitly.

@nicoddemus - definitely. I created an issue for that: #458

[nicoddemus]

Hmm instead of adding a new option, how about:

• If the user's environment has $PYTHONPATH set and passenv does not have PYTHONPATH: discard PYTHONPATH in the tox run, but warn the user (in yellow):

  Discarding $PYTHONPATH from environment, to override specify PYTHONPATH in 'passenv' in your environment.

• If user explicitly adds PYTHONPATH to passenv, it is not discarded and no warning is produced.

Seems to cover the use case presented here and also preserve the solution to the problems in #367.

[obestwalter]

Great idea. No option clutter and this would also squash #458.

[jarshwah]

That sounds sensible to me and like the most obvious solution.

…

On Fri., 17 Feb. 2017 at 23:16, Bruno Oliveira ***@***.***> wrote: Hmm instead of adding a new option, how about: - If the user's environment has $PYTHONPATH set and passenv does not have PYTHONPATH: discard PYTHONPATH in the tox run, but warn the user (in yellow): *Discarding $PYTHONPATH from environment, to override specify PYTHONPATH in 'passenv' in your environment.* - If user explicitly adds PYTHONPATH to passenv, it is not discarded and no warning is produced. Seems to cover the use case presented here and also preserve the solution to the problems in #367 <#367>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#457 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAhBm_R-1JG5UfuEnc7AzMQ84L6hmHvBks5rdY-0gaJpZM4MBsL9> .

[obestwalter]

For potential PRs: this should be explained in the docs as well ...

[jarshwah]

I'll put together a PR in the next couple of days.