[git-buildpackage] [PATCH 01/11] common/buildpackage: support for different archive formats

[Guido Günther]

I'll just use git-format-patch to format your series into patches and
add the comments. Won't get far today but lets start.

On Tue, May 15, 2012 at 04:37:33PM +0300, Markus Lehtonen wrote:
> Adds support for defining the archive format of the output of
> git_archive_single(), e.g. 'zip'. Defaults to 'tar', as before.
> 
> Signed-off-by: Ed Bartosh <eduard.bartosh at intel.com>
> Signed-off-by: Markus Lehtonen <markus.lehtonen at linux.intel.com>
> ---
>  debian/control                     |  2 +-
>  gbp/command_wrappers.py            | 10 ++++++++
>  gbp/scripts/common/buildpackage.py | 48 ++++++++++++++++++++++----------------
>  3 files changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/debian/control b/debian/control
> index e679bdf..985b7d8 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -47,7 +47,7 @@ Depends: ${python:Depends},
>   python-pkg-resources,
>   python-six,
>  Recommends: pristine-tar (>= 0.5), cowbuilder, python-requests
> -Suggests: python-notify, unzip
> +Suggests: python-notify, unzip, zipmerge

Do we need this? Debian does not support zip as upstream tarball
format. It might make sense on the rpm tools though.

>  Description: Suite to help with Debian packages in Git repositories
>   This package contains the following tools:
>    * gbp import-{dsc,dscs}: import existing Debian source packages into a git
> diff --git a/gbp/command_wrappers.py b/gbp/command_wrappers.py
> index 3e834c6..0f6fc88 100644
> --- a/gbp/command_wrappers.py
> +++ b/gbp/command_wrappers.py
> @@ -283,6 +283,16 @@ class UnpackZipArchive(Command):
>          self.run_error = 'Couldn\'t unpack "%s": {err_reason}' % self.archive
>  
>  
> +class CatenateZipArchive(Command):
> +    """Wrap zipmerge tool to catenate a zip file with the next"""
> +    def __init__(self, archive, **kwargs):
> +        self.archive = archive
> +        Command.__init__(self, 'zipmerge', [archive], **kwargs)
> +
> +    def __call__(self, target):
> +        Command.__call__(self, [target])
> +
> +

Can we improve here by setting a proper run_error ? Maybe capturing
stderr? (I admit that this feature wasn't there when you wrote the code).

>  class GitCommand(Command):
>      "Mother/Father of all git commands"
>      def __init__(self, cmd, args=[], **kwargs):
> diff --git a/gbp/scripts/common/buildpackage.py b/gbp/scripts/common/buildpackage.py
> index 2e53b78..f95147e 100644
> --- a/gbp/scripts/common/buildpackage.py
> +++ b/gbp/scripts/common/buildpackage.py
> @@ -22,7 +22,7 @@ import os, os.path
>  import pipes
>  import tempfile
>  import shutil
> -from gbp.command_wrappers import (CatenateTarArchive)
> +from gbp.command_wrappers import (CatenateTarArchive, CatenateZipArchive)
>  from gbp.errors import GbpError
>  import gbp.log
>  
> @@ -50,51 +50,59 @@ def sanitize_prefix(prefix):
>      return '/'
>  
>  
> -def git_archive_submodules(repo, treeish, output, prefix, comp_type, comp_level, comp_opts):
> +def git_archive_submodules(repo, treeish, output, prefix, comp_type, comp_level,
> +                           comp_opts, format='tar'):
>      """
> -    Create tar.gz of an archive with submodules
> +    Create a source tree archive with submodules.
>  
> -    since git-archive always writes an end of tarfile trailer we concatenate
> +    Since git-archive always writes an end of tarfile trailer we concatenate
>      the generated archives using tar and compress the result.

end of tarfile and tar doesn't make sense here anymore.

>  
>      Exception handling is left to the caller.
>      """
>      prefix = sanitize_prefix(prefix)
> -    tarfile = output.rsplit('.', 1)[0]
>      tempdir = tempfile.mkdtemp()
> -    submodule_tarfile = os.path.join(tempdir, "submodule.tar")
> +    main_archive = os.path.join(tempdir, "main.%s" % format)
> +    submodule_archive = os.path.join(tempdir, "submodule.%s" % format)
>      try:
> -        # generate main tarfile
> -        repo.archive(format='tar', prefix=prefix,
> -                     output=tarfile, treeish=treeish)
> +        # generate main (tmp) archive
> +        repo.archive(format=format, prefix=prefix,
> +                     output=main_archive, treeish=treeish)
>  
> -        # generate each submodule's tarfile and append it to the main archive
> +        # generate each submodule's arhive and append it to the main archive
>          for (subdir, commit) in repo.get_submodules(treeish):
>              tarpath = [subdir, subdir[2:]][subdir.startswith("./")]
>  
>              gbp.log.debug("Processing submodule %s (%s)" % (subdir, commit[0:8]))
> -            repo.archive(format='tar', prefix='%s%s/' % (prefix, tarpath),
> -                         output=submodule_tarfile, treeish=commit, cwd=subdir)
> -            CatenateTarArchive(tarfile)(submodule_tarfile)
> +            repo.archive(format=format, prefix='%s%s/' % (prefix, tarpath),
> +                         output=submodule_archive, treeish=commit, cwd=subdir)
> +            if format == 'tar':
> +                CatenateTarArchive(main_archive)(submodule_archive)
> +            elif format == 'zip':
> +                CatenateZipArchive(main_archive)(submodule_archive)
>  
>          # compress the output
> -        ret = os.system("%s -%s %s %s" % (comp_type, comp_level, comp_opts, tarfile))
> -        if ret:
> -            raise GbpError("Error creating %s: %d" % (output, ret))
> +        if comp_type:
> +            ret = os.system("%s --stdout -%s %s %s > %s" % (comp_type, comp_level, comp_opts, main_archive, output))

Why would we need to go through stdout here ?

> +            if ret:
> +                raise GbpError("Error creating %s: %d" % (output, ret))
> +        else:
> +            shutil.move(main_archive, output)
>      finally:
>          shutil.rmtree(tempdir)
>  
>  
> -def git_archive_single(treeish, output, prefix, comp_type, comp_level, comp_opts):
> +def git_archive_single(treeish, output, prefix, comp_type, comp_level, comp_opts, format='tar'):
>      """
> -    Create tar.gz of an archive without submodules
> +    Create an archive without submodules
>  
>      Exception handling is left to the caller.
>      """
>      prefix = sanitize_prefix(prefix)
>      pipe = pipes.Template()
> -    pipe.prepend("git archive --format=tar --prefix=%s %s" % (prefix, treeish), '.-')
> -    pipe.append('%s -c -%s %s' % (comp_type, comp_level, comp_opts),  '--')
> +    pipe.prepend("git archive --format=%s --prefix=%s %s" % (format, prefix, treeish), '.-')
> +    if comp_type:
> +        pipe.append('%s -c -%s %s' % (comp_type, comp_level, comp_opts),  '--')
>      ret = pipe.copy('', output)
>      if ret:
>          raise GbpError("Error creating %s: %d" % (output, ret))
> -- 

Some minor comments above. Overall the right thing. Having some tests
would be great though, now that we add on complexity.
Cheers,
 -- Guido