Group transactions: fail on missing non-optional packages

If any default or mandatory package listed in a group does not
exist, `raise dnf.exceptions.MarkingErrors`.

This is a small change with a big impact and a lot of history.
In the early days, dnf treated both 'package doesn't exist'
and 'package exists but is not installable' as fatal errors.

Shortly after Fedora and RHEL switched from yum to dnf, this was
changed, because historically yum had not behaved this way, and
our existing comps definitions had lots of missing packages;
conditional and arch-specific comps entries also were not
properly handled by all tools, so cleaning up comps was not
possible.

dnf was switched for a while to treat neither as fatal, which
turned out to be too permissive, so eventually we gave it the
same behaviour yum used to have, in #1038.

These days, conditional and arch-specific comps entries work. I
have just cleaned up Fedora's comps file for Rawhide, so there
should be no 'missing' mandatory or default packages in any
group: https://pagure.io/fedora-comps/pull-request/767

I don't know if RHEL's or CentOS's comps have been cleaned up,
but if not, this presents an excellent opportunity to do it.

Links to the history here:
https://bugzilla.redhat.com/show_bug.cgi?id=1292892
https://bugzilla.redhat.com/show_bug.cgi?id=1427365
https://bugzilla.redhat.com/show_bug.cgi?id=1461539
#1038

Signed-off-by: Adam Williamson <[email protected]>

dnf/base.py

@@ -75,6 +75,7 @@
 import gc
 import hawkey
 import itertools
+import libcomps
 import logging
 import math
 import os
@@ -1695,6 +1696,7 @@ def trans_remove(query, remove_query, comps_pkg):
                    (trans.upgrade, trans_upgrade),
                    (trans.remove, trans_remove))
 
+        missing_fatals = []
         for (attr, fn) in attr_fn:
             for comps_pkg in attr:
                 query_args = {'name': comps_pkg.name}
@@ -1706,13 +1708,21 @@ def trans_remove(query, remove_query, comps_pkg):
                     package_string = comps_pkg.name
                     if comps_pkg.basearchonly:
                         package_string += '.' + basearch
-                    logger.warning(_('No match for group package "{}"').format(package_string))
+                    if comps_pkg.type == libcomps.PACKAGE_TYPE_OPTIONAL:
+                        logger.warning(_('No match for group package "{}"').format(package_string))
+                    else:
+                        missing_fatals.append(package_string)
                     continue
                 remove_query = fn(q, remove_query, comps_pkg)
                 self._goal.group_members.add(comps_pkg.name)
 
+        # we do this before raising the exception for missing packages
+        # so apps can catch the exception and proceed if desired
         self._remove_if_unneeded(remove_query)
 
+        if missing_fatals:
+            raise dnf.exceptions.MarkingErrors(no_match_pkg_specs=missing_fatals)
+
     def _build_comps_solver(self):
         def reason_fn(pkgname):
             q = self.sack.query().installed().filterm(name=pkgname)

tests/repos/main_comps.xml

@@ -43,17 +43,25 @@
     <id>broken-group</id>
     <name>Broken Group</name>
     <packagelist>
-      <packagereq type="mandatory">meaning-of-life</packagereq>
       <packagereq type="mandatory">lotus</packagereq>
       <packagereq type="default" requires="no-such-package">librita</packagereq>
       <packagereq type="optional">brokendeps</packagereq>
+      <packagereq type="optional">no-such-package</packagereq>
+    </packagelist>
+  </group>
+  <group>
+    <id>broken-group-2</id>
+    <name>Broken Group 2</name>
+    <packagelist>
+      <packagereq type="mandatory">no-such-package</packagereq>
+      <packagereq type="default">no-such-package-2</packagereq>
     </packagelist>
   </group>
   <group>
     <id>missing-name-group</id>
     <name></name>
     <packagelist>
-      <packagereq type="mandatory">meaning-of-life</packagereq>
+      <packagereq type="mandatory">lotus</packagereq>
     </packagelist>
   </group>
   <category>

tests/support.py

@@ -95,7 +95,7 @@ def mock_open(mock=None, data=None):
 MAIN_NSOLVABLES = 9
 UPDATES_NSOLVABLES = 4
 AVAILABLE_NSOLVABLES = MAIN_NSOLVABLES + UPDATES_NSOLVABLES
-TOTAL_GROUPS = 5
+TOTAL_GROUPS = 6
 TOTAL_NSOLVABLES = SYSTEM_NSOLVABLES + AVAILABLE_NSOLVABLES
 
 

tests/test_comps.py

@@ -107,15 +107,15 @@ def test_group_packages(self):
     def test_iteration(self):
         comps = self.comps
         self.assertEqual([g.name for g in comps.groups_iter()],
-                         ['Base', 'Solid Ground', "Pepper's", "Broken Group", None])
+                         ['Base', 'Solid Ground', "Pepper's", "Broken Group", "Broken Group 2", None])
         self.assertEqual([c.name for c in comps.categories_iter()],
                          ['Base System'])
         g = dnf.util.first(comps.groups_iter())
         self.assertEqual(g.desc_by_lang['cs'], TRANSLATION)
 
     def test_group_display_order(self):
         self.assertEqual([g.name for g in self.comps.groups],
-                         ["Pepper's", 'Base', 'Solid Ground', 'Broken Group', None])
+                         ["Pepper's", 'Base', 'Solid Ground', 'Broken Group', 'Broken Group 2', None])
 
     def test_packages(self):
         comps = self.comps
