Closed Bug 518840 Opened 10 years ago Closed 10 years ago

settings.py references absolute paths

Categories

(Webtools :: ISPDB Server, defect)

All
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ehenry5, Assigned: ehenry5)

Details

Attachments

(1 file, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/532.0 (KHTML, like Gecko) Chrome/4.0.211.7 Safari/532.0
Build Identifier: 

The settings.py file has absolute path references to a specific filesystem not generalised for any user.

For example:

MEDIA_ROOT = '/Users/davida/src/ispdb/media/'

Reproducible: Always

Steps to Reproduce:
Checked out files from the repository, followed steps in README, then runserver
Actual Results:  
django error can't find the template

Expected Results:  
ispdb site
Attachment #402842 - Flags: review?(bwinton)
>Index: production_settings.py
>+from settings import *

I normally frown on import *, but in this case it makes sense, since you're overriding the base settings.

>+DEBUG = True
>+TEMPLATE_DEBUG = DEBUG

Does it make sense for the production settings to have DEBUG set to true?

>+CWD = os.path.abspath("./")

This almost seems more like a developer/local setting than a production setting.

What do you think about calling this something like "APP_ROOT", or "ISPDB_ROOT"?

>Index: settings.py
> DATABASE_ENGINE = 'sqlite3'           # 'postgresql_psycopg2', 'postgresql', 'mysql', 'sqlite3' or 'oracle'.
>-DATABASE_NAME = 'ispdb.sqlite'             # Or path to database file if using sqlite3.
>-DATABASE_USER = ''             # Not used with sqlite3.
>-DATABASE_PASSWORD = ''         # Not used with sqlite3.
>-DATABASE_HOST = ''             # Set to empty string for localhost. Not used with sqlite3.
>-DATABASE_PORT = ''             # Set to empty string for default. Not used with sqlite3.
>+DATABASE_NAME = 'ispdb.sqlite'        # Or path to database file if using sqlite3.
>+DATABASE_USER = ''                    # Not used with sqlite3.
>+DATABASE_PASSWORD = ''                # Not used with sqlite3.
>+DATABASE_HOST = ''                    # Set to empty string for localhost. Not used with sqlite3.
>+DATABASE_PORT = ''                    # Set to empty string for default. Not used with sqlite3.

These seem like they might be different between production, staging, and development.

> # If you set this to False, Django will make some optimizations so as not
> # to load the internationalization machinery.
>-USE_I18N = True
>+#USE_I18N = True
>+USE_I18N = False

That seems wrong.

>+LOCAL_DEVELOPMENT = True

What does this do?  Could you add a comment?

It might be easier to rename "production_settings.py" to "local_settings.py", because then many of my issues would kind of go away.  :)

Thanks,
Blake.
Assignee: nobody → ehenry5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 402842 [details] [diff] [review]
Splits settings.py into 2 files and uses os.path.abspath("./")

I'm going to set this to review-, which means that you'll need to upload a new patch and re-ask me for a review.

And, while you're submitting that, you should ask gozer for a super-review (or sr, I forget what the ui uses).

Thanks,
Blake.
Attachment #402842 - Flags: review?(bwinton) → review-
Attachment #402908 - Flags: review?(bwinton) → review-
Comment on attachment 402908 [details] [diff] [review]
Splits settings.py into 2 files and uses os.path.abspath("./") (new attempt)

We're getting much closer with this patch, but there are still a couple of things I'ld like changed.

>+++ b/local_settings.py
>@@ -0,0 +1,32 @@
>+# Not sure what this should be since I don't have this directory at all,
>+# so commenting it out is my best idea.
>+# - ehenry5
>+#DOCS_ROOT = os.path.join(ISPDB_ROOT, "/Users/davida/src/django/docs/")

That looks like it comes from http://code.google.com/p/django-docs/ which we aren't using yet.
Instead of commenting it out, we should just delete it, because we can always get it back from an older version.  :)

>+++ b/settings.py
>@@ -1,56 +1,43 @@
>+USE_I18N = False
>+# USE_I18N was set to false at the bottom of this original file, so I moved it here.
>+# NOTE: should probably fix this in a later bug.
>+# - ehenry5

It would be better to log a bug, and remove your name.  ;)
(I believe we're able to see who added lines of code with svn blame, or something similar.)

>+# used in urls.py, but only for true. (see line 31 in urls.py)

That tells me where it's used, but not really what it does.

>+# - ehenry5

Remove your name from here as well, please.

>+LOCAL_DEVELOPMENT = True

And now that I've looked up what it does, I think it probably belongs in local_settings.py.

Thanks,
Blake.
Please let this be the one that gets through!
Attachment #405191 - Flags: review?(bwinton)
Attachment #405191 - Flags: review?(bwinton)
Attachment #405191 - Attachment is obsolete: true
okay, this one looks good.
Attachment #405196 - Flags: review?(bwinton)
Comment on attachment 405196 [details] [diff] [review]
Splits settings.py into 2 files and uses os.path.abspath("./") (10.7.09)

>diff --git a/settings.py b/settings.py
>--- a/settings.py
>+++ b/settings.py
>+LOCAL_DEVELOPMENT = True

I still think that this belongs in local_settings.  :)

