Merge lp:~james-w/linaro-image-tools/tarfile-improvements into lp:linaro-image-tools/11.11

[James Westby]

Hi,

This adds a subclass of the stdlib tarfile.TarFile that aids
in creating tarfiles without touching the filesystem.

I want this for two reasons, the first being for testing my next
branch, which adds some matchers for tarfile contents. I want to
test those without hitting the filesystem.

The second reason is that if possible I want to create the hwpack
tarball without putting all the files on the filesystem first.

Thanks,

James

[lp:~james-w/linaro-image-tools/tarfile-improvements]

66. By James Westby on 2010-08-31

Merged config-docs into tarfile-improvements.

67. By James Westby on 2010-08-31

Factor out some common code in the tests.

68. By James Westby on 2010-08-31

Use dict.pop instead of the custom implementation. Thanks Michael.

[Michael Hudson-Doyle (mwhudson)]

hwpack/better_tarfile.py could use a module docstring.

It's not clear to me why standard_tarfile or better_tarfile have default arguments: they're both only ever called with the non-defaulted arguments supplied (relatedly, I wonder what the combination of mode and fileobj to TarFile.__init__ means.....)

The standard_tarfile context manager is only used in tests, so maybe it should be defined in the test module.

I don't like the way that the tests "know" that create_simple_tarball creates a file called foo with content bar. I think it would be better to use a pattern like bzrlib's build_tree where you say create_simple_tarball([('foo', 'bar')]) or something.

All else looks ok.

review: Approve

[James Westby (james-w)]

On Tue, 31 Aug 2010 01:13:40 -0000, Michael Hudson <email address hidden> wrote:
> Review: Approve
> hwpack/better_tarfile.py could use a module docstring.

Done, thanks.

> It's not clear to me why standard_tarfile or better_tarfile have
> default arguments: they're both only ever called with the
> non-defaulted arguments supplied (relatedly, I wonder what the
> combination of mode and fileobj to TarFile.__init__ means.....)

Later branches supply different mode arguments, so I probably should
have made that change there.

mode can also be used to specify the compression that will be used, so
while a mode of "w" on a read-only file object makes no sense, it is
needed to have both.

> The standard_tarfile context manager is only used in tests, so maybe
> it should be defined in the test module.

It's used in two test modules later, which is why I moved it here, but
I'll move it to a testing module (if it stays given your comments on the
next proposal.)

> I don't like the way that the tests "know" that create_simple_tarball
> creates a file called foo with content bar. I think it would be
> better to use a pattern like bzrlib's build_tree where you say
> create_simple_tarball([('foo', 'bar')]) or something.

I think that's a good idea. I had them all explicitly creating the
paths, but given that they all created identical paths I just factored
it out.

The other option would be fixture, where the path would be an attribute,
and so wouldn't be hardcoded like that.

> All else looks ok.

Thanks, tweaks are on their way.

James

[lp:~james-w/linaro-image-tools/tarfile-improvements]

69. By James Westby on 2010-08-31

Add a module docstring to better_tarfile. Thanks Michael.

70. By James Westby on 2010-08-31

Less magic in the tests. Thanks Michael.

[James Westby (james-w)]

On Tue, 31 Aug 2010 10:48:06 -0400, James Westby <email address hidden> wrote:
> On Tue, 31 Aug 2010 01:13:40 -0000, Michael Hudson <email address hidden> wrote:
> > The standard_tarfile context manager is only used in tests, so maybe
> > it should be defined in the test module.
>
> It's used in two test modules later, which is why I moved it here, but
> I'll move it to a testing module (if it stays given your comments on the
> next proposal.)

Moved.

> > I don't like the way that the tests "know" that create_simple_tarball
> > creates a file called foo with content bar. I think it would be
> > better to use a pattern like bzrlib's build_tree where you say
> > create_simple_tarball([('foo', 'bar')]) or something.
>
> I think that's a good idea. I had them all explicitly creating the
> paths, but given that they all created identical paths I just factored
> it out.

Made the change as suggested. I don't like the duplication with the
helper I add in the next branch, but I'm not sure I like indirecting
through a helper in order to test the functionality.

Thanks,

James

[lp:~james-w/linaro-image-tools/tarfile-improvements]

71. By James Westby on 2010-08-31

Merged config-docs into tarfile-improvements.

