Missing basedir/apps in sys.path?

On Thu, 2011-02-03 at 12:04 +0100, Dirk Wallenstein wrote:
[...]
> > 
> > --- a/lib/apache2/patchwork.wsgi
> > +++ b/lib/apache2/patchwork.wsgi
> > @@ -11,6 +11,7 @@ import sys
> >  
> >  basedir = os.path.dirname(__file__)
> >  sys.path.append(basedir)
> > +sys.path.append(os.path.join(basedir, 'apps'))
> >  
> >  os.environ['DJANGO_SETTINGS_MODULE'] = 'apps.settings'
> >  import django.core.handlers.wsgi
> 
> Looks good.
> However, I find it a bit confusing to construct a path that relies on
> the fact that this module is found through a symlink.  I would hardcode
> '/srv/patchwork' and '/srv/patchwork/apps' like in the mod_python

That means one more place people will have to remember to change when
deploying/moving an instance, so I don't think it's a good idea.

> config.  Or would it be possible to get rid of the symlink
> '/srv/patchwork/patchwork.wsgi' when changing it in
> apache2/patchwork.wsgi.conf and then optionally construct the paths
> relative from the real location?

Yes, it's possible to use the real path to patchwork.wsgi in the apache
config and then have patchwork.wsgi construct the path from its real
location, but using the symlink may be a good idea because it provides
some flexibility in case there's ever a need to move patchwork.wsgi to a

On Thu, Feb 03, 2011 at 11:47:56AM -0200, Guilherme Salgado wrote:
> On Thu, 2011-02-03 at 12:04 +0100, Dirk Wallenstein wrote:
> [...]
> > > 
> > > --- a/lib/apache2/patchwork.wsgi
> > > +++ b/lib/apache2/patchwork.wsgi
> > > @@ -11,6 +11,7 @@ import sys
> > >  
> > >  basedir = os.path.dirname(__file__)
> > >  sys.path.append(basedir)
> > > +sys.path.append(os.path.join(basedir, 'apps'))
> > >  
> > >  os.environ['DJANGO_SETTINGS_MODULE'] = 'apps.settings'
> > >  import django.core.handlers.wsgi
> > 
> > Looks good.
> > However, I find it a bit confusing to construct a path that relies on
> > the fact that this module is found through a symlink.  I would hardcode
> > '/srv/patchwork' and '/srv/patchwork/apps' like in the mod_python
> 
> That means one more place people will have to remember to change when
> deploying/moving an instance, so I don't think it's a good idea.
> 
> > config.  Or would it be possible to get rid of the symlink
> > '/srv/patchwork/patchwork.wsgi' when changing it in
> > apache2/patchwork.wsgi.conf and then optionally construct the paths
> > relative from the real location?
> 
> Yes, it's possible to use the real path to patchwork.wsgi in the apache
> config and then have patchwork.wsgi construct the path from its real
> location, but using the symlink may be a good idea because it provides
> some flexibility in case there's ever a need to move patchwork.wsgi to a
> different place.  That flexibility may never be used, though.

Currently, only one can be used unless os.path.realpath is added into
the mix.  I would favor to remove duplication here -- if it was broken
anyway, that is.

> This is what the diff looks like:
> 
> diff --git a/lib/apache2/patchwork.wsgi b/lib/apache2/patchwork.wsgi
> index 0488b48..869bb9d 100644
> --- a/lib/apache2/patchwork.wsgi
> +++ b/lib/apache2/patchwork.wsgi
> @@ -9,8 +9,10 @@
>  import os
>  import sys
>  
> -basedir = os.path.dirname(__file__)
> +basedir = os.path.join(
> +    os.path.dirname(__file__), os.path.pardir, os.path.pardir)
>  sys.path.append(basedir)
> +sys.path.append(os.path.join(basedir, 'apps'))
>  
>  os.environ['DJANGO_SETTINGS_MODULE'] = 'apps.settings'
>  import django.core.handlers.wsgi
> diff --git a/lib/apache2/patchwork.wsgi.conf b/lib/apache2/patchwork.wsgi.conf
> index e99f8c6..3756e5a 100644
> --- a/lib/apache2/patchwork.wsgi.conf
> +++ b/lib/apache2/patchwork.wsgi.conf
> @@ -16,5 +16,5 @@
>          </Directory>
>  </IfModule>
>  
> -WSGIScriptAlias / "/srv/patchwork/patchwork.wsgi"
> +WSGIScriptAlias / "/srv/patchwork/lib/apache2/patchwork.wsgi"
>  WSGIPassAuthorization On

Wonderful.

On Thu, 2011-02-03 at 15:34 +0100, Dirk Wallenstein wrote:
> On Thu, Feb 03, 2011 at 11:47:56AM -0200, Guilherme Salgado wrote:
> > On Thu, 2011-02-03 at 12:04 +0100, Dirk Wallenstein wrote:
> > [...]
> > > > 
> > > > --- a/lib/apache2/patchwork.wsgi
> > > > +++ b/lib/apache2/patchwork.wsgi
> > > > @@ -11,6 +11,7 @@ import sys
> > > >  
> > > >  basedir = os.path.dirname(__file__)
> > > >  sys.path.append(basedir)
> > > > +sys.path.append(os.path.join(basedir, 'apps'))
> > > >  
> > > >  os.environ['DJANGO_SETTINGS_MODULE'] = 'apps.settings'
> > > >  import django.core.handlers.wsgi
> > > 
> > > Looks good.
> > > However, I find it a bit confusing to construct a path that relies on
> > > the fact that this module is found through a symlink.  I would hardcode
> > > '/srv/patchwork' and '/srv/patchwork/apps' like in the mod_python
> > 
> > That means one more place people will have to remember to change when
> > deploying/moving an instance, so I don't think it's a good idea.
> > 
> > > config.  Or would it be possible to get rid of the symlink
> > > '/srv/patchwork/patchwork.wsgi' when changing it in
> > > apache2/patchwork.wsgi.conf and then optionally construct the paths
> > > relative from the real location?
> > 
> > Yes, it's possible to use the real path to patchwork.wsgi in the apache
> > config and then have patchwork.wsgi construct the path from its real
> > location, but using the symlink may be a good idea because it provides
> > some flexibility in case there's ever a need to move patchwork.wsgi to a
> > different place.  That flexibility may never be used, though.
> 
> Currently, only one can be used unless os.path.realpath is added into
> the mix.  I would favor to remove duplication here -- if it was broken
> anyway, that is.

Oh, sure, but I thought the idea was to use only the symlink at the
root, as that's what the example config uses. Also, you can only have
the flexibility I was talking about if you use the symlink, but then one
could also create the symlink whenever that flexibility is needed.

> 
> Wonderful.

Cool, is this a +1? :)

I guess what I'm really asking is if there's anything else I should be
doing to push this through review and then have it committed?

Cheers,

On Fri, Feb 04, 2011 at 02:55:32PM -0200, Guilherme Salgado wrote:

[...]
 
> I guess what I'm really asking is if there's anything else I should be
> doing to push this through review and then have it committed?

Basically, wait for Jeremy and what he says.  Generally, you could
familiarize yourself with git-format-patch and git-send-email.  It will
help with every project that uses Git.

http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches
http://www.kernel.org/doc/Documentation/SubmittingPatches