Merge lp:~javier.collado/utah/bug1087620 into lp:utah
- bug1087620
- Merge into dev
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Max Brustkern | ||||
Approved revision: | 797 | ||||
Merged at revision: | 788 | ||||
Proposed branch: | lp:~javier.collado/utah/bug1087620 | ||||
Merge into: | lp:utah | ||||
Diff against target: |
274 lines (+83/-97) 2 files modified
utah/iso.py (+72/-61) utah/isotest/iso_static_validation.py (+11/-36) |
||||
To merge this branch: | bzr merge lp:~javier.collado/utah/bug1087620 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Max Brustkern (community) | Approve | ||
Javier Collado (community) | Needs Resubmitting | ||
Review via email: mp+138808@code.launchpad.net |
Commit message
Description of the change
This branch applies some refactoring to ISO.{getrealfil
Both metods were too complicated, so I decided to simplify them:
- `getrealfile` the handling of links didn't seem to be used anywhere.
- `extract` was inconsistent in its return time it returned the output
`subprocess.
After the changes, I've run the validation tests on ISOs for desktop/server for
i386/amd64 and they worked fine.
In addition to this, I've run the `pass.run` runlist, but I wasn't successful
with that because openssh-server wasn't installed correctly, but I'm not sure
if that was because of these changes. Please have a look at them and let me
know what do you think.
Javier Collado (javier.collado) wrote : | # |
I've tested again this branch and this time I haven't found any problem like
the last time.
ISO validation works for all images I've tested (server, desktop, i386, amd64)
and the `pass.run` runlist works fine for desktop i386, so this doesn't seem ot
break something that depends on `dump` or `extractall`.
The reason I'm not happy with the changes is because the extract `getrealfile`
method doesn't reproduce symbolic links as the old version used to do. Anyway,
I find the implementation is clearer now and it can be refactored later to be
more efficient with regard to hard/symbolic links.
- 785. By Javier Collado
-
Merged changes to fix duplicated test cases docstrings
Source branch: lp:~joetalbott/utah/fix_duplicate_docstrings
Max Brustkern (nuclearbob) wrote : | # |
I don't think the lack of reproduction of symbolic links is an issue for our current purposes. I mainly put that in before to try to save space.
The previous validation looked for anything in stderr when extracting the files. It looks like we're discarding that now. Should we be looking for errors, in stderr or in the return code of the extraction process? I think it might be nice in general to raise an exception if a file fails to extract.
At some point, a similar overhaul of the dump function (to return the contents of the file rather than a subprocess) would probably be nice as well, but that's not urgent.
- 786. By Max Brustkern
-
Added baremetal fixes
- 787. By Max Brustkern
-
Updating to version 0.6
Javier Collado (javier.collado) wrote : | # |
I've replaced calls to `subprocess.Popen` with calls to
`subprocess.
`subprocess.
friendlier error message.
Aside from that, I've also refactored `ISO.dump` to return just the file
contents (or raise an exception if the file cannot be extracted).
- 790. By Javier Collado
-
Added check to make sure that bsdtar list command returned a success code
`subprocess.
check_output` is used instead of `subprocess.Popen` - 791. By Javier Collado
-
Added check to make sure that bsdtar extract command returned a success code
`subprocess.
check_output` is used instead of `subprocess.Popen` - 792. By Javier Collado
-
Refactored `ISO.dump` to check bsdtar return code and return just output file
- 793. By Javier Collado
-
Use "extract" in the error message for the bsdtar extract command
- 794. By Javier Collado
-
Refactored `ISO.getrealfile` method
bsdtar error and output parsing error are handled in different try/except
clauses to provide better information in the log (parsing error message
contains the otuput from bsdtar) - 795. By Javier Collado
-
Updated `ISO.dump` docstring
- 796. By Javier Collado
-
Added seealso pointer in docstring
- 797. By Javier Collado
-
Added keyword arguments to subprocess.
check_output call
Javier Collado (javier.collado) wrote : | # |
Since there were a couple of merge conflicts in the diff, I've rebased my
changes and retested them.
Everything worked fine.
Max Brustkern (nuclearbob) wrote : | # |
Looks good to me.
Preview Diff
1 | === modified file 'utah/iso.py' | |||
2 | --- utah/iso.py 2012-12-08 02:10:12 +0000 | |||
3 | +++ utah/iso.py 2012-12-12 11:31:23 +0000 | |||
4 | @@ -79,7 +79,7 @@ | |||
5 | 79 | # Ubuntu 12.10 "Quantal Quetzal" - Alpha amd64 (20120820) | 79 | # Ubuntu 12.10 "Quantal Quetzal" - Alpha amd64 (20120820) |
6 | 80 | # i.e. Ubuntu Version (LTS) "Series Codename" - Alpha/Beta/Release | 80 | # i.e. Ubuntu Version (LTS) "Series Codename" - Alpha/Beta/Release |
7 | 81 | # arch (buildnumber) | 81 | # arch (buildnumber) |
9 | 82 | self.media_info = self.dump(self.infofile).communicate()[0] | 82 | self.media_info = self.dump(self.infofile) |
10 | 83 | self.arch = self.getarch() | 83 | self.arch = self.getarch() |
11 | 84 | self.series = self.getseries() | 84 | self.series = self.getseries() |
12 | 85 | self.buildnumber = self.getbuildnumber() | 85 | self.buildnumber = self.getbuildnumber() |
13 | @@ -107,8 +107,8 @@ | |||
14 | 107 | self.logger.addHandler(self.filehandler) | 107 | self.logger.addHandler(self.filehandler) |
15 | 108 | if config.debuglog is not None: | 108 | if config.debuglog is not None: |
16 | 109 | self.logger.setLevel(logging.DEBUG) | 109 | self.logger.setLevel(logging.DEBUG) |
19 | 110 | self.debughandler = ( | 110 | self.debughandler = (logging.handlers |
20 | 111 | logging.handlers.WatchedFileHandler(config.debuglog)) | 111 | .WatchedFileHandler(config.debuglog)) |
21 | 112 | self.debughandler.setFormatter(file_formatter) | 112 | self.debughandler.setFormatter(file_formatter) |
22 | 113 | self.debughandler.setLevel(logging.DEBUG) | 113 | self.debughandler.setLevel(logging.DEBUG) |
23 | 114 | self.logger.addHandler(self.debughandler) | 114 | self.logger.addHandler(self.debughandler) |
24 | @@ -183,77 +183,88 @@ | |||
25 | 183 | return subprocess.Popen(cmd, stdout=subprocess.PIPE, | 183 | return subprocess.Popen(cmd, stdout=subprocess.PIPE, |
26 | 184 | stderr=subprocess.PIPE).communicate() | 184 | stderr=subprocess.PIPE).communicate() |
27 | 185 | 185 | ||
31 | 186 | def getrealfile(self, filename, outdir=None): | 186 | def getrealfile(self, filename): |
32 | 187 | """ | 187 | """Return a command to safely extract a file from an ISO. |
33 | 188 | Return a command to safely extract a file from an ISO. | 188 | |
34 | 189 | Based on unbsdtar-safelink from ubuntu ISO testing. | 189 | Based on unbsdtar-safelink from ubuntu ISO testing. |
35 | 190 | |||
36 | 191 | :params filename: Path to the file in the ISO to be extracted | ||
37 | 192 | :type filename: str | ||
38 | 193 | :returns: Command that can be passed to a subprocess method | ||
39 | 194 | :rtype: list | ||
40 | 195 | |||
41 | 190 | """ | 196 | """ |
42 | 191 | cmd = ['bsdtar', '-t', '-v', '-f', self.image, filename] | 197 | cmd = ['bsdtar', '-t', '-v', '-f', self.image, filename] |
43 | 192 | self.logger.debug('bsdtar list command: ' + ' '.join(cmd)) | 198 | self.logger.debug('bsdtar list command: ' + ' '.join(cmd)) |
44 | 193 | output = (subprocess.Popen(cmd, stdout=subprocess.PIPE) | ||
45 | 194 | .communicate()[0].split('\n')[0].split()) | ||
46 | 195 | try: | 199 | try: |
62 | 196 | if output[0].startswith('l'): | 200 | output = subprocess.check_output(cmd) |
63 | 197 | if outdir is None: | 201 | except subprocess.CalledProcessError: |
64 | 198 | cmd = ['ln', '-s', output[-1], output[-3]] | 202 | raise UTAHException('Cannot list {path} in {image}' |
50 | 199 | else: | ||
51 | 200 | cmd = ['ln', '-s', output[-1], | ||
52 | 201 | os.path.join(outdir, output[-3])] | ||
53 | 202 | else: | ||
54 | 203 | realfile = output[-1] | ||
55 | 204 | if outdir is None: | ||
56 | 205 | cmd = ['bsdtar', '-x', '-f', self.image, '-O', realfile] | ||
57 | 206 | else: | ||
58 | 207 | cmd = ['bsdtar', '-x', '-f', self.image, '-C', outdir, | ||
59 | 208 | '-O', realfile] | ||
60 | 209 | except IndexError: | ||
61 | 210 | raise UTAHException('Cannot unpack {path} from {image}' | ||
65 | 211 | .format(path=filename, image=self.image)) | 203 | .format(path=filename, image=self.image)) |
66 | 204 | try: | ||
67 | 205 | columns = output.splitlines()[0].split() | ||
68 | 206 | realfile = columns[-1] | ||
69 | 207 | except (IndexError, subprocess.CalledProcessError): | ||
70 | 208 | raise UTAHException('Cannot parse bsdtar list output ' | ||
71 | 209 | 'for {path} in {image}: {output}' | ||
72 | 210 | .format(path=filename, image=self.image, | ||
73 | 211 | output=output)) | ||
74 | 212 | cmd = ['bsdtar', '-x', '-f', self.image, '-O', realfile] | ||
75 | 212 | self.logger.debug('bsdtar extract command: ' + ' '.join(cmd)) | 213 | self.logger.debug('bsdtar extract command: ' + ' '.join(cmd)) |
76 | 213 | return cmd | 214 | return cmd |
77 | 214 | 215 | ||
89 | 215 | def extract(self, filename, outdir=None, outfile=None, **kw): | 216 | def extract(self, filename, outdir='', outfile=None, **kw): |
90 | 216 | """ | 217 | """Extract file from an ISO. |
91 | 217 | Return a subprocess to extract a file from an ISO, passing any | 218 | |
92 | 218 | additional keyword arguments to the subprocess. | 219 | :param filename: Path to the file in the ISO to be extracted |
93 | 219 | If outfile is None, extract to standard out. | 220 | :type filename: str |
94 | 220 | If outdir is present and outfile is None, extract the file to outdir | 221 | :param outdir: Destination directory |
95 | 221 | with the same name. | 222 | :type outdir: str |
96 | 222 | If outdir and outfile are both present, extract to outfile in outdir. | 223 | :param outfile: Destination filename |
97 | 223 | If the file is a hardlink, extract the actual file. | 224 | :type outfile: str |
98 | 224 | If the file is a symlink, return a command to create the symlink. | 225 | :returns: Path to the extracted file |
99 | 225 | """ | 226 | :rtype: str |
100 | 227 | |||
101 | 228 | .. seealso:: :meth:`getrealfile` | ||
102 | 229 | |||
103 | 230 | """ | ||
104 | 231 | cmd = self.getrealfile(filename) | ||
105 | 232 | try: | ||
106 | 233 | stdout = subprocess.check_output(cmd, **kw) | ||
107 | 234 | except subprocess.CalledProcessError: | ||
108 | 235 | raise UTAHException('Cannot extract {path} from {image}' | ||
109 | 236 | .format(path=filename, image=self.image)) | ||
110 | 237 | |||
111 | 226 | if outfile is None: | 238 | if outfile is None: |
134 | 227 | if outdir is None: | 239 | outfile = os.path.join(outdir, filename) |
135 | 228 | cmd = self.getrealfile(filename) | 240 | |
136 | 229 | if cmd[-1] != filename: | 241 | dirname = os.path.dirname(outfile) |
137 | 230 | self.logger.debug(filename + ' appears to be a link to ' | 242 | if not os.path.isdir(dirname): |
138 | 231 | + cmd[-1]) | 243 | os.makedirs(dirname) |
139 | 232 | dirname = os.path.dirname(filename) | 244 | with open(outfile, 'w') as fp: |
140 | 233 | if not os.path.isdir(dirname): | 245 | fp.write(stdout) |
141 | 234 | os.makedirs(dirname) | 246 | |
142 | 235 | os.chmod(dirname, 0775) | 247 | return outfile |
121 | 236 | if os.path.isfile(filename): | ||
122 | 237 | os.chmod(filename, 0775) | ||
123 | 238 | return subprocess.call(cmd, stdout=open(filename, 'w'), | ||
124 | 239 | **kw) | ||
125 | 240 | else: | ||
126 | 241 | cmd = self.getrealfile(filename, outdir=outdir) | ||
127 | 242 | if '-O' in cmd: | ||
128 | 243 | cmd.remove('-O') | ||
129 | 244 | return subprocess.Popen(cmd, stdout=subprocess.PIPE, | ||
130 | 245 | stderr=subprocess.PIPE, **kw).communicate() | ||
131 | 246 | else: | ||
132 | 247 | cmd = self.getrealfile(filename) | ||
133 | 248 | return subprocess.call(cmd, stdout=open(outfile, 'w'), **kw) | ||
143 | 249 | 248 | ||
144 | 250 | def dump(self, filename, **kw): | 249 | def dump(self, filename, **kw): |
148 | 251 | """ | 250 | """Extract file contents from an ISO. |
149 | 252 | Return a subprocess dumping a file in an ISO to standard out, passing | 251 | |
150 | 253 | any additional keyword arguments to the subprocess. | 252 | :param filename: Name of the file to be extracted |
151 | 253 | :type filename: str | ||
152 | 254 | :returns: Contents of the file | ||
153 | 255 | :rtype: str | ||
154 | 256 | |||
155 | 257 | .. seealso:: :meth:`getrealfile` | ||
156 | 258 | |||
157 | 254 | """ | 259 | """ |
158 | 255 | cmd = self.getrealfile(filename) | 260 | cmd = self.getrealfile(filename) |
160 | 256 | return subprocess.Popen(cmd, stdout=subprocess.PIPE, **kw) | 261 | try: |
161 | 262 | stdout = subprocess.check_output(cmd, **kw) | ||
162 | 263 | except subprocess.CalledProcessError: | ||
163 | 264 | raise UTAHException('Cannot extract {path} from {image}' | ||
164 | 265 | .format(path=filename, image=self.image)) | ||
165 | 266 | |||
166 | 267 | return stdout | ||
167 | 257 | 268 | ||
168 | 258 | def extractall(self): | 269 | def extractall(self): |
169 | 259 | """ | 270 | """ |
170 | @@ -383,7 +394,7 @@ | |||
171 | 383 | 'boot/grub/grub.cfg', 'boot/grub/loopback.cfg'] | 394 | 'boot/grub/grub.cfg', 'boot/grub/loopback.cfg'] |
172 | 384 | for cfgfile in cfgfiles: | 395 | for cfgfile in cfgfiles: |
173 | 385 | if cfgfile in self.listfiles(returnlist=True): | 396 | if cfgfile in self.listfiles(returnlist=True): |
175 | 386 | stdout = self.dump(cfgfile).communicate()[0] | 397 | stdout = self.dump(cfgfile) |
176 | 387 | for line in stdout.splitlines(): | 398 | for line in stdout.splitlines(): |
177 | 388 | fragments = line.split() | 399 | fragments = line.split() |
178 | 389 | if (len(fragments) >= 2 and | 400 | if (len(fragments) >= 2 and |
179 | 390 | 401 | ||
180 | === modified file 'utah/isotest/iso_static_validation.py' | |||
181 | --- utah/isotest/iso_static_validation.py 2012-12-10 12:09:29 +0000 | |||
182 | +++ utah/isotest/iso_static_validation.py 2012-12-12 11:31:23 +0000 | |||
183 | @@ -218,15 +218,9 @@ | |||
184 | 218 | self.fail("Failed to fetch manifest file from the server") | 218 | self.fail("Failed to fetch manifest file from the server") |
185 | 219 | 219 | ||
186 | 220 | logging.debug('Extracting manifest from the iso') | 220 | logging.debug('Extracting manifest from the iso') |
194 | 221 | (stdout, stderr) = self.iso.extract('casper/filesystem.manifest', | 221 | manifest_path = self.iso.extract('casper/filesystem.manifest', |
195 | 222 | self.temp_dir) | 222 | self.temp_dir) |
189 | 223 | logging.debug('Check for error in extracting manifest info from iso') | ||
190 | 224 | self.assertEqual(stderr, '') | ||
191 | 225 | |||
192 | 226 | manifest_path = os.path.join(self.temp_dir, | ||
193 | 227 | 'casper/filesystem.manifest') | ||
196 | 228 | manifest_local = open(manifest_path) | 223 | manifest_local = open(manifest_path) |
197 | 229 | |||
198 | 230 | for manifest_entry in manifest: | 224 | for manifest_entry in manifest: |
199 | 231 | logging.debug('Checking manifest are the same in iso and repo') | 225 | logging.debug('Checking manifest are the same in iso and repo') |
200 | 232 | self.assertIn(manifest_entry, manifest_local) | 226 | self.assertIn(manifest_entry, manifest_local) |
201 | @@ -235,11 +229,7 @@ | |||
202 | 235 | # the same as that given in distro info | 229 | # the same as that given in distro info |
203 | 236 | def test_build_id(self): | 230 | def test_build_id(self): |
204 | 237 | logging.debug('Extract distro information of the image from the iso') | 231 | logging.debug('Extract distro information of the image from the iso') |
210 | 238 | (stdout, stderr) = self.iso.extract("./.disk/info", self.temp_dir) | 232 | buildid_path = self.iso.extract("./.disk/info", self.temp_dir) |
206 | 239 | logging.debug('Checking for error in extracting distro info from iso') | ||
207 | 240 | self.assertEqual(stderr, '') | ||
208 | 241 | |||
209 | 242 | buildid_path = os.path.join(self.temp_dir, './.disk/info') | ||
211 | 243 | buildinfo = open(buildid_path) | 233 | buildinfo = open(buildid_path) |
212 | 244 | 234 | ||
213 | 245 | try: | 235 | try: |
214 | @@ -266,12 +256,7 @@ | |||
215 | 266 | "Skipping images that aren't ubiquity-based.") | 256 | "Skipping images that aren't ubiquity-based.") |
216 | 267 | def test_wubi(self): | 257 | def test_wubi(self): |
217 | 268 | logging.debug('Extracting wubi from the desktop images') | 258 | logging.debug('Extracting wubi from the desktop images') |
224 | 269 | (stdout, stderr) = self.iso.extract('./wubi.exe', self.temp_dir) | 259 | wubi_path = self.iso.extract('./wubi.exe', self.temp_dir) |
219 | 270 | logging.debug('Check for error in extracting wubi.exe from the iso') | ||
220 | 271 | self.assertEqual(stderr, '') | ||
221 | 272 | |||
222 | 273 | wubi_path = os.path.join(self.temp_dir, './wubi.exe') | ||
223 | 274 | |||
225 | 275 | cmd = ["file", wubi_path] | 260 | cmd = ["file", wubi_path] |
226 | 276 | output = subprocess.Popen(cmd, stdout=subprocess.PIPE, | 261 | output = subprocess.Popen(cmd, stdout=subprocess.PIPE, |
227 | 277 | stderr=subprocess.PIPE) | 262 | stderr=subprocess.PIPE) |
228 | @@ -346,40 +331,30 @@ | |||
229 | 346 | @unittest.skipUnless(st_arch != 'powerpc', | 331 | @unittest.skipUnless(st_arch != 'powerpc', |
230 | 347 | "vmlinuz is not present only for powerpc images") | 332 | "vmlinuz is not present only for powerpc images") |
231 | 348 | def test_vmlinuz(self): | 333 | def test_vmlinuz(self): |
232 | 334 | logging.debug('Extract kernel image from the iso') | ||
233 | 349 | path = iso.kernelpath() | 335 | path = iso.kernelpath() |
241 | 350 | (stdout, stderr) = self.iso.extract(path, self.temp_dir) | 336 | vmlinuz_path = self.iso.extract(path, self.temp_dir) |
242 | 351 | logging.debug('Asserting that vmlinuz is present in the iso') | 337 | cmd = ['file', vmlinuz_path] |
236 | 352 | self.assertEqual(stderr, '') | ||
237 | 353 | |||
238 | 354 | vmlinuz_path = os.path.join(self.temp_dir, path) | ||
239 | 355 | |||
240 | 356 | cmd = ["file", vmlinuz_path] | ||
243 | 357 | output = subprocess.Popen(cmd, stdout=subprocess.PIPE, | 338 | output = subprocess.Popen(cmd, stdout=subprocess.PIPE, |
244 | 358 | stderr=subprocess.PIPE) | 339 | stderr=subprocess.PIPE) |
245 | 359 | (stdout, stderr) = output.communicate() | 340 | (stdout, stderr) = output.communicate() |
246 | 360 | logging.debug('Check if vmlinuz present in the iso is a Linux kernel') | 341 | logging.debug('Check if vmlinuz present in the iso is a Linux kernel') |
247 | 361 | self.assertEqual(stderr, '') | 342 | self.assertEqual(stderr, '') |
249 | 362 | self.assertIn("Linux kernel", stdout) | 343 | self.assertIn('Linux kernel', stdout) |
250 | 363 | 344 | ||
251 | 364 | # Test if filesystem.squashfs is present and | 345 | # Test if filesystem.squashfs is present and |
252 | 365 | # if that is a squash fs for ubiquity based images | 346 | # if that is a squash fs for ubiquity based images |
253 | 366 | @unittest.skipUnless(ubiquity_image, "Skipping for non ubiquity images") | 347 | @unittest.skipUnless(ubiquity_image, "Skipping for non ubiquity images") |
254 | 367 | def test_filesystem_squashfs(self): | 348 | def test_filesystem_squashfs(self): |
255 | 368 | logging.debug('Extracting the filesystem.squashfs') | 349 | logging.debug('Extracting the filesystem.squashfs') |
264 | 369 | (stdout, stderr) = self.iso.extract('casper/filesystem.squashfs', | 350 | squashfs_path = self.iso.extract('casper/filesystem.squashfs', |
265 | 370 | self.temp_dir) | 351 | self.temp_dir) |
258 | 371 | logging.debug('Check for error in extracting filesystem.squashfs') | ||
259 | 372 | self.assertEqual(stderr, '') | ||
260 | 373 | |||
261 | 374 | squashfs_path = os.path.join(self.temp_dir, | ||
262 | 375 | 'casper/filesystem.squashfs') | ||
263 | 376 | |||
266 | 377 | logging.debug('Checking the file is an actual .squashfs') | 352 | logging.debug('Checking the file is an actual .squashfs') |
267 | 378 | cmd = ["file", squashfs_path] | 353 | cmd = ["file", squashfs_path] |
268 | 379 | output = subprocess.Popen(cmd, stdout=subprocess.PIPE, | 354 | output = subprocess.Popen(cmd, stdout=subprocess.PIPE, |
269 | 380 | stderr=subprocess.PIPE) | 355 | stderr=subprocess.PIPE) |
270 | 381 | (stdout, stderr) = output.communicate() | 356 | (stdout, stderr) = output.communicate() |
272 | 382 | logging.debug('Check if signature contains \"Squashfs filesystem\"') | 357 | logging.debug('Check if signature contains "Squashfs filesystem"') |
273 | 383 | self.assertEqual(stderr, '') | 358 | self.assertEqual(stderr, '') |
274 | 384 | self.assertIn("Squashfs filesystem", stdout) | 359 | self.assertIn("Squashfs filesystem", stdout) |
275 | 385 | 360 |
Rejecting this merge proposal for the reasons explained in: /bugs.launchpad .net/utah/ +bug/1087620/ comments/ 3
https:/