Keep going if a dep is bad for user requests to improve errors

We exit early from installing dependencies of a package only if it is
not a user request to avoid polluting the state with installs which
might not be needed (or detrimental even) for alternative choices.

We do continue with installing dependencies though if it is a user
request as it will improve error reporting for apt and can even help
aptitude not hang itself so much as we trim the problem space down for
its resolver dealing with all the easy things.

Similar things can be said about the testcase I have short-circuit
previously… keep going test, do what you should do to report errors!

6 files changed, 75 insertions, 27 deletions

diff --git a/apt-pkg/depcache.cc b/apt-pkg/depcache.cc
index f86ae7d24..83a04a8e8 100644
--- a/apt-pkg/depcache.cc
+++ b/apt-pkg/depcache.cc
@@ -1138,11 +1138,12 @@ bool pkgDepCache::MarkInstall_CollectDependencies(pkgCache::VerIterator const &P
    return true;
 }
 									/*}}}*/
-bool pkgDepCache::MarkInstall_RemoveConflictsIfNotUpgradeable(pkgCache::VerIterator const &PV, unsigned long Depth, std::vector<pkgCache::DepIterator> &toRemove, APT::PackageVector &toUpgrade) /*{{{*/
+bool pkgDepCache::MarkInstall_RemoveConflictsIfNotUpgradeable(pkgCache::VerIterator const &PV, unsigned long Depth, std::vector<pkgCache::DepIterator> &toRemove, APT::PackageVector &toUpgrade, bool const propagateProctected, bool const FromUser) /*{{{*/
 {
    /* Negative dependencies have no or-group
       If the dependency isn't versioned, we try if an upgrade might solve the problem.
       Otherwise we remove the offender if needed */
+   bool failedToRemoveSomething = false;
    for (auto const &D : toRemove)
    {
       std::unique_ptr<Version *[]> List(D.AllTargets());
@@ -1167,40 +1168,51 @@ bool pkgDepCache::MarkInstall_RemoveConflictsIfNotUpgradeable(pkgCache::VerItera
 	    toUpgrade.push_back(Pkg);
 	 else
 	 {
-	    if(DebugAutoInstall == true)
+	    if(DebugAutoInstall)
 	       std::clog << OutputInDepth(Depth) << " Removing: " << Pkg.Name() << " as upgrade is not an option for " << PV.ParentPkg().FullName() << "(" << PV.VerStr() << ")\n";
 	    if (not MarkDelete(Pkg, false, Depth + 1, false))
-	       return false;
-	    if (PkgState[PV.ParentPkg()->ID].Protect())
+	    {
+	       failedToRemoveSomething = true;
+	       if (not propagateProctected && not FromUser)
+		  break;
+	    }
+	    if (propagateProctected)
 	       MarkProtected(Pkg);
 	 }
       }
    }
    toRemove.clear();
-   return true;
+   return not failedToRemoveSomething;
 }
 									/*}}}*/