=== added file 'hwpack/better_tarfile.py'
--- hwpack/better_tarfile.py	1970-01-01 00:00:00 +0000
+++ hwpack/better_tarfile.py	2010-09-01 20:48:41 +0000
@@ -0,0 +1,97 @@
+from contextlib import contextmanager
+from StringIO import StringIO
+from tarfile import DIRTYPE, TarFile as StandardTarFile, TarInfo
+
+"""Improvements to the standard library's tarfile module.
+
+In particular this module provides a tarfile.TarFile subclass that aids
+in adding paths to the tarfile that aren't present on the filesystem,
+with the ability to specify file content as strings, and provide
+default values for the mtime, uid, etc. of the created paths.
+"""
+
+@contextmanager
+def writeable_tarfile(backing_file, mode="w", **kwargs):
+    """A context manager to get a writeable better tarfile.
+
+    :param backing_file: a file object to write the tarfile contents
+        to.
+    :param mode: the mode to open the tarfile with. Default is
+        "w".
+    :param kwargs: other keyword arguments to pass to the TarFile
+        constructor.
+    """
+    tf = TarFile.open(mode=mode, fileobj=backing_file, **kwargs)
+    try:
+        yield tf
+    finally:
+        tf.close()
+
+
+class TarFile(StandardTarFile):
+    """An improvement to tarfile that can add paths not on the filesystem.
+
+    With the standard tarfile implementation adding paths that are not
+    present on the filesystem is convoluted. This subclass adds methods
+    to create paths in the tarfile that are not present on the filesystem.
+
+    In addition, it can take constructor parameters to set the defaults
+    of various attributes of the paths that it adds.
+    """
+
+    def __init__(self, *args, **kwargs):
+        """Create a TarFile.
+
+        :param default_mtime: the default mtime to create paths with,
+            an int or None to use the stdlib default.
+        :param default_uid: the default user id to set as the owner of
+            created paths, an int or None to use the stdlib default.
+        :param default_gid: the default group id to set as the owner of
+            created paths, an int or None to use the stdlib default.
+        :param default_uname: the default user name to set as the owner
+            of created paths, a string, or None to use the stdlib default.
+        :param default_gname: the default group name ot set as the owner
+            of created paths, a string, or None to use the stdlib default.
+        """
+        self.default_mtime = kwargs.pop("default_mtime", None)
+        self.default_uid = kwargs.pop("default_uid", None)
+        self.default_gid = kwargs.pop("default_gid", None)
+        self.default_uname = kwargs.pop("default_uname", None)
+        self.default_gname = kwargs.pop("default_gname", None)
+        super(TarFile, self).__init__(*args, **kwargs)
+
+    def _set_defaults(self, tarinfo):
+        if self.default_mtime is not None:
+            tarinfo.mtime = self.default_mtime
+        if self.default_uid is not None:
+            tarinfo.uid = self.default_uid
+        if self.default_gid is not None:
+            tarinfo.gid = self.default_gid
+        if self.default_uname is not None:
+            tarinfo.uname = self.default_uname
+        if self.default_gname is not None:
+            tarinfo.gname = self.default_gname
+
+    def create_file_from_string(self, filename, content):
+        """Create a file with the contents passed as a string.
+
+        :param filename: the path to put the file at inside the
+            tarfile.
+        :param content: the content to put in the created file.
+        """
+        tarinfo = TarInfo(name=filename)
+        tarinfo.size = len(content)
+        self._set_defaults(tarinfo)
+        fileobj = StringIO(content)
+        self.addfile(tarinfo, fileobj=fileobj)
+
+    def create_dir(self, path):
+        """Create a directory within the tarfile.
+
+        :param path: the path to put the directory at.
+        """
+        tarinfo = TarInfo(name=path)
+        tarinfo.type = DIRTYPE
+        tarinfo.mode = 0755
+        self._set_defaults(tarinfo)
+        self.addfile(tarinfo)

=== modified file 'hwpack/tests/__init__.py'
--- hwpack/tests/__init__.py	2010-08-27 19:35:40 +0000
+++ hwpack/tests/__init__.py	2010-09-01 20:48:41 +0000
@@ -2,6 +2,7 @@
 
 def test_suite():
     module_names = ['hwpack.tests.test_config',
+                    'hwpack.tests.test_better_tarfile',
                    ]
     loader = unittest.TestLoader()
     suite = loader.loadTestsFromNames(module_names)

