[anaconda][rhel7-branch][PATCH 0/2] Fix environment handling

[Martin Kolman]

So it turns out that there are multiple issues with how we handle environment
specifications from kickstart:

1) an invalid (not known by the payload) environment specified in kickstart causes a crash
2) valid human readable environment specifications, such as:
   "Infrastructure Server", which is the human readable form of "infrastructure-server-environment",
   are not considered valid, the same thing happens for translated human readable environment names
3) environments that have been manually selected in the UI are not written to the output
   kickstart

The first patch should fix issues 1) + 3) and the second patch should fix issue 2).

Martin Kolman (2):
  Don't crash if incorrect environment is set in kickstart (#1234890)
  Handle human readable environment names from kickstart (#1234890)

pyanaconda/packaging/yumpayload.py   |  14 +++++
 pyanaconda/ui/gui/spokes/software.py | 117 ++++++++++++++++++++++++++---------
 2 files changed, 101 insertions(+), 30 deletions(-)

[PATCH 1/2] Don't crash if incorrect environment is set in kickstart (#1234890)

[Martin Kolman]

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.

Resolves: rhbz#1234890
Signed-off-by: Martin Kolman 
---
 pyanaconda/ui/gui/spokes/software.py | 76 ++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 21 deletions(-)

diff --git a/pyanaconda/ui/gui/spokes/software.py b/pyanaconda/ui/gui/spokes/software.py
index 17498af..4801643 100644
--- a/pyanaconda/ui/gui/spokes/software.py
+++ b/pyanaconda/ui/gui/spokes/software.py
@@ -67,7 +67,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")
@@ -100,15 +99,44 @@ class SoftwareSelectionSpoke(NormalSpoke):
     def _downloading_group_md(self):
         hubQ.send_message(self.__class__.__name__, _("Downloading group metadata..."))

+    @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 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.environment 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.data.packages.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):
         hubQ.send_message(self.__class__.__name__, payloadMgr.error)

def _apply(self):
-        env = self._get_selected_environment()
-        if not env:
+        if not self.environment:
             return

addons = self._get_selected_addons()
@@ -118,8 +146,7 @@ class SoftwareSelectionSpoke(NormalSpoke):

self._selectFlag = False
         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)

@@ -161,20 +188,21 @@ class SoftwareSelectionSpoke(NormalSpoke):
         # we should always check processingDone before checking the other variables,
         # as they might be inconsistent until processing is finished
         if flags.automatedInstall:
-            return processingDone and self.data.packages.seen
+            # we can't let the automated installatin proceed until a valid environment
+            # has been set
+            return processingDone and self.data.packages.seen and self.environment_valid
         else:
-            return processingDone and self._get_selected_environment() is not None
+            return processingDone and self.environment is not None

@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

@@ -211,17 +239,21 @@ class SoftwareSelectionSpoke(NormalSpoke):
         if not self.txid_valid:
             return _("Source changed - please verify")

-        env = self._get_selected_environment()
-        if not env:
+        if not self.environment:
             # 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 flags.automatedInstall and self.data.packages.seen:
-                return _("Custom software selected")
+                if self.environment_valid:
+                    return _("Custom software selected")
+                else:
+                    # It is impossible to set an invalid environment in the GUI,
+                    # os any invalid environment must have come from kickstart.
+                    return _("Invalid environment specified in kickstart")

return _("Nothing selected")

-        return self.payload.environmentDescription(env)[0]
+        return self.payload.environmentDescription(self.environment)[0]

def initialize(self):
         NormalSpoke.initialize(self)
@@ -308,6 +340,14 @@ class SoftwareSelectionSpoke(NormalSpoke):
         self._environmentListBox.show_all()
         self._addonListBox.show_all()

+        # Pre-select the default environment when the user enters the software spoke
+        # to fix an invalid environment set in kickstart. Otherwise the default environment
+        # would still look as selected but the selection would not be applied after returning
+        # to the hub.
+        if flags.automatedInstall and self.environment is None:
+            row = self._environmentListBox.get_row_at_index(0)
+            row.activate()
+
     def _addAddon(self, grp):
         (name, desc) = self.payload.groupDescription(grp)

@@ -385,12 +425,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)

[PATCH 1/2] Don't crash if incorrect environment is set in kickstart (#1234890)

[Vratislav Podzimek]

