Re: [PATCH 3/4] unpack.py: add SRC_URI parameter unpack=<bool> (default: true)

[Andreas Oberritter <obi@opendreambox.org>]

Hello Bernhard,

thanks for your review. When creating the patch I tried to use the least
invasive way, i.e. to change as few lines of code as possible, in order
not to break anything. Most of your remarks apply to already existing
code which was either copied or indented. I think, that your suggestions
should be incorporated in a later patch by someone more experienced in
python than me.

Please see my comments below.

On 01/11/2011 10:48 PM, Bernhard Reutner-Fischer wrote:
> On Tue, Jan 11, 2011 at 09:31:25PM +0100, Andreas Oberritter wrote:
>> Ping.
>>
>> On 01/06/2011 01:48 PM, Andreas Oberritter wrote:
>>> * This allows to download compressed files without extracting them
>>> * Use case: gcj requires ecj.jar, which must be downloaded separately
>>>   and put into the gcc source directory before configure gets executed.
>>>
>>> Signed-off-by: Andreas Oberritter <obi@opendreambox.org>
>>> CC: Chris Larson <chris_larson@mentor.com>
>>> ---
>>>  classes/base.bbclass                      |    3 +-
>>>  docs/usermanual/reference/var_src_uri.xml |   13 ++++-
>>>  lib/oe/unpack.py                          |   81 +++++++++++++++++------------
>>>  3 files changed, 61 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/classes/base.bbclass b/classes/base.bbclass
>>> index c76b77d..25d72d4 100644
>>> --- a/classes/base.bbclass
>>> +++ b/classes/base.bbclass
>>> @@ -190,12 +190,11 @@ def oe_unpack(d, local, urldata):
>>>          bb.mkdirhier(destdir)
>>>      else:
>>>          destdir = workdir
>>> -    dos = urldata.parm.get("dos")
>>>  I don't know whether this would 
>>>      bb.note("Unpacking %s to %s/" % (base_path_out(local, d),
>>>                                       base_path_out(destdir, d)))
>>>      try:
>>> -        unpack_file(local, destdir, env={"PATH": d.getVar("PATH", True)}, dos=dos)
>>> +        unpack_file(local, destdir, urldata.parm, env={"PATH": d.getVar("PATH", True)})
>>>      except UnpackError, exc:
>>>          bb.fatal(str(exc))
>>>  
>>> diff --git a/docs/usermanual/reference/var_src_uri.xml b/docs/usermanual/reference/var_src_uri.xml
>>> index 706452f..521c7d5 100644
>>> --- a/docs/usermanual/reference/var_src_uri.xml
>>> +++ b/docs/usermanual/reference/var_src_uri.xml
>>> @@ -91,7 +91,8 @@ SRC_URI[sha256sum] = &quot;36bdb85c97b39ac604bc58cb7857ee08295242c78a12848ef8a31
>>>    it is unpacked into the work directory, <command>${WORKDIR}</command>. The
>>>    unpacker recognises several archive and compression types and for these it
>>>    will decompress any compressed files and extract all of the files from
>>> -  archives into the work directory. The supported types are:</para>
>>> +  archives into the work directory, unless the option <command>unpack=no</command>
>>> +  is set for the given file. The supported types are:</para>
>>>  
>>>    <variablelist>
>>>      <varlistentry>
>>> @@ -192,6 +193,16 @@ SRC_URI[sha256sum] = &quot;36bdb85c97b39ac604bc58cb7857ee08295242c78a12848ef8a31
>>>            md5sum option provided.</para>
>>>          </listitem>
>>>        </varlistentry>
>>> +
>>> +      <varlistentry>
>>> +        <term>unpack={yes|no}</term>
>>> +
>>> +        <listitem>
>>> +          <para>If set to 'yes' (default) and the source file is an archive,
>>> +          then the archive will be decompressed and unpacked into the ${WORKDIR}.
>>> +	  Otherwise, the archive will be copied into the ${WORKDIR}.</para>
> 
> mixed tab and spaces for indentation.

OK, will fix that if this patch is going to be acked.

