[git-buildpackage] [PATCH 04/11] Rewrite gbp.tmpfile

[Guido Günther]

On Wed, Feb 04, 2015 at 02:09:39PM +0200, Markus Lehtonen wrote:
> This commit changes the tmpfile module to only contain minimal
> init_tmpdir() and del_tmpdir() functions, intended to be called at the
> beginning and end of the scipts. The init function creates a base
> tempdir and configures the Python tempfile module to use that as the
> (default) directory for temporary files and directories.
> Correspondingly, the del function removes the base tempdir and restores
> the configuration of the tempfile module. The new logic ensures that all
> temporary files/directories will be created in the desired base tempdir
> (assuming that no 'dir' argument is given to the tempfile functions).

Good idea!

> 
> The base tempdir is not removed by del_tmpdir() if a non-empty value of
> GBP_TMPFILE_NOCLEAN is found in environment. This can be useful e.g. in
> debugging.
> 
> Signed-off-by: Markus Lehtonen <markus.lehtonen at linux.intel.com>
> ---
>  gbp/scripts/import_srpm.py | 22 +++++++++-------------
>  gbp/scripts/pq_rpm.py      | 19 ++++++++-----------
>  gbp/tmpfile.py             | 37 ++++++++++++++++++++++++++-----------

If do it this way pq.py and import_dsc need to get this too.

