Re: [anaconda][rhel7-branch/master][v2][PATCH] Add support for autostep and --autoscreenshot (#1059295)

[Vratislav Podzimek]

On Mon, 2014-09-29 at 21:12 +0200, Martin Kolman wrote:
> Add the missing support for the autostep [--autoscreenshot] kickstart
> command to Anaconda. If just autostep is specified, Anaconda will
> iterate over all active spokes just before leaving a hub. If the
> --autoscreenshot option specified, it will also take a screenshot of
> all hubs and of each spoke.
>
> Also turn the take-screenshot code into a proper function that can be
> called when auto-screenshotting and remove the old copy-screenshot
> post-script (the last batch of screenshots is taken after the
> postscripts have run, so screenshot copying is now handled
> elsewhere).
>
> Resolves: rhbz#1059295
> Signed-off-by: Martin Kolman
>
> data/post-scripts/90-copy-screenshots.ks |  10 ---
>  pyanaconda/constants.py                  |   4 +
>  pyanaconda/iutil.py                      |  23 ++++++
>  pyanaconda/ui/gui/__init__.py            | 135 ++++++++++++++++++++++++++-----
>  pyanaconda/ui/gui/hubs/__init__.py       |  45 +++++++++++
>  pyanaconda/ui/gui/spokes/__init__.py     |   9 +++
>  pyanaconda/ui/gui/spokes/welcome.py      |   5 +-
>  7 files changed, 201 insertions(+), 30 deletions(-)
>  delete mode 100644 data/post-scripts/90-copy-screenshots.ks
>
> diff --git a/data/post-scripts/90-copy-screenshots.ks b/data/post-scripts/90-copy-screenshots.ks
> deleted file mode 100644
> index 471e2b1..0000000
> --- a/data/post-scripts/90-copy-screenshots.ks
> +++ /dev/null
> @@ -1,10 +0,0 @@
> -%post --nochroot
>
> -if [ ! -d /tmp/anaconda-screenshots ]; then
> exit 0
>
> -fi
>
> -mkdir -m 0750 -p $ANA_INSTALL_PATH/root/anaconda-screenshots
> -cp -a /tmp/anaconda-screenshots/*.png $ANA_INSTALL_PATH/root/anaconda-screenshots/
>
> -%end
> diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py
> index 2c8ca11..45b2ca8 100644
> --- a/pyanaconda/constants.py
> +++ b/pyanaconda/constants.py
> @@ -150,3 +150,7 @@ IPMI_FAILED   = 0xA
>
> # Recognizing a tarfile
>  TAR_SUFFIX = (".tar", ".tbz", ".tgz", ".txz", ".tar.bz2", "tar.gz", "tar.xz")
>
> +# screenshots
> +SCREENSHOTS_DIRECTORY = "/tmp/anaconda-screenshots"
> +SCREENSHOTS_TARGET_DIRECTORY = "/root/anaconda-screenshots"
> diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
> index d30947a..b13ce09 100644
> --- a/pyanaconda/iutil.py
> +++ b/pyanaconda/iutil.py
> @@ -28,11 +28,13 @@ import errno
>  import subprocess
>  import tempfile
>  import unicodedata
> +import shutil
>  from threading import Thread
>  from Queue import Queue, Empty
>
> from pyanaconda.flags import flags
>  from pyanaconda.constants import DRACUT_SHUTDOWN_EJECT, ROOT_PATH, TRANSLATIONS_UPDATE_DIR, UNSUPPORTED_HW
> +from pyanaconda.constants import SCREENSHOTS_DIRECTORY, SCREENSHOTS_TARGET_DIRECTORY
>  from pyanaconda.regexes import PROXY_URL_PARSE
>
> import logging
> @@ -856,3 +858,24 @@ def ipmi_report(event):
>      execWithCapture("ipmitool", ["sel", "add", path])
>  os.remove(path)
>
> +def save_screenshots():
> """Save screenshots to the installed system"""
> if not os.path.exists(SCREENSHOTS_DIRECTORY):
>    # there are no screenshots to copy
>    return
> target_path = os.path.normpath(getSysroot() + SCREENSHOTS_TARGET_DIRECTORY)

I think a function like 'iutil.sysroot_path(path)' that would correctly
make the 'path' argument "sysrooted" whether it is a relative or an
absolute path could be useful.
> log.info("saving screenshots taken during the installation to:")
> log.info(target_path)

This should be a single log.info() call.
> try:
>    # create the screenshots directory
>    if not os.path.exists(target_path):
>        # it is unlikely the directory exists as we format the root directory,
>        # but better be safe here
>        os.makedirs(target_path)

Believe or not, but we have a function iutil.mkdirChain that does this
with the check included.
>    # copy all screenshots
>    for filename in os.listdir(SCREENSHOTS_DIRECTORY):
>        shutil.copy(os.path.join(SCREENSHOTS_DIRECTORY, filename), target_path)
> except OSError:
>    log.exception("saving screenshots to installed system failed")
>
> diff --git a/pyanaconda/ui/gui/__init__.py b/pyanaconda/ui/gui/__init__.py
> index b64d983..9bc6a0c 100644
> --- a/pyanaconda/ui/gui/__init__.py
> +++ b/pyanaconda/ui/gui/__init__.py
> @@ -25,10 +25,10 @@ from gi.repository import Gdk, Gtk, AnacondaWidgets, Keybinder
>
> from pyanaconda.i18n import _
>  from pyanaconda.constants import IPMI_ABORTED
> -from pyanaconda import product, iutil
> +from pyanaconda import product, iutil, constants
>
> from pyanaconda.ui import UserInterface, common
> -from pyanaconda.ui.gui.utils import enlightbox, gtk_action_wait, busyCursor, unbusyCursor
> +from pyanaconda.ui.gui.utils import enlightbox, gtk_action_wait, gtk_call_once, busyCursor, unbusyCursor
>  import os.path
>
> import logging
> @@ -84,7 +84,7 @@ class GUIObject(common.UIObject):
>      uiFile = ""
>      translationDomain = "anaconda"
> screenshots_directory = "/tmp/anaconda-screenshots"
>
> handles_autostep = False
>
> def __init__(self, data):
>     """Create a new UIObject instance, including loading its uiFile and
>
> @@ -132,6 +132,9 @@ class GUIObject(common.UIObject):
>          Keybinder.init()
>          Keybinder.bind("<Shift>Print", self._handlePrntScreen, [])
>    self._automaticEntry = False
>    self._autostepRunning = False
>    self._autostepDone = False
>
> def _findUIFile(self):
>        path = os.environ.get("UIPATH", "./:/tmp/updates/:/tmp/updates/ui/:/usr/share/anaconda/ui/")
>
> @@ -148,29 +151,107 @@ class GUIObject(common.UIObject):
>          raise IOError("Could not load UI file '%s' for object '%s'" % (self.uiFile, self))
>  def _handlePrntScreen(self, *args, **kwargs):
>    global _screenshotIndex
>    global _last_screenshot_timestamp
>    # as a single press of the assigned key generates
>    # multiple callbacks, we need to skip additional
>    # callbacks for some time once a screenshot is taken
>    if (time.time() - _last_screenshot_timestamp) >= SCREENSHOT_DELAY:
>        # Make sure the screenshot directory exists.
>        if not os.access(self.screenshots_directory, os.W_OK):
>            os.makedirs(self.screenshots_directory)
>        fn = os.path.join(self.screenshots_directory,
>                          "screenshot-%04d.png" % _screenshotIndex)
>        root_window = Gdk.get_default_root_window()
>        pixbuf = Gdk.pixbuf_get_from_window(root_window, 0, 0,
>                                            root_window.get_width(),
>                                            root_window.get_height())
>        pixbuf.savev(fn, 'png', [], [])
>        log.info("screenshot nr. %d taken", _screenshotIndex)
>        _screenshotIndex += 1
>        self.take_screenshot()
>        # start counting from the time the screenshot operation is done
>        _last_screenshot_timestamp = time.time()
>
> def take_screenshot(self, name=None):
>    """Take a screenshot of the whole screen (works even with multiple displays)
>    :param name: optional name for the screenshot that will be appended to the filename,
>                 after the standard prefix & screenshot number
>    :type name: str or NoneType
>    """
>    global _screenshotIndex
>    # Make sure the screenshot directory exists.
>    if not os.access(constants.SCREENSHOTS_DIRECTORY, os.W_OK):
>        os.makedirs(constants.SCREENSHOTS_DIRECTORY)

iutil.mkdirChain could be used here as well.
>    if name is None:
>        screenshot_filename = "screenshot-%04d.png" % _screenshotIndex
>    else:
>        screenshot_filename = "screenshot-%04d-%s.png" % (_screenshotIndex, name)
>    fn = os.path.join(constants.SCREENSHOTS_DIRECTORY, screenshot_filename)
>    root_window = Gdk.get_default_root_window()
>    pixbuf = Gdk.pixbuf_get_from_window(root_window, 0, 0,
>                                        root_window.get_width(),
>                                        root_window.get_height())
>    pixbuf.savev(fn, 'png', [], [])
>    log.info("%s taken", screenshot_filename)
>    _screenshotIndex += 1
> @property
> def automaticEntry(self):
>    """Report if the given GUIObject has been displayed under automatic control
>    This is needed for example for installations with an incomplete kickstart,
>    as we need to differentiate the automatic screenshot pass from the user
>    entering a spoke to manually configure things. We also need to skip applying
>    changes if the spoke is entered automatically.
>    """
>    return self._automaticEntry
> @automaticEntry.setter
> def automaticEntry(self, value):
>    self._automaticEntry = value
> @property
> def autostepRunning(self):
>    """Report if the GUIObject is currently running autostep"""
>    return self._autostepRunning
> @autostepRunning.setter
> def autostepRunning(self, value):
>    self._autostepRunning = value
> @property
> def autostepDone(self):
>    """Report if autostep for this GUIObject has been finished"""
>    return self._autostepDone
> @autostepDone.setter
> def autostepDone(self, value):
>    self._autostepDone = value
> def autostep(self):
>    """Autostep through this graphical object and through
>    any graphical objects managed by it (such as through spokes for a hub)
>    """
>    # report that autostep is running to prevent another from starting
>    self.autostepRunning = True
>    # take a screenshot of the current graphical object
>    if self.data.autostep.autoscreenshot:
>        # as autostep is triggered just before leaving a screen,
>        # we can safely take a screenshot of the "parent" object at once
>        # without using idle_add
>        self.take_screenshot("%s" % self.__class__.__name__)

Is self.__class__.__name__ something else than a string that we need to
convert it to a string? Otherwise you can drop the string formatting
here.
>    self._doAutostep()
>    # done
>    self.autostepRunning = False
>    self.autostepDone = True
>    self._doPostAutostep()
> def _doPostAutostep(self):
>    """To be overridden by the given GUIObject sub-class with custom code
>    that brings the GUI from the autostepping mode back to the normal mode.
>    This usually means to "click" the continue button or its equivalent.
>    """
>    pass
> def _doAutostep(self):
>    """To be overridden by the given GUIObject sub-class with customized
>    autostepping code - if needed
>    (this is for example used to step though spokes in a hub)

^through
>    """
>    pass
> @property
>    def window(self):
>        """Return the top-level object out of the GtkBuilder representation
>
> @@ -447,8 +528,26 @@ class GraphicalUserInterface(UserInterface):
>      ### SIGNAL HANDLING METHODS
>      ###
>      def _on_continue_clicked(self):
>    # Autostep needs to be triggered just before switching to the next screen
>    # (or before quiting the installation if there are no more screens) to be consistent
>    # in both fully automatic kickstart installation and for installation with an incomplete
>    # kickstart. Therefore we basically "hook" the continue-clicked signal, start autostepping
>    # and ignore any other continue-clicked signals until autostep is done.
>    # Once autostep finishes, it emits the appropriate continue-clicked signal itself,
>    # switching to the next screen (if any).
>    if self.data.autostep.seen and self._currentAction.handles_autostep:
>        if self._currentAction.autostepRunning:
>            log.debug("ignoring the continue-clicked signal - autostep is running")
>            return
>        elif not self._currentAction.autostepDone:
>            self._currentAction.autostep()
>            return
>    # If we're on the last screen, clicking Continue quits.
>    if len(self._actions) == 1:
>        # save the screenshots to the installed system before killing Anaconda
>        # (the kickstart post scripts run to early, so we need to copy the screenshots now)
>        iutil.save_screenshots()
>        Gtk.main_quit()
>        return
>
> diff --git a/pyanaconda/ui/gui/hubs/__init__.py b/pyanaconda/ui/gui/hubs/__init__.py
> index ce71929..00be429 100644
> --- a/pyanaconda/ui/gui/hubs/__init__.py
> +++ b/pyanaconda/ui/gui/hubs/__init__.py
> @@ -60,6 +60,8 @@ class Hub(GUIObject, common.Hub):
>         additional standalone screens either before or after them.
>      """
> handles_autostep = True
> def __init__(self, data, storage, payload, instclass):
>        """Create a new Hub instance.
>
> @@ -111,6 +113,11 @@ class Hub(GUIObject, common.Hub):
>          action.window.set_transient_for(self.window)
>          action.window.show_all()
>    # step through the spoke if required
>    if action.automaticEntry:
>        # we need to use idle_add here to give GTK time to render the spoke
>        gtk_call_once(self._autostep_spoke, action)
>    # Start a recursive main loop for this spoke, which will prevent
>    # signals from going to the underlying (but still displayed) Hub and
>    # prevent the user from switching away.  It's up to the spoke's back
>
> @@ -118,6 +125,11 @@ class Hub(GUIObject, common.Hub):
>          Gtk.main()
>          action.window.set_transient_for(None)
>    # don't apply any actions if the spoke was visited automatically
>    if action.automaticEntry:
>        action.automaticEntry = False
>        return
>    action._visitedSinceApplied = True
>
>    # Don't take _visitedSinceApplied into account here.  It will always be
>
> @@ -394,6 +406,39 @@ class Hub(GUIObject, common.Hub):
>      def quitButton(self):
>          return None
> def _doAutostep(self):
>    """Autostep through all spokes managed by this hub"""
>    log.info("autostepping through all spokes on hub %s" % self.__class__.__name__)
>    index = 1
>    spokes_kv = self._spokes.items()
>    spoke_count = len(spokes_kv)
>    spokes_kv.sort(key=lambda x:x[0])

I think this could be rewritten as:
spokes_count = len(self._spokes)
spokes_kv = sorted(self._spokes.items(), key=lambda x:x[0])
>    # take a screenshot of all spokes in the alphabetical order of their names
>    for spoke_name, spoke in spokes_kv:

You can use enumerate() here and define the index variable here. The 'i'
would be a better name then.
>        log.debug("stepping to spoke %s (%d/%d)" % (spoke_name, index, spoke_count))
>        spoke.automaticEntry = True
>        self._on_spoke_clicked(None, None, spoke)
>        index += 1
>    log.info("autostep for hub %s finished" % self.__class__.__name__)
>
> def _autostep_spoke(self, spoke):
>    """Step through a spoke and make a screenshot if required and
>    then kill it's recursive GTK main loop. :)
>    This will return control back to the primary main loop,
>    so it can show another spoke or got to next hub.
>    """
>    from gi.repository import Gtk
>    # it might be possible that autostep is specified, but autoscreenshot isn't
>    if self.data.autostep.autoscreenshot:
>        self.take_screenshot(spoke.__class__.__name__)
>    spoke.window.hide()
>    Gtk.main_quit()
>
> def _doPostAutostep(self):
>    # we are done, re-emit the continue clicked signal we "consumed" previously
>    # so that the Anaconda GUI can switch to the next screen
>    gtk_call_once(self.continueButton.emit, "clicked")
>
> ### SIGNAL HANDLERS
>
> def register_event_cb(self, event, cb):
>
> diff --git a/pyanaconda/ui/gui/spokes/__init__.py b/pyanaconda/ui/gui/spokes/__init__.py
> index 32a1469..75231ac 100644
> --- a/pyanaconda/ui/gui/spokes/__init__.py
> +++ b/pyanaconda/ui/gui/spokes/__init__.py
> @@ -22,6 +22,7 @@
>  from pyanaconda.ui import common
>  from pyanaconda.ui.common import collect
>  from pyanaconda.ui.gui import GUIObject
> +from pyanaconda.ui.gui.utils import gtk_call_once
>  import os.path
>
> __all__ = ["StandaloneSpoke", "NormalSpoke", "PersonalizationSpoke",
> @@ -65,6 +66,9 @@ class Spoke(GUIObject):
>  # Inherit abstract methods from common.StandaloneSpoke
>  # pylint: disable-msg=W0223
>  class StandaloneSpoke(Spoke, common.StandaloneSpoke):
> handles_autostep = True
> def __init__(self, data, storage, payload, instclass):
>        Spoke.__init__(self, data)
>        common.StandaloneSpoke.__init__(self, data, storage, payload, instclass)
>
> @@ -79,6 +83,11 @@ class StandaloneSpoke(Spoke, common.StandaloneSpoke):
>          elif event == "quit":
>              self.window.connect("quit-clicked", lambda *args: cb())
> def _doPostAutostep(self):
>    # we are done, re-emit the continue clicked signal we "consumed" previously
>    # so that the Anaconda GUI can switch to the next screen
>    gtk_call_once(self.window.emit, "continue-clicked")
>
> # Inherit abstract methods from common.NormalSpoke
>  # pylint: disable-msg=W0223
>  class NormalSpoke(Spoke, common.NormalSpoke):
> diff --git a/pyanaconda/ui/gui/spokes/welcome.py b/pyanaconda/ui/gui/spokes/welcome.py
> index 548dd02..80017f6 100644
> --- a/pyanaconda/ui/gui/spokes/welcome.py
> +++ b/pyanaconda/ui/gui/spokes/welcome.py
> @@ -273,8 +273,9 @@ class WelcomeLanguageSpoke(LangLocaleHandler, StandaloneSpoke):
>      # Override the default in StandaloneSpoke so we can display the beta
>      # warning dialog first.
>      def _on_continue_clicked(self, cb):
>    # Don't display the betanag dialog if this is the final release.
>    if not isFinal:
>    # Don't display the betanag dialog if this is the final release or
>    # when autostep has been requested as betanag breaks the autostep logic.
>    if not isFinal and not self.data.autostep.seen:
>        dlg = self.builder.get_object("betaWarnDialog")
>        with enlightbox(self.window, dlg):
>            rc = dlg.run()

Otherwise this looks good to me as long as it works.