From edbd86922b2733fd36622abadca15e744d12bfde Mon Sep 17 00:00:00 2001 From: Adam Williamson 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 --- 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 @@ Broken Group meaning-of-life - lotus + lotus + librita + brokendeps 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