[PATCH] Change the structure of the temporary directory used by morph

Lars Wirzenius lars.wirzenius at codethink.co.uk
Wed Jun 5 17:21:31 BST 2013


On Wed, Jun 05, 2013 at 04:13:11PM +0100, Tiago Gomes wrote:
>              if 'TMPDIR' in os.environ:
> -                self.settings['tempdir'] = os.environ['TMPDIR']
> +                tmpdir = os.path.join(os.environ['TMPDIR'], 'morph')
> +                self.settings['tempdir'] = tmpdir
>              else:
> -                self.settings['tempdir'] = '/tmp'
> +                tmpdir = os.path.join('/tmp', 'morph')
> +                self.settings['tempdir'] = tmpdir

The above seems a bit confused. Why set self.settings['tempdir'] twice? This
should be refactored to something like:

    tmpdir_base = os.environ.get('TMPDIR', '/tmp')
    tmpdir = os.path.join(tmpdir_base, 'morph')
    self.settings['tempdir'] = tmpdir

Except I don't think we need to set self.settings['tempdir'] at all, but if
we do, set it only once. Later: I see we use it in other parts of the code,
so it's needed to be set.

> --- a/morphlib/buildcommand.py
> +++ b/morphlib/buildcommand.py
> @@ -333,7 +333,8 @@ class BuildCommand(object):
>          '''Create the staging area for building a single artifact.'''
>  
>          self.app.status(msg='Creating staging area')
> -        staging_dir = tempfile.mkdtemp(dir=self.app.settings['tempdir'])
> +        staging_dir = os.path.join(self.app.settings['tempdir'], 'staging')
> +        staging_dir = tempfile.mkdtemp(dir=staging_dir)

I'd prefer to not have the staging_dir variable name reused, but that's
a very minor nitpick.

> @@ -158,6 +160,7 @@ class DeployPlugin(cliapp.Plugin):
>              # morphlib.app already took care of ensuring the tempdir setting
>              # is good, so use it if we don't have one already set.
>              env['TMPDIR'] = self.app.settings['tempdir']
> +            env['TMPDIR'] = os.path.join(env['TMPDIR'], 'deployments')

This should probably be refactored to not set env['TMPDIR'] twice,
to avoid reader confusion.

Other than these nitpicks, if "./check --full" passes, I'm ok with this to
be merged.

-- 
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/



More information about the baserock-dev mailing list