Code

Opened 6 years ago

Last modified 23 months ago

#8754 new Bug

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

Reported by: Richard Davies <richard.davies@…> Owned by: nobody
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: richard.davies@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

8754_run_test.diff (586 bytes) - added by Richard Davies <richard.davies@…> 6 years ago.
8754_run_test_aggressive.diff (1.8 KB) - added by Richard Davies <richard.davies@…> 5 years ago.
Alternative more aggressive patch if we believe []

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by Richard Davies <richard.davies@…>

comment:1 Changed 6 years ago by Richard Davies <richard.davies@…>

  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 Changed 6 years ago by mtredinnick

  • milestone 1.0 deleted

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.

comment:3 Changed 6 years ago by kmtracey

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?

comment:4 Changed 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 5 years ago by Richard Davies <richard.davies@…>

  • milestone set to 1.1

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

comment:6 Changed 5 years ago by mtredinnick

  • milestone 1.1 deleted

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

comment:7 Changed 5 years ago by Richard Davies <richard.davies@…>

  • milestone set to 1.1

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.

Changed 5 years ago by Richard Davies <richard.davies@…>

Alternative more aggressive patch if we believe []

comment:8 Changed 5 years ago by 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.

comment:9 Changed 5 years ago by russellm

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?

comment:10 Changed 5 years ago by russellm

  • milestone 1.1 deleted

comment:11 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:12 Changed 3 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

8754_run_test_aggressive.diff fails to apply cleanly on to trunk

comment:13 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:14 Changed 23 months ago by akaariai

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.