Merge lp:~javier.collado/utah/bug1087620 into lp:utah

[Javier Collado]

This branch applies some refactoring to ISO.{getrealfile,extract} methods.

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.Popen` or the one from `subprocess.call`.

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)]

Rejecting this merge proposal for the reasons explained in:
https://bugs.launchpad.net/utah/+bug/1087620/comments/3

review: Disapprove

[Javier Collado (javier.collado)]

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.

review: Needs Resubmitting

[lp:~javier.collado/utah/bug1087620]

785. By Javier Collado on 2012-12-11

Merged changes to fix duplicated test cases docstrings

Source branch: lp:~joetalbott/utah/fix_duplicate_docstrings

[Max Brustkern (nuclearbob)]

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.

review: Needs Information

[lp:~javier.collado/utah/bug1087620]

786. By Max Brustkern on 2012-12-11

Added baremetal fixes

787. By Max Brustkern on 2012-12-11

Updating to version 0.6

[Javier Collado (javier.collado)]

I've replaced calls to `subprocess.Popen` with calls to
`subprocess.check_output` so that `bsdtar` return code is checked properly. The
`subprocess.CalledProcessError` exception will be caught an re-raised with a
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).

review: Needs Resubmitting

[lp:~javier.collado/utah/bug1087620]

790. By Javier Collado on 2012-12-12

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 on 2012-12-12

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 on 2012-12-12

Refactored `ISO.dump` to check bsdtar return code and return just output file

793. By Javier Collado on 2012-12-12

Use "extract" in the error message for the bsdtar extract command

794. By Javier Collado on 2012-12-12

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 on 2012-12-12

Updated `ISO.dump` docstring

796. By Javier Collado on 2012-12-12

Added seealso pointer in docstring

797. By Javier Collado on 2012-12-12

Added keyword arguments to subprocess.check_output call

[Javier Collado (javier.collado)]

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)]

Looks good to me.

review: Approve

=== modified file 'utah/iso.py'
--- utah/iso.py	2012-12-08 02:10:12 +0000
+++ utah/iso.py	2012-12-12 11:31:23 +0000
@@ -79,7 +79,7 @@
         # Ubuntu 12.10 "Quantal Quetzal" - Alpha amd64 (20120820)
         # i.e. Ubuntu Version (LTS) "Series Codename" - Alpha/Beta/Release
         # arch (buildnumber)
-        self.media_info = self.dump(self.infofile).communicate()[0]
+        self.media_info = self.dump(self.infofile)
         self.arch = self.getarch()
         self.series = self.getseries()
         self.buildnumber = self.getbuildnumber()
@@ -107,8 +107,8 @@
         self.logger.addHandler(self.filehandler)
         if config.debuglog is not None:
             self.logger.setLevel(logging.DEBUG)
-            self.debughandler = (
-                logging.handlers.WatchedFileHandler(config.debuglog))
+            self.debughandler = (logging.handlers
+                                 .WatchedFileHandler(config.debuglog))
             self.debughandler.setFormatter(file_formatter)
             self.debughandler.setLevel(logging.DEBUG)
             self.logger.addHandler(self.debughandler)
@@ -183,77 +183,88 @@
             return subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                     stderr=subprocess.PIPE).communicate()
 
-    def getrealfile(self, filename, outdir=None):
-        """
-        Return a command to safely extract a file from an ISO.
+    def getrealfile(self, filename):
+        """Return a command to safely extract a file from an ISO.
+
         Based on unbsdtar-safelink from ubuntu ISO testing.
+
+        :params filename: Path to the file in the ISO to be extracted
+        :type filename: str
+        :returns: Command that can be passed to a subprocess method
+        :rtype: list
+
         """
         cmd = ['bsdtar', '-t', '-v', '-f', self.image, filename]
         self.logger.debug('bsdtar list command: ' + ' '.join(cmd))
-        output = (subprocess.Popen(cmd, stdout=subprocess.PIPE)
-                  .communicate()[0].split('\n')[0].split())
         try:
-            if output[0].startswith('l'):
-                if outdir is None:
-                    cmd = ['ln', '-s', output[-1], output[-3]]
-                else:
-                    cmd = ['ln', '-s', output[-1],
-                           os.path.join(outdir, output[-3])]
-            else:
-                realfile = output[-1]
-                if outdir is None:
-                    cmd = ['bsdtar', '-x', '-f', self.image, '-O', realfile]
-                else:
-                    cmd = ['bsdtar', '-x', '-f', self.image, '-C', outdir,
-                           '-O', realfile]
-        except IndexError:
-            raise UTAHException('Cannot unpack {path} from {image}'
+            output = subprocess.check_output(cmd)
+        except subprocess.CalledProcessError:
+            raise UTAHException('Cannot list {path} in {image}'
                                 .format(path=filename, image=self.image))
+        try:
+            columns = output.splitlines()[0].split()
+            realfile = columns[-1]
+        except (IndexError, subprocess.CalledProcessError):
+            raise UTAHException('Cannot parse bsdtar list output '
+                                'for {path} in {image}: {output}'
+                                .format(path=filename, image=self.image,
+                                        output=output))
+        cmd = ['bsdtar', '-x', '-f', self.image, '-O', realfile]
         self.logger.debug('bsdtar extract command: ' + ' '.join(cmd))
         return cmd
 
-    def extract(self, filename, outdir=None, outfile=None, **kw):
-        """
-        Return a subprocess to extract a file from an ISO, passing any
-        additional keyword arguments to the subprocess.
-        If outfile is None, extract to standard out.
-        If outdir is present and outfile is None, extract the file to outdir
-        with the same name.
-        If outdir and outfile are both present, extract to outfile in outdir.
-        If the file is a hardlink, extract the actual file.
-        If the file is a symlink, return a command to create the symlink.
-        """
+    def extract(self, filename, outdir='', outfile=None, **kw):
+        """Extract file from an ISO.
+
+        :param filename: Path to the file in the ISO to be extracted
+        :type filename: str
+        :param outdir: Destination directory
+        :type outdir: str
+        :param outfile: Destination filename
+        :type outfile: str
+        :returns: Path to the extracted file
+        :rtype: str
+
+        .. seealso:: :meth:`getrealfile`
+
+        """
+        cmd = self.getrealfile(filename)
+        try:
+            stdout = subprocess.check_output(cmd, **kw)
+        except subprocess.CalledProcessError:
+            raise UTAHException('Cannot extract {path} from {image}'
+                                .format(path=filename, image=self.image))
+
         if outfile is None:
-            if outdir is None:
-                cmd = self.getrealfile(filename)
-                if cmd[-1] != filename:
-                    self.logger.debug(filename + ' appears to be a link to '
-                                      + cmd[-1])
-                    dirname = os.path.dirname(filename)
-                    if not os.path.isdir(dirname):
-                        os.makedirs(dirname)
-                    os.chmod(dirname, 0775)
-                    if os.path.isfile(filename):
-                        os.chmod(filename, 0775)
-                    return subprocess.call(cmd, stdout=open(filename, 'w'),
-                                           **kw)
-            else:
-                cmd = self.getrealfile(filename, outdir=outdir)
-            if '-O' in cmd:
-                cmd.remove('-O')
-            return subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                                    stderr=subprocess.PIPE, **kw).communicate()
-        else:
-            cmd = self.getrealfile(filename)
-            return subprocess.call(cmd, stdout=open(outfile, 'w'), **kw)
+            outfile = os.path.join(outdir, filename)
+
+        dirname = os.path.dirname(outfile)
+        if not os.path.isdir(dirname):
+            os.makedirs(dirname)
+        with open(outfile, 'w') as fp:
+            fp.write(stdout)
+
+        return outfile
 
     def dump(self, filename, **kw):
-        """
-        Return a subprocess dumping a file in an ISO to standard out, passing
-        any additional keyword arguments to the subprocess.
+        """Extract file contents from an ISO.
+
+        :param filename: Name of the file to be extracted
+        :type filename: str
+        :returns: Contents of the file
+        :rtype: str
+
+        .. seealso:: :meth:`getrealfile`
+
         """
         cmd = self.getrealfile(filename)
-        return subprocess.Popen(cmd, stdout=subprocess.PIPE, **kw)
+        try:
+            stdout = subprocess.check_output(cmd, **kw)
+        except subprocess.CalledProcessError:
+            raise UTAHException('Cannot extract {path} from {image}'
+                                .format(path=filename, image=self.image))
+
+        return stdout
 
     def extractall(self):
         """
@@ -383,7 +394,7 @@
                         'boot/grub/grub.cfg', 'boot/grub/loopback.cfg']
             for cfgfile in cfgfiles:
                 if cfgfile in self.listfiles(returnlist=True):
-                    stdout = self.dump(cfgfile).communicate()[0]
+                    stdout = self.dump(cfgfile)
                     for line in stdout.splitlines():
                         fragments = line.split()
                         if (len(fragments) >= 2 and

=== modified file 'utah/isotest/iso_static_validation.py'
--- utah/isotest/iso_static_validation.py	2012-12-10 12:09:29 +0000
+++ utah/isotest/iso_static_validation.py	2012-12-12 11:31:23 +0000
@@ -218,15 +218,9 @@
             self.fail("Failed to fetch manifest file from the server")
 
         logging.debug('Extracting manifest from the iso')
-        (stdout, stderr) = self.iso.extract('casper/filesystem.manifest',
-                                            self.temp_dir)
-        logging.debug('Check for error in extracting manifest info from iso')
-        self.assertEqual(stderr, '')
-
-        manifest_path = os.path.join(self.temp_dir,
-                                     'casper/filesystem.manifest')
+        manifest_path = self.iso.extract('casper/filesystem.manifest',
+                                         self.temp_dir)
         manifest_local = open(manifest_path)
-
         for manifest_entry in manifest:
             logging.debug('Checking manifest are the same in iso and repo')
             self.assertIn(manifest_entry, manifest_local)
@@ -235,11 +229,7 @@
     # the same as that given in distro info
     def test_build_id(self):
         logging.debug('Extract distro information of the image from the iso')
-        (stdout, stderr) = self.iso.extract("./.disk/info", self.temp_dir)
-        logging.debug('Checking for error in extracting distro info from iso')
-        self.assertEqual(stderr, '')
-
-        buildid_path = os.path.join(self.temp_dir, './.disk/info')
+        buildid_path = self.iso.extract("./.disk/info", self.temp_dir)
         buildinfo = open(buildid_path)
 
         try:
@@ -266,12 +256,7 @@
                          "Skipping images that aren't ubiquity-based.")
     def test_wubi(self):
         logging.debug('Extracting wubi from the desktop images')
-        (stdout, stderr) = self.iso.extract('./wubi.exe', self.temp_dir)
-        logging.debug('Check for error in extracting wubi.exe from the iso')
-        self.assertEqual(stderr, '')
-
-        wubi_path = os.path.join(self.temp_dir, './wubi.exe')
-
+        wubi_path = self.iso.extract('./wubi.exe', self.temp_dir)
         cmd = ["file", wubi_path]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                   stderr=subprocess.PIPE)
@@ -346,40 +331,30 @@
     @unittest.skipUnless(st_arch != 'powerpc',
                          "vmlinuz is not present only for powerpc images")
     def test_vmlinuz(self):
+        logging.debug('Extract kernel image from the iso')
         path = iso.kernelpath()
-        (stdout, stderr) = self.iso.extract(path, self.temp_dir)
-        logging.debug('Asserting that vmlinuz is present in the iso')
-        self.assertEqual(stderr, '')
-
-        vmlinuz_path = os.path.join(self.temp_dir, path)
-
-        cmd = ["file", vmlinuz_path]
+        vmlinuz_path = self.iso.extract(path, self.temp_dir)
+        cmd = ['file', vmlinuz_path]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                   stderr=subprocess.PIPE)
         (stdout, stderr) = output.communicate()
         logging.debug('Check if vmlinuz present in the iso is a Linux kernel')
         self.assertEqual(stderr, '')
-        self.assertIn("Linux kernel", stdout)
+        self.assertIn('Linux kernel', stdout)
 
     # Test if filesystem.squashfs is present and
     # if that is a squash fs for ubiquity based images
     @unittest.skipUnless(ubiquity_image, "Skipping for non ubiquity images")
     def test_filesystem_squashfs(self):
         logging.debug('Extracting the filesystem.squashfs')
-        (stdout, stderr) = self.iso.extract('casper/filesystem.squashfs',
-                                            self.temp_dir)
-        logging.debug('Check for error in extracting filesystem.squashfs')
-        self.assertEqual(stderr, '')
-
-        squashfs_path = os.path.join(self.temp_dir,
-                                     'casper/filesystem.squashfs')
-
+        squashfs_path = self.iso.extract('casper/filesystem.squashfs',
+                                         self.temp_dir)
         logging.debug('Checking the file is an actual .squashfs')
         cmd = ["file", squashfs_path]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                   stderr=subprocess.PIPE)
         (stdout, stderr) = output.communicate()
-        logging.debug('Check if signature contains \"Squashfs filesystem\"')
+        logging.debug('Check if signature contains "Squashfs filesystem"')
         self.assertEqual(stderr, '')
         self.assertIn("Squashfs filesystem", stdout)