[pacman-dev] [PATCH 3/3] Added tests for -Q --check (both fast(files) and full(mtree)).
Andrew Gregory
andrew.gregory.8 at gmail.com
Fri Oct 4 11:22:50 EDT 2013
On 10/02/13 at 11:48am, Jeremy Heiner wrote:
> Two new "hooks" were made available in pmtest: genhook is called after
> everything is generated (just before the snapshot is taken), and
> snaphook is called just after the snapshot. A hook is a list of
> strings. Calling a hook is "exec"ing each string in the list.
>
> Some helper functions were added to pmtest, notably pacmanrun and
> addrule__pacman_warned. pacmanrun and rootjoin are used in a hook to
> install a package and then intentionally mess it up, which then allows
> the "-Qk" test output to be verified (with addrule__pacman_warned).
>
> Signed-off-by: Jeremy Heiner <ScalaProtractor at gmail.com>
> ---
> test/pacman/pmpkg.py | 47 ++++++++++++++++++++++++++++
> test/pacman/pmtest.py | 63 +++++++++++++++++++++++++++-----------
> test/pacman/tests/querycheck001.py | 18 +++++++++++
> test/pacman/tests/querycheck002.py | 24 +++++++++++++++
> test/pacman/tests/querycheck003.py | 24 +++++++++++++++
> test/pacman/tests/querycheck004.py | 27 ++++++++++++++++
> test/pacman/tests/querycheck005.py | 33 ++++++++++++++++++++
> test/pacman/tests/querycheck006.py | 20 ++++++++++++
> test/pacman/tests/querycheck007.py | 20 ++++++++++++
> test/pacman/tests/querycheck008.py | 33 ++++++++++++++++++++
> test/pacman/tests/querycheck009.py | 30 ++++++++++++++++++
> test/pacman/tests/querycheck010.py | 27 ++++++++++++++++
> test/pacman/util.py | 1 +
> 13 files changed, 349 insertions(+), 18 deletions(-)
> create mode 100644 test/pacman/tests/querycheck001.py
> create mode 100644 test/pacman/tests/querycheck002.py
> create mode 100644 test/pacman/tests/querycheck003.py
> create mode 100644 test/pacman/tests/querycheck004.py
> create mode 100644 test/pacman/tests/querycheck005.py
> create mode 100644 test/pacman/tests/querycheck006.py
> create mode 100644 test/pacman/tests/querycheck007.py
> create mode 100644 test/pacman/tests/querycheck008.py
> create mode 100644 test/pacman/tests/querycheck009.py
> create mode 100644 test/pacman/tests/querycheck010.py
There is A LOT going on in this patch. Please try to break your
patches up into smaller changes. At least mtree and hook support
definitely should have been individual patches.
Before I get into specifics, pacman already has "hooks" of a sort:
install scripts. I think I would prefer to see support added for
install scripts in local db packages than the ability to run arbitrary
python code. You could then modify files as needed from
post_install(). This would match our existing use of install scripts
as hooks and could be run within fakechroot. Anybody else have any
thoughts on this?
> diff --git a/test/pacman/pmpkg.py b/test/pacman/pmpkg.py
> index 9c9e447..bb0facf 100644
> --- a/test/pacman/pmpkg.py
> +++ b/test/pacman/pmpkg.py
> @@ -23,6 +23,8 @@
> import shutil
> from StringIO import StringIO
> import tarfile
> +import hashlib
> +import zlib
>
> import util
>
> @@ -59,6 +61,7 @@ def __init__(self, name, version = "1.0-1"):
> # files
> self.files = []
> self.backup = []
> + self.mtree = False
> # install
> self.install = {
> "pre_install": "",
> @@ -104,6 +107,33 @@ def parse_filename(name):
> filename, extra = filename.split("|")
> return filename
>
> + def mtreefile(self, info, data):
> + if self.mtree:
> + self.mtree.append(
> + "./%s uid=%d gid=%d mode=%o time=%d.%d"
> + " type=file size=%d md5digest=%s sha256digest=%s"
> + % (info.name, info.uid, info.gid, info.mode,
> + info.mtime >> 32, info.mtime & 0xFFFFFFFF,
> + info.size, hashlib.md5(data).hexdigest(),
> + hashlib.sha256(data).hexdigest()))
> +
> + def mtreedir(self, info):
> + if self.mtree:
> + self.mtree.append(
> + "./%s uid=%d gid=%d mode=%o time=%d.%d"
> + " type=dir"
> + % (info.name, info.uid, info.gid, info.mode,
> + info.mtime >> 32, info.mtime & 0xFFFFFFFF))
> +
> + def mtreelink(self, info):
> + if self.mtree:
> + self.mtree.append(
> + "./%s uid=%d gid=%d mode=%o time=%d.%d"
> + " type=link link=%s"
> + % (info.name, info.uid, info.gid, info.mode,
> + info.mtime >> 32, info.mtime & 0xFFFFFFFF,
> + info.linkname))
> +
Since these alter the mtree, it would be nice to have 'add' in the
function names to make that clear. A comment explaining the bit
operations on mtime would also be nice
> def makepkg(self, path):
> """Creates an Arch Linux package archive.
>
> @@ -148,11 +178,14 @@ def makepkg(self, path):
> util.mkdir(os.path.dirname(self.path))
>
> # Generate package metadata
> + if self.mtree:
> + self.mtree = ["#mtree"]
I don't like this dual use of mtree as a boolean and then a list. It
would be clearer to use something like buildmtree for the boolean
option.
> tar = tarfile.open(self.path, "w:gz")
> for name, data in archive_files:
> info = tarfile.TarInfo(name)
> info.size = len(data)
> tar.addfile(info, StringIO(data))
> + self.mtreefile(info, data)
>
> # Generate package file system
> for name in self.files:
> @@ -162,18 +195,32 @@ def makepkg(self, path):
> info.mode = fileinfo["perms"]
> elif fileinfo["isdir"]:
> info.mode = 0755
> + elif fileinfo["islink"]:
> + info.mode = 0777
> if fileinfo["isdir"]:
> info.type = tarfile.DIRTYPE
> tar.addfile(info)
> + self.mtreedir(info)
> elif fileinfo["islink"]:
> info.type = tarfile.SYMTYPE
> info.linkname = fileinfo["link"]
> tar.addfile(info)
> + self.mtreelink(info)
> else:
> # TODO wow what a hack, adding a newline to match mkfile?
> filedata = name + "\n"
> info.size = len(filedata)
> tar.addfile(info, StringIO(filedata))
> + self.mtreefile(info, filedata)
> +
> + # .MTREE
> + if self.mtree:
> + filedata = "\n".join(self.mtree)+"\n" # zlib.compress(filedata)?
> + # but that causes "Unrecognized archive format" error, and this
> + # seems to work anyway (libalpm is happy with uncompressed file)
> + info = tarfile.TarInfo(".MTREE")
> + info.size = len(filedata)
> + tar.addfile(info, StringIO(filedata))
>
> tar.close()
>
> diff --git a/test/pacman/pmtest.py b/test/pacman/pmtest.py
> index a0a1455..0afb222 100644
> --- a/test/pacman/pmtest.py
> +++ b/test/pacman/pmtest.py
> @@ -18,6 +18,7 @@
>
>
> import os
> +import re
> import shlex
> import shutil
> import stat
> @@ -99,6 +100,9 @@ def load(self):
> self.localpkgs = []
> self.createlocalpkgs = False
> self.filesystem = []
> + self.rundir = self.rootjoin(util.TMPDIR)
> + self.genhook = []
> + self.snaphook = []
>
> self.description = ""
> self.option = {}
> @@ -198,12 +202,50 @@ def generate(self):
>
> # Done.
> vprint(" Taking a snapshot of the file system")
> + for cmd in self.genhook:
> + vprint("\texec "+cmd)
> + exec cmd
> for roots, dirs, files in os.walk(root):
> for i in files:
> filename = os.path.join(roots, i)
> f = pmfile.PacmanFile(root, self.rootremove(filename))
> self.files.append(f)
> vprint("\t%s" % f.name)
> + for cmd in self.snaphook:
> + vprint("\texec "+cmd)
> + exec cmd
> +
> + def pacmanbin(self):
> + return([self.env.pacman["bin"],
> + "--config", self.rootjoin(util.PACCONF),
> + "--root", self.env.root,
> + "--dbpath", self.rootjoin(util.PM_DBPATH),
> + "--cachedir", self.rootjoin(util.PM_CACHEDIR)])
> +
> + def pacmansub(self, cmd, output):
> + vprint("\trunning: %s" % " ".join(cmd))
> + time_start = time.time()
> + retcode = subprocess.call(cmd, stdout=output, stderr=output,
> + cwd=self.rundir, env={'LC_ALL': 'C'})
> + time_end = time.time()
> + vprint("\ttime elapsed: %.2fs" % (time_end - time_start))
> + vprint("\tretcode = %s" % retcode)
> + return retcode
> +
> + def pacmanrun(self, args):
> + cmd = self.env.cmdroot()
> + cmd.extend(self.pacmanbin())
> + cmd.append("--noconfirm")
> + cmd.extend(shlex.split(args))
> + output = open(self.rootjoin(util.GENFILE), 'a')
> + self.pacmansub(cmd, output)
> + output.close()
The only reason to refactor this code into functions seems to be so
that you can run pacman from genhook, but you only do that to install
packages, which should actually be done by adding them to the local
db, so I'm not really sure what the point of this is. Furthermore,
I'm not sure how wise it is to use pacman to set up its own test
environment.
> +
> + def addrule__pacman_warned(self, pkg, path, msg):
> + if not path.startswith(self.env.root):
> + path = self.rootjoin(path)
> + match = re.escape("warning: %s: %s (%s)" % (pkg.name, path, msg))
> + self.addrule("PACMAN_OUTPUT=^"+match+"$")
I am against this. For starters, not all of pacman's warnings match
this format, so the name is not appropriate. Also, I'm not against
helper functions for rules generally, but I'd rather them not be quite
so specialized as this and they should be put under pmrule.
self.addrule(pmrule.foo(...))
>
> def run(self):
> if os.path.isfile(util.PM_LOCK):
> @@ -226,11 +268,7 @@ def run(self):
> "--tool=memcheck", "--leak-check=full",
> "--show-reachable=yes",
> "--suppressions=%s" % suppfile])
> - cmd.extend([pacman["bin"],
> - "--config", self.rootjoin(util.PACCONF),
> - "--root", root,
> - "--dbpath", self.rootjoin(util.PM_DBPATH),
> - "--cachedir", self.rootjoin(util.PM_CACHEDIR)])
> + cmd.extend(self.pacmanbin())
> if not pacman["manual-confirm"]:
> cmd.append("--noconfirm")
> if pacman["debug"]:
> @@ -240,28 +278,17 @@ def run(self):
> output = open(self.rootjoin(util.LOGFILE), 'w')
> else:
> output = None
> - vprint("\trunning: %s" % " ".join(cmd))
> -
> - # Change to the tmp dir before running pacman, so that local package
> - # archives are made available more easily.
> - tmpdir = self.rootjoin(util.TMPDIR)
> - time_start = time.time()
> - self.retcode = subprocess.call(cmd, stdout=output, stderr=output,
> - cwd=tmpdir, env={'LC_ALL': 'C'})
> - time_end = time.time()
> - vprint("\ttime elapsed: %.2fs" % (time_end - time_start))
>
> + self.retcode = self.pacmansub(cmd, output)
> if output:
> output.close()
>
> - vprint("\tretcode = %s" % self.retcode)
> -
> # Check if the lock is still there
> if os.path.isfile(util.PM_LOCK):
> tap.diag("\tERROR: %s not removed" % util.PM_LOCK)
> os.unlink(util.PM_LOCK)
> # Look for a core file
> - if os.path.isfile(os.path.join(tmpdir, "core")):
> + if os.path.isfile(os.path.join(self.rundir, "core")):
> tap.diag("\tERROR: pacman dumped a core file")
>
> def check(self):
More information about the pacman-dev
mailing list