-bool pkgDepCache::MarkInstall_UpgradeOrRemoveConflicts(bool const propagateProctected, unsigned long Depth, bool const ForceImportantDeps, APT::PackageVector &toUpgrade) /*{{{*/
+bool pkgDepCache::MarkInstall_UpgradeOrRemoveConflicts(unsigned long Depth, bool const ForceImportantDeps, APT::PackageVector &toUpgrade, bool const propagateProctected, bool const FromUser) /*{{{*/
 {
+   bool failedToRemoveSomething = false;
    for (auto const &InstPkg : toUpgrade)
       if (not MarkInstall(InstPkg, true, Depth + 1, false, ForceImportantDeps))
       {
 	 if (DebugAutoInstall)
 	    std::clog << OutputInDepth(Depth) << " Removing: " << InstPkg.FullName() << " as upgrade is not possible\n";
 	 if (not MarkDelete(InstPkg, false, Depth + 1, false))
-	    return false;
-	 if (propagateProctected)
+	 {
+	    failedToRemoveSomething = true;
+	    if (not propagateProctected && not FromUser)
+	       break;
+	 }
+	 else if (propagateProctected)
 	    MarkProtected(InstPkg);
       }
    toUpgrade.clear();
-   return true;
+   return not failedToRemoveSomething;
 }
 									/*}}}*/
 bool pkgDepCache::MarkInstall_InstallDependencies(PkgIterator const &Pkg, unsigned long Depth, bool const ForceImportantDeps, std::vector<pkgCache::DepIterator> &toInstall, APT::PackageVector *const toMoveAuto, bool const propagateProctected, bool const FromUser) /*{{{*/
 {
    auto const IsSatisfiedByInstalled = [&](auto const D) { return (DepState[D.ID] & DepInstall) == DepInstall; };
+   bool failedToInstallSomething = false;
    for (auto &&Dep : toInstall)
    {
+      auto const Copy = Dep;
       pkgDepCache::DepIterator Start, End;
       Dep.GlobOr(Start, End);
       if (std::any_of(Start, Dep, IsSatisfiedByInstalled))
@@ -1301,8 +1313,11 @@ bool pkgDepCache::MarkInstall_InstallDependencies(PkgIterator const &Pkg, unsign
 	 foundSolution = true;
 	 break;
       }
+      if (DebugMarker && not foundSolution)
+	 std::clog << OutputInDepth(Depth+1) << APT::PrettyDep(this, Copy) << " can't be satisfied! (dep)\n";
       if (not foundSolution && IsCriticalDep)
       {
+	 failedToInstallSomething = true;
 	 if (not propagateProctected && not FromUser)
 	 {
 	    StateCache &State = PkgState[Pkg->ID];
@@ -1317,12 +1332,12 @@ bool pkgDepCache::MarkInstall_InstallDependencies(PkgIterator const &Pkg, unsign
 	    AddStates(Pkg);
 	    Update(Pkg);
 	    AddSizes(Pkg);
+	    break;
 	 }
-	 return false;
       }
    }
    toInstall.clear();
-   return true;
+   return not failedToInstallSomething;
 }
 									/*}}}*/
 // DepCache::MarkInstall - Put the package in the install state		/*{{{*/
@@ -1359,6 +1374,8 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg, bool AutoInst,
       return false;
 
    bool const AutoSolve = AutoInst && _config->Find("APT::Solver", "internal") == "internal";
+   bool const failEarly = not P.Protect() && not FromUser;
+   bool hasFailed = false;
 
    std::vector<pkgCache::DepIterator> toInstall, toRemove;
    APT::PackageVector toUpgrade;
@@ -1370,15 +1387,19 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg, bool AutoInst,
       if (not MarkInstall_CollectDependencies(PV, toInstall, toRemove))
 	 return false;
 
-      if (not MarkInstall_RemoveConflictsIfNotUpgradeable(PV, Depth, toRemove, toUpgrade))
-	 return false;
+      if (not MarkInstall_RemoveConflictsIfNotUpgradeable(PV, Depth, toRemove, toUpgrade, P.Protect(), FromUser))
+      {
+	 if (failEarly)
+	    return false;
+	 hasFailed = true;
+      }
    }
 
    if (not FromUser && not MarkInstall_StateChange(Pkg, AutoInst, FromUser))
       return false;
 
    if (not AutoSolve)
-      return true;
+      return not hasFailed;
 
    if (DebugMarker)
       std::clog << OutputInDepth(Depth) << "MarkInstall " << APT::PrettyPkg(this, Pkg) << " FU=" << FromUser << '\n';
@@ -1402,8 +1423,12 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg, bool AutoInst,
       operator bool() noexcept { return already; }
    } propagateProctected{PkgState[Pkg->ID]};
 
-   if (not MarkInstall_UpgradeOrRemoveConflicts(propagateProctected, Depth, ForceImportantDeps, toUpgrade))
-      return false;
+   if (not MarkInstall_UpgradeOrRemoveConflicts(Depth, ForceImportantDeps, toUpgrade, propagateProctected, FromUser))
+   {
+      if (failEarly)
+	 return false;
+      hasFailed = true;
+   }
 
    bool const MoveAutoBitToDependencies = [&]() {
       VerIterator const PV = P.InstVerIter(*this);
@@ -1423,7 +1448,11 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg, bool AutoInst,
 
    APT::PackageVector toMoveAuto;
    if (not MarkInstall_InstallDependencies(Pkg, Depth, ForceImportantDeps, toInstall, MoveAutoBitToDependencies ? &toMoveAuto : nullptr, propagateProctected, FromUser))
-      return false;
+   {
+      if (failEarly)
+	 return false;
+      hasFailed = true;
+   }
 
    if (MoveAutoBitToDependencies)
    {
@@ -1438,7 +1467,7 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg, bool AutoInst,
 	 MarkAuto(InstPkg, false);
       }
    }
-   return true;
+   return not hasFailed;
 }
 									/*}}}*/
 // DepCache::IsInstallOk - check if it is ok to install this package	/*{{{*/
