From edbd86922b2733fd36622abadca15e744d12bfde Mon Sep 17 00:00:00 2001

From: Adam Williamson <awilliam@redhat.com>

Date: Tue, 13 Mar 2018 15:33:24 -0700

Subject: [PATCH] Restore 'strict' choice for group installs (#1461539)

This makes the `strict` parameter to `group_install` mean

something again.

When it is set `True`, DNF will behave as yum previously did,

which has been a request for several years now. 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

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

And commits:

91f9ce98dbb630800017f44180d636af395435cd

1e1901822748b754d038dd396655c997281a7201

We have been around the houses on this multiple times. @mikem23

actually got things right way way back in #1292892:

"yum-deprecated behaves more sanely here.

In case 1 [package exists but has dependency issues], it reports

the missing dependency and exits with an error

...

In case 2 [package cannot be found at all], it exits cleanly, but

prints a warning."

Note that this applies to yum's *default* behaviour: it had an

option, --skip-broken, which caused it to skip packages with

dependency errors.

That's exactly what we've wanted all along, and it's also what

1461539 requests. Unfortunately, the initial commit intended to

fix #1292892 - that's 91f9ce9 - did not quite behave as Mike

requested. It treated any 'mandatory' package having dependency

errors *or* not existing at all as fatal errors, while not

treating non-existence *or* errors on the part of a 'default'

or 'optional' package as a fatal error. This introduced *two*

differences from yum's behaviour (making the comps 'type' matter,

which it never did to yum, and failing on non-existence), which

I think was the source of quite a lot of confusion.

We then wound up filing bugs on this - like #1337731 - and the

response to that was 1e19018, which causes dnf to treat *neither*

case as a fatal error for *any* package, which is where we stand

at present.

When resolving a transaction, currently, if it cannot find a

package corresponding to an entry in a group, dnf will log a

warning and carry on. This is good and fine and just what we want

it to do. However, when a package is available but cannot be

installed, it does the wrong thing. The various forks of this

`trans_install` internal function all wind up passing

`optional=True` to the Goal, which tells it that it's fine to

just leave the package out if its dependencies can't be resolved.

Additionally, if this happens, `resolve()` itself does not log

anything.

If you try a group install through the CLI, it will tell you

about packages that have been left out due to dependency errors,

via the `_skipped_packages` function in `dnf/output.py` which

actually sucks them out of the Goal instance itself. But that's

no use to anything besides the CLI. Other things which use

`resolve()`, like anaconda, have no means of accessing this

information, so when this happens to them, the omission of the

package is not logged in any way at all, and they have no way to

find out about it.

If `strict` is set to `False`, the current behaviour described

above will be used: non-installable packages will be skipped. We

implement this by restoring the old separation between `install`

and `install_opt` which was removed in 1e19018. When group_install

is called with `strict=True` we add the packages to the `install`

set; when it's called with `strict=False` we add the packages to

the `install_opt` set. Then `_finalize_comps_trans` requests

strict behaviour for `install` and non-strict for `install_opt`.

It continues to be the case that the comps 'type' - mandatory,

default, or optional - does not matter; the same behaviour will be

used for all three. Again, this is how yum behaved (in the

absence of --skip-broken)

The comps 'types' were *never* intended to dictate yum/dnf

behaviour in terms of whether it should fail on error or not, as I

understand it; they were in fact related to installer UI behaviour.

In the old installer UI, after you selected a group, you would see

all the packages in the group listed. 'mandatory' packages would

be pre-checked for installation and the check would be greyed out

- you couldn't unselect them. 'default' packages would be

pre-checked for installation, but you *could* de-select them if you

wanted. 'optional' packages would not be pre-checked for

installation, but you could *select* them if you wanted. This was

the intent of the comps 'type', as I understand it.

Importantly, a package not existing at all will continue to *not*

be a fatal error whatever `strict` is set to. as this is handled

*before* we reach the `trans_install` function.

Signed-off-by: Adam Williamson <awilliam@redhat.com>

---

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

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

 tests/repos/broken_group.repo |   4 ++

 tests/repos/main_comps.xml    |   4 +-

 tests/test_groups.py          | 122 +++++++++++++++++++++++++++-------

 5 files changed, 145 insertions(+), 33 deletions(-)

 create mode 100644 tests/repos/broken_group.repo

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

index 88e9467a..dacc42d9 100644

--- a/dnf/base.py

+++ b/dnf/base.py

@@ -1450,17 +1450,17 @@ class Base(object):

             self._goal.upgrade(select=sltr)

             return remove_query

-        def trans_install(query, remove_query, comps_pkg):

+        def trans_install(query, remove_query, comps_pkg, strict):

             if self.conf.multilib_policy == "all":

                 if not comps_pkg.requires:

