avocado-framework / avocado-vt

Avocado VT Plugin
https://avocado-vt.readthedocs.org/
Other
83 stars 241 forks source link

boottool.py requires six, but dep is not guaranteed on guest #1508

Open clebergnu opened 6 years ago

clebergnu commented 6 years ago

As part of the utilities to run Autotest tests on guests, Avocado-VT will copy Autotest libraries to the guest. Besides those stock Autotest librariies, it will copy a so-called "non-crippled" version of boottool.py:

https://github.com/avocado-framework/avocado-vt/blob/master/virttest/utils_test/__init__.py#L1322

The problem is that the version of boottool used is the one provided by Avocado-VT, which happens to now require six as part of the Python 3 port work. When boottool gets run (imported) on the guest, the following occurs:

2018-04-17 18:16:04,960 __init__         L1348 INFO | Running autotest control file iozone.control on guest, timeout 14400s
2018-04-17 18:16:04,961 __init__         L1363 INFO | ---------------- Test output ----------------
2018-04-17 18:16:04,961 client           L1042 DEBUG| Sending command: python -x ./autotest-local  --verbose control & wait ${!}
2018-04-17 18:16:05,064 client           L0780 INFO | [1] 718
2018-04-17 18:16:05,672 client           L0780 INFO | Traceback (most recent call last):
2018-04-17 18:16:05,673 client           L0780 INFO |   File "./autotest-local", line 10, in <module>
2018-04-17 18:16:05,673 client           L0780 INFO |     from autotest.client import autotest_local
2018-04-17 18:16:05,673 client           L0780 INFO |   File "/usr/local/autotest/autotest_local.py", line 21, in <module>
2018-04-17 18:16:05,674 client           L0780 INFO |     from autotest.client import job
2018-04-17 18:16:05,674 client           L0780 INFO |   File "/usr/local/autotest/job.py", line 26, in <module>
2018-04-17 18:16:05,674 client           L0780 INFO |     from autotest.client.shared import base_job, boottool, utils_memory
2018-04-17 18:16:05,674 client           L0780 INFO |   File "/usr/local/autotest/shared/boottool.py", line 19, in <module>
2018-04-17 18:16:05,674 client           L0780 INFO |     from autotest.client.tools.boottool import Grubby, install_grubby_if_necessary, EfiToolSys
2018-04-17 18:16:05,675 client           L0780 INFO |   File "/usr/local/autotest/tools/boottool.py", line 20, in <module>
2018-04-17 18:16:05,675 client           L0780 INFO |     import six
2018-04-17 18:16:05,675 client           L0780 INFO | ImportError: No module named six

Because six is not required, and usually present, on Avocado-VT guests.

The change to copy Avocado-VT's own version of boottool, was introduced in 121af4733e, and it was supposed to work around the use of RPM based installs of Autotest version 0.14.3. This is most probably no longer needed, and those changes should probably be reverted or at the very least be made optional.

clebergnu commented 6 years ago

On my system:

$ rpm -qf /usr/lib/python2.7/site-packages/autotest/client/tools/boottool.py
autotest-framework-0.16.2-6.fc26.noarch

And the difference from Avocado-VT's own version:

$ cd ~/src/avocado/avocado-vt
$ git rev-parse HEAD 
8ddccbc456346354a3796b81e2c234a4f07604f7
$ diff -u /usr/lib/python2.7/site-packages/autotest/client/tools/boottool.py shared/deps/run_autotest/boottool.py > /tmp/boottool.diff

Are basically the ones to port it to Python 3:

