Re: [PATCH 16/16] Split ksdata execute and setup methods for addons

[Vratislav Podzimek]

On Thu, 2012-12-06 at 16:46 +0100, Martin Sivak wrote:
> pyanaconda/addons.py          | 47 ++++++++++++++++++++++++++++++++-----------
>  pyanaconda/install.py         |  5 ++++-
>  pyanaconda/kickstart.py       |  2 +-
>  pyanaconda/ui/gui/__init__.py | 10 ++++++---
>  4 files changed, 47 insertions(+), 17 deletions(-)
>
> diff --git a/pyanaconda/addons.py b/pyanaconda/addons.py
> index 0220107..c2510fa 100644
> --- a/pyanaconda/addons.py
> +++ b/pyanaconda/addons.py
> @@ -23,7 +23,8 @@ __all__ = ["AddonSection", "AddonRegistry", "AddonData", "collect_addon_paths"]
>
> import os
>  from pykickstart.sections import Section
>
> +from pykickstart.parser import KSOptionParser
>
> def collect_addon_paths(toplevel_addon_paths, ui_subdir = "gui"):
>      """This method looks into the directories present
>         in toplevel_addon_paths and registers each subdirectory
> @@ -39,13 +40,13 @@ def collect_addon_paths(toplevel_addon_paths, ui_subdir = "gui"):
>          "ks": [],
>          "categories": []
>          }
> for path in toplevel_addon_paths:
>        try:
>            files = os.listdir(path)
>        except OSError:
>            files = []
>    for addon_id in files:
>        addon_ks_path = os.path.join(path, addon_id, "ks")
>        if os.path.isdir(addon_ks_path):
>
> @@ -68,7 +69,7 @@ class AddonRegistry(object):
>     It acts as a proxy during kickstart save.
>  """
> def __init__(self, dictionary):
>        self.__dict__ = dictionary
>
> @@ -76,11 +77,20 @@ class AddonRegistry(object):
>          return reduce(lambda acc,(id, addon): acc + "%%addon %s\n%s%%end\n" % (id, str(addon)),
>                        self.__dict__.iteritems(), "")
> def execute(self, storage, ksdata, instClass):
> # pylint: disable-msg=C0103
> def execute(self, storage, ksdata, instClass, users):
>     """This method calls execute on all the registered addons."""
>     for k, v in self.__dict__.iteritems():
>        v.execute(storage, ksdata, instClass)
>        if hasattr(v, "execute"):
>            v.execute(storage, ksdata, instClass, users)

Isn't execute method mandatory? I guess you are trying to prevent
tracebacks, but we should probably check that much sooner, or provide

def execute(...):
    pass

in the generic Addon class.
> def setup(self, storage, ksdata, instClass):
>    """This method calls setup on all the registered addons."""
>    for k, v in self.__dict__.iteritems():
>        if hasattr(v, "setup"):
>            v.setup(storage, ksdata, instClass)
>
> class AddonData(object):
>      """This is a common parent class for loading and storing
>         3rd party data to kickstart. It is instantiated by
> @@ -96,7 +106,7 @@ class AddonData(object):
>         There is also a mandatory method execute, which should
>         make all the described changes to the installed system.
>      """
> def __init__(self, name):
>        self.name = name
>        self.content = ""
>
> @@ -104,8 +114,21 @@ class AddonData(object):
>      def __str__(self):
>          return self.content
> def execute(self, storage, ksdata, instClass):
>    """Make the changes to the underlying system."""
>
> # pylint: disable-msg=C0103
>
> def setup(self, storage, ksdata, instClass):
>    """Make the changes to the install system.
>       This method is called before the installation
>       is started and directly from spokes. It must be possible
>       to call it multiple times without breaking the environment."""
>    pass
>
> def execute(self, storage, ksdata, instClass, users):
>    """Make the changes to the underlying system.
>       This method is called only once in the post-install
>       setup phase.
>    """
>    pass
>
> def handle_line(self, line):
>
> @@ -133,10 +156,10 @@ class AddonSection(Section):
>          """Process the arguments to the %addon header."""
>          Section.handleHeader(self, lineno, args)
>          op = KSOptionParser(version=self.version)
>    (opts, extra) = op.parse_args(args=args[1:], lineno=lineno)
>    (_opts, extra) = op.parse_args(args=args[1:], lineno=lineno)