-                    self._install_multiarch(query, strict=False)

+                    self._install_multiarch(query, strict=strict)

                 else:

                     # it installs only one arch for conditional packages

                     installed_query = query.installed().apply()

                     self._report_already_installed(installed_query)

                     sltr = dnf.selector.Selector(self.sack)

                     sltr.set(provides="({} if {})".format(comps_pkg.name, comps_pkg.requires))

-                    self._goal.install(select=sltr, optional=True)

+                    self._goal.install(select=sltr, optional=not strict)

             else:

                 sltr = dnf.selector.Selector(self.sack)

@@ -1468,7 +1468,7 @@ class Base(object):

                     sltr.set(provides="({} if {})".format(comps_pkg.name, comps_pkg.requires))

                 else:

                     sltr.set(pkg=query)

-                self._goal.install(select=sltr, optional=True)

+                self._goal.install(select=sltr, optional=not strict)

             return remove_query

         def trans_remove(query, remove_query, comps_pkg):

@@ -1476,7 +1476,8 @@ class Base(object):

             return remove_query

         remove_query = self.sack.query().filterm(empty=True)

-        attr_fn = ((trans.install, trans_install),

+        attr_fn = ((trans.install, functools.partial(trans_install, strict=True)),

+                   (trans.install_opt, functools.partial(trans_install, strict=False)),

                    (trans.upgrade, trans_upgrade),

                    (trans.remove, trans_remove))

@@ -1547,6 +1548,10 @@ class Base(object):

         """Installs packages of selected group

         :param exclude: list of package name glob patterns

             that will be excluded from install set

+        :param strict: 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)

         """

         def _pattern_to_pkgname(pattern):

             if dnf.util.is_glob_pattern(pattern):

@@ -1568,8 +1573,12 @@ class Base(object):

                                           strict)

         if not trans:

             return 0

+        if strict:

+            instlog = trans.install

+        else:

+            instlog = trans.install_opt

         logger.debug(_("Adding packages from group '%s': %s"),

-                     grp_id, trans.install)

+                     grp_id, instlog)

         return self._add_comps_trans(trans)

     def env_group_install(self, patterns, types, strict=True, exclude=None, exclude_groups=None):

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

index 079d21be..8743812a 100644

--- a/dnf/comps.py

+++ b/dnf/comps.py

@@ -476,18 +476,20 @@ class CompsTransPkg(object):

 class TransactionBunch(object):

     def __init__(self):

         self._install = set()

+        self._install_opt = set()

         self._remove = set()

         self._upgrade = set()

     def __iadd__(self, other):

         self._install.update(other._install)

+        self._install_opt.update(other._install_opt)

         self._upgrade.update(other._upgrade)

         self._remove = (self._remove | other._remove) - \

-            self._install - self._upgrade

+            self._install - self._install_opt - self._upgrade

         return self

     def __len__(self):

-        return len(self.install) + len(self.upgrade) + len(self.remove)

+        return len(self.install) + len(self.install_opt) + len(self.upgrade) + len(self.remove)

     @staticmethod

     def _set_value(param, val):

@@ -499,12 +501,28 @@ class TransactionBunch(object):

     @property

     def install(self):

+        """

+        Packages to be installed with strict=True - transaction will

+        fail if they cannot be installed due to dependency errors etc.

+        """

         return self._install

     @install.setter

     def install(self, value):

         self._set_value(self._install, value)

+    @property

+    def install_opt(self):

+        """

+        Packages to be installed with strict=False - they will be

+        skipped if they cannot be installed

+        """

+        return self._install_opt

+

+    @install_opt.setter

+    def install_opt(self, value):

+        self._set_value(self._install_opt, value)

+

     @property

     def remove(self):

         return self._remove

@@ -641,7 +659,10 @@ class Solver(object):

         trans = TransactionBunch()

         # TODO: remove exclude

-        trans.install.update(self._pkgs_of_type(comps_group, pkg_types, exclude=[]))

+        if strict:

+            trans.install.update(self._pkgs_of_type(comps_group, pkg_types, exclude=[]))

+        else:

+            trans.install_opt.update(self._pkgs_of_type(comps_group, pkg_types, exclude=[]))

         return trans

     def _group_remove(self, group_id):

diff --git a/tests/repos/broken_group.repo b/tests/repos/broken_group.repo

new file mode 100644

index 00000000..7f151971

--- /dev/null

+++ b/tests/repos/broken_group.repo

@@ -0,0 +1,4 @@

+=Ver: 2.0

+#

+=Pkg: brokendeps 20 2 x86_64

+=Req: nosuchpackage >= 1.2-0

