From f0f037db8219b1e74be4ed86f5eea53b63ca1d88 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Hr=C3=A1zk=C3=BD?= <lhrazky@redhat.com>

Date: Tue, 20 Jul 2021 15:29:59 +0200

Subject: [PATCH] comps: Make the install_or_skip() method not catch CompsError

 anymore

According to its docstring, the original intention of the method was to

not fail on installing an already installed group/environment.

However, the CompsError is no longer thrown when attempting to install

an already installed group or environment. It was changed to logging a

warning directly in 5210b9dc and then the check was removed completely

in 217ca0fa.

For the other case for which an instance of CompsError can be thrown

from the install_group() and install_environment() methods, which is

when a group or environment is not found, we certainly want to throw an

error (see the linked bugs), therefore there's no reason to catch the

exception anymore.

The install_or_skip() method is preserved as part of the API so as not

to break compatibility any more than necessary.

msg: API: Raise CompsError when group/env not found in install_group and install_environment

type: bugfix

resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1947958

related: https://bugzilla.redhat.com/show_bug.cgi?id=1943206

---

 dnf/base.py               |  8 ++------

 dnf/cli/commands/group.py |  4 ++--

 dnf/comps.py              | 20 ++++++++++----------

 doc/api_base.rst          |  4 ++--

 4 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/dnf/base.py b/dnf/base.py

index c258a5a7..babca31d 100644

--- a/dnf/base.py

+++ b/dnf/base.py

@@ -1668,9 +1668,7 @@ class Base(object):

         if not isinstance(types, int):

             types = libdnf.transaction.listToCompsPackageType(types)

-        trans = dnf.comps.install_or_skip(solver._environment_install,

-                                          env_id, types, exclude or set(),

-                                          strict, exclude_groups)

+        trans = solver._environment_install(env_id, types, exclude or set(), strict, exclude_groups)

         if not trans:

             return 0

         return self._add_comps_trans(trans)

@@ -1713,9 +1711,7 @@ class Base(object):

         if not isinstance(pkg_types, int):

             pkg_types = libdnf.transaction.listToCompsPackageType(pkg_types)

-        trans = dnf.comps.install_or_skip(solver._group_install,

-                                          grp_id, pkg_types, exclude_pkgnames,

-                                          strict)

+        trans = solver._group_install(grp_id, pkg_types, exclude_pkgnames, strict)

         if not trans:

             return 0

         if strict:

diff --git a/dnf/cli/commands/group.py b/dnf/cli/commands/group.py

index cf542799..fd723c48 100644

--- a/dnf/cli/commands/group.py

+++ b/dnf/cli/commands/group.py

@@ -244,9 +244,9 @@ class GroupCommand(commands.Command):

             types = tuple(self.base.conf.group_package_types)

         pkg_types = libdnf.transaction.listToCompsPackageType(types)

         for env_id in res.environments:

-            dnf.comps.install_or_skip(solver._environment_install, env_id, pkg_types)

+            solver._environment_install(env_id, pkg_types)

         for group_id in res.groups:

-            dnf.comps.install_or_skip(solver._group_install, group_id, pkg_types)

+            solver._group_install(group_id, pkg_types)

     def _mark_remove(self, patterns):

         q = CompsQuery(self.base.comps, self.base.history,

diff --git a/dnf/comps.py b/dnf/comps.py

index 89765337..461eb274 100644

--- a/dnf/comps.py

+++ b/dnf/comps.py

@@ -93,15 +93,15 @@ def _fn_display_order(group):

 def install_or_skip(install_fnc, grp_or_env_id, types, exclude=None,

                     strict=True, exclude_groups=None):

-    """Either mark in persistor as installed given `grp_or_env` (group

-       or environment) or skip it (if it's already installed).

-       `install_fnc` has to be Solver._group_install

-       or Solver._environment_install.

-       """

-    try:

-        return install_fnc(grp_or_env_id, types, exclude, strict, exclude_groups)

-    except dnf.comps.CompsError as e:

-        logger.warning("%s, %s", ucd(e)[:-1], _("skipping."))

+    """

+    Installs a group or an environment identified by grp_or_env_id.

+    This method is preserved for API compatibility. It used to catch an

+    exception thrown when a gorup or env was already installed, which is no

+    longer thrown.

+    `install_fnc` has to be Solver._group_install or

+    Solver._environment_install.

+    """

+    return install_fnc(grp_or_env_id, types, exclude, strict, exclude_groups)

 class _Langs(object):

@@ -592,7 +592,7 @@ class Solver(object):

         assert dnf.util.is_string_type(group_id)

         return self.history.env.is_removable_group(group_id)

-    def _environment_install(self, env_id, pkg_types, exclude, strict=True, exclude_groups=None):

+    def _environment_install(self, env_id, pkg_types, exclude=None, strict=True, exclude_groups=None):

         assert dnf.util.is_string_type(env_id)

         comps_env = self.comps._environment_by_id(env_id)

         if not comps_env:

diff --git a/doc/api_base.rst b/doc/api_base.rst

index 20d7945e..03396b69 100644

--- a/doc/api_base.rst

+++ b/doc/api_base.rst

@@ -179,7 +179,7 @@

   .. method:: group_install(group_id, pkg_types, exclude=None, strict=True)

-    Mark group with corresponding `group_id` installed and mark the packages in the group for installation. Return the number of packages that the operation has marked for installation. `pkg_types` is a sequence of strings determining the kinds of packages to be installed, where the respective groups can be selected by including ``"mandatory"``, ``"default"`` or ``"optional"`` in it. If `exclude` is given, it has to be an iterable of package name glob patterns: :meth:`.group_install` will then not mark the respective packages for installation whenever possible. Parameter `strict` is a boolean indicating whether group packages that exist but are non-installable due to e.g. dependency issues should be skipped (False) or cause transaction to fail to resolve (True).

+    Mark group with corresponding `group_id` installed and mark the packages in the group for installation. Return the number of packages that the operation has marked for installation. `pkg_types` is a sequence of strings determining the kinds of packages to be installed, where the respective groups can be selected by including ``"mandatory"``, ``"default"`` or ``"optional"`` in it. If `exclude` is given, it has to be an iterable of package name glob patterns: :meth:`.group_install` will then not mark the respective packages for installation whenever possible. Parameter `strict` is a boolean indicating whether group packages that exist but are non-installable due to e.g. dependency issues should be skipped (False) or cause transaction to fail to resolve (True).  Raises :exc:`dnf.exceptions.CompsError` in case the group doesn't exist.

   .. method:: group_remove(group_id)

@@ -191,7 +191,7 @@

   .. method:: environment_install(env_id, types, exclude=None, strict=True, exclude_groups=None)

-    Similar to :meth:`.group_install` but operates on environmental groups. `exclude_groups` is an iterable of group IDs that will not be marked as installed.

+    Similar to :meth:`.group_install` but operates on environmental groups. `exclude_groups` is an iterable of group IDs that will not be marked as installed.  Raises :exc:`dnf.exceptions.CompsError` in case the group doesn't exist.

   .. method:: environment_remove(env_id)

-- 

2.35.1