On Fri, 2015-06-26 at 15:31 +0200, Martin Kolman wrote:
> 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.
>
> Resolves: rhbz#1234890
> Signed-off-by: Martin Kolman
>
> pyanaconda/ui/gui/spokes/software.py | 76 ++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 21 deletions(-)
>
> diff --git a/pyanaconda/ui/gui/spokes/software.py b/pyanaconda/ui/gui/spokes/software.py
> index 17498af..4801643 100644
> --- a/pyanaconda/ui/gui/spokes/software.py
> +++ b/pyanaconda/ui/gui/spokes/software.py
> @@ -67,7 +67,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")
>
> @@ -100,15 +99,44 @@ class SoftwareSelectionSpoke(NormalSpoke):
>      def _downloading_group_md(self):
>          hubQ.send_message(self.__class__.__name__, _("Downloading group metadata..."))
> @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 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.environment 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.data.packages.environment)

AFAICT this may be misleading because self.environment_valid is True if
self.environment is None (and thus not set in kickstart at all). Or is
this callback triggered only when some environment was set/requested?

Also, it should use self.environment not self.data.packages.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):
>        hubQ.send_message(self.__class__.__name__, payloadMgr.error)
>
> def _apply(self):
>    env = self._get_selected_environment()
>    if not env:
>    if not self.environment:
>        return
>
>    addons = self._get_selected_addons()
>
> @@ -118,8 +146,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
>      self._selectFlag = False
>      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)
>
> @@ -161,20 +188,21 @@ class SoftwareSelectionSpoke(NormalSpoke):
>          # we should always check processingDone before checking the other variables,
>          # as they might be inconsistent until processing is finished
>          if flags.automatedInstall:
>        return processingDone and self.data.packages.seen
>        # we can't let the automated installatin proceed until a valid environment

^typo
>        # has been set
>        return processingDone and self.data.packages.seen and self.environment_valid
>    else:
>        return processingDone and self._get_selected_environment() is not None
>        return processingDone and self.environment is not None
>
> @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
>
> @@ -211,17 +239,21 @@ class SoftwareSelectionSpoke(NormalSpoke):
>          if not self.txid_valid:
>              return _("Source changed - please verify")
>    env = self._get_selected_environment()
>    if not env:
>    if not self.environment:
>        # 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 flags.automatedInstall and self.data.packages.seen:
>            return _("Custom software selected")
>            if self.environment_valid:
>                return _("Custom software selected")
>            else:
>                # It is impossible to set an invalid environment in the GUI,
>                # os any invalid environment must have come from kickstart.

^^so?
>                return _("Invalid environment specified in kickstart")
>
>        return _("Nothing selected")
>
>    return self.payload.environmentDescription(env)[0]
>    return self.payload.environmentDescription(self.environment)[0]
>
> def initialize(self):
>        NormalSpoke.initialize(self)
>
> @@ -308,6 +340,14 @@ class SoftwareSelectionSpoke(NormalSpoke):
>          self._environmentListBox.show_all()
>          self._addonListBox.show_all()
>    # Pre-select the default environment when the user enters the software spoke
>    # to fix an invalid environment set in kickstart. Otherwise the default environment
>    # would still look as selected but the selection would not be applied after returning
>    # to the hub.
>    if flags.automatedInstall and self.environment is None:
>        row = self._environmentListBox.get_row_at_index(0)
>        row.activate()

Shouldn't this check 'self.environment is False' as set above in
_payload_finished()?

[PATCH 1/2] Don't crash if incorrect environment is set in kickstart (#1234890)

[Martin Kolman]