diff --git a/tests/repos/main_comps.xml b/tests/repos/main_comps.xml

index 3cf8faa5..9e694d13 100644

--- a/tests/repos/main_comps.xml

+++ b/tests/repos/main_comps.xml

@@ -44,7 +44,9 @@

     <name>Broken Group</name>

     <packagelist>

       <packagereq type="mandatory">meaning-of-life</packagereq>

-      <packagereq>lotus</packagereq>

+      <packagereq type="mandatory">lotus</packagereq>

+      <packagereq type="default" requires="no-such-package">librita</packagereq>

+      <packagereq type="optional">brokendeps</packagereq>

     </packagelist>

   </group>

   <category>

diff --git a/tests/test_groups.py b/tests/test_groups.py

index ec10a619..fe388f96 100644

--- a/tests/test_groups.py

+++ b/tests/test_groups.py

@@ -183,29 +183,6 @@ class PresetPersistorTest(tests.support.ResultTestCase):

         swdb_group = self.history.group.get(comps_group.id)

         self.assertIsNotNone(swdb_group)

-    """

-    this should be reconsidered once relengs document comps

-    def test_group_install_broken(self):

-        grp = self.base.comps.group_by_pattern('Broken Group')

-        p_grp = self.history.group.get('broken-group')

-        self.assertFalse(p_grp.installed)

-

-        self.assertRaises(dnf.exceptions.MarkingError,

-                          self.base.group_install, grp.id,

-                          ('mandatory', 'default'))

-        p_grp = self.history.group.get('broken-group')

-        self.assertFalse(p_grp.installed)

-

-        self.assertEqual(self.base.group_install(grp.id,

-                                                 ('mandatory', 'default'),

-                                                 strict=False), 1)

-        inst, removed = self.installed_removed(self.base)

-        self.assertLength(inst, 1)

-        self.assertEmpty(removed)

-        p_grp = self.history.group.get('broken-group')

-        self.assertTrue(p_grp.installed)

-    """

-

     def test_group_remove(self):

         self._install_test_group()

         group_id = 'somerset'

@@ -220,6 +197,105 @@ class PresetPersistorTest(tests.support.ResultTestCase):

         self._swdb_end()

+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

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

+    """

+

+    REPOS = ['main', 'broken_group']

+    COMPS = True

+    COMPS_SEED_PERSISTOR = True

+

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

+        """

+        comps_group = self.base.comps.group_by_pattern('Broken Group')

+        swdb_group = self.history.group.get(comps_group.id)

+        self.assertIsNone(swdb_group)

+

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

+

+        inst, removed = self.installed_removed(self.base)

+        # the above should work, but only 'lotus' actually installed

+        self.assertLength(inst, 1)

+        self.assertEmpty(removed)

+

+    def test_group_install_broken_default(self):

+        """Here we will test installing the group with only mandatory

+        and default packages. Again we expect this to succeed: the new

+        factor is an entry pulling in librita if no-such-package is

+        also included or installed. We expect this not to actually

+        pull in librita (as no-such-package obviously *isn't* there),

+        but also not to cause a fatal error.

+        """

+        comps_group = self.base.comps.group_by_pattern('Broken Group')

+        swdb_group = self.history.group.get(comps_group.id)

+        self.assertIsNone(swdb_group)

+

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

+

+        inst, removed = self.installed_removed(self.base)

+        # the above should work, but only 'lotus' actually installed

+        self.assertLength(inst, 1)

+        self.assertEmpty(removed)

+

+    def test_group_install_broken_optional(self):

+        """Here we test installing the group with optional packages

+        included. We expect this to fail, as a package that exists but

+        has broken dependencies is now included.

+        """

+        comps_group = self.base.comps.group_by_pattern('Broken Group')

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

+

+        self._swdb_commit()

+        # this should fail, as optional 'brokendeps' is now pulled in

+        self.assertRaises(dnf.exceptions.DepsolveError, self.base.resolve)

+

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

+        """

+        comps_group = self.base.comps.group_by_pattern('Broken Group')

+        swdb_group = self.history.group.get(comps_group.id)

+        self.assertIsNone(swdb_group)

+

+        cnt = self.base.group_install(comps_group.id, ('mandatory', 'default', 'optional'),

+                                      strict=False)

+        self._swdb_commit()

+        self.base.resolve()

+        self.assertEqual(cnt, 4)

+

+        inst, removed = self.installed_removed(self.base)

+        # the above should work, but only 'lotus' actually installed

+        self.assertLength(inst, 1)

+        self.assertEmpty(removed)

+

+

 class EnvironmentInstallTest(tests.support.ResultTestCase):

     """Set up a test where sugar is considered not installed."""

-- 

2.19.0