=== added file 'hwpack/tests/test_better_tarfile.py'
--- hwpack/tests/test_better_tarfile.py	1970-01-01 00:00:00 +0000
+++ hwpack/tests/test_better_tarfile.py	2010-09-01 20:48:41 +0000
@@ -0,0 +1,176 @@
+from contextlib import contextmanager
+from StringIO import StringIO
+import tarfile
+
+from testtools import TestCase
+
+from hwpack.better_tarfile import writeable_tarfile
+
+
+@contextmanager
+def standard_tarfile(backing_file, mode="r", seek=True):
+    """A context manager to open a stdlib tarfile.
+
+    :param backing_file: the file object to take the tarfile
+        contents from.
+    :param mode: the mode to open the tarfile with.
+    :param seek: whether to seek the backing file to 0 before
+        opening.
+    """
+    if seek:
+        backing_file.seek(0)
+    tf = tarfile.TarFile.open(mode=mode, fileobj=backing_file)
+    try:
+        yield tf
+    finally:
+        tf.close()
+
+
+class TarFileTests(TestCase):
+
+    def test_creates_empty_tarfile(self):
+        backing_file = StringIO()
+        with writeable_tarfile(backing_file):
+            pass
+        with standard_tarfile(backing_file, seek=False) as tf:
+            self.assertEqual([], tf.getnames())
+
+    def create_simple_tarball(self, contents, **kwargs):
+        backing_file = StringIO()
+        with writeable_tarfile(backing_file, **kwargs) as tf:
+            for path, content in contents:
+                if path[-1] == '/':
+                    tf.create_dir(path)
+                else:
+                    tf.create_file_from_string(path, content)
+        return backing_file
+
+    def test_create_file_from_string_adds_path(self):
+        backing_file = self.create_simple_tarball([("foo", "bar")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(["foo"], tf.getnames())
+
+    def test_create_file_from_string_uses_content(self):
+        backing_file = self.create_simple_tarball([("foo", "bar")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual("bar", tf.extractfile("foo").read())
+
+    def test_create_file_from_string_sets_size(self):
+        backing_file = self.create_simple_tarball([("foo", "bar")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(3, tf.getmember("foo").size)
+
+    def test_create_file_from_string_sets_mode(self):
+        backing_file = self.create_simple_tarball([("foo", "bar")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(0644, tf.getmember("foo").mode)
+
+    def test_create_file_from_string_sets_type(self):
+        backing_file = self.create_simple_tarball([("foo", "bar")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(tarfile.REGTYPE, tf.getmember("foo").type)
+
+    def test_create_file_from_string_sets_linkname(self):
+        backing_file = self.create_simple_tarball([("foo", "bar")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual('', tf.getmember("foo").linkname)
+
+    def test_create_file_uses_default_mtime(self):
+        now = 126793
+        backing_file = self.create_simple_tarball(
+            [("foo", "bar")], default_mtime=now)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(now, tf.getmember("foo").mtime)
+
+    def test_create_file_uses_default_uid(self):
+        uid = 1259
+        backing_file = self.create_simple_tarball(
+            [("foo", "bar")], default_uid=uid)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(uid, tf.getmember("foo").uid)
+
+    def test_create_file_uses_default_gid(self):
+        gid = 2259
+        backing_file = self.create_simple_tarball(
+            [("foo", "bar")], default_gid=gid)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(gid, tf.getmember("foo").gid)
+
+    def test_create_file_uses_default_uname(self):
+        uname = "someperson"
+        backing_file = self.create_simple_tarball(
+            [("foo", "bar")], default_uname=uname)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(uname, tf.getmember("foo").uname)
+
+    def test_create_file_uses_default_gname(self):
+        gname = "somegroup"
+        backing_file = self.create_simple_tarball(
+            [("foo", "bar")], default_gname=gname)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(gname, tf.getmember("foo").gname)
+
+    def test_create_dir_adds_path(self):
+        backing_file = self.create_simple_tarball([("foo/", "")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(["foo"], tf.getnames())
+
+    def test_create_dir_sets_name(self):
+        backing_file = self.create_simple_tarball([("foo/", "")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual("foo", tf.getmember("foo").name)
+
+    def test_create_dir_sets_type(self):
+        backing_file = self.create_simple_tarball([("foo/", "")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(tarfile.DIRTYPE, tf.getmember("foo").type)
+
+    def test_create_dir_sets_size(self):
+        backing_file = self.create_simple_tarball([("foo/", "")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(0, tf.getmember("foo").size)
+
+    def test_create_dir_sets_mode(self):
+        backing_file = self.create_simple_tarball([("foo/", "")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(0755, tf.getmember("foo").mode)
+
+    def test_create_dir_sets_linkname(self):
+        backing_file = self.create_simple_tarball([("foo/", "")])
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual('', tf.getmember("foo").linkname)
+
+    def test_create_dir_uses_default_mtime(self):
+        now = 126793
+        backing_file = self.create_simple_tarball(
+            [("foo/", "")], default_mtime=now)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(now, tf.getmember("foo").mtime)
+
+    def test_create_dir_uses_default_uid(self):
+        uid = 1259
+        backing_file = self.create_simple_tarball(
+            [("foo/", "")], default_uid=uid)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(uid, tf.getmember("foo").uid)
+
+    def test_create_dir_uses_default_gid(self):
+        gid = 2259
+        backing_file = self.create_simple_tarball(
+            [("foo/", "")], default_gid=gid)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(gid, tf.getmember("foo").gid)
+
+    def test_create_dir_uses_default_uname(self):
+        uname = "someperson"
+        backing_file = self.create_simple_tarball(
+            [("foo/", "")], default_uname=uname)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(uname, tf.getmember("foo").uname)
+
+    def test_create_dir_uses_default_gname(self):
+        gname = "somegroup"
+        backing_file = self.create_simple_tarball(
+            [("foo/", "")], default_gname=gname)
+        with standard_tarfile(backing_file) as tf:
+            self.assertEqual(gname, tf.getmember("foo").gname)