[elbe-devel] [PATCH 3/3] Unpack the archives depending on volume attribute
Torben Hohn
torben.hohn at linutronix.de
Thu Sep 5 11:58:48 CEST 2019
On Wed, Sep 04, 2019 at 04:03:20PM +0200, Akash Satamkar wrote:
> The volume attribute can be used to specify the volume number
> in which the archive is added.
>
> When volume attribute is not specified, the archive is added
> to each volume.
first of all, i want the changes to the preprocessor and the unpacking
of the archives split into separate patches.
the commit log does not reflect, that both changes are done here.
the volume attribute being optional calls for trouble.
but it seems to be possible to select 'archive[count(@volume)=0]'
and find the the nodes for all.
but i think the code would be easier to read, when 'volume="all"'
was used.
please note that the userfacing <archivedir> tag needs to support
the absence of volume, but it could just replace it with 'volume="all"'
>
> Signed-off-by: Akash Satamkar <akash at linutronix.de>
> ---
> elbepack/archivedir.py | 46 ++++++++++++++++++++++++++++-------------
> elbepack/cdroms.py | 13 ++++++++++++
> elbepack/repomanager.py | 7 +++++--
> 3 files changed, 50 insertions(+), 16 deletions(-)
>
> diff --git a/elbepack/archivedir.py b/elbepack/archivedir.py
> index 10155228..c6fc3a47 100644
> --- a/elbepack/archivedir.py
> +++ b/elbepack/archivedir.py
> @@ -6,6 +6,7 @@
> import os
> import re
> import sys
> +import glob
>
> # The urlparse module is renamed to urllib.parse in Python 3.
> try:
> @@ -17,7 +18,7 @@ from base64 import standard_b64encode
> from bz2 import compress as bz2compress
> from subprocess import CalledProcessError
>
> -from elbepack.treeutils import etree
> +from elbepack.treeutils import etree, elem
> from elbepack.shellhelper import system
>
> class ArchivedirError(Exception):
> @@ -90,32 +91,49 @@ def get_and_append_method(url):
> 'file': get_and_append_local,
> }.get(urlparse(url).scheme, get_and_append_unknown)
>
> -def _combinearchivedir(xml):
> +def _combinearchivedir(xml, xpath):
> elbexml = etree(None)
> elbexml.et = xml
> -
> archive = '.combinedarchive.tar'
> - for archivedir in xml.iterfind("archivedir"):
> + for archivedir in xml.iterfind(xpath):
> try:
> archiveurl = urljoin(archivedir.base, archivedir.text)
> keep = elbexml.check_boolean(archivedir, "keep-attributes")
> + parent = archivedir.getparent()
> + volume_attr = archivedir.get('volume')
this could default to "all" (get allows to specify a default.
> + if volume_attr:
> + arch = parent.get("archive[@volume='%s']" % volume_attr)
> + if not arch:
> + arch = elem(parent).append("archive")
> + arch.et.set("volume", volume_attr)
> + else:
> + arch = elem(parent).ensure_child("archive")
> + if arch.et.get('volume'):
> + arch = elem(parent).append("archive")
this is overly complicated.
if volume was mandatory and "all" for all cdroms, the first block
of the if statement would cover all cases. and the confusing usage of
ensure_child could be dropped.
however, care must be taken for the old archive code, which does not
accept the volume attribute.
But a bool parameter 'use_volume' could take care of that.
> +
> get_and_append = get_and_append_method(archiveurl)
> - get_and_append(archiveurl, archive, keep)
> - archivedir.getparent().remove(archivedir)
> + if volume_attr:
> + get_and_append(archiveurl, volume_attr + archive, keep)
volume_attr + archive yields 0.combinedarchive.tar this is not nice.
the file was supposed to be hidden, and start with a .
anyways... the archives need to reside in a tmpdir.
Please use elbepack.filessystem.TmpdirFilesystem
to store the archives.
> + arch.set_text(enbase(volume_attr + archive, True))
> + else:
> + get_and_append(archiveurl, archive, keep)
> + arch.set_text(enbase(archive, True))
> +
> + parent.remove(archivedir)
> except (CalledProcessError, OSError):
> msg = "Failure while processing \"" + archivedir.text + "\":\n"
> msg += str(sys.exc_info()[1])
> raise ArchivedirError(msg)
>
> - arch = elbexml.ensure_child("archive")
> - arch.set_text(enbase(archive, True))
> -
> - os.remove(archive)
> -
> - return xml
> + if os.path.exists(archive):
> + os.remove(archive)
> + for rem_archive in glob.glob("*" + archive):
> + os.remove(rem_archive)
removing files through a glob is not ok.
As suggested above, use TmpdirFilesystem, which will remove
the whole directory when the object is destroyed.
>
> def combinearchivedir(xml):
> - if xml.find("archivedir") is None:
> + if xml.find("//archivedir") is None:
> return xml
>
> - return _combinearchivedir(xml)
> + _combinearchivedir(xml, "archivedir")
> + _combinearchivedir(xml, "src-cdrom/archivedir")
> + return xml
> diff --git a/elbepack/cdroms.py b/elbepack/cdroms.py
> index 3ba67f7b..0f5fa394 100644
> --- a/elbepack/cdroms.py
> +++ b/elbepack/cdroms.py
> @@ -18,6 +18,8 @@ from elbepack.aptpkgutils import XMLPackage
> from elbepack.filesystem import Filesystem, hostfs
> from elbepack.shellhelper import CommandError
> from elbepack.isooptions import get_iso_options
> +from elbepack.treeutils import etree, elem
> +from elbepack.shellhelper import system
>
> CDROM_SIZE = 640 * 1000 * 1000
>
> @@ -105,6 +107,17 @@ def mk_source_cdrom(
>
> if xml is not None:
> options = get_iso_options(log, xml)
> +
> + if xml.has("src-cdrom/archive") and xml.text("src-cdrom/archive"):
this check is not necessary.
> + for i, arch in enumerate(xml.node('src-cdrom').all('archive')):
the loop will not iterate over anything, if no node is found.
> + volume_attr = arch.et.get('volume')
> + if volume_attr:
> + volume_list = [int(v) for v in volume_attr.split(",")]
> + else:
> + volume_list = repo.volume_indexes()
> + for volume_number in volume_list:
> + with xml.archive_tmpfile("src-cdrom/archive[%i]" % (i+1)) as fp:
this is an artificial construct. you already have a handle to arch.
then you make archive_tmpfile() select it again via xpath, because
archive_tmpfile() takes an xpath parameter.
when archive_tmpfile() takes an elem() as the parameter, this would not
be necessary.
> + log.do('tar xvfj "%s" -h -C "%s"' % (fp.name, repo.get_volume_fs(volume_number).path))
> else:
> options = ""
>
> diff --git a/elbepack/repomanager.py b/elbepack/repomanager.py
> index 18fa6926..b0f2ad82 100644
> --- a/elbepack/repomanager.py
> +++ b/elbepack/repomanager.py
> @@ -99,7 +99,8 @@ class RepoBase(object):
>
> def get_volume_fs(self, volume):
> if self.maxsize:
> - volname = os.path.join(self.vol_path, "vol%02d" % volume)
> + offset = volume if volume >= 0 else self.volume_count + 1 + volume
why do you call this variable offset ?
it seems to be an absolute volume number.
please write this more clearly, and add one comment, that states,
that negative numbers represent volumes counted from the last.
> + volname = os.path.join(self.vol_path, "vol%02d" % offset)
> return Filesystem(volname)
>
> return Filesystem(self.vol_path)
> @@ -313,7 +314,7 @@ class RepoBase(object):
> (options, fname, new_path))
> files.append(fname)
> else:
> - for i in range(self.volume_count + 1):
> + for i in self.volume_indexes():
> volfs = self.get_volume_fs(i)
> newname = fname + ("%02d" % i)
> self.log.do(
> @@ -323,6 +324,8 @@ class RepoBase(object):
>
> return files
>
> + def volume_indexes(self):
> + return range(self.volume_count + 1)
>
> class UpdateRepo(RepoBase):
> def __init__(self, xml, path, log):
> --
> 2.20.1
>
>
> _______________________________________________
> elbe-devel mailing list
> elbe-devel at linutronix.de
> https://lists.linutronix.de/mailman/listinfo/elbe-devel
--
Torben Hohn
Linutronix GmbH | Bahnhofstrasse 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 18; Fax.: +49 7556 25 999 99
Hinweise zum Datenschutz finden Sie hier (Informations on data privacy
can be found here): https://linutronix.de/kontakt/Datenschutz.php
Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen |
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700
806 | Geschäftsführer (Managing Directors): Heinz Egger, Thomas Gleixner
More information about the elbe-devel
mailing list