--- /usr/lib/python2.7/site-packages/autotest/client/tools/boottool.py  2015-03-18 10:48:32.000000000 -0400
+++ shared/deps/run_autotest/boottool.py    2018-04-10 18:11:49.541448074 -0400
@@ -1,3 +1,4 @@
+#!/usr/bin/env python

 '''
 A boottool clone, but written in python and relying mostly on grubby[1].
@@ -11,12 +12,15 @@
 import optparse
 import logging
 import subprocess
-import urllib
 import tarfile
 import tempfile
 import shutil
 import struct

+import six
+from six.moves import urllib
+
+
 #
 # Get rid of DeprecationWarning messages on newer Python version while still
 # making it run on properly on Python 2.4
@@ -161,25 +165,25 @@
         '''
         Returns the variable name in a list ready for struct.pack()
         '''
-        l = []
+        normalized_name = []
         for i in range(512):
-            l.append(0)
+            normalized_name.append(0)

         for i in range(len(self.name)):
-            l[i] = ord(self.name[i])
-        return l
+            normalized_name[i] = ord(self.name[i])
+        return normalized_name

     def get_data(self):
         '''
         Returns the variable data in a list ready for struct.pack()
         '''
-        l = []
+        normalized_data = []
         for i in range(512):
-            l.append(0)
+            normalized_data.append(0)

         for i in range(len(self.data)):
-            l[i] = ord(self.data[i])
-        return l
+            normalized_data[i] = ord(self.data[i])
+        return normalized_data

     def get_packed(self):
         '''
@@ -408,7 +412,7 @@
         '''
         output = ''

-        for key, val in self.global_options_to_add.items():
+        for key, val in list(self.global_options_to_add.items()):
             output += self.keyval_to_line((key, val))

         eliloconf = open(self.path, 'r')
@@ -523,8 +527,9 @@
                                       stdin=subprocess.PIPE,
                                       stdout=subprocess.PIPE,
                                       stderr=subprocess.PIPE,
+                                      universal_newlines=True,
                                       close_fds=True).stdout.read()
-            if output != 'install ok installed':
+            if not output == 'install ok installed':
                 result = False
         return result

@@ -567,6 +572,7 @@
                                       stdin=subprocess.PIPE,
                                       stdout=subprocess.PIPE,
                                       stderr=subprocess.PIPE,
+                                      universal_newlines=True,
                                       close_fds=True).stdout.read()
             if not output.startswith(p):
                 result = False
@@ -594,9 +600,9 @@
         else:
             try:
                 args = ['zypper', '-n', '--no-cd', 'install'] + self.PKGS
-                result = subprocess.call(args,
-                                         stdout=subprocess.PIPE,
-                                         stderr=subprocess.PIPE)
+                subprocess.call(args,
+                                stdout=subprocess.PIPE,
+                                stderr=subprocess.PIPE)
             except OSError:
                 pass
         return self.check()
@@ -620,7 +626,8 @@
         '''
         Initializes a new dep installer, taking into account RHEL version
         '''
-        match = self.REDHAT_RELEASE_RE.match(open('/etc/redhat-release').read())
+        match = self.REDHAT_RELEASE_RE.match(
+            open('/etc/redhat-release').read())
         if match:
             major, minor = match.groups()
             if int(major) <= 5:
@@ -643,9 +650,9 @@
                 args += ['/usr/include/popt.h',
                          '/usr/include/blkid/blkid.h']

-                result = subprocess.call(args,
-                                         stdout=subprocess.PIPE,
-                                         stderr=subprocess.PIPE)
+                subprocess.call(args,
+                                stdout=subprocess.PIPE,
+                                stderr=subprocess.PIPE)
             except OSError:
                 pass
         return self.check()
@@ -728,7 +735,7 @@
         self.opts = opts
         self.log = logging.getLogger(self.__class__.__name__)

-        if os.environ.has_key('BOOTTOOL_DEBUG_RUN'):
+        if 'BOOTTOOL_DEBUG_RUN' in os.environ:
             self.debug_run = True
         else:
             self.debug_run = False
@@ -786,8 +793,9 @@
             result = subprocess.Popen(arguments, shell=False,
                                       stdin=subprocess.PIPE,
                                       stdout=subprocess.PIPE,
+                                      universal_newlines=True,
                                       close_fds=True).stdout.read()
-        except:
+        except Exception:
             pass

         if result is not None:
@@ -812,8 +820,9 @@
                                       stdin=subprocess.PIPE,
                                       stdout=subprocess.PIPE,
                                       stderr=subprocess.PIPE,
+                                      universal_newlines=True,
                                       close_fds=True).stdout.read()
-        except:
+        except Exception:
             pass

         if result is not None:
@@ -929,7 +938,7 @@
         '''
         Returns the indexes found in a get_info() output

