Merge branch 'solver3' into 'main'

solver3: Fix Recommends/Suggests vs Enhances confusion

See merge request apt-team/apt!461

6 files changed, 62 insertions, 22 deletions

diff --git a/apt-pkg/solver3.cc b/apt-pkg/solver3.cc
index 739309c59..cebface27 100644
--- a/apt-pkg/solver3.cc
+++ b/apt-pkg/solver3.cc
@@ -831,44 +831,54 @@ APT::Solver::Clause APT::Solver::TranslateOrGroup(pkgCache::DepIterator start, p
    if (clause.optional && start.ParentPkg()->CurrentVer)
    {
       bool important = policy.IsImportantDep(start);
-      bool newOptional = true;
-      bool wasImportant = false;
-      for (auto D = start.ParentPkg().CurrentVer().DependsList(); not D.end(); D++)
-	 if (not D.IsCritical() && not D.IsNegative() && D.TargetPkg() == start.TargetPkg())
-	    newOptional = false, wasImportant = policy.IsImportantDep(D);
-
+      auto importantToKeep = [this](pkgCache::DepIterator d)
+      {
+	 return policy.IsImportantDep(d) || (KeepRecommends && d->Type == pkgCache::Dep::Recommends) || (KeepSuggests && d->Type == pkgCache::Dep::Suggests);
+      };
       bool satisfied = std::any_of(clause.solutions.begin(), clause.solutions.end(), [this](auto var)
 				   { return var.Pkg(cache) ? var.Pkg(cache)->CurrentVer != nullptr : Var(var.CastPkg(cache).CurrentVer()) == var; });
 
-      if (important && wasImportant && not newOptional && not satisfied)
+      // Find the existing dependency
+      pkgCache::DepIterator existing;
+      for (auto D = start.ParentPkg().CurrentVer().DependsList(); not D.end(); D++)
+	 if (not D.IsCritical() && importantToKeep(D) && D.TargetPkg() == start.TargetPkg())
+	 {
+	    existing = D;
+	    break;
+	 }
+
+      if (not existing.end() && not important && importantToKeep(start) && satisfied)
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Ignoring unsatisfied Recommends " << clause.toString(cache) << std::endl;
-	 clause.solutions.clear();
+	    std::cerr << "Try to keep satisfied: " << clause.toString(cache, true) << std::endl;
       }
-      else if (not important && not wasImportant && not newOptional && satisfied)
+      else if (not important)
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Promoting satisfied Suggests to Recommends: " << clause.toString(cache) << std::endl;
-	 important = true;
+	    std::cerr << "Ignore unimportant clause: " << clause.toString(cache, true) << std::endl;
+	 return Clause{reason, Group::Satisfy, true};
       }
-      else if (satisfied && important && wasImportant && clause.solutions.size() > 0)
+      else if (not existing.end() && policy.IsImportantDep(existing) && not satisfied)
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Promoting existing Recommends " << clause.toString(cache) << " to depends in upgrade" << std::endl;
-	 clause.optional = false;
+	    std::cerr << "Ignoring unsatisfied clause: " << clause.toString(cache, true) << std::endl;
+	 return Clause{reason, Group::Satisfy, true};
       }
-      else if (newOptional && important && reason.Ver() && clause.solutions.size() > 0 && reason.Ver(cache) != reason.CastPkg(cache).CurrentVer() && IsUpgrade)
+      else if (IsUpgrade && not existing.end() && satisfied)
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Promoting new Recommends " << clause.toString(cache) << " to depends in upgrade" << std::endl;
+	    std::cerr << "Promoting previously satisfied clause to hard dependency: " << clause.toString(cache, true) << std::endl;
 	 clause.optional = false;
       }
-      else if (not important)
+      else if (
+	 IsUpgrade && not(AllowRemove && AllowInstall)				    // promote Recommends to Depends in upgrade, not in dist-upgrade.
+	 && reason.Ver() && reason.Ver(cache) != reason.CastPkg(cache).CurrentVer() // and if this an upgrade to an installed package
+	 && (existing.end() || not policy.IsImportantDep(existing))		    // new Recommends, or upgraded from Suggests
+      )
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Ignoring Suggests " << clause.toString(cache) << std::endl;
-	 return Clause{reason, Group::Satisfy, true};
+	    std::cerr << "Promoting new clause to hard dependency: " << clause.toString(cache) << std::endl;
+	 clause.optional = false;
       }
    }
 