@@ -127,7 +127,7 @@ def test_packages(self):
 
     def test_size(self):
         comps = self.comps
-        self.assertLength(comps, 7)
+        self.assertLength(comps, 8)
         self.assertLength(comps.groups, tests.support.TOTAL_GROUPS)
         self.assertLength(comps.categories, 1)
         self.assertLength(comps.environments, 1)

tests/test_groups.py

@@ -199,27 +199,53 @@ def test_group_remove(self):
 
 class ProblemGroupTest(tests.support.ResultTestCase):
     """Test some cases involving problems in groups: packages that
-    don't exist, and packages that exist but cannot be installed. The
-    "broken" group lists three packages. "meaning-of-life", explicitly
-    'default', does not exist. "lotus", implicitly 'mandatory' (no
-    explicit type), exists and is installable. "brokendeps",
-    explicitly 'optional', exists but has broken dependencies. See
+    don't exist, and packages that exist but cannot be installed.
+    broken_group lists four packages. "lotus", explicitly
+    'mandatory', exists and is installable. "librita", explicitly
+    'default', is a conditional entry that exists, but requires a
+    package that doesn't exist. "brokendeps", explicitly 'optional',
+    exists but has broken dependencies. "no-such-package", explicitly
+    'optional', does not exist.
+    broken_group_2 lists two packages, one explicitly mandatory and
+    one explicitly default, neither of which exists.
+    missing_name_group is a group which has no name. The package it
+    contains exists and is not broken.
+    See
     https://bugzilla.redhat.com/show_bug.cgi?id=1292892,
     https://bugzilla.redhat.com/show_bug.cgi?id=1337731,
-    https://bugzilla.redhat.com/show_bug.cgi?id=1427365, and
-    https://bugzilla.redhat.com/show_bug.cgi?id=1461539 for some of
-    the background on this.
+    https://bugzilla.redhat.com/show_bug.cgi?id=1427365,
+    https://bugzilla.redhat.com/show_bug.cgi?id=1461539 and
+    https://github.com/rpm-software-management/dnf/pull/1848
+    for some of the background on this.
     """
 
     REPOS = ['main', 'broken_group']
     COMPS = True
     COMPS_SEED_PERSISTOR = True
 
+    def test_group_install_missing(self):
+        """Here we will test installing a group with two packages,
+        one mandatory and one default, both non-existent. We should
+        raise an error with both packages in it.
+        """
+        comps_group = self.base.comps.group_by_pattern('Broken Group 2')
+        swdb_group = self.history.group.get(comps_group.id)
+        self.assertIsNone(swdb_group)
+
+        cnt = self.base.group_install(comps_group.id, ('mandatory', 'default', 'optional'))
+        self.assertEqual(cnt, 2)
+
+        self._swdb_commit()
+        with self.assertRaises(dnf.exceptions.MarkingErrors) as exccm:
+            self.base.resolve()
+        missing = set(exccm.exception.no_match_pkg_specs)
+        expected = set(['no-such-package', 'no-such-package-2'])
+        self.assertEqual(missing, expected)
+
     def test_group_install_broken_mandatory(self):
         """Here we will test installing the group with only mandatory
-        packages. We expect this to succeed, leaving out the
-        non-existent 'meaning-of-life': it should also log a warning,
-        but we don't test that.
+        packages. We expect this to succeed. It shouldn't log any
+        warnings either, but we don't test that.
         """
         comps_group = self.base.comps.group_by_pattern('Broken Group')
         swdb_group = self.history.group.get(comps_group.id)
@@ -228,11 +254,8 @@ def test_group_install_broken_mandatory(self):
         cnt = self.base.group_install(comps_group.id, ('mandatory',))
         self._swdb_commit()
         self.base.resolve()
-        # this counts packages *listed* in the group, so 2
-        self.assertEqual(cnt, 2)
-
+        self.assertEqual(cnt, 1)
         inst, removed = self.installed_removed(self.base)
-        # the above should work, but only 'lotus' actually installed
         self.assertLength(inst, 1)
         self.assertEmpty(removed)
 
@@ -251,8 +274,8 @@ def test_group_install_broken_default(self):
         cnt = self.base.group_install(comps_group.id, ('mandatory', 'default'))
         self._swdb_commit()
         self.base.resolve()
-        # this counts packages *listed* in the group, so 3
-        self.assertEqual(cnt, 3)
+        # this counts packages *listed* in the group, so 2
+        self.assertEqual(cnt, 2)
 
         inst, removed = self.installed_removed(self.base)
         # the above should work, but only 'lotus' actually installed
@@ -278,7 +301,8 @@ def test_group_install_broken_optional(self):
     def test_group_install_broken_optional_nonstrict(self):
         """Here we test installing the group with optional packages
         included, but with strict=False. We expect this to succeed,
-        skipping the package with broken dependencies.
+        skipping the package with broken dependencies, and the package
+        that doesn't exist.
         """
         comps_group = self.base.comps.group_by_pattern('Broken Group')
         swdb_group = self.history.group.get(comps_group.id)