On Mon, 2015-06-29 at 12:47 +0200, Vratislav Podzimek wrote:
> On Fri, 2015-06-26 at 15:31 +0200, Martin Kolman wrote:
> > 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.
> >
> > Resolves: rhbz#1234890
> > Signed-off-by: Martin Kolman
> >
> > pyanaconda/ui/gui/spokes/software.py | 76 
> > ++++++++++++++++++++++++++----------
> >  1 file changed, 55 insertions(+), 21 deletions(-)
> >
> > diff --git a/pyanaconda/ui/gui/spokes/software.py 
> > b/pyanaconda/ui/gui/spokes/software.py
> > index 17498af..4801643 100644
> > --- a/pyanaconda/ui/gui/spokes/software.py
> > +++ b/pyanaconda/ui/gui/spokes/software.py
> > @@ -67,7 +67,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")
> > @@ -100,15 +99,44 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >      def _downloading_group_md(self):
> >          hubQ.send_message(self.__class__.__name__, _("Downloading 
> > group metadata..."))
> > @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 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.environment 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.data.packages.environment)
>
> AFAICT this may be misleading because self.environment_valid is True 
> if
> self.environment is None (and thus not set in kickstart at all). Or 
> is
> this callback triggered only when some environment was set/requested?

I think this triggers when the source changes - so on startup on when
the user changes the installation source during the installation.
> From this point of view environment being None is valid - it means it

has not been set and a default value will be used. And if there is no
%package section at all the user will be prompted to verify that the 
default environment is OK.
> Also, it should use self.environment not 
> self.data.packages.environment.

Fixing. :)
> >    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):
> >        hubQ.send_message(self.__class__.__name__,
> >
> > payloadMgr.error)
> >  def _apply(self):
> >    env = self._get_selected_environment()
> >    if not env:
> >    if not self.environment:
> >        return
> >
> >    addons = self._get_selected_addons()
> >
> > @@ -118,8 +146,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >      self._selectFlag = False
> >      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)
> >
> > @@ -161,20 +188,21 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >          # we should always check processingDone before checking 
> > the other variables,
> >          # as they might be inconsistent until processing is 
> > finished
> >          if flags.automatedInstall:
> >        return processingDone and self.data.packages.seen
> >        # we can't let the automated installatin proceed until 
> >
> > a valid environment
>                                        ^typo

Fixing. :)
> >        # has been set
> >        return processingDone and self.data.packages.seen and 
> >
> > self.environment_valid
> >          else:
> >        return processingDone and 
> >
> > self._get_selected_environment() is not None
> >        return processingDone and self.environment is not None
> >
> > @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
> >
> > @@ -211,17 +239,21 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >          if not self.txid_valid:
> >              return _("Source changed - please verify")
> >    env = self._get_selected_environment()
> >    if not env:
> >    if not self.environment:
> >        # 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 flags.automatedInstall and self.data.packages.seen:
> >            return _("Custom software selected")
> >            if self.environment_valid:
> >                return _("Custom software selected")
> >            else:
> >                # It is impossible to set an invalid 
> >
> > environment in the GUI,
> >                # os any invalid environment must have come 
> >
> > from kickstart.
>                     ^^so?