Can you convince me that it doesn't, or move it?

And while I was reviewing this patch, some other patches landed, so now I can't cleanly apply this patch.  Could you please update your copy of the source, and create a new patch that I can apply?

Thanks,
Blake.
Attachment #405196 - Flags: review?(bwinton) → review-
Attachment #406932 - Flags: review?(bwinton)
Comment on attachment 406932 [details] [diff] [review]
Splits settings.py into 2 files and uses os.path.abspath("./") (10.18.09)

You should be able to apply this patch to the most recent SVN (as of 10.18.09 15:38 EST)
This only works if you run manage.py from within the project folder. If you try to run it from its parent folder, 
i.e. python ispdb/manage.py test --settings=local_settings
I might be missing something, but I sounds like you were going to say something else...

Also, when I try python ispdb/manage.py runserver --settings=local_settings, I have no problem.  I do have a problem when I run ispdb/manage.py test --settings=local_settings, but I would assume that's a problem with the test...

(In reply to comment #11)
> This only works if you run manage.py from within the project folder. If you try
> to run it from its parent folder, 
> i.e. python ispdb/manage.py test --settings=local_settings
(In reply to comment #12)
> I might be missing something, but I sounds like you were going to say
> something else...

Haha yeah my bad. I get a template not found error for both the tests and runserver. Correct me if I'm wrong, I haven't looked too much into it, but os.path.abspath("./") gets the current working directory which if run from the parent directory it will return that. So when setting TEMPLATE_DIRS in local_settings.py it will set that to the wrong directory and hence the template not found error.
Okay, you are right about the template not found error with that path thing, but if you get that to go away there is another error.

try replacing the os.path.abspath("./") with os.path.dirname(__file__).  You'll see the other bug.  I might be wrong (since I don't know Django very well), but I'm gonna guess there's something not right with running it outside of the directory...

Blake?

(In reply to comment #13)
> (In reply to comment #12)
> > I might be missing something, but I sounds like you were going to say
> > something else...
> 
> Haha yeah my bad. I get a template not found error for both the tests and
> runserver. Correct me if I'm wrong, I haven't looked too much into it, but
> os.path.abspath("./") gets the current working directory which if run from the
> parent directory it will return that. So when setting TEMPLATE_DIRS in
> local_settings.py it will set that to the wrong directory and hence the
> template not found error.
Attachment #406932 - Attachment is obsolete: true
Attachment #406932 - Flags: review?(bwinton)
This one replaces the os.path.abspath("./") with os.path.dirname(__file__).
Attachment #402842 - Attachment is obsolete: true
Attachment #402908 - Attachment is obsolete: true
Attachment #405196 - Attachment is obsolete: true
Attachment #407180 - Flags: review?(bwinton)
Comment on attachment 407180 [details] [diff] [review]
Splits settings.py into 2 files and uses os.path.abspath("./") (10.19.09)

>+++ b/local_settings.py
>+ISPDB_ROOT = os.path.dirname(__file__)
>
>+++ b/settings.py
>+DATABASE_NAME = 'ispdb.sqlite'      # Or path to database file if using sqlite3.

The problem with running it outside of its directory for me is this line.
It leaves me with "ispdb.sqlite" files all over my computer.

I think it would be cool to add a line like:
DATABASE_NAME = os.path.join(ISPDB_ROOT, 'ispdb.sqlite')
to local_settings.py to prevent that.

r=me with that change.

Since this is a config-related change, I would recommend Gozer for your second review.

Thanks,
Blake.
Attachment #407180 - Flags: review?(bwinton) → review+
Attachment #407180 - Flags: review?(gozer)
Comment on attachment 407180 [details] [diff] [review]
Splits settings.py into 2 files and uses os.path.abspath("./") (10.19.09)

(In reply to comment #16)
> (From update of attachment 407180 [details] [diff] [review])
> >+++ b/local_settings.py
> >+ISPDB_ROOT = os.path.dirname(__file__)
> >
> >+++ b/settings.py
> >+DATABASE_NAME = 'ispdb.sqlite'      # Or path to database file if using sqlite3.
> 
> The problem with running it outside of its directory for me is this line.
> It leaves me with "ispdb.sqlite" files all over my computer.
> 
> I think it would be cool to add a line like:
> DATABASE_NAME = os.path.join(ISPDB_ROOT, 'ispdb.sqlite')
> to local_settings.py to prevent that.

Yes, definitely a better way to handle it. It's a good patch nontheless, so r=me with that little improvement as well
Attachment #407180 - Flags: review?(gozer) → review+
Attachment #407180 - Attachment is obsolete: true
Keywords: checkin-needed
missed one of the intermediate changes...
Attachment #409990 - Attachment is obsolete: true
Checked in as http://viewvc.svn.mozilla.org/vc?view=revision&revision=54978
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Component: ispdb → ISPDB Server
Product: Mozilla Messaging → Webtools
You need to log in before you can comment on or make changes to this bug.