Why this   ^^^^^^ renaming?
>      self.addon_id = extra[0]
>
>      # if the addon is not registered, create dummy placeholder for it
>      if self.addon_id and not hasattr(self.handler.addon, self.addon_id):
>        setattr(self.handler.addon, self.addon_id, AnacondaKSAddon(self.addon_id))
>        setattr(self.handler.addon, self.addon_id, AddonData(self.addon_id))
>
> diff --git a/pyanaconda/install.py b/pyanaconda/install.py
> index 4e552ea..af214ba 100644
> --- a/pyanaconda/install.py
> +++ b/pyanaconda/install.py
> @@ -109,9 +109,12 @@ def doInstall(storage, payload, ksdata, instClass):
>      steps = len(storage.devicetree.findActions(type="create", object="format")) + \
>              len(storage.devicetree.findActions(type="resize", object="format")) + \
>              len(storage.devicetree.findActions(type="migrate", object="format"))
> steps += 4  # packages setup, packages, bootloader, post install
>
> steps += 5  # pre setup phase, packages setup, packages, bootloader, post install
> progress.send_init(steps)
>
> with progress_report(_("Setting up the install environment")):
>    ksdata.addon.setup(storage, ksdata, instClass)
>
> # Do partitioning.
>    payload.preStorage()
>    turnOnFilesystems(storage)
>
> diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py
> index 693ecd4..b242e92 100644
> --- a/pyanaconda/kickstart.py
> +++ b/pyanaconda/kickstart.py
> @@ -1432,7 +1432,7 @@ def preScriptPass(f):
>  def parseKickstart(f):
>      # preprocessing the kickstart file has already been handled in initramfs.
> handler = AnacondaKSHandler(constants.addon_paths)
> handler = AnacondaKSHandler(addon_paths)

Will this work? Are you importing addon_paths from the constants module
somewhere?
>  ksparser = AnacondaKSParser(handler)
>
>  # We need this so all the /dev/disk/* stuff is set up before parsing.
>
> diff --git a/pyanaconda/ui/gui/__init__.py b/pyanaconda/ui/gui/__init__.py
> index 2056307..1ffb728 100644
> --- a/pyanaconda/ui/gui/__init__.py
> +++ b/pyanaconda/ui/gui/__init__.py
> @@ -18,7 +18,7 @@
>  #
>  # Red Hat Author(s): Chris Lumens 
>  #
> -import importlib, inspect, os, sys, time
> +import inspect, os, sys, time
>  import meh.ui.gui
>
> from gi.repository import Gdk
> @@ -120,8 +120,12 @@ class GUIObject(common.UIObject):
>  def _findUIFile(self):
>      path = os.environ.get("UIPATH", "./:/tmp/updates/:/tmp/updates/ui/:/usr/share/anaconda/ui/")
>    # look for files in the same dir as where the class is defined
>    path += ":%s" % os.path.dirname(inspect.getfile(self.__class__))
>    
>    for d in path.split(":"):
>        testPath = os.path.normpath(d + self.uiFile)
>        testPath = os.path.normpath(os.path.join(d, self.uiFile))
>        if os.path.isfile(testPath) and os.access(testPath, os.R_OK):
>            return testPath
>
> @@ -265,7 +269,7 @@ class GraphicalUserInterface(UserInterface):
>          return [SummaryHub, ProgressHub]
>  def _is_standalone(self, obj):
>    """Is the spoke passes as obj standalone?"""
>    """Is the spoke passed as obj standalone?"""

Okay, there is a fix for one of my previous comments.