>>> +        </listitem>
>>> +      </varlistentry>
>>>      </variablelist>
>>>  
>>>      <para>Related variables:</para>
>>> diff --git a/lib/oe/unpack.py b/lib/oe/unpack.py
>>> index e4fe5d8..8e8bf36 100644
>>> --- a/lib/oe/unpack.py
>>> +++ b/lib/oe/unpack.py
>>> @@ -47,47 +47,62 @@ def subprocess_setup():
>>>      # non-Python subprocesses expect.
>>>      signal.signal(signal.SIGPIPE, signal.SIG_DFL)
>>>  
>>> -def unpack_file(file, destdir, dos=False, env=None):
>>> +def unpack_file(file, destdir, parameters, env=None):
>>>      import subprocess, shutil
>>>  
>>> +    try:
>>> +        dos = to_boolean(parameters.get("dos"), False)
> 
> dict().get defaults to None. (None,'',u'',False) is False, so passing in
> a default of False to to_boolean -- which will return False "if not
> string" (and should
> if not string or not isinstance(string, basestring):
> 	return False
> , as a sidenote)
> is superfluous.

I know, but I preferred the more readable way. I can change it if you like.

>>> +    except ValueError, exc:
> 
> I think the more modern for is to use 'as'

That was copy'n'paste from the already existing user of to_boolean().

>>> +        bb.fatal("Invalid value for 'dos' parameter for %s: %s" %
>>> +                 (filename, parameters.get("dos")))
> 
> I would have let to_boolean raise this through, but as you prefer.

Same as above.

>>> +
>>> +    try:
>>> +        unpack = to_boolean(parameters.get("unpack"), True)
>>> +    except ValueError, exc:
>>> +        bb.fatal("Invalid value for 'unpack' parameter for %s: %s" %
>>> +                 (filename, parameters.get("unpack")))
> 
> ditto.

Same as above.

>>> +
>>>      dest = os.path.join(destdir, os.path.basename(file))
>>>      if os.path.exists(dest):
>>>          if os.path.samefile(file, dest):
>>>              return True
>>>  
>>>      cmd = None
>>> -    if file.endswith('.tar'):
>>> -        cmd = 'tar x --no-same-owner -f %s' % file
>>> -    elif file.endswith('.tgz') or file.endswith('.tar.gz') or file.endswith('.tar.Z'):
>>> -        cmd = 'tar xz --no-same-owner -f %s' % file
>>> -    elif file.endswith('.tbz') or file.endswith('.tbz2') or file.endswith('.tar.bz2'):
>>> -        cmd = 'bzip2 -dc %s | tar x --no-same-owner -f -' % file
>>> -    elif file.endswith('.gz') or file.endswith('.Z') or file.endswith('.z'):
>>> -        root, ext = os.path.splitext(file)
>>> -        cmd = 'gzip -dc %s > %s' % (file, os.path.basename(root))
>>> -    elif file.endswith('.bz2'):
>>> -        root, ext = os.path.splitext(file)
>>> -        cmd = 'bzip2 -dc %s > %s' % (file, os.path.basename(root))
>>> -    elif file.endswith('.tar.xz'):
>>> -        cmd = 'xz -dc %s | tar x --no-same-owner -f -' % file
>>> -    elif file.endswith('.xz'):
>>> -        root, ext = os.path.splitext(file)
>>> -        cmd = 'xz -dc %s > %s' % (file, os.path.basename(root))
>>> -    elif file.endswith('.tar.lz'):
>>> -        cmd = 'lzip -dc %s | tar x --no-same-owner -f -' % file
>>> -    elif file.endswith('.lz'):
>>> -        root, ext = os.path.splitext(file)
>>> -        cmd = 'lzip -dc %s > %s' % (file, os.path.basename(root))
>>> -    elif file.endswith('.zip') or file.endswith('.jar'):
>>> -        cmd = 'unzip -q -o'
>>> -        if dos:
>>> -            cmd = '%s -a' % cmd
>>> -        cmd = "%s '%s'" % (cmd, file)
>>> -    elif os.path.isdir(file):
>>> -        shutil.rmtree(dest, True)
>>> -        shutil.copytree(file, dest, True)
>>> -    else:
>>> -        shutil.copy2(file, dest)
>>> +    if unpack:
>>> +        if file.endswith('.tar'):
>>> +            cmd = 'tar x --no-same-owner -f %s' % file
>>> +        elif file.endswith('.tgz') or file.endswith('.tar.gz') or file.endswith('.tar.Z'):
>>> +            cmd = 'tar xz --no-same-owner -f %s' % file
>>> +        elif file.endswith('.tbz') or file.endswith('.tbz2') or file.endswith('.tar.bz2'):
>>> +            cmd = 'bzip2 -dc %s | tar x --no-same-owner -f -' % file
>>> +        elif file.endswith('.gz') or file.endswith('.Z') or file.endswith('.z'):
>>> +            root, ext = os.path.splitext(file)
>>> +            cmd = 'gzip -dc %s > %s' % (file, os.path.basename(root))
>>> +        elif file.endswith('.bz2'):
>>> +            root, ext = os.path.splitext(file)
>>> +            cmd = 'bzip2 -dc %s > %s' % (file, os.path.basename(root))
>>> +        elif file.endswith('.tar.xz'):
>>> +            cmd = 'xz -dc %s | tar x --no-same-owner -f -' % file
>>> +        elif file.endswith('.xz'):
>>> +            root, ext = os.path.splitext(file)
>>> +            cmd = 'xz -dc %s > %s' % (file, os.path.basename(root))
>>> +        elif file.endswith('.tar.lz'):
>>> +            cmd = 'lzip -dc %s | tar x --no-same-owner -f -' % file
>>> +        elif file.endswith('.lz'):
>>> +            root, ext = os.path.splitext(file)
>>> +            cmd = 'lzip -dc %s > %s' % (file, os.path.basename(root))
>>> +        elif file.endswith('.zip') or file.endswith('.jar'):
>>> +            cmd = 'unzip -q -o'
>>> +            if dos:
>>> +                cmd = '%s -a' % cmd
>>> +            cmd = "%s '%s'" % (cmd, file)
>>> +
>>> +    if not unpack or not cmd:
>>> +        if os.path.isdir(file):
>>> +            shutil.rmtree(dest, True)
>>> +            shutil.copytree(file, dest, True)
>>> +        else:
>>> +            shutil.copy2(file, dest)
> 
> Don't know offhand but
> - bb.utils.remove(dest, True)
> or equivalent facility (or wrapper) in lib/oe?

