[anaconda][master/f23-branch][PATCH] Don't crash if incorrect environment is set in kickstart (#1234890)

Martin Kolman mkolman at redhat.com
Wed Aug 12 13:40:06 UTC 2015


Instead of crashing prompt the user to select a valid environment from
the list in the software spoke. Also:

- make sure the selected environment is written in the installation
  describing kickstart file.
- handle human readable environment names from kickstart (including
  localized names)
- keep the specification in its original format as long as it is
  valid, so that the input and output kickstarts have the same
  environment specifications
- handle previously valid environment becoming invalid
  (this can happen due to source switching)
- make the code more sane and maintainable

Resolves: rhbz#1234890
Signed-off-by: Martin Kolman <mkolman at redhat.com>
---
 pyanaconda/packaging/dnfpayload.py   |   7 ++
 pyanaconda/ui/gui/spokes/software.py | 202 +++++++++++++++++++++++------------
 2 files changed, 139 insertions(+), 70 deletions(-)

diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py
index c24aaf1..cc0f6d0 100644
--- a/pyanaconda/packaging/dnfpayload.py
+++ b/pyanaconda/packaging/dnfpayload.py
@@ -598,6 +598,13 @@ class DNFPayload(packaging.PackagePayload):
             raise packaging.NoSuchGroup(environmentid)
         return (env.ui_name, env.ui_description)
 
+    def environmentId(self, environment):
+        """ Return environment id for the environment specified by id or name."""
+        env = self._base.comps.environment_by_pattern(environment)
+        if env is None:
+            raise packaging.NoSuchGroup(environment)
+        return env.id
+
     def environmentGroups(self, environmentid, optional=True):
         env = self._base.comps.environment_by_pattern(environmentid)
         if env is None:
diff --git a/pyanaconda/ui/gui/spokes/software.py b/pyanaconda/ui/gui/spokes/software.py
index d8b3402..9b50a98 100644
--- a/pyanaconda/ui/gui/spokes/software.py
+++ b/pyanaconda/ui/gui/spokes/software.py
@@ -27,7 +27,7 @@ from gi.repository import Gtk, Pango
 
 from pyanaconda.flags import flags
 from pyanaconda.i18n import _, C_, CN_
-from pyanaconda.packaging import PackagePayload, payloadMgr
+from pyanaconda.packaging import PackagePayload, payloadMgr, NoSuchGroup
 from pyanaconda.threads import threadMgr, AnacondaThread
 from pyanaconda import constants, iutil
 
@@ -75,7 +75,6 @@ class SoftwareSelectionSpoke(NormalSpoke):
 
         self.selectedGroups = []
         self.excludedGroups = []
-        self.environment = None
 
         self._environmentListBox = self.builder.get_object("environmentListBox")
         self._addonListBox = self.builder.get_object("addonListBox")
@@ -112,6 +111,11 @@ class SoftwareSelectionSpoke(NormalSpoke):
         payloadMgr.addListener(payloadMgr.STATE_FINISHED, self._payload_finished)
         payloadMgr.addListener(payloadMgr.STATE_ERROR, self._payload_error)
 
+        # Add an invisible radio button so that we can show the environment
+        # list with no radio buttons ticked
+        self._fakeRadio = Gtk.RadioButton(group=None)
+        self._fakeRadio.set_active(True)
+
     # Payload event handlers
     def _downloading_package_md(self):
         # Reset the error state from previous payloads
@@ -122,28 +126,74 @@ class SoftwareSelectionSpoke(NormalSpoke):
     def _downloading_group_md(self):
         hubQ.send_message(self.__class__.__name__, _(constants.PAYLOAD_STATUS_GROUP_MD))
 
+    @property
+    def environment(self):
+        """A wrapper for the environment specification in kickstart"""
+        return self.data.packages.environment
+
+    @environment.setter
+    def environment(self, value):
+        self.data.packages.environment = value
+
+    @property
+    def environmentid(self):
+        """Return the "machine readable" environment id
+
+        Alternatively we could have just "canonicalized" the
+        environment description to the "machine readable" format
+        when reading it from kickstart for the first time.
+        But this could result in input and output kickstart,
+        which would be rather confusing for the user.
+        So we don't touch the specification from kickstart
+        if it is valid and use this property when we need
+        the "machine readable" form.
+        """
+        try:
+            return self.payload.environmentId(self.environment)
+        except NoSuchGroup:
+            return None
+
+    @property
+    def environment_valid(self):
+        """Return if the currently set environment is valid
+        (represents an environment known by the payload)
+        """
+        # None means the environment has not been set by the user,
+        # which means:
+        # * set the default environment during interactive installation
+        # * ask user to specify an environment during kickstart installation
+        if self.environment is None:
+            return True
+        else:
+            return self.environmentid in self.payload.environments
+
     def _payload_finished(self):
-        self.environment = self.data.packages.environment
+        if self.environment_valid:
+            log.info("using environment from kickstart: %s", self.environment)
+        else:
+            log.error("unknown environment has been specified in kickstart and will be ignored: %s",
+                      self.data.packages.environment)
+            # False means that the environment has been set to an invalid value and needs to
+            # be manually set to a valid one.
+            self.environment = False
 
     def _payload_error(self):
         self._error = True
         hubQ.send_message(self.__class__.__name__, payloadMgr.error)
 
     def _apply(self):