diff --git a/apt-pkg/solver3.h b/apt-pkg/solver3.h
index b7000c52e..1bdc13559 100644
--- a/apt-pkg/solver3.h
+++ b/apt-pkg/solver3.h
@@ -243,6 +243,11 @@ class Solver
    // \brief If set, we use strict pinning.
    int Timeout{_config->FindI("APT::Solver::Timeout", 10)};
 
+   // \brief Keep recommends installed
+   bool KeepRecommends{_config->FindB("APT::AutoRemove::RecommendsImportant", true)};
+   // \brief Keep suggests installed
+   bool KeepSuggests{_config->FindB("APT::AutoRemove::SuggestsImportant", true)};
+
    // \brief Discover a variable, translating the underlying dependencies to the SAT presentation
    //
    // This does a breadth-first search of the entire dependency tree of var,
diff --git a/test/integration/solver3.broken b/test/integration/solver3.broken
index 4a4e1532b..0e2005d60 100644
--- a/test/integration/solver3.broken
+++ b/test/integration/solver3.broken
@@ -3,6 +3,5 @@ test-apt-get-upgrade-by-source                              # TBD: Upgrading by
 test-apt-install-order-matters-a-bit                        # Wontfix: Cannot fix, the order is not recorded in the depcache
 test-bug-470115-new-and-tighten-recommends                  # TBD: Calculation of what is already satisfied Recommends is broken
 test-prevent-markinstall-multiarch-same-versionscrew        # TBD: We consider the skewed ones obsolete and remove them...
-test-resolve-by-keep-new-recommends                         # TBD: Fixing this seems to break test-bug-591882-conkeror, why?
 test-resolve-by-keep-obsolete-removals                      # TBD: ResolveByKeep() usage is badly aligned here
 test-ubuntu-bug-1304403-obsolete-priority-standard          # TBD: Solver3 here happily removes 10 deps to upgrade a package
diff --git a/test/integration/test-bug-598669-install-postfix-gets-exim-heavy b/test/integration/test-bug-598669-install-postfix-gets-exim-heavy
index 98479b5fd..b3f11956b 100755
--- a/test/integration/test-bug-598669-install-postfix-gets-exim-heavy
+++ b/test/integration/test-bug-598669-install-postfix-gets-exim-heavy
@@ -1,4 +1,4 @@
-t-bug-598669-install-postfix-gets-exim-heavy!/bin/sh
+#!/bin/sh
 set -e
 
 TESTDIR="$(readlink -f "$(dirname "$0")")"
diff --git a/test/integration/test-resolve-by-keep-new-recommends b/test/integration/test-resolve-by-keep-new-recommends
index 3591ed84d..16acbe0ce 100755
--- a/test/integration/test-resolve-by-keep-new-recommends
+++ b/test/integration/test-resolve-by-keep-new-recommends
@@ -19,3 +19,21 @@ The following packages have been kept back:
 0 upgraded, 0 newly installed, 0 to remove and 1 not upgraded."
 testsuccessequal "$UPGRADE_KEEP" aptget upgrade -s
 
+UPGRADE_KEEP="Reading package lists...
+Building dependency tree...
+Calculating upgrade...
+The following packages have been kept back:
+  foo
+0 upgraded, 0 newly installed, 0 to remove and 1 not upgraded."
+testsuccessequal "$UPGRADE_KEEP" apt upgrade -s
+
+UPGRADE_KEEP="Reading package lists...
+Building dependency tree...
+Calculating upgrade...
+The following packages will be upgraded:
+  foo
+1 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
+Inst foo [1.0] (2.0 unstable [i386])
+Conf foo (2.0 unstable [i386])"
+testsuccessequal "$UPGRADE_KEEP" aptget dist-upgrade -s
+
diff --git a/test/integration/test-solver-recommends-depends b/test/integration/test-solver-recommends-depends
index 90c5050a7..4b2f91254 100755
--- a/test/integration/test-solver-recommends-depends
+++ b/test/integration/test-solver-recommends-depends
@@ -13,9 +13,17 @@ insertpackage 'unstable' 'x' 'all' '3' 'Depends: xx
 Recommends: b'
 insertpackage 'unstable' 'xx' 'all' '3' 'Depends: a | b
 Recommends: b'
+insertpackage 'installed,unstable' 'recommends-and-enhances' 'all' '3' 'Recommends: b
+Enhances:b'
 
 setupaptarchive
 
+msgmsg "Test that the Enhances does not confuse the Recommends algorithm"
+testsuccessequal "Reading package lists...
+Building dependency tree...
+Calculating upgrade...
+0 upgraded, 0 newly installed, 0 to remove and 0 not upgraded." apt full-upgrade -s
+
 msgmsg "Test that the Recommends does not influence the Depends"
 testsuccessequal "The following additional packages will be installed:
   a b