Also fixing. :)
> >                return _("Invalid environment specified in 
> >
> > kickstart")
> >          return _("Nothing selected")
> >
> >    return self.payload.environmentDescription(env)[0]
> >    return 
> >
> > self.payload.environmentDescription(self.environment)[0]
> >  def initialize(self):
> >      NormalSpoke.initialize(self)
> >
> > @@ -308,6 +340,14 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >          self._environmentListBox.show_all()
> >          self._addonListBox.show_all()
> >    # Pre-select the default environment when the user enters 
> >
> > the software spoke
> >    # to fix an invalid environment set in kickstart. 
> >
> > Otherwise the default environment
> >    # would still look as selected but the selection would not 
> >
> > be applied after returning
> >    # to the hub.
> >    if flags.automatedInstall and self.environment is None:
> >        row = self._environmentListBox.get_row_at_index(0)
> >        row.activate()
>
> Shouldn't this check 'self.environment is False' as set above in
> _payload_finished()?

I think I'll refactor this part a bit more because the current code
here is too insane.

[PATCH 2/2] Handle human readable environment names from kickstart (#1234890)

[Martin Kolman]

Correctly support human readable & localized human readable
environment specifications from kickstart, not just the
"machine readable" environment id.

Also 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.

Related: rhbz#1234890
Signed-off-by: Martin Kolman 
---
 pyanaconda/packaging/yumpayload.py   | 14 ++++++++++++
 pyanaconda/ui/gui/spokes/software.py | 43 +++++++++++++++++++++++++++---------
 2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
index ee9a059..11e4f3b 100644
--- a/pyanaconda/packaging/yumpayload.py
+++ b/pyanaconda/packaging/yumpayload.py
@@ -1006,6 +1006,20 @@ reposdir=%s

return (environment.ui_name, environment.ui_description)

+    def environmentId(self, environmentid):
+        """ Return environment id for the environment specified by id or name."""
+        groups = self._yumGroups
+        if not groups:
+            return environmentid
+
+        with _yum_lock:
+            if not groups.has_environment(environmentid):
+                raise NoSuchGroup(environmentid)
+
+            environment = groups.return_environment(environmentid)
+
+            return environment.environmentid
+
     def selectEnvironment(self, environmentid):
         groups = self._yumGroups
         if not groups:
diff --git a/pyanaconda/ui/gui/spokes/software.py b/pyanaconda/ui/gui/spokes/software.py
index 4801643..a32c4f7 100644
--- a/pyanaconda/ui/gui/spokes/software.py
+++ b/pyanaconda/ui/gui/spokes/software.py
@@ -23,7 +23,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

@@ -109,6 +109,24 @@ class SoftwareSelectionSpoke(NormalSpoke):
         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)
@@ -120,7 +138,12 @@ class SoftwareSelectionSpoke(NormalSpoke):
         if self.environment is None:
             return True
         else:
-            return self.environment in self.payload.environments
+            try:
+                # check if the given environment name or id has a description,
+                # as a test of environment specification validity
+                return bool(self.payload.environmentDescription(self.environment))
+            except NoSuchGroup:
+                return False

def _payload_finished(self):
         if self.environment_valid:
@@ -310,7 +333,7 @@ class SoftwareSelectionSpoke(NormalSpoke):

threadMgr.wait(constants.THREAD_PAYLOAD)

-        if self.environment not in self.payload.environments:
+        if self.environmentid not in self.payload.environments:
             self.environment = None

firstEnvironment = True
@@ -369,7 +392,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
         self._add_row(self._addonListBox, name, desc, check, self.on_checkbox_toggled)

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,
@@ -382,10 +405,10 @@ class SoftwareSelectionSpoke(NormalSpoke):
             # state will be used. Otherwise, the add-on will be selected if it is a default
             # for this environment.

-            addSep = len(self.payload.environmentAddons[self.environment][0]) > 0 and \
-                     len(self.payload.environmentAddons[self.environment][1]) > 0
+            addSep = len(self.payload.environmentAddons[self.environmentid][0]) > 0 and \
+                     len(self.payload.environmentAddons[self.environmentid][1]) > 0

-            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
@@ -393,7 +416,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
             if 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
@@ -404,9 +427,9 @@ class SoftwareSelectionSpoke(NormalSpoke):
             self.clear_info()

def _allAddons(self):
-        return self.payload.environmentAddons[self.environment][0] + \
+        return self.payload.environmentAddons[self.environmentid][0] + \
                [""] + \
-               self.payload.environmentAddons[self.environment][1]
+               self.payload.environmentAddons[self.environmentid][1]

def _get_selected_addons(self):
         retval = []

[PATCH 2/2] Handle human readable environment names from kickstart (#1234890)

[Vratislav Podzimek]

On Fri, 2015-06-26 at 15:31 +0200, Martin Kolman wrote:
> Correctly support human readable & localized human readable
> environment specifications from kickstart, not just the
> "machine readable" environment id.
>
> Also 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.
>
> Related: rhbz#1234890
> Signed-off-by: Martin Kolman
>
> pyanaconda/packaging/yumpayload.py   | 14 ++++++++++++
>  pyanaconda/ui/gui/spokes/software.py | 43 +++++++++++++++++++++++++++---------
>  2 files changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
> index ee9a059..11e4f3b 100644
> --- a/pyanaconda/packaging/yumpayload.py
> +++ b/pyanaconda/packaging/yumpayload.py
> @@ -1006,6 +1006,20 @@ reposdir=%s
>          return (environment.ui_name, environment.ui_description)
>
> def environmentId(self, environmentid):

I think 'environment' would be a better name for the argument as this
looks confusing otherwise.
>    """ Return environment id for the environment specified by id or name."""
>    groups = self._yumGroups
>    if not groups:
>        return environmentid
>    with _yum_lock:
>        if not groups.has_environment(environmentid):
>            raise NoSuchGroup(environmentid)
>        environment = groups.return_environment(environmentid)
>        return environment.environmentid
> def selectEnvironment(self, environmentid):
>        groups = self._yumGroups
>        if not groups:

[PATCH 2/2] Handle human readable environment names from kickstart (#1234890)

[Martin Kolman]

On Mon, 2015-06-29 at 13:40 +0200, Vratislav Podzimek wrote:
> On Fri, 2015-06-26 at 15:31 +0200, Martin Kolman wrote:
> > Correctly support human readable & localized human readable
> > environment specifications from kickstart, not just the
> > "machine readable" environment id.
> >
> > Also 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.
> >
> > Related: rhbz#1234890
> > Signed-off-by: Martin Kolman
> >
> > pyanaconda/packaging/yumpayload.py   | 14 ++++++++++++
> >  pyanaconda/ui/gui/spokes/software.py | 43 
> > +++++++++++++++++++++++++++---------
> >  2 files changed, 47 insertions(+), 10 deletions(-)
> >
> > diff --git a/pyanaconda/packaging/yumpayload.py 
> > b/pyanaconda/packaging/yumpayload.py
> > index ee9a059..11e4f3b 100644
> > --- a/pyanaconda/packaging/yumpayload.py
> > +++ b/pyanaconda/packaging/yumpayload.py
> > @@ -1006,6 +1006,20 @@ reposdir=%s
> >          return (environment.ui_name, 
> >
> > environment.ui_description)
> > def environmentId(self, environmentid):
>
> I think 'environment' would be a better name for the argument as this
> looks confusing otherwise.

OK, fixing locally. :)
> >    """ Return environment id for the environment specified by 
> >
> > id or name."""
> >    groups = self._yumGroups
> >    if not groups:
> >        return environmentid
> >    with _yum_lock:
> >        if not groups.has_environment(environmentid):
> >            raise NoSuchGroup(environmentid)
> >        environment = groups.return_environment(environmentid)
> >        return environment.environmentid
> > def selectEnvironment(self, environmentid):
> >        groups = self._yumGroups
> >        if not groups:

[Vratislav Podzimek]

On Fri, 2015-06-26 at 15:31 +0200, Martin Kolman wrote:
> So it turns out that there are multiple issues with how we handle environment
> specifications from kickstart:
> an invalid (not known by the payload) environment specified in kickstart causes a crash
> valid human readable environment specifications, such as:
> "Infrastructure Server", which is the human readable form of "infrastructure-server-environment",
> are not considered valid, the same thing happens for translated human readable environment names
> environments that have been manually selected in the UI are not written to the output
> kickstart
>
> The first patch should fix issues 1) + 3) and the second patch should fix issue 2).
>
> Martin Kolman (2):
>   Don't crash if incorrect environment is set in kickstart (#1234890)
>   Handle human readable environment names from kickstart (#1234890)
>
> pyanaconda/packaging/yumpayload.py   |  14 +++++
>  pyanaconda/ui/gui/spokes/software.py | 117 ++++++++++++++++++++++++++---------
>  2 files changed, 101 insertions(+), 30 deletions(-)

Looks good to me other than the few inline comments/questions.

[anaconda][rhel7-branch][v2][PATCH] Fix environment handling

[Martin Kolman]

I discovered more potential issues with how we handle environments
and I've also found a few more way of making the code more usable and robust.
Changes from v1:

- squash the to patches together for better readability
- further refactoring of the refresh, completed and status properties
- more comments for the various states the software spoke can be in
- handle previously valid environments becoming invalid due to source
  switching
- don't tick any radio buttons if no environment is selected
-> such as if the user specified a wrong environment in kickstart and we want
   him to select one of the valid environments from the list
-> thanks to Vratislav & David Shea for the idea how to do that
   (invisible RadioButton)

Martin Kolman (1):
  Don't crash if incorrect environment is set in kickstart (#1234890)

pyanaconda/packaging/yumpayload.py   |  14 +++
 pyanaconda/ui/gui/spokes/software.py | 173 +++++++++++++++++++++++------------
 2 files changed, 131 insertions(+), 56 deletions(-)

[PATCH] Don't crash if incorrect environment is set in kickstart (#1234890)

[Martin Kolman]

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)
- don't tick any radio buttons if no environment is selected
- make the code more sane and maintainable

