518840 - settings.py references absolute paths

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect)

Description

[Eric Henry]

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

Comment 1

[Eric Henry]

Attachment: Splits settings.py into 2 files and uses os.path.abspath("./")

Comment 2

[Blake Winton (: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.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 4

[Eric Henry]

Attachment: Splits settings.py into 2 files and uses os.path.abspath("./") (new attempt)

Comment 5

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 6

[Eric Henry]

Attachment: Splits settings.py into 2 files and uses os.path.abspath("./") (10.7.09)

Please let this be the one that gets through!

Comment 7

[Eric Henry]

Attachment: Splits settings.py into 2 files and uses os.path.abspath("./") (10.7.09)

okay, this one looks good.

Comment 8

[Blake Winton (: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.

Comment 9

[Eric Henry]

Attachment: Splits settings.py into 2 files and uses os.path.abspath("./") (10.18.09)

Comment 10

[Eric Henry]

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)

Comment 11

[Jay Schmidek]

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

Comment 12

[Eric Henry]

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

Comment 13

[Jay Schmidek]

(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.

Comment 14

[Eric Henry]

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.

Comment 15

[Eric Henry]

Attachment: Splits settings.py into 2 files and uses os.path.abspath("./") (10.19.09)

This one replaces the os.path.abspath("./") with os.path.dirname(__file__).

Comment 16

[Blake Winton (: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.

Comment 17

[Philippe M. Chiasson (: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

Comment 18

[Eric Henry]

Attachment: Splits settings.py into 2 files and uses os.path.abspath("./") (11.3.09)

Comment 19

[Eric Henry]

Attachment: Splits settings.py into 2 files and uses os.path.abspath("./") (11.3.09 b)

missed one of the intermediate changes...

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Checked in as http://viewvc.svn.mozilla.org/vc?view=revision&revision=54978