diff --git a/apt-pkg/depcache.h b/apt-pkg/depcache.h
index 858de5505..e6edcfc29 100644
--- a/apt-pkg/depcache.h
+++ b/apt-pkg/depcache.h
@@ -520,8 +520,8 @@ class APT_PUBLIC pkgDepCache : protected pkgCache::Namespace
 
    APT_HIDDEN bool MarkInstall_StateChange(PkgIterator const &Pkg, bool AutoInst, bool FromUser);
    APT_HIDDEN bool MarkInstall_CollectDependencies(pkgCache::VerIterator const &PV, std::vector<pkgCache::DepIterator> &toInstall, std::vector<pkgCache::DepIterator> &toRemove);
-   APT_HIDDEN bool MarkInstall_RemoveConflictsIfNotUpgradeable(pkgCache::VerIterator const &PV, unsigned long Depth, std::vector<pkgCache::DepIterator> &toRemove, APT::PackageVector &toUpgrade);
-   APT_HIDDEN bool MarkInstall_UpgradeOrRemoveConflicts(bool const propagateProtected, unsigned long Depth, bool const ForceImportantDeps, APT::PackageVector &toUpgrade);
+   APT_HIDDEN bool MarkInstall_RemoveConflictsIfNotUpgradeable(pkgCache::VerIterator const &PV, unsigned long Depth, std::vector<pkgCache::DepIterator> &toRemove, APT::PackageVector &toUpgrade, bool const propagateProtected, bool const FromUser);
+   APT_HIDDEN bool MarkInstall_UpgradeOrRemoveConflicts(unsigned long Depth, bool const ForceImportantDeps, APT::PackageVector &toUpgrade, bool const propagateProtected, bool const FromUser);
    APT_HIDDEN bool MarkInstall_InstallDependencies(PkgIterator const &Pkg, unsigned long Depth, bool const ForceImportantDeps, std::vector<pkgCache::DepIterator> &toInstall, APT::PackageVector *const toMoveAuto, bool const propagateProtected, bool const FromUser);
 };
 
diff --git a/test/integration/test-bug-549968-install-depends-of-not-installed b/test/integration/test-bug-549968-install-depends-of-not-installed
index fab67c4e9..75e7474ff 100755
--- a/test/integration/test-bug-549968-install-depends-of-not-installed
+++ b/test/integration/test-bug-549968-install-depends-of-not-installed
@@ -18,6 +18,7 @@ testsuccessequal "Reading package lists...
 Building dependency tree...
   MarkInstall coolstuff:i386 < none -> 1.0 @un puN IPb > FU=1
     Ignore MarkInstall of extracoolstuff:i386 < none | 1.0 @hn puH > as its mode (Keep) is protected
+    coolstuff:i386 Recommends on extracoolstuff:i386 < none | 1.0 @hn puH > can't be satisfied! (dep)
 Package 'extracoolstuff' is not installed, so not removed
 Recommended packages:
   extracoolstuff
diff --git a/test/integration/test-bug-618848-always-respect-user-requests b/test/integration/test-bug-618848-always-respect-user-requests
index 230683a99..1e144f1ee 100755
--- a/test/integration/test-bug-618848-always-respect-user-requests
+++ b/test/integration/test-bug-618848-always-respect-user-requests
@@ -13,11 +13,12 @@ insertpackage 'unstable' 'exim4-daemon-heavy' 'all' '1.0' 'Depends: libdb4.8'
 
 setupaptarchive
 
-testsuccessequal 'Reading package lists...
+testsuccessequal "Reading package lists...
 Building dependency tree...
   MarkDelete libdb4.8:i386 < 1.0 @ii pmK > FU=1
   MarkDelete exim4-daemon-light:i386 < 1.0 @ii mK Ib > FU=0
     MarkInstall exim4-daemon-heavy:i386 < none -> 1.0 @un uN Ib > FU=0
+      exim4-daemon-heavy:i386 Depends on libdb4.8:i386 < 1.0 @ii pmR > can't be satisfied! (dep)
   MarkDelete exim4:i386 < 1.0 @ii mK Ib > FU=0
 The following packages will be REMOVED:
   exim4 exim4-daemon-light libdb4.8
@@ -27,4 +28,4 @@ Remv exim4 [1.0]
   MarkDelete exim4-daemon-light:i386 < 1.0 @ii K > FU=1
 Remv exim4-daemon-light [1.0]
   MarkDelete libdb4.8:i386 < 1.0 @ii K > FU=1
