[PATCH] Add support for autostep and --autoscreenshot (#1059295)

Anne Mulhern amulhern at redhat.com
Fri Sep 26 13:47:49 UTC 2014





----- Original Message -----
> From: "Martin Kolman" <mkolman at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Thursday, September 25, 2014 8:49:36 AM
> Subject: [PATCH] Add support for autostep and --autoscreenshot (#1059295)
> 
> 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 <mkolman at redhat.com>
> ---
>  data/post-scripts/90-copy-screenshots.ks |  10 ---
>  pyanaconda/constants.py                  |   4 +
>  pyanaconda/iutil.py                      |  23 ++++++
>  pyanaconda/ui/gui/__init__.py            | 127
>  ++++++++++++++++++++++++++-----
>  pyanaconda/ui/gui/hubs/__init__.py       |  43 +++++++++++
>  5 files changed, 179 insertions(+), 28 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"

If this path is ever going to be relative to anything,
should make it relative, too.

> +SCREENSHOTS_TARGET_DIRECTORY = "/root/anaconda-screenshots"

Should make above relative, not absolute, path.

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

Should use os.path.join() here.

> +    log.info("saving screenshots taken during the installation to:")
> +    log.info(target_path)

Why not combine above lines into one line?

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

Why not just remove the directory and make it w/out the check?
Are you expecting to be adding screenshots incrementally?

Method header should probably indicate whether this method appends or replaces.

> +        # copy all screenshots
> +        for filename in os.listdir(SCREENSHOTS_DIRECTORY):
> +            shutil.copy(os.path.join(SCREENSHOTS_DIRECTORY, filename),
> target_path)

To be really punctilious, you should set things up so that you can continue
if copy of one file fails.

> +
> +    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..afe3901 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,99 @@ 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)
> +
> +        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__)
> +        self._doAutostep()
> +        # done
> +        self.autostepRunning = False
> +        self.autostepDone = True
> +

Is there any way to handle this autostepRunning on/off setting using a context manager?

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

Could you specify what the method does in the docstring? Do the methods that
override it have to observe any constraints? Can they rely on any guarantees?

I'm puzzled, because many things override GUIobject, and now they all have
autostep() method but it doesn't seem appropriate for all.
Should some of this be lower in class hierarchy?

> +        """
> +        pass
> +
>      @property
>      def window(self):
>          """Return the top-level object out of the GtkBuilder representation
> @@ -447,8 +520,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,

It actually seems to be the _doAutostep() function in HUB, but maybe autostep()
is the correct place?

> +        # 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)

I'm puzzled...isn't this the last possible place you could save the screenshots?
Why do kickstart post scripts matter?

> +            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..c0e7fde 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,37 @@ 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])
> +        # take a screenshot of all spokes in the alphabetical order of their
> names
> +        for spoke_name, spoke in spokes_kv:
> +            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__)
> +        # 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")
> +
> +    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.

"got to" -> "go to" ?

> +        """
> +        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()
> +
>      ### SIGNAL HANDLERS
>  
>      def register_event_cb(self, event, cb):
> --
> 1.9.3
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

- mulhern


More information about the anaconda-patches mailing list