That's just changed indentation of already existing code.

For your reference, here's the patch for unpack.py without changes to
whitespace:

> --- lib/oe/unpack.py.orig	2011-01-11 22:11:51.000000000 +0000
> +++ lib/oe/unpack.py	2011-01-11 22:12:23.000000000 +0000
> @@ -47,15 +47,28 @@
>      # non-Python subprocesses expect.
>      signal.signal(signal.SIGPIPE, signal.SIG_DFL)
>  
> -def unpack_file(file, destdir, dos=False, env=None):
> +def unpack_file(file, destdir, parameters, env=None):
>      import subprocess, shutil
>  
> +    try:
> +        dos = to_boolean(parameters.get("dos"), False)
> +    except ValueError, exc:
> +        bb.fatal("Invalid value for 'dos' parameter for %s: %s" %
> +                 (filename, parameters.get("dos")))
> +
> +    try:
> +        unpack = to_boolean(parameters.get("unpack"), True)
> +    except ValueError, exc:
> +        bb.fatal("Invalid value for 'unpack' parameter for %s: %s" %
> +                 (filename, parameters.get("unpack")))
> +
>      dest = os.path.join(destdir, os.path.basename(file))
>      if os.path.exists(dest):
>          if os.path.samefile(file, dest):
>              return True
>  
>      cmd = None
> +    if unpack:
>      if file.endswith('.tar'):
>          cmd = 'tar x --no-same-owner -f %s' % file
>      elif file.endswith('.tgz') or file.endswith('.tar.gz') or file.endswith('.tar.Z'):
> @@ -83,7 +96,9 @@
>          if dos:
>              cmd = '%s -a' % cmd
>          cmd = "%s '%s'" % (cmd, file)
> -    elif os.path.isdir(file):
> +
> +    if not unpack or not cmd:
> +        if os.path.isdir(file):
>          shutil.rmtree(dest, True)
>          shutil.copytree(file, dest, True)
>      else:


Regards,
Andreas