PYTHONPATH not passed through to regressiontests/admin_scripts/AdminScriptTestCase.run_test()

[Richard Davies <richard.davies@…>]

In my set up, on shared hosting, Django depends on various packages in my ~/site-python directory, which I include in my PYTHONPATH

When regressiontests/admin_scripts starts a subprocess with AdminScriptTestCase.run_test(), it doesn't pass PYTHONPATH through, so those subprocesses fail when they hit one of these Django dependencies (e.g. psycopg2).

I attach a patch which passes PYTHONPATH through correctly.

[Richard Davies <richard.davies@…>]

Probably not that common, but does cause the test suite to fail on one of my machines on 1.0 rc1 and has a simple fix.

In my case it's psycopg2 in ~/site-python which is still needed by the subprocesses.

[Malcolm Tredinnick]

You're right: not that common and at this point in the release process, the bar is very high. This doesn't cause Django to crash in normal operations and doesn't eat data. Pushing to after 1.0.

[Karen Tracey]

This same problem was fixed once before, in [8149], but then it was un-done in [8238] for reasons I don't quite follow (having no Jython experience). Subsequently [8400] added use of JYTHONPATH instead of PYTHONPATH when running Jython. Given the way the code is written now, the current patch will append any pre-existing JYTHONPATH setting to the one used for the subprocess, yet the ticket that introduced use of JYTHONPATH (#8268) specifically says JYTHONPATH "should be cleared before calling a jython subprocess". So while it seems the patch would be fine for Python (it's essentially what was done in r8149) I'm concerned about the Jython implications. Though perhaps now that different names are being used for the environment variables the issue behind [8238] that caused the removal is no longer a factor? (Or it could be the discarding of use of old_python_path is [8238] was inadvertent -- that changeset is not very clear to me.) Could someone involved in those earlier changesets give an opinion on this patch?

[Richard Davies <richard.davies@…>]

Tested this morning on r10628. Bug still exists, and patch still works for me. Let's get this into 1.1.

[Malcolm Tredinnick]

Last comment completely fails to acknowledge having read and addressed Karen's comments in comment:3.

[Richard Davies <richard.davies@…>]

Apologies - I had assumed that Jacob's "Accepted" was in reply to Karen.

I'm not a Jython user, but have now read Karen's comments and references.

There are two scenarios:
A) external database backends (which all of the patches refer to. so is presumably more common),
B) built-in database backends which need to import the actual database driver (i.e. psycopg itself; my case)

In either case, PYTHONPATH, etc. may be needed to locate the relevant modules.

r8149 identified problem A, fixed for standard Python it by passing PYTHONPATH through, and hence also fixed B.

r8238 realized that the above did not work for Jython of that time, where PYTHONPATH is not used and JYTHONPATH did not yet exist. Hence it replaced r8149 with _ext_backend_path() which locates external database backends under either Python or Jython. This fixes A on Jython too, but coincidentally breaks B on both platforms.

r8400 doesn't address either A or B. It deal with the fact that Jython started to support configuration using JYTHONPATH (rather than -J-Dpython.path=), and hence we should be using that to configure the subprocesses. I think the comment on #8268 that Karen refers to is poorly phrased - JYTHONPATH does not need to be cleared for subprocesses (and indeed is not today!), but rather should be set and reset just like PYTHONPATH. [] What I'm not clear about with r8400 is whether we can not safely read JYTHONPATH, as well as writing to it. If this were the case, then we could back out all the complexity of r8238 and _ext_backend_path() completely.

So, in conclusion, r8238 did indeed inadvertently break scenario B (only scenario A was considered). I don't believe that there is ever any danger in adding to the end of either PYTHONPATH or JYTHONPATH - the code was just removed for cleanliness, given that _ext_backend_path() was meant to provide the same functionality.

Given that _ext_backend_path() does not in fact provide this functionality in scenario B, my original patch remains a valid fix, under Python or (I believe) Jython.

[Richard Davies <richard.davies@…>]

Alternative more aggressive patch if we believe []

[Richard Davies <richard.davies@…>]

As mentioned above under [] it may be that with the advent of JYTHONPATH the incoming configuration is always readable by that mechanism. If that's the case then we could now back out r8238 completely and lose some complexity. Here's a patch which does this - but would need someone who uses Jython with external database backends to test it before I actually prefer it to the less aggressive version.

[Russell Keith-Magee]

The reason for removing the full system PYTHONPATH from the test-time environment was that it was causing test failures that were very hard to diagnose. The closest I can come to documentation of this reasoning is ​comment 5 of #8047. However, it's easy to demonstrate that this is still a problem:

1. Create a directory that contains a valid settings.py.
2. Put that directory in your PYTHONPATH.
3. Apply one of the patches from this ticket.
4. Run the admin_scripts tests.
5. Enjoy your brand new 11 failures from 109 tests.
6. Profit.

We're kind of stuck between a rock and a hard place here. On the one hand, some users (like Richard) will need extra PYTHONPATH settings in order to make the admin_script test suite pass; on the other hand, some users (I have a vague recollection that either Malcolm or Karen was one of the victims here) will accidentally break the test suite by virtue of their PYTHONPATH.

I'm +0 to including this patch on the grounds that documenting the required test conditions is a known workaround for those people affected by PYTHONPATH-based test failures, but there is no workaround if you actually need the extra PYTHONPATH entries. Any other opinions?

[patchhammer]

8754_run_test_aggressive.diff fails to apply cleanly on to trunk

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Anssi Kääriäinen]

I just hit this issue, I ran tests using this setup:

cd tests
ln -s ../django django
./runtests --settings=test_sqlite admin_scripts

However, the tests use my system's Django, not the django from tests/django. Using virtualenv for test setup fixed this.

[Tim Graham]

I think using a virtual environment is the correct solution these days.