-        :type info: list of lines
+        :type info: builtin.list
         :param info: result of utility method get_info()
         :return: maximum index number
         '''
@@ -972,7 +981,7 @@
         '''
         Filters info, looking for keys, optionally set with a given value

-        :type info: list of lines
+        :type info: builtin.list
         :param info: result of utility method get_info()
         :type key: string
         :param key: filter based on this key
@@ -1089,9 +1098,9 @@

         This is much simpler version then the original boottool version, that
         does not attempt to filter the result of the command / system call
-        that returns the archicture.
+        that returns the architecture.

-        :return: string with system archicteture, such as x86_64, ppc64, etc
+        :return: string with system architecture, such as x86_64, ppc64, etc
         '''
         return os.uname()[4]

@@ -1232,7 +1241,7 @@
                 with the title for the found entry, otherwise returns None
         """
         entries = self.get_entries()
-        for entry in entries.itervalues():
+        for entry in six.itervalues(entries):
             if entry.get('kernel') == path:
                 return entry['title']
         return None
@@ -1401,10 +1410,11 @@
         :param path: path to the binary that should be backed up
         '''
         backup_path = '%s.boottool.bkp' % path
-        if (os.path.exists(path) and not os.path.exists(backup_path)):
+        if (os.path.exists(path) and
+                not os.path.exists(backup_path)):
             try:
                 shutil.move(path, backup_path)
-            except:
+            except Exception:
                 self.log.warn('Failed to backup the current grubby binary')

     def grubby_install_fetch_tarball(self, topdir):
@@ -1417,21 +1427,20 @@
         try:
             tarball = tarball_name
             f = open(tarball)
-        except:
+        except Exception:
             try:
                 # then the autotest source directory
-                # pylint: disable=E0611
                 from autotest.client.shared.settings import settings
                 top_path = settings.get_value('COMMON', 'autotest_top_path')
                 tarball = os.path.join(top_path, tarball_name)
                 f = open(tarball)
-            except:
+            except Exception:
                 # then try to grab it from github
                 try:
                     tarball = os.path.join(topdir, tarball_name)
-                    urllib.urlretrieve(GRUBBY_TARBALL_URI, tarball)
+                    urllib.request.urlretrieve(GRUBBY_TARBALL_URI, tarball)
                     f = open(tarball)
-                except:
+                except Exception:
                     return None

         tarball_md5 = md5.md5(f.read()).hexdigest()
@@ -1454,15 +1463,14 @@
             self.log.debug('No popt.h header present, skipping build')
             return False

-        tarball_name = os.path.basename(tarball)
-
         srcdir = os.path.join(topdir, 'src')
         srcdir = self._extract_tarball(tarball, srcdir)
         os.chdir(srcdir)
         self.grubby_install_patch_makefile()
         result = subprocess.Popen(['make'],
                                   stdout=subprocess.PIPE,
-                                  stderr=subprocess.PIPE)
+                                  stderr=subprocess.PIPE,
+                                  universal_newlines=True)
         if result.wait() != 0:
             self.log.debug('Failed to build grubby during "make" step')
             log_lines(result.stderr.read().splitlines())
@@ -1472,7 +1480,8 @@
         os.environ['DESTDIR'] = install_root
         result = subprocess.Popen(['make', 'install'],
                                   stdout=subprocess.PIPE,
-                                  stderr=subprocess.PIPE)
+                                  stderr=subprocess.PIPE,
+                                  universal_newlines=True)
         if result.wait() != 0:
             self.log.debug('Failed to build grubby during "make install" step')
             log_lines(result.stderr.read().splitlines())
@@ -1514,7 +1523,6 @@
         if tarball is None:
             raise GrubbyInstallException('Failed to fetch grubby tarball')

