Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15372 closed Bug (fixed)

manage.py adds to sys.path and forcibly imports the parent dir of the settings module

Reported by: Jorge Vargas <jorge.vargas@…> Owned by: russellm
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: lrekucki@…, iacobcatalin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have tested this with django 1.2.5

Due to django's custom handling of apps and using it's own backport of importlib it is leaving some cruft behind in the module name. Although it's doing the right job as cleaning everything else.

The ofending code is in django/core/management/init.py The last couple of lines from

def setup_environ(settings_mod, original_settings_path=None):

     ............

    # Import the project module. We add the parent directory to PYTHONPATH to
    # avoid some of the path errors new users can have.
    sys.path.append(os.path.join(project_directory, os.pardir))
    project_module = import_module(project_name)
    sys.path.pop()

I have created a vanilla project with some print/trace statements that show the problem.The code is here https://bitbucket.org/elpargo/django_import_cruft_test if you run that you will see the problem.

(test_4148)elpargo@elpargo-linux:~/venvs/test_4148/src/import_cruft_test$ python manage.py runserver
pre setting 1
********************************************************************************
in settings.py
********************************************************************************
post settings 2
********************************************************************************
pre execute_manager 3
********************************************************************************
in settings.py
import_cruft_test.sys None
import_cruft_test <module 'import_cruft_test' from '/home/elpargo/venvs/test_4148/src/import_cruft_test/../import_cruft_test/__init__.pyc'>
import_cruft_test.settings <module 'import_cruft_test.settings' from '/home/elpargo/venvs/test_4148/src/import_cruft_test/../import_cruft_test/settings.pyc'>
********************************************************************************
pre setting 1
********************************************************************************
in settings.py
********************************************************************************
post settings 2
********************************************************************************
pre execute_manager 3
********************************************************************************
in settings.py
import_cruft_test.sys None
import_cruft_test <module 'import_cruft_test' from '/home/elpargo/venvs/test_4148/src/import_cruft_test/../import_cruft_test/__init__.pyc'>
import_cruft_test.settings <module 'import_cruft_test.settings' from '/home/elpargo/venvs/test_4148/src/import_cruft_test/../import_cruft_test/settings.pyc'>
********************************************************************************
Validating models...
0 errors found

Django version 1.2.5, using settings 'import_cruft_test.settings'
Development server is running at http://127.0.0.1:8000/
Quit the server with CONTROL-C.

The attached patch seems to fix this, my colleague ran the 2.x test suite against it and everything passes. I try to run it against trunk (1.3.0 pre-beta) however I think there is something wrong with my setup as I get 13 failures (with or without the patch) In either case We don't think it breaks anything however there is a change this is not tested extensibly.

Why is all of this relevant? The django-hudson extension looks into sys.modules in order to generate the coverage report. Which means that in hudson UI you get some ugly paths like venv/src/your_app/../your_app/path/to/your/code

Attachments (2)

django_module_name_fix (595 bytes) - added by Jorge Vargas <jorge.vargas@…> 4 years ago.
t15372.diff (55.8 KB) - added by carljm 4 years ago.
deprecate setup_environ and fix default project layout and manage.py

Download all attachments as: .zip

Change History (14)

Changed 4 years ago by Jorge Vargas <jorge.vargas@…>

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Resolution set to duplicate
  • Status changed from new to closed
  • Triage Stage changed from Unreviewed to Accepted

Closing as a duplicate of #15064 -- It isn't strictly the same bug, but the root cause is the same: django-admin shouldn't be fiddling with sys.path like this.

comment:2 Changed 4 years ago by Jorge Vargas <jorge.vargas@…>

  • Resolution duplicate deleted
  • Status changed from closed to reopened

I don't think it's the same bug. Please note my problem is not limited to runserver. I used that as an example. The actual command we run is django manage.py hudson from the django-hudson app.

As you point out it's the same cause. IMO this bug is the main bug and #15064 is a consequence of this.

