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

[Markus Lehtonen]

Hi,

On 01/04/15 21:04, "Guido Günther" <agx at sigxcpu.org> wrote:

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

RPM packages may use zip archive. I put the dependency here because the
new functionality touches the common parts of gbp. In practice though, the
zip format is only used by the rpm tools. Thus, the dependency could
probably be moved to git-buildpackage-rpm subpackage. Would you be ok with
this?



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

I can add this. And yes, probably the main reason for missing this is the
age of the original patch.



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

True. I'll fix this.



>>      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 ?

Hmm, that looks a bit cryptic. I think the original reason has been to
avoid guessing/determining the output filename of the compression
operation. Otherwise we'd need to determine the filename and then
move/rename to the final output filename (defined by the 'output'
parameter). At least a comment would be in order. What do you think?



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

Let's see if i can cook up some simple tests.


Thanks,
  Markus