Resolves: rhbz#1234890
Signed-off-by: Martin Kolman 
---
 pyanaconda/packaging/yumpayload.py   |  14 +++
 pyanaconda/ui/gui/spokes/software.py | 173 +++++++++++++++++++++++------------
 2 files changed, 131 insertions(+), 56 deletions(-)

diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
index ee9a059..12b48f8 100644
--- a/pyanaconda/packaging/yumpayload.py
+++ b/pyanaconda/packaging/yumpayload.py
@@ -1006,6 +1006,20 @@ reposdir=%s

return (environment.ui_name, environment.ui_description)

+    def environmentId(self, environment):
+        """ Return environment id for the environment specified by id or name."""
+        groups = self._yumGroups
+        if not groups:
+            return environment
+
+        with _yum_lock:
+            if not groups.has_environment(environment):
+                raise NoSuchGroup(environment)
+
+            environment_instance = groups.return_environment(environment)
+
+            return environment_instance.environmentid
+
     def selectEnvironment(self, environmentid):
         groups = self._yumGroups
         if not groups:
diff --git a/pyanaconda/ui/gui/spokes/software.py b/pyanaconda/ui/gui/spokes/software.py
index 17498af..25eb596 100644
--- a/pyanaconda/ui/gui/spokes/software.py
+++ b/pyanaconda/ui/gui/spokes/software.py
@@ -23,7 +23,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