You said the patch needs improvement but do not point to how. I don't know this code much but removing the sys.path modifications probably means a mayor rewrite of the way django imports apps. And this code (according to SVN history) hasn't been touch significantly in over two years.

comment:3 Changed 4 years ago by Jorge Vargas <jorge.vargas@…>

  • Owner changed from nobody to russellm
  • Status changed from reopened to new

comment:4 follow-up: Changed 4 years ago by lrekucki

  • Cc lrekucki@… added
  • Resolution set to duplicate
  • Status changed from new to closed

1) Don't assign tickets to people without their permission.

2) This problem has two causes: one is sys.path mangling done by Django; second is a poor report generator in coverage.py (this is the module that actually creates the Cobertura report - not django-hudson). First is being tracked by #15064, second is out of scope for Django.

Personally, I tweaked the way reports are generated in my fork of django-hudson to enjoy pretty names relative to my codebase: https://github.com/lqc/django-hudson/blob/unittest2/django_hudson/plugins/coverage.py#L173

comment:5 in reply to: ↑ 4 Changed 4 years ago by Jorge Vargas <jorge.vargas@…>

Replying to lrekucki:

1) Don't assign tickets to people without their permission.

Just making the notification work, as he was the one that assigned the status.

2) This problem has two causes: one is sys.path mangling done by Django; second is a poor report generator in coverage.py (this is the module that actually creates the Cobertura report - not django-hudson). First is being tracked by #15064, second is out of scope for Django.

Again I believe #15064 is a subset of this, as this ticket explains the problem better. As for coverage, I don't agree, it's ok that coverage.py generates full path reports based on the module. But as you said that's out of scope.

Personally, I tweaked the way reports are generated in my fork of django-hudson to enjoy pretty names relative to my codebase: https://github.com/lqc/django-hudson/blob/unittest2/django_hudson/plugins/coverage.py#L173

Thanks for the pointer. I'll take a look, although the problem still is django messing up sys.path.

comment:6 Changed 4 years ago by carljm

  • Easy pickings unset
  • Resolution duplicate deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Bug
  • UI/UX unset

Reopening - this was closed as duplicate of #15064, but the fix for #15064 didn't actually deal with the core problem, the sys.path mangling done by setup_environ. This ticket is the clearest explanation of that problem that I can find, and thus should remain open.

To be clear, this ticket will be fixed when Django is no longer mucking with sys.path, not just if the mucking is done with an absolute path; thus the attached patch is not adequate.

comment:7 Changed 4 years ago by carljm

  • Needs tests set
  • Patch needs improvement unset
  • Summary changed from Django modifies the module name while importing it. to manage.py adds to sys.path and forcibly imports the parent dir of the settings module
  • Version changed from 1.2 to SVN

Proposed patch: https://github.com/carljm/django/compare/master...t15372

Marking "needs tests" tentatively - all current tests pass, and the code changes here are almost entirely to the project template, which is currently untested and would need quite a bit of new test harness work to make testable. But I may look into what tests could reasonably be added there.

comment:8 Changed 4 years ago by carljm

Added the patch as an attachment for those who prefer that; also created a pull request at https://github.com/django/django/pull/62, to allow for inline comments on the patch.

comment:9 Changed 4 years ago by carljm

Much more detailed breakdown of the problem and proposed solution at http://groups.google.com/group/django-developers/browse_frm/thread/44b70a37ff73298b

comment:10 Changed 4 years ago by cataliniacob

  • Cc iacobcatalin@… added

Changed 4 years ago by carljm

deprecate setup_environ and fix default project layout and manage.py

comment:11 Changed 4 years ago by carljm

  • Resolution set to fixed
  • Status changed from reopened to closed

In [16964]:

Fixed #15372 -- Switched to a startproject default layout that allows us to avoid sys.path hacks.

comment:12 Changed 4 years ago by Jorge Vargas <jorge.vargas@…>

This looks like a great solution, Thanks Carl!

Note: See TracTickets for help on using tickets.
Back to Top