Opened 6 years ago

Closed 5 years ago

Last modified 5 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: Russell Keith-Magee
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@…> 6 years ago.
t15372.diff (55.8 KB) - added by Carl Meyer 5 years ago.
deprecate setup_environ and fix default project layout and manage.py

Download all attachments as: .zip

Change History (14)

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

Attachment: django_module_name_fix added

comment:1 Changed 6 years ago by Russell Keith-Magee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: set
Resolution: duplicate
Status: newclosed
Triage Stage: UnreviewedAccepted

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 6 years ago by Jorge Vargas <jorge.vargas@…>

Resolution: duplicate
Status: closedreopened

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 6 years ago by Jorge Vargas <jorge.vargas@…>

Owner: changed from nobody to Russell Keith-Magee
Status: reopenednew

comment:4 Changed 6 years ago by Łukasz Rekucki

Cc: lrekucki@… added
Resolution: duplicate
Status: newclosed

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 6 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 5 years ago by Carl Meyer

Easy pickings: unset
Resolution: duplicate
Severity: Normal
Status: closedreopened
Type: 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 5 years ago by Carl Meyer

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

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 5 years ago by Carl Meyer

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 5 years ago by Carl Meyer

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 5 years ago by Catalin Iacob

Cc: iacobcatalin@… added

Changed 5 years ago by Carl Meyer

Attachment: t15372.diff added

deprecate setup_environ and fix default project layout and manage.py

comment:11 Changed 5 years ago by Carl Meyer

Resolution: fixed
Status: reopenedclosed

In [16964]:

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

comment:12 Changed 5 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