On Tue, Mar 11, 2014 at 12:11:57PM +0100, David Kalnischkies wrote: > On Thu, Mar 06, 2014 at 10:24:37AM +0800, Daniel Hartwig wrote: > > commit 446551c8ffd2c9cb9dcd707c94590e73009f7dd9 > […] > > I need to investigate also whether a previous change to call MarkDelete > > under the same situation also impacts this use case. > > No idea really, but I would presume that if this is called for a "leaf" > package it is just another unsatisfied dependency to be resolved. > If on the other hand it is a request coming from the user it is > protected by FromUser (assuming MarkInstall is called this way). > > I wonder a bit how this all works at all though with this loopbreaking > (the MarkDelete is just the topping) as it was introduced in df77d8a5 > (May 2011) by - surprise surprise - me. For interactive use it would > probably be better to continue and let the user fix the breakage later. > > I guess we should move this check out into a IsInstallOk submethod so > that you can at least disable this check (and we can stop at the > beginning rather than somewhere in the middle). Will see if I can make > this work. Attached are two changes which should implement it this way and I guess are better suited to fix the problem as they remove this specific loopbreaking from MarkInstall and moving it to IsInstallOk which a frontend can override and hence disable this and/or other checks. I can see in the aptitude code that you are overriding this method already, so in case we would go with this everything should magically work for aptitude again as it used to be back in the "old days", right? Or is there something I am missing? Looking with codesearch suggests that no other frontend is playing with IsInstallOk, so the behavior changes only in so far that breakage (introduced in May 2011, so it might very well be expected behavior now) is now more obvious as the loop is not even started instead of broken somewhere down the line, which sounds like a good thing and an easy way out of this problem is present as well if need be. Best regards David Kalnischkies
commit 7eb850824976006bd586036ca7f97f6408d9aeff
Author: David Kalnischkies <david@kalnischkies.de>
Date: Tue Mar 11 10:31:44 2014 +0100
discard candidates via IsInstallOk to allow override
In commit 446551c8 I changed MarkInstall to discard the candidate if the
candidate can't satisfy the dependency. This breaks interactive solvers
like aptitude which can change the candidate on-the-fly later.
In commit df77d8a5 I introduced this 'early' loop-breaking to begin with
which can't be that helpful for interactive solvers as well, but makes
perfect sense for non-interactives to stop them from exploring trees
which can't be satisfied, but it isn't perfect as ideally we would check
this before auto-installing the first dependency.
This commit therefore moves the loop into its own IsInstallOk hook so
that frontends can override this check if they want to and in exchange
removes the loop-breaking from MarkInstall itself.
diff --git a/apt-pkg/depcache.cc b/apt-pkg/depcache.cc
index 5fa88a5..19a6e0d 100644
--- a/apt-pkg/depcache.cc
+++ b/apt-pkg/depcache.cc
@@ -1122,32 +1122,22 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg,bool AutoInst,
continue;
/* Check if this dep should be consider for install. If it is a user
- defined important dep and we are installed a new package then
+ defined important dep and we are installed a new package then
it will be installed. Otherwise we only check for important
- deps that have changed from the installed version
- */
+ deps that have changed from the installed version */
if (IsImportantDep(Start) == false)
continue;
- /* If we are in an or group locate the first or that can
- succeed. We have already cached this.. */
+ /* If we are in an or group locate the first or that can
+ succeed. We have already cached this… */
for (; Ors > 1 && (DepState[Start->ID] & DepCVer) != DepCVer; --Ors)
++Start;
+
+ /* unsatisfiable dependency: IsInstallOkDependenciesSatisfiableByCandidates
+ would have prevented us to get here if not overridden, so just skip
+ over the problem here as the frontend will know what it is doing */
if (Ors == 1 && (DepState[Start->ID] &DepCVer) != DepCVer && Start.IsNegative() == false)
- {
- if(DebugAutoInstall == true)
- std::clog << OutputInDepth(Depth) << Start << " can't be satisfied!" << std::endl;
- if (Start.IsCritical() == false)
- continue;
- // if the dependency was critical, we have absolutely no chance to install it,
- // so if it wasn't installed remove it again. If it was, discard the candidate
- // as the problemresolver will trip over it otherwise trying to install it (#735967)
- if (Pkg->CurrentVer == 0)
- MarkDelete(Pkg,false,Depth + 1, false);
- else
- SetCandidateVersion(Pkg.CurrentVer());
- return false;
- }
+ continue;
/* Check if any ImportantDep() (but not Critical) were added
* since we installed the package. Also check for deps that
@@ -1299,7 +1289,8 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg,bool AutoInst,
bool pkgDepCache::IsInstallOk(PkgIterator const &Pkg,bool AutoInst,
unsigned long Depth, bool FromUser)
{
- return IsInstallOkMultiArchSameVersionSynced(Pkg,AutoInst, Depth, FromUser);
+ return IsInstallOkMultiArchSameVersionSynced(Pkg,AutoInst, Depth, FromUser) &&
+ IsInstallOkDependenciesSatisfiableByCandidates(Pkg,AutoInst, Depth, FromUser);
}
bool pkgDepCache::IsInstallOkMultiArchSameVersionSynced(PkgIterator const &Pkg,
bool const /*AutoInst*/, unsigned long const Depth, bool const FromUser)
@@ -1344,6 +1335,53 @@ bool pkgDepCache::IsInstallOkMultiArchSameVersionSynced(PkgIterator const &Pkg,
return true;
}
+bool pkgDepCache::IsInstallOkDependenciesSatisfiableByCandidates(PkgIterator const &Pkg,
+ bool const AutoInst, unsigned long const Depth, bool const /*FromUser*/)
+{
+ if (AutoInst == false)
+ return true;
+
+ VerIterator const CandVer = PkgState[Pkg->ID].CandidateVerIter(*this);
+ if (unlikely(CandVer.end() == true) || CandVer == Pkg.CurrentVer())
+ return true;
+
+ for (DepIterator Dep = CandVer.DependsList(); Dep.end() != true;)
+ {
+ // Grok or groups
+ DepIterator Start = Dep;
+ bool Result = true;
+ unsigned Ors = 0;
+ for (bool LastOR = true; Dep.end() == false && LastOR == true; ++Dep, ++Ors)
+ {
+ LastOR = (Dep->CompareOp & Dep::Or) == Dep::Or;
+
+ if ((DepState[Dep->ID] & DepInstall) == DepInstall)
+ Result = false;
+ }
+
+ if (Start.IsCritical() == false || Start.IsNegative() == true || Result == false)
+ continue;
+
+ /* If we are in an or group locate the first or that can succeed.
+ We have already cached this… */
+ for (; Ors > 1 && (DepState[Start->ID] & DepCVer) != DepCVer; --Ors)
+ ++Start;
+
+ if (Ors == 1 && (DepState[Start->ID] &DepCVer) != DepCVer)
+ {
+ if (DebugAutoInstall == true)
+ std::clog << OutputInDepth(Depth) << Start << " can't be satisfied!" << std::endl;
+
+ // the dependency is critical, but can't be installed, so discard the candidate
+ // as the problemresolver will trip over it otherwise trying to install it (#735967)
+ if (Pkg->CurrentVer != 0)
+ SetCandidateVersion(Pkg.CurrentVer());
+ return false;
+ }
+ }
+
+ return true;
+}
/*}}}*/
// DepCache::SetReInstall - Set the reinstallation flag /*{{{*/
// ---------------------------------------------------------------------
diff --git a/apt-pkg/depcache.h b/apt-pkg/depcache.h
index bde648c..bec6512 100644
--- a/apt-pkg/depcache.h
+++ b/apt-pkg/depcache.h
@@ -506,6 +506,8 @@ class pkgDepCache : protected pkgCache::Namespace
// methods call by IsInstallOk
bool IsInstallOkMultiArchSameVersionSynced(PkgIterator const &Pkg,
bool const AutoInst, unsigned long const Depth, bool const FromUser);
+ bool IsInstallOkDependenciesSatisfiableByCandidates(PkgIterator const &Pkg,
+ bool const AutoInst, unsigned long const Depth, bool const FromUser);
// methods call by IsDeleteOk
bool IsDeleteOkProtectInstallRequests(PkgIterator const &Pkg,
diff --git a/test/integration/test-bug-735967-lib32-to-i386-unavailable b/test/integration/test-bug-735967-lib32-to-i386-unavailable
index 4dbe1d2..e9f3bf9 100755
--- a/test/integration/test-bug-735967-lib32-to-i386-unavailable
+++ b/test/integration/test-bug-735967-lib32-to-i386-unavailable
@@ -12,6 +12,9 @@ insertpackage 'unstable' 'libnss-mdns' 'amd64,i386' '0.10-6' 'Multi-Arch: same
Breaks: lib32nss-mdns (<< 0.10-6)'
insertpackage 'unstable' 'libnss-mdns-i386' 'i386' '0.10-6' 'Multi-Arch: foreign
Depends: libnss-mdns'
+# introduce some dummies so that there are versions, but none works
+insertpackage 'unstable' 'libnss-mdns-i386' 'amd64' '0.1-6'
+insertpackage 'experimental' 'libnss-mdns-amd64' 'i386,amd64' '0.10-6' 'Provides: libnss-mdns-i386'
insertpackage 'unstable' 'foo' 'amd64' '1' 'Depends: libfoo'
insertpackage 'unstable' 'libfoo' 'amd64' '1' 'Depends: libfoo-bin'
commit c2881935cfbd65da30f66ab478e89564cdea09ff
Author: David Kalnischkies <david@kalnischkies.de>
Date: Tue Mar 11 15:32:40 2014 +0100
do IsInstallOk call in MarkInstall unconditionally
Hooked checks could be influenced by AutoInst as a lot can happen
between a call without and one with this bit set. The real cache-hit
check is above this call already. Individual hooked checks can then
inspect the state if they want to cache. Calling them multiple times
shouldn't be a problem either way.
diff --git a/apt-pkg/depcache.cc b/apt-pkg/depcache.cc
index e2c4127..5fa88a5 100644
--- a/apt-pkg/depcache.cc
+++ b/apt-pkg/depcache.cc
@@ -1059,10 +1059,9 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg,bool AutoInst,
return true;
}
- // check if we are allowed to install the package (if we haven't already)
- if (P.Mode != ModeInstall || P.InstallVer != P.CandidateVer)
- if (IsInstallOk(Pkg,AutoInst,Depth,FromUser) == false)
- return false;
+ // check if we are allowed to install the package
+ if (IsInstallOk(Pkg,AutoInst,Depth,FromUser) == false)
+ return false;
ActionGroup group(*this);
P.iFlags &= ~AutoKept;
@@ -1308,6 +1307,11 @@ bool pkgDepCache::IsInstallOkMultiArchSameVersionSynced(PkgIterator const &Pkg,
if (FromUser == true) // as always: user is always right
return true;
+ // if we have checked before and it was okay, it will still be okay
+ if (PkgState[Pkg->ID].Mode == ModeInstall &&
+ PkgState[Pkg->ID].InstallVer == PkgState[Pkg->ID].CandidateVer)
+ return true;
+
// ignore packages with none-M-A:same candidates
VerIterator const CandVer = PkgState[Pkg->ID].CandidateVerIter(*this);
if (unlikely(CandVer.end() == true) || CandVer == Pkg.CurrentVer() ||
Attachment:
signature.asc
Description: Digital signature