-Remv libdb4.8 [1.0]' aptget remove libdb4.8 -s -o Debug::pkgDepCache::Marker=1
+Remv libdb4.8 [1.0]" aptget remove libdb4.8 -s -o Debug::pkgDepCache::Marker=1
diff --git a/test/integration/test-explore-or-groups-in-markinstall b/test/integration/test-explore-or-groups-in-markinstall
index 763852fa5..7f49b23cb 100755
--- a/test/integration/test-explore-or-groups-in-markinstall
+++ b/test/integration/test-explore-or-groups-in-markinstall
@@ -9,16 +9,17 @@ configarchitecture 'amd64'
 insertpackage 'unstable' 'okay' 'all' '1'
 insertpackage 'unstable' 'upgrade' 'all' '2'
 insertpackage 'unstable' 'unneeded' 'all' '1'
+insertpackage 'unstable' 'unneeded2' 'all' '1' 'Depends: bad-level1'
 insertpackage 'unstable' 'later' 'all' '1'
 
 insertpackage 'unstable' 'bad-level0' 'all' '1' 'Depends: unneeded, unknown'
 insertpackage 'unstable' 'bad-level1' 'all' '1' 'Depends: bad-level0'
 insertpackage 'unstable' 'bad-upgrade-level0' 'all' '2' 'Depends: unneeded, unknown'
-insertpackage 'unstable' 'bad-upgrade-level1' 'all' '2' 'Depends: bad-upgrade-level0 (>= 2)'
+insertpackage 'unstable' 'bad-upgrade-level1' 'all' '2' 'Depends: bad-upgrade-level0 (>= 2), unneeded2'
 insertpackage 'unstable' 'bad-conflict-level0' 'all' '1' 'Depends: unneeded
 Conflicts: bad-conflict-level2'
 insertpackage 'unstable' 'bad-conflict-level1' 'all' '1' 'Depends: bad-conflict-level0'
-insertpackage 'unstable' 'bad-conflict-level2' 'all' '1' 'Depends: bad-conflict-level1'
+insertpackage 'unstable' 'bad-conflict-level2' 'all' '1' 'Depends: bad-conflict-level1, unneeded2'
 
 insertinstalledpackage 'upgrade' 'all' '1'
 insertinstalledpackage 'bad-upgrade' 'all' '1'
@@ -112,8 +113,25 @@ Would download/install/remove packages.'
 	testsuccesstailequal 3 "$OKAYAPTITUDE" aptitude install foo-r-conflict -sy
 fi
 
-testfailure apt install bad-upgrade-level1 -s
-testfailure apt install bad-conflict-level2 -s
+BADSOLVETEXT='Reading package lists...
+Building dependency tree...
+Some packages could not be installed. This may mean that you have
+requested an impossible situation or if you are using the unstable
+distribution that some required packages have not yet been created
+or been moved out of Incoming.
+The following information may help to resolve the situation:
+'
+
+testfailureequal "$BADSOLVETEXT
+The following packages have unmet dependencies:
+ bad-level0 : Depends: unknown but it is not installable
+ bad-upgrade-level0 : Depends: unknown but it is not installable
+E: Error, pkgProblemResolver::Resolve generated breaks, this may be caused by held packages." apt install bad-upgrade-level1 -s
+testfailureequal "$BADSOLVETEXT
+The following packages have unmet dependencies:
+ bad-conflict-level0 : Conflicts: bad-conflict-level2 but 1 is to be installed
+ bad-level0 : Depends: unknown but it is not installable
+E: Error, pkgProblemResolver::Resolve generated breaks, this may be caused by held packages." apt install bad-conflict-level2 -s
 
 if $TEST_WITH_APTITUDE; then
 	testsuccesstailequal 6 'The following packages have been kept back:
diff --git a/test/integration/test-multiarch-allowed b/test/integration/test-multiarch-allowed
index b74853993..167f0c584 100755
--- a/test/integration/test-multiarch-allowed
+++ b/test/integration/test-multiarch-allowed
@@ -72,7 +72,6 @@ The following packages have unmet dependencies:
  foo : Conflicts: foo:i386 but 1 is to be installed
  foo:i386 : Conflicts: foo but 1 is to be installed
 E: Error, pkgProblemResolver::Resolve generated breaks, this may be caused by held packages." aptget install needsfoo foo:i386 -s
-exit
 
 solveableinsinglearch1() {
 	testsuccessequal "Reading package lists...