>  3 files changed, 43 insertions(+), 35 deletions(-)
> 
> diff --git a/gbp/scripts/import_srpm.py b/gbp/scripts/import_srpm.py
> index 587e2f2..cc2201c 100755
> --- a/gbp/scripts/import_srpm.py
> +++ b/gbp/scripts/import_srpm.py
> @@ -27,8 +27,8 @@ import shutil
>  import errno
>  import urllib2
>  
> -import gbp.tmpfile as tempfile
>  import gbp.command_wrappers as gbpc
> +from gbp.tmpfile import init_tmpdir, del_tmpdir, tempfile
>  from gbp.rpm import (parse_srpm, guess_spec, SpecFile, NoSpecError,
>                       RpmUpstreamSource, compose_version_str)
>  from gbp.rpm.git import (RpmGitRepository, GitRepositoryError)
> @@ -63,14 +63,14 @@ def download_file(target_dir, url):
>          raise GbpError("Download failed: %s" % err.reason)
>      return local_fn
>  
> -def download_source(pkg, dirs):
> +def download_source(pkg):
>      """Download package from a remote location"""
>      if re.match(r'[a-z]{1,5}://', pkg):
>          mode = 'python urllib2'
>      else:
>          mode = 'yumdownloader'
>  
> -    tmpdir = tempfile.mkdtemp(dir=dirs['tmp_base'], prefix='download_')
> +    tmpdir = tempfile.mkdtemp(prefix='download_')
>      gbp.log.info("Trying to download '%s' using '%s'..." % (pkg, mode))
>      if mode == 'yumdownloader':
>          gbpc.RunAtCommand('yumdownloader',
> @@ -209,23 +209,21 @@ def main(argv):
>          gbp.log.err("Need to give exactly one package to import. Try --help.")
>          return 1
>      try:
> -        dirs['tmp_base'] = tempfile.mkdtemp(dir=options.tmp_dir,
> -                                            prefix='import-srpm')
> +        dirs['tmp_base'] = init_tmpdir(options.tmp_dir, 'import-srpm_')
>      except GbpError as err:
>          gbp.log.err(err)
>          return 1
>      try:
>          srpm = args[0]
>          if options.download:
> -            srpm = download_source(srpm, dirs)
> +            srpm = download_source(srpm)
>  
>          # Real srpm, we need to unpack, first
>          true_srcrpm = False
>          if not os.path.isdir(srpm) and not srpm.endswith(".spec"):
>              src = parse_srpm(srpm)
>              true_srcrpm = True
> -            dirs['pkgextract'] = tempfile.mkdtemp(dir=dirs['tmp_base'],
> -                                                  prefix='pkgextract_')
> +            dirs['pkgextract'] = tempfile.mkdtemp(prefix='pkgextract_')
>              gbp.log.info("Extracting src rpm to '%s'" % dirs['pkgextract'])
>              src.unpack(dirs['pkgextract'])
>              preferred_spec = src.name + '.spec'
> @@ -267,10 +265,8 @@ def main(argv):
>              set_bare_repo_options(options)
>  
>          # Create more tempdirs
> -        dirs['origsrc'] = tempfile.mkdtemp(dir=dirs['tmp_base'],
> -                                           prefix='origsrc_')
> -        dirs['packaging_base'] = tempfile.mkdtemp(dir=dirs['tmp_base'],
> -                                                  prefix='packaging_')
> +        dirs['origsrc'] = tempfile.mkdtemp(prefix='origsrc_')
> +        dirs['packaging_base'] = tempfile.mkdtemp(prefix='packaging_')
>          dirs['packaging'] = os.path.join(dirs['packaging_base'],
>                                           options.packaging_dir)
>          try:
> @@ -458,7 +454,7 @@ def main(argv):
>          skipped = True
>      finally:
>          os.chdir(dirs['top'])
> -        gbpc.RemoveTree(dirs['tmp_base'])()
> +        del_tmpdir()
>  
>      if not ret and not skipped:
>          gbp.log.info("Version '%s' imported under '%s'" % (ver_str, spec.name))
> diff --git a/gbp/scripts/pq_rpm.py b/gbp/scripts/pq_rpm.py
> index a05fdff..c4f9d4d 100755
> --- a/gbp/scripts/pq_rpm.py
> +++ b/gbp/scripts/pq_rpm.py
> @@ -28,7 +28,7 @@ import shutil
>  import sys
>  
>  import gbp.log
> -import gbp.tmpfile as tempfile
> +from gbp.tmpfile import init_tmpdir, del_tmpdir, tempfile
>  from gbp.config import GbpOptionParserRpm
>  from gbp.rpm.git import GitRepositoryError, RpmGitRepository
>  from gbp.git.modifier import GitModifier
> @@ -209,18 +209,16 @@ def export_patches(repo, options):
>      GitCommand('status')(['--', spec.specdir])
>  
>  
> -def safe_patches(queue, tmpdir_base):
> +def safe_patches(queue):
>      """
>      Safe the current patches in a temporary directory
> -    below 'tmpdir_base'. Also, uncompress compressed patches here.
>  
>      @param queue: an existing patch queue
> -    @param tmpdir_base: base under which to create tmpdir
> -    @return: tmpdir and a safed queue (with patches in tmpdir)
> +    @return: safed queue (with patches in tmpdir)
>      @rtype: tuple
>      """
>  
> -    tmpdir = tempfile.mkdtemp(dir=tmpdir_base, prefix='patchimport_')
> +    tmpdir = tempfile.mkdtemp(prefix='patchimport_')
>      safequeue = PatchSeries()
>  
>      if len(queue) > 0:
> @@ -302,13 +300,13 @@ def import_spec_patches(repo, options):
>  
>      # Put patches in a safe place
>      if spec_treeish:
> -        packaging_tmp = tempfile.mkdtemp(prefix='dump_', dir=options.tmp_dir)
> +        packaging_tmp = tempfile.mkdtemp(prefix='dump_')
>          packaging_tree = '%s:%s' % (spec_treeish, options.packaging_dir)
>          dump_tree(repo, packaging_tmp, packaging_tree, with_submodules=False,
>                    recursive=False)
>          spec.specdir = packaging_tmp
>      in_queue = spec.patchseries()
> -    queue = safe_patches(in_queue, options.tmp_dir)
> +    queue = safe_patches(in_queue)
>      # Do import
>      try:
>          gbp.log.info("Switching to branch '%s'" % pq_branch)
> @@ -431,8 +429,7 @@ def main(argv):
>  
>      try:
>          # Create base temporary directory for this run
> -        options.tmp_dir = tempfile.mkdtemp(dir=options.tmp_dir,
> -                                           prefix='gbp-pq-rpm_')
> +        init_tmpdir(options.tmp_dir, prefix='pq-rpm_')
>          current = repo.get_branch()
>          if action == "export":
>              export_patches(repo, options)
> @@ -457,7 +454,7 @@ def main(argv):
>              gbp.log.err(err)
>          retval = 1
>      finally:
> -        shutil.rmtree(options.tmp_dir, ignore_errors=True)
> +        del_tmpdir()
>  
>      return retval
>  
> diff --git a/gbp/tmpfile.py b/gbp/tmpfile.py
> index e1ad308..b90527f 100644
> --- a/gbp/tmpfile.py
> +++ b/gbp/tmpfile.py
> @@ -1,6 +1,6 @@
>  # vim: set fileencoding=utf-8 :
>  #
> -# (C) 2012 Intel Corporation <markus.lehtonen at linux.intel.com>
> +# (C) 2012, 2015 Intel Corporation <markus.lehtonen at linux.intel.com>
>  #    This program is free software; you can redistribute it and/or modify
>  #    it under the terms of the GNU General Public License as published by
>  #    the Free Software Foundation; either version 2 of the License, or
> @@ -17,22 +17,37 @@
>  """Temporary directory handling"""
>  
>  import os
> +import shutil
>  import tempfile
>  
>  from gbp.errors import GbpError
>  
> -def mkdtemp(dir, **kwargs):
> -    """Create temporary directory"""
> -    try:
> -        if not os.path.exists(dir):
> -            os.makedirs(dir)
> -    except OSError as (dummy_e, msg):
> -        raise GbpError, "Unable to create dir %s (%s)" % (dir, msg)
>  
> +_old_tempdirs = []
> +
> +def init_tmpdir(path, prefix):
> +    """Initialize a temporary directory structure"""
>      try:
> -        return os.path.abspath(tempfile.mkdtemp(dir=dir, **kwargs))
> -    except OSError as (dummy_e, msg):
> -        raise GbpError, "Unable to create tmpdir under %s (%s)" % (dir, msg)
> +        if not os.path.exists(path):
> +            os.makedirs(path)
> +    except OSError as err:
> +        raise GbpError("Unable to create tmpdir %s (%s)" % (path, err))
> +
> +    tmpdir = tempfile.mkdtemp(dir=path, prefix=prefix)
> +
> +    # Set newly created dir as the default value for all further tempfile
> +    # calls
> +    _old_tempdirs.append(tempfile.tempdir)
> +    tempfile.tempdir = tmpdir
> +    return tmpdir
> +
> +def del_tmpdir():
> +    """Remove tempdir and restore tempfile module"""
> +    if _old_tempdirs:
> +        if os.path.exists(tempfile.tempdir) and \
> +                not os.getenv('GBP_TMPFILE_NOCLEAN'):
> +            shutil.rmtree(tempfile.tempdir)
> +        tempfile.tempdir = _old_tempdirs.pop()

This doesn't fit well with different callers calling del_tempdir in
different order than init_tempdir. Do we really want to support more
than one invocation of init_tmpdir ?

Cheers,
 -- Guido