-        env = self._get_selected_environment()
+        if not self.environment:
+            return
 
-        # Not a kickstart with packages, setup the environment and groups
-        if env and not self._kickstarted:
-            addons = self._get_selected_addons()
-            for group in addons:
-                if group not in self.selectedGroups:
-                    self.selectedGroups.append(group)
+        addons = self._get_selected_addons()
+        for group in addons:
+            if group not in self.selectedGroups:
+                self.selectedGroups.append(group)
 
             self._selectFlag = False
             self.payload.data.packages.packageList = []
             self.payload.data.packages.groupList = []
-            self.payload.selectEnvironment(env)
-            self.environment = env
+            self.payload.selectEnvironment(self.environment)
             for group in self.selectedGroups:
                 self.payload.selectGroup(group)
 
@@ -158,7 +208,6 @@ class SoftwareSelectionSpoke(NormalSpoke):
 
     def apply(self):
         self._apply()
-        self.data.packages.seen = True
 
     def checkSoftwareSelection(self):
         from pyanaconda.packaging import DependencyError
@@ -182,23 +231,32 @@ class SoftwareSelectionSpoke(NormalSpoke):
                               not threadMgr.get(constants.THREAD_PAYLOAD) and
                               not self._errorMsgs and self.txid_valid)
 
-        # we should always check processingDone before checking the other variables,
-        # as they might be inconsistent until processing is finished
-        if self._kickstarted:
-            return processingDone and self.data.packages.seen
+        # * we should always check processingDone before checking the other variables,
+        #   as they might be inconsistent until processing is finished
+        # * we can't let the installation proceed until a valid environment has been set
+        if processingDone:
+            if self.environment is not None:
+                # if we have environment it needs to be valid
+                return self.environment_valid
+            # if we don't have environment we need to at least have the %packages
+            # section in kickstart
+            elif flags.automatedInstall and self.data.packages.seen:
+                return True
+            # no environment and no %packages section -> manual intervention is needed
+            else:
+                return False
         else:
-            return processingDone and self._get_selected_environment() is not None
+            return False
 
     @property
     def changed(self):
-        env = self._get_selected_environment()
-        if not env:
+        if not self.environment:
             return True
 
         addons = self._get_selected_addons()
 
         # Don't redo dep solving if nothing's changed.
-        if env == self._origEnvironment and set(addons) == set(self._origAddons) and \
+        if self.environment == self._origEnvironment and set(addons) == set(self._origAddons) and \
            self.txid_valid:
             return False
 
@@ -235,17 +293,26 @@ class SoftwareSelectionSpoke(NormalSpoke):
         if not self.txid_valid:
             return _("Source changed - please verify")
 
-        env = self._get_selected_environment()
-        if not env:
-            # Kickstart installs with %packages will have a row selected, unless
-            # they did an install without a desktop environment.  This should
-            # catch that one case.
-            if self._kickstarted:
-                return _("Custom software selected")
-
-            return _("Nothing selected")
-
-        return self.payload.environmentDescription(env)[0]
+        # kickstart installation
+        if flags.automatedInstall:
+            if self.data.packages.seen:
+                # %packages section is present in kickstart but environment is not set
+                if self.environment is None:
+                    return _("Custom software selected")
+                # environment is set to an invalid value
+                elif not self.environment_valid:
+                    return _("Invalid environment specified in kickstart")
+            # we have no packages section in the kickstart and no environment has been set
+            elif not self.environment:
+                return _("Nothing selected")
+
+        if not flags.automatedInstall and not self.environment_valid:
+            # selected environment is not valid, this can happen when a valid environment
+            # is selected (by default, manually or from kickstart) and then the installation
+            # source is switched to one where the selected environment is no longer valid
+            return _("Selected environment is not valid")
+
+        return self.payload.environmentDescription(self.environment)[0]
 
     def initialize(self):
         NormalSpoke.initialize(self)
@@ -254,7 +321,6 @@ class SoftwareSelectionSpoke(NormalSpoke):
 
     def _initialize(self):
         threadMgr.wait(constants.THREAD_PAYLOAD)
-
         if not self._kickstarted:
             # having done all the slow downloading, we need to do the first refresh
             # of the UI here so there's an environment selected by default.  This
@@ -303,8 +369,9 @@ class SoftwareSelectionSpoke(NormalSpoke):
 
         threadMgr.wait(constants.THREAD_PAYLOAD)
 
-        if self.environment not in self.payload.environments:
-            self.environment = None
+        firstEnvironment = True
+
+        self._clear_listbox(self._environmentListBox)
 
         # If no environment is selected, use the default from the instclass.
         # If nothing is set in the instclass, the first environment will be
@@ -313,26 +380,28 @@ class SoftwareSelectionSpoke(NormalSpoke):
                 self.payload.instclass.defaultPackageEnvironment in self.payload.environments:
             self.environment = self.payload.instclass.defaultPackageEnvironment
 