-        srcdir = os.path.join(topdir, 'src')
         install_root = os.path.join(topdir, 'install_root')
         os.mkdir(install_root)

@@ -1530,7 +1538,7 @@
                                          'binary to directory "%s"' % inst_dir)
         try:
             shutil.copy(grubby_bin, path)
-        except:
+        except Exception:
             raise GrubbyInstallException('Failed to copy grubby binary to '
                                          'directory "%s"' % inst_dir)

@@ -1599,7 +1607,8 @@
             p = subprocess.Popen([grub_binary, '--batch'],
                                  stdin=subprocess.PIPE,
                                  stdout=subprocess.PIPE,
-                                 stderr=subprocess.PIPE)
+                                 stderr=subprocess.PIPE,
+                                 universal_newlines=True)
             out, err = p.communicate(grub_instructions_text)

             complete_out = ''
@@ -1664,7 +1673,8 @@
             return -1

         # Make sure the "set default" entry in the configuration file is set
-        # to "${saved_entry}. Assuming the config file is at /boot/grub/grub.cfg
+        # to "${saved_entry}. Assuming the config file is at
+        # /boot/grub/grub.cfg
         deb_grub_cfg_path = '/boot/grub/grub.cfg'
         deb_grub_cfg_bkp_path = '%s.boottool.bak' % deb_grub_cfg_path

@@ -1698,8 +1708,9 @@
             prev_saved_return = self._run_get_return([grub_set_default_exec,
                                                       '%s' % default_index])
             if prev_saved_return != 0:
-                self.log.error('Could not make entry %s the previous saved entry',
-                               default_index)
+                self.log.error(
+                    'Could not make entry %s the previous saved entry',
+                    default_index)
                 return prev_saved_return

         # Finally set the boot once entry
@@ -1926,7 +1937,7 @@
         action = self.opts_get_action(opts)
         if action in ACTIONS_REQUIRE_TITLE:
             if opts.title is None:
-                print 'Action %s requires a --title parameter' % action
+                print('Action %s requires a --title parameter' % action)
                 raise SystemExit

         return (opts, args)
@@ -1971,7 +1982,7 @@
         if level > max_level:
             level = max_level

-        if os.environ.has_key('BOOTTOOL_DEBUG_RUN'):
+        if 'BOOTTOOL_DEBUG_RUN' in os.environ:
             logging_level = logging.DEBUG
         else:
             logging_level = log_map.get(level)
@@ -1994,7 +2005,7 @@
             self.log.debug('Forcing bootloader "%s"', self.opts.bootloader)
             try:
                 self.grubby._set_bootloader(self.opts.bootloader)
-            except ValueError, msg:
+            except ValueError as msg:
                 self.log.error(msg)
                 sys.exit(-1)

@@ -2015,7 +2026,7 @@
             if result is None:
                 result = 0
             elif isinstance(result, str):
-                print result
+                print(result)
                 result = 0
             sys.exit(result)

@@ -2031,12 +2042,12 @@
         '''
         version = self.grubby.get_grubby_version()
         if version is not None:
-            print "%s.%s" % version
+            print("%s.%s" % version)
             return

         version = self.grubby.get_grubby_version_raw()
         if version is not None:
-            print version
+            print(version)

     def action_grubby_version_check(self):
         '''
@@ -2092,15 +2103,15 @@
         else:
             entries = {info_index: self.grubby.get_entry(info_index)}

-        for index, entry in entries.items():
-            print
-            for key, val in entry.items():
+        for index, entry in list(entries.items()):
+            print()
+            for key, val in list(entry.items()):
                 # remove quotes
                 if isinstance(val, str):
                     if val.startswith('"') and val.endswith('"'):
                         val = val[1:-1]

-                print '%-8s: %s' % (key, val)
+                print('%-8s: %s' % (key, val))

     def action_add_kernel(self):
         '''
@@ -2177,7 +2188,7 @@
         """
         Get the default entry index
         """
-        print self.grubby.get_default_index()
+        print(self.grubby.get_default_index())

     def action_set_default(self):
         """