@@ -67,7 +67,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")
@@ -93,6 +92,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):
         hubQ.send_message(self.__class__.__name__, _("Downloading package metadata..."))
@@ -100,15 +104,62 @@ class SoftwareSelectionSpoke(NormalSpoke):
     def _downloading_group_md(self):
         hubQ.send_message(self.__class__.__name__, _("Downloading group metadata..."))

+    @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):
         hubQ.send_message(self.__class__.__name__, payloadMgr.error)

def _apply(self):
-        env = self._get_selected_environment()
-        if not env:
+        if not self.environment:
             return

addons = self._get_selected_addons()
@@ -118,8 +169,7 @@ class SoftwareSelectionSpoke(NormalSpoke):

self._selectFlag = False
         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)

@@ -134,7 +184,6 @@ class SoftwareSelectionSpoke(NormalSpoke):

def apply(self):
         self._apply()
-        self.data.packages.seen = True

def checkSoftwareSelection(self):
         from pyanaconda.packaging import DependencyError
@@ -158,23 +207,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 flags.automatedInstall:
-            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

@@ -211,17 +269,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 flags.automatedInstall and self.data.packages.seen:
-                return _("Custom software selected")
+        # 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
+            else:
+                return _("Nothing selected")

-            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(env)[0]
+        return self.payload.environmentDescription(self.environment)[0]

def initialize(self):
         NormalSpoke.initialize(self)
@@ -278,31 +345,31 @@ class SoftwareSelectionSpoke(NormalSpoke):

threadMgr.wait(constants.THREAD_PAYLOAD)

-        if self.environment not in self.payload.environments:
-            self.environment = None
-
         firstEnvironment = True
-        firstRadio = None

self._clear_listbox(self._environmentListBox)

-        for environment in self.payload.environments:
-            (name, desc) = self.payload.environmentDescription(environment)
+        # create rows for all valid environments
+        for environmentid in self.payload.environments:
+            (name, desc) = self.payload.environmentDescription(environmentid)

-            radio = Gtk.RadioButton(group=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 flags.automatedInstall 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)
-            firstRadio = firstRadio or radio
-
-            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 install just tick the first radio button
+                # and select the first environment
+                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()
@@ -329,7 +396,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
         self._add_row(self._addonListBox, name, desc, check, self.on_checkbox_toggled)

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,
@@ -342,10 +409,10 @@ class SoftwareSelectionSpoke(NormalSpoke):
             # state will be used. Otherwise, the add-on will be selected if it is a default
             # for this environment.

-            addSep = len(self.payload.environmentAddons[self.environment][0]) > 0 and \
-                     len(self.payload.environmentAddons[self.environment][1]) > 0
+            addSep = len(self.payload.environmentAddons[self.environmentid][0]) > 0 and \
+                     len(self.payload.environmentAddons[self.environmentid][1]) > 0

-            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
@@ -353,7 +420,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
             if 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
@@ -364,9 +431,9 @@ class SoftwareSelectionSpoke(NormalSpoke):
             self.clear_info()

def _allAddons(self):
-        return self.payload.environmentAddons[self.environment][0] + \
+        return self.payload.environmentAddons[self.environmentid][0] + \
                [""] + \
-               self.payload.environmentAddons[self.environment][1]
+               self.payload.environmentAddons[self.environmentid][1]

def _get_selected_addons(self):
         retval = []
@@ -385,12 +452,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)

[PATCH] Don't crash if incorrect environment is set in kickstart (#1234890)

[Vratislav Podzimek]

ACK.