-        firstEnvironment = True
+        # create rows for all valid environments
+        for environmentid in self.payload.environments:
+            (name, desc) = self.payload.environmentDescription(environmentid)
 
-        self._clear_listbox(self._environmentListBox)
-
-        for environment in self.payload.environments:
-            (name, desc) = self.payload.environmentDescription(environment)
-
-            radio = Gtk.RadioButton(group=self._firstRadio)
+            # use the invisible radio button as a group for all environment
+            # radio buttons
+            radio = Gtk.RadioButton(group=self._fakeRadio)
 
             # automatically select an environment if this is an interactive install
-            active = environment == self.environment or \
-                     not self._kickstarted and not self.environment and firstEnvironment
-            radio.set_active(active)
-            if active:
-                self.environment = environment
-
-            self._add_row(self._environmentListBox, name, desc, radio,
-                    self.on_radio_button_toggled)
-
-            firstEnvironment = False
+            if flags.automatedInstall:  # kickstart installation
+                # tick the radio button if the environment from kickstart is both valid
+                # and equal to the environment corresponding to the current row
+                radio.set_active(self.environment_valid and self.environmentid == environmentid)
+            elif firstEnvironment:  # manual installation
+                # for manual installs that don't have a default provided by the install class
+                # just tick the first radio button and select the first environment
+                if not self.environment_valid:
+                    radio.set_active(True)
+                    self.environment = environmentid
+                firstEnvironment = False
+
+            self._add_row(self._environmentListBox, name, desc, radio, self.on_radio_button_toggled)
 
         self.refreshAddons()
         self._environmentListBox.show_all()
@@ -350,9 +419,9 @@ class SoftwareSelectionSpoke(NormalSpoke):
                 selected = False
             # Otherwise, use the default state
             else:
-                selected = self.payload.environmentOptionIsDefault(self.environment, grp)
+                selected = self.payload.environmentOptionIsDefault(self.environmentid, grp)
         else:
-            selected = self.payload.environmentOptionIsDefault(self.environment, grp)
+            selected = self.payload.environmentOptionIsDefault(self.environmentid, grp)
 
         check = Gtk.CheckButton()
         check.set_active(selected)
@@ -361,11 +430,11 @@ class SoftwareSelectionSpoke(NormalSpoke):
     @property
     def _addSep(self):
         """ Whether the addon list contains a separator. """
-        return len(self.payload.environmentAddons[self.environment][0]) > 0 and \
-                len(self.payload.environmentAddons[self.environment][1]) > 0
+        return len(self.payload.environmentAddons[self.environmentid][0]) > 0 and \
+            len(self.payload.environmentAddons[self.environmentid][1]) > 0
 
     def refreshAddons(self):
-        if self.environment and (self.environment in self.payload.environmentAddons):
+        if self.environment and (self.environmentid in self.payload.environmentAddons):
             self._clear_listbox(self._addonListBox)
 
             # We have two lists:  One of addons specific to this environment,
@@ -378,7 +447,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
             # state will be used. Otherwise, the add-on will be selected if it is a default
             # for this environment.
 
-            for grp in self.payload.environmentAddons[self.environment][0]:
+            for grp in self.payload.environmentAddons[self.environmentid][0]:
                 self._addAddon(grp)
 
             # This marks a separator in the view - only add it if there's both environment
@@ -386,7 +455,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
             if self._addSep:
                 self._addonListBox.insert(Gtk.Separator(), -1)
 
-            for grp in self.payload.environmentAddons[self.environment][1]:
+            for grp in self.payload.environmentAddons[self.environmentid][1]:
                 self._addAddon(grp)
 
         self._selectFlag = True
@@ -397,14 +466,13 @@ class SoftwareSelectionSpoke(NormalSpoke):
             self.clear_info()
 
     def _allAddons(self):
-        if self.environment in self.payload.environmentAddons:
-            addons = copy.copy(self.payload.environmentAddons[self.environment][0])
+        if self.environmentid in self.payload.environmentAddons:
+            addons = copy.copy(self.payload.environmentAddons[self.environmentid][0])
             if self._addSep:
                 addons.append('')
-            addons += self.payload.environmentAddons[self.environment][1]
+            addons += self.payload.environmentAddons[self.environmentid][1]
         else:
             addons = []
-
         return addons
 
     def _get_selected_addons(self):
@@ -424,12 +492,6 @@ class SoftwareSelectionSpoke(NormalSpoke):
 
         return retval
 
-    def _get_selected_environment(self):
-        # Returns the currently selected environment (self.environment
-        # is set in both initilize() and apply(), so we don't need to
-        # care about the state of the internal data model at all)
-        return self.environment
-
     def _clear_listbox(self, listbox):
         for child in listbox.get_children():
             listbox.remove(child)
@@ -465,7 +527,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
         # Remove all the groups that were selected by the previously
         # selected environment.
         if self.environment:
-            for groupid in self.payload.environmentGroups(self.environment):
+            for groupid in self.payload.environmentGroups(self.environmentid):
                 if groupid in self.selectedGroups:
                     self.selectedGroups.remove(groupid)
 
-- 
2.4.3



More information about the anaconda-patches mailing list