Merge lp:~james-w/linaro-image-tools/tarfile-improvements into lp:linaro-image-tools/11.11
- tarfile-improvements
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Michael Hudson-Doyle |
Approved revision: | 68 |
Merged at revision: | 54 |
Proposed branch: | lp:~james-w/linaro-image-tools/tarfile-improvements |
Merge into: | lp:linaro-image-tools/11.11 |
Prerequisite: | lp:~james-w/linaro-image-tools/config-docs |
Diff against target: |
294 lines (+274/-0) 3 files modified
hwpack/better_tarfile.py (+97/-0) hwpack/tests/__init__.py (+1/-0) hwpack/tests/test_better_tarfile.py (+176/-0) |
To merge this branch: | bzr merge lp:~james-w/linaro-image-tools/tarfile-improvements |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Hudson-Doyle (community) | Approve | ||
Review via email: mp+34144@code.launchpad.net |
Commit message
Description of the change
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
- 66. By James Westby
-
Merged config-docs into tarfile-
improvements. - 67. By James Westby
-
Factor out some common code in the tests.
- 68. By James Westby
-
Use dict.pop instead of the custom implementation. Thanks Michael.
James Westby (james-w) wrote : | # |
On Tue, 31 Aug 2010 01:13:40 -0000, Michael Hudson <email address hidden> wrote:
> Review: Approve
> hwpack/
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_
> 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_
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
- 69. By James Westby
-
Add a module docstring to better_tarfile. Thanks Michael.
- 70. By James Westby
-
Less magic in the tests. Thanks Michael.
James Westby (james-w) wrote : | # |
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_
> > 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_
>
> 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
- 71. By James Westby
-
Merged config-docs into tarfile-
improvements.
Preview Diff
1 | === added file 'hwpack/better_tarfile.py' | |||
2 | --- hwpack/better_tarfile.py 1970-01-01 00:00:00 +0000 | |||
3 | +++ hwpack/better_tarfile.py 2010-09-01 20:48:41 +0000 | |||
4 | @@ -0,0 +1,97 @@ | |||
5 | 1 | from contextlib import contextmanager | ||
6 | 2 | from StringIO import StringIO | ||
7 | 3 | from tarfile import DIRTYPE, TarFile as StandardTarFile, TarInfo | ||
8 | 4 | |||
9 | 5 | """Improvements to the standard library's tarfile module. | ||
10 | 6 | |||
11 | 7 | In particular this module provides a tarfile.TarFile subclass that aids | ||
12 | 8 | in adding paths to the tarfile that aren't present on the filesystem, | ||
13 | 9 | with the ability to specify file content as strings, and provide | ||
14 | 10 | default values for the mtime, uid, etc. of the created paths. | ||
15 | 11 | """ | ||
16 | 12 | |||
17 | 13 | @contextmanager | ||
18 | 14 | def writeable_tarfile(backing_file, mode="w", **kwargs): | ||
19 | 15 | """A context manager to get a writeable better tarfile. | ||
20 | 16 | |||
21 | 17 | :param backing_file: a file object to write the tarfile contents | ||
22 | 18 | to. | ||
23 | 19 | :param mode: the mode to open the tarfile with. Default is | ||
24 | 20 | "w". | ||
25 | 21 | :param kwargs: other keyword arguments to pass to the TarFile | ||
26 | 22 | constructor. | ||
27 | 23 | """ | ||
28 | 24 | tf = TarFile.open(mode=mode, fileobj=backing_file, **kwargs) | ||
29 | 25 | try: | ||
30 | 26 | yield tf | ||
31 | 27 | finally: | ||
32 | 28 | tf.close() | ||
33 | 29 | |||
34 | 30 | |||
35 | 31 | class TarFile(StandardTarFile): | ||
36 | 32 | """An improvement to tarfile that can add paths not on the filesystem. | ||
37 | 33 | |||
38 | 34 | With the standard tarfile implementation adding paths that are not | ||
39 | 35 | present on the filesystem is convoluted. This subclass adds methods | ||
40 | 36 | to create paths in the tarfile that are not present on the filesystem. | ||
41 | 37 | |||
42 | 38 | In addition, it can take constructor parameters to set the defaults | ||
43 | 39 | of various attributes of the paths that it adds. | ||
44 | 40 | """ | ||
45 | 41 | |||
46 | 42 | def __init__(self, *args, **kwargs): | ||
47 | 43 | """Create a TarFile. | ||
48 | 44 | |||
49 | 45 | :param default_mtime: the default mtime to create paths with, | ||
50 | 46 | an int or None to use the stdlib default. | ||
51 | 47 | :param default_uid: the default user id to set as the owner of | ||
52 | 48 | created paths, an int or None to use the stdlib default. | ||
53 | 49 | :param default_gid: the default group id to set as the owner of | ||
54 | 50 | created paths, an int or None to use the stdlib default. | ||
55 | 51 | :param default_uname: the default user name to set as the owner | ||
56 | 52 | of created paths, a string, or None to use the stdlib default. | ||
57 | 53 | :param default_gname: the default group name ot set as the owner | ||
58 | 54 | of created paths, a string, or None to use the stdlib default. | ||
59 | 55 | """ | ||
60 | 56 | self.default_mtime = kwargs.pop("default_mtime", None) | ||
61 | 57 | self.default_uid = kwargs.pop("default_uid", None) | ||
62 | 58 | self.default_gid = kwargs.pop("default_gid", None) | ||
63 | 59 | self.default_uname = kwargs.pop("default_uname", None) | ||
64 | 60 | self.default_gname = kwargs.pop("default_gname", None) | ||
65 | 61 | super(TarFile, self).__init__(*args, **kwargs) | ||
66 | 62 | |||
67 | 63 | def _set_defaults(self, tarinfo): | ||
68 | 64 | if self.default_mtime is not None: | ||
69 | 65 | tarinfo.mtime = self.default_mtime | ||
70 | 66 | if self.default_uid is not None: | ||
71 | 67 | tarinfo.uid = self.default_uid | ||
72 | 68 | if self.default_gid is not None: | ||
73 | 69 | tarinfo.gid = self.default_gid | ||
74 | 70 | if self.default_uname is not None: | ||
75 | 71 | tarinfo.uname = self.default_uname | ||
76 | 72 | if self.default_gname is not None: | ||
77 | 73 | tarinfo.gname = self.default_gname | ||
78 | 74 | |||
79 | 75 | def create_file_from_string(self, filename, content): | ||
80 | 76 | """Create a file with the contents passed as a string. | ||
81 | 77 | |||
82 | 78 | :param filename: the path to put the file at inside the | ||
83 | 79 | tarfile. | ||
84 | 80 | :param content: the content to put in the created file. | ||
85 | 81 | """ | ||
86 | 82 | tarinfo = TarInfo(name=filename) | ||
87 | 83 | tarinfo.size = len(content) | ||
88 | 84 | self._set_defaults(tarinfo) | ||
89 | 85 | fileobj = StringIO(content) | ||
90 | 86 | self.addfile(tarinfo, fileobj=fileobj) | ||
91 | 87 | |||
92 | 88 | def create_dir(self, path): | ||
93 | 89 | """Create a directory within the tarfile. | ||
94 | 90 | |||
95 | 91 | :param path: the path to put the directory at. | ||
96 | 92 | """ | ||
97 | 93 | tarinfo = TarInfo(name=path) | ||
98 | 94 | tarinfo.type = DIRTYPE | ||
99 | 95 | tarinfo.mode = 0755 | ||
100 | 96 | self._set_defaults(tarinfo) | ||
101 | 97 | self.addfile(tarinfo) | ||
102 | 0 | 98 | ||
103 | === modified file 'hwpack/tests/__init__.py' | |||
104 | --- hwpack/tests/__init__.py 2010-08-27 19:35:40 +0000 | |||
105 | +++ hwpack/tests/__init__.py 2010-09-01 20:48:41 +0000 | |||
106 | @@ -2,6 +2,7 @@ | |||
107 | 2 | 2 | ||
108 | 3 | def test_suite(): | 3 | def test_suite(): |
109 | 4 | module_names = ['hwpack.tests.test_config', | 4 | module_names = ['hwpack.tests.test_config', |
110 | 5 | 'hwpack.tests.test_better_tarfile', | ||
111 | 5 | ] | 6 | ] |
112 | 6 | loader = unittest.TestLoader() | 7 | loader = unittest.TestLoader() |
113 | 7 | suite = loader.loadTestsFromNames(module_names) | 8 | suite = loader.loadTestsFromNames(module_names) |
114 | 8 | 9 | ||
115 | === added file 'hwpack/tests/test_better_tarfile.py' | |||
116 | --- hwpack/tests/test_better_tarfile.py 1970-01-01 00:00:00 +0000 | |||
117 | +++ hwpack/tests/test_better_tarfile.py 2010-09-01 20:48:41 +0000 | |||
118 | @@ -0,0 +1,176 @@ | |||
119 | 1 | from contextlib import contextmanager | ||
120 | 2 | from StringIO import StringIO | ||
121 | 3 | import tarfile | ||
122 | 4 | |||
123 | 5 | from testtools import TestCase | ||
124 | 6 | |||
125 | 7 | from hwpack.better_tarfile import writeable_tarfile | ||
126 | 8 | |||
127 | 9 | |||
128 | 10 | @contextmanager | ||
129 | 11 | def standard_tarfile(backing_file, mode="r", seek=True): | ||
130 | 12 | """A context manager to open a stdlib tarfile. | ||
131 | 13 | |||
132 | 14 | :param backing_file: the file object to take the tarfile | ||
133 | 15 | contents from. | ||
134 | 16 | :param mode: the mode to open the tarfile with. | ||
135 | 17 | :param seek: whether to seek the backing file to 0 before | ||
136 | 18 | opening. | ||
137 | 19 | """ | ||
138 | 20 | if seek: | ||
139 | 21 | backing_file.seek(0) | ||
140 | 22 | tf = tarfile.TarFile.open(mode=mode, fileobj=backing_file) | ||
141 | 23 | try: | ||
142 | 24 | yield tf | ||
143 | 25 | finally: | ||
144 | 26 | tf.close() | ||
145 | 27 | |||
146 | 28 | |||
147 | 29 | class TarFileTests(TestCase): | ||
148 | 30 | |||
149 | 31 | def test_creates_empty_tarfile(self): | ||
150 | 32 | backing_file = StringIO() | ||
151 | 33 | with writeable_tarfile(backing_file): | ||
152 | 34 | pass | ||
153 | 35 | with standard_tarfile(backing_file, seek=False) as tf: | ||
154 | 36 | self.assertEqual([], tf.getnames()) | ||
155 | 37 | |||
156 | 38 | def create_simple_tarball(self, contents, **kwargs): | ||
157 | 39 | backing_file = StringIO() | ||
158 | 40 | with writeable_tarfile(backing_file, **kwargs) as tf: | ||
159 | 41 | for path, content in contents: | ||
160 | 42 | if path[-1] == '/': | ||
161 | 43 | tf.create_dir(path) | ||
162 | 44 | else: | ||
163 | 45 | tf.create_file_from_string(path, content) | ||
164 | 46 | return backing_file | ||
165 | 47 | |||
166 | 48 | def test_create_file_from_string_adds_path(self): | ||
167 | 49 | backing_file = self.create_simple_tarball([("foo", "bar")]) | ||
168 | 50 | with standard_tarfile(backing_file) as tf: | ||
169 | 51 | self.assertEqual(["foo"], tf.getnames()) | ||
170 | 52 | |||
171 | 53 | def test_create_file_from_string_uses_content(self): | ||
172 | 54 | backing_file = self.create_simple_tarball([("foo", "bar")]) | ||
173 | 55 | with standard_tarfile(backing_file) as tf: | ||
174 | 56 | self.assertEqual("bar", tf.extractfile("foo").read()) | ||
175 | 57 | |||
176 | 58 | def test_create_file_from_string_sets_size(self): | ||
177 | 59 | backing_file = self.create_simple_tarball([("foo", "bar")]) | ||
178 | 60 | with standard_tarfile(backing_file) as tf: | ||
179 | 61 | self.assertEqual(3, tf.getmember("foo").size) | ||
180 | 62 | |||
181 | 63 | def test_create_file_from_string_sets_mode(self): | ||
182 | 64 | backing_file = self.create_simple_tarball([("foo", "bar")]) | ||
183 | 65 | with standard_tarfile(backing_file) as tf: | ||
184 | 66 | self.assertEqual(0644, tf.getmember("foo").mode) | ||
185 | 67 | |||
186 | 68 | def test_create_file_from_string_sets_type(self): | ||
187 | 69 | backing_file = self.create_simple_tarball([("foo", "bar")]) | ||
188 | 70 | with standard_tarfile(backing_file) as tf: | ||
189 | 71 | self.assertEqual(tarfile.REGTYPE, tf.getmember("foo").type) | ||
190 | 72 | |||
191 | 73 | def test_create_file_from_string_sets_linkname(self): | ||
192 | 74 | backing_file = self.create_simple_tarball([("foo", "bar")]) | ||
193 | 75 | with standard_tarfile(backing_file) as tf: | ||
194 | 76 | self.assertEqual('', tf.getmember("foo").linkname) | ||
195 | 77 | |||
196 | 78 | def test_create_file_uses_default_mtime(self): | ||
197 | 79 | now = 126793 | ||
198 | 80 | backing_file = self.create_simple_tarball( | ||
199 | 81 | [("foo", "bar")], default_mtime=now) | ||
200 | 82 | with standard_tarfile(backing_file) as tf: | ||
201 | 83 | self.assertEqual(now, tf.getmember("foo").mtime) | ||
202 | 84 | |||
203 | 85 | def test_create_file_uses_default_uid(self): | ||
204 | 86 | uid = 1259 | ||
205 | 87 | backing_file = self.create_simple_tarball( | ||
206 | 88 | [("foo", "bar")], default_uid=uid) | ||
207 | 89 | with standard_tarfile(backing_file) as tf: | ||
208 | 90 | self.assertEqual(uid, tf.getmember("foo").uid) | ||
209 | 91 | |||
210 | 92 | def test_create_file_uses_default_gid(self): | ||
211 | 93 | gid = 2259 | ||
212 | 94 | backing_file = self.create_simple_tarball( | ||
213 | 95 | [("foo", "bar")], default_gid=gid) | ||
214 | 96 | with standard_tarfile(backing_file) as tf: | ||
215 | 97 | self.assertEqual(gid, tf.getmember("foo").gid) | ||
216 | 98 | |||
217 | 99 | def test_create_file_uses_default_uname(self): | ||
218 | 100 | uname = "someperson" | ||
219 | 101 | backing_file = self.create_simple_tarball( | ||
220 | 102 | [("foo", "bar")], default_uname=uname) | ||
221 | 103 | with standard_tarfile(backing_file) as tf: | ||
222 | 104 | self.assertEqual(uname, tf.getmember("foo").uname) | ||
223 | 105 | |||
224 | 106 | def test_create_file_uses_default_gname(self): | ||
225 | 107 | gname = "somegroup" | ||
226 | 108 | backing_file = self.create_simple_tarball( | ||
227 | 109 | [("foo", "bar")], default_gname=gname) | ||
228 | 110 | with standard_tarfile(backing_file) as tf: | ||
229 | 111 | self.assertEqual(gname, tf.getmember("foo").gname) | ||
230 | 112 | |||
231 | 113 | def test_create_dir_adds_path(self): | ||
232 | 114 | backing_file = self.create_simple_tarball([("foo/", "")]) | ||
233 | 115 | with standard_tarfile(backing_file) as tf: | ||
234 | 116 | self.assertEqual(["foo"], tf.getnames()) | ||
235 | 117 | |||
236 | 118 | def test_create_dir_sets_name(self): | ||
237 | 119 | backing_file = self.create_simple_tarball([("foo/", "")]) | ||
238 | 120 | with standard_tarfile(backing_file) as tf: | ||
239 | 121 | self.assertEqual("foo", tf.getmember("foo").name) | ||
240 | 122 | |||
241 | 123 | def test_create_dir_sets_type(self): | ||
242 | 124 | backing_file = self.create_simple_tarball([("foo/", "")]) | ||
243 | 125 | with standard_tarfile(backing_file) as tf: | ||
244 | 126 | self.assertEqual(tarfile.DIRTYPE, tf.getmember("foo").type) | ||
245 | 127 | |||
246 | 128 | def test_create_dir_sets_size(self): | ||
247 | 129 | backing_file = self.create_simple_tarball([("foo/", "")]) | ||
248 | 130 | with standard_tarfile(backing_file) as tf: | ||
249 | 131 | self.assertEqual(0, tf.getmember("foo").size) | ||
250 | 132 | |||
251 | 133 | def test_create_dir_sets_mode(self): | ||
252 | 134 | backing_file = self.create_simple_tarball([("foo/", "")]) | ||
253 | 135 | with standard_tarfile(backing_file) as tf: | ||
254 | 136 | self.assertEqual(0755, tf.getmember("foo").mode) | ||
255 | 137 | |||
256 | 138 | def test_create_dir_sets_linkname(self): | ||
257 | 139 | backing_file = self.create_simple_tarball([("foo/", "")]) | ||
258 | 140 | with standard_tarfile(backing_file) as tf: | ||
259 | 141 | self.assertEqual('', tf.getmember("foo").linkname) | ||
260 | 142 | |||
261 | 143 | def test_create_dir_uses_default_mtime(self): | ||
262 | 144 | now = 126793 | ||
263 | 145 | backing_file = self.create_simple_tarball( | ||
264 | 146 | [("foo/", "")], default_mtime=now) | ||
265 | 147 | with standard_tarfile(backing_file) as tf: | ||
266 | 148 | self.assertEqual(now, tf.getmember("foo").mtime) | ||
267 | 149 | |||
268 | 150 | def test_create_dir_uses_default_uid(self): | ||
269 | 151 | uid = 1259 | ||
270 | 152 | backing_file = self.create_simple_tarball( | ||
271 | 153 | [("foo/", "")], default_uid=uid) | ||
272 | 154 | with standard_tarfile(backing_file) as tf: | ||
273 | 155 | self.assertEqual(uid, tf.getmember("foo").uid) | ||
274 | 156 | |||
275 | 157 | def test_create_dir_uses_default_gid(self): | ||
276 | 158 | gid = 2259 | ||
277 | 159 | backing_file = self.create_simple_tarball( | ||
278 | 160 | [("foo/", "")], default_gid=gid) | ||
279 | 161 | with standard_tarfile(backing_file) as tf: | ||
280 | 162 | self.assertEqual(gid, tf.getmember("foo").gid) | ||
281 | 163 | |||
282 | 164 | def test_create_dir_uses_default_uname(self): | ||
283 | 165 | uname = "someperson" | ||
284 | 166 | backing_file = self.create_simple_tarball( | ||
285 | 167 | [("foo/", "")], default_uname=uname) | ||
286 | 168 | with standard_tarfile(backing_file) as tf: | ||
287 | 169 | self.assertEqual(uname, tf.getmember("foo").uname) | ||
288 | 170 | |||
289 | 171 | def test_create_dir_uses_default_gname(self): | ||
290 | 172 | gname = "somegroup" | ||
291 | 173 | backing_file = self.create_simple_tarball( | ||
292 | 174 | [("foo/", "")], default_gname=gname) | ||
293 | 175 | with standard_tarfile(backing_file) as tf: | ||
294 | 176 | self.assertEqual(gname, tf.getmember("foo").gname) |
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.