From 2fbc04b95005ae00639a5eb304500bee7bbf7334 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 6 Jun 2012 15:39:41 +0200 Subject: [PATCH 1/6] Make decisions countable and use foreach to iterate them in solver --- src/Composer/DependencyResolver/Decisions.php | 6 +++--- src/Composer/DependencyResolver/Solver.php | 13 ++++--------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/Composer/DependencyResolver/Decisions.php b/src/Composer/DependencyResolver/Decisions.php index 33e0fb27f..cf4419e55 100644 --- a/src/Composer/DependencyResolver/Decisions.php +++ b/src/Composer/DependencyResolver/Decisions.php @@ -17,7 +17,7 @@ namespace Composer\DependencyResolver; * * @author Nils Adermann */ -class Decisions implements \Iterator +class Decisions implements \Iterator, \Countable { const DECISION_LITERAL = 0; const DECISION_REASON = 1; @@ -178,9 +178,9 @@ class Decisions implements \Iterator array_pop($this->decisionQueue); } - public function getMaxOffset() + public function count() { - return count($this->decisionQueue) - 1; + return count($this->decisionQueue); } public function rewind() diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index 971f69cfb..e726e0961 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -48,7 +48,7 @@ class Solver // aka solver_makeruledecisions private function makeAssertionRuleDecisions() { - $decisionStart = $this->decisions->getMaxOffset(); + $decisionStart = count($this->decisions) - 1; for ($ruleIndex = 0; $ruleIndex < count($this->rules); $ruleIndex++) { $rule = $this->rules->ruleById($ruleIndex); @@ -242,7 +242,7 @@ class Solver } $this->decisions->revertLast(); - $this->propagateIndex = $this->decisions->getMaxOffset() + 1; + $this->propagateIndex = count($this->decisions); } while (!empty($this->branches)) { @@ -343,7 +343,7 @@ class Solver $seen = array(); $learnedLiterals = array(null); - $decisionId = $this->decisions->getMaxOffset() + 1; + $decisionId = count($this->decisions); $this->learnedPool[] = array(); @@ -483,12 +483,7 @@ class Solver $seen[abs($literal)] = true; } - $decisionId = $this->decisions->getMaxOffset() + 1; - - while ($decisionId > 0) { - $decisionId--; - - $decision = $this->decisions->atOffset($decisionId); + foreach ($this->decisions as $decision) { $literal = $decision[Decisions::DECISION_LITERAL]; // skip literals that are not in this rule From 5b1a48663e5846e64c9ff3549f5190d4dc983ec2 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 6 Jun 2012 15:42:03 +0200 Subject: [PATCH 2/6] DecisionQueueFree is no longer needed --- src/Composer/DependencyResolver/Decisions.php | 50 ++++++++----------- src/Composer/DependencyResolver/Solver.php | 2 +- 2 files changed, 22 insertions(+), 30 deletions(-) diff --git a/src/Composer/DependencyResolver/Decisions.php b/src/Composer/DependencyResolver/Decisions.php index cf4419e55..77289169b 100644 --- a/src/Composer/DependencyResolver/Decisions.php +++ b/src/Composer/DependencyResolver/Decisions.php @@ -25,7 +25,6 @@ class Decisions implements \Iterator, \Countable protected $pool; protected $decisionMap; protected $decisionQueue = array(); - protected $decisionQueueFree = array(); public function __construct($pool) { @@ -38,37 +37,13 @@ class Decisions implements \Iterator, \Countable } } - protected function addDecision($literal, $level) - { - $packageId = abs($literal); - - $previousDecision = $this->decisionMap[$packageId]; - if ($previousDecision != 0) { - $literalString = $this->pool->literalToString($literal); - $package = $this->pool->literalToPackage($literal); - throw new SolverBugException( - "Trying to decide $literalString on level $level, even though $package was previously decided as ".(int) $previousDecision."." - ); - } - - if ($literal > 0) { - $this->decisionMap[$packageId] = $level; - } else { - $this->decisionMap[$packageId] = -$level; - } - } - - public function decide($literal, $level, $why, $addToFreeQueue = false) + public function decide($literal, $level, $why) { $this->addDecision($literal, $level); $this->decisionQueue[] = array( self::DECISION_LITERAL => $literal, self::DECISION_REASON => $why, ); - - if ($addToFreeQueue) { - $this->decisionQueueFree[count($this->decisionQueue) - 1] = true; - } } public function contain($literal) @@ -159,15 +134,12 @@ class Decisions implements \Iterator, \Countable while ($decision = array_pop($this->decisionQueue)) { $this->decisionMap[abs($decision[self::DECISION_LITERAL])] = 0; } - - $this->decisionQueueFree = array(); } public function resetToOffset($offset) { while (count($this->decisionQueue) > $offset + 1) { $decision = array_pop($this->decisionQueue); - unset($this->decisionQueueFree[count($this->decisionQueue)]); $this->decisionMap[abs($decision[self::DECISION_LITERAL])] = 0; } } @@ -212,4 +184,24 @@ class Decisions implements \Iterator, \Countable { return count($this->decisionQueue) === 0; } + + protected function addDecision($literal, $level) + { + $packageId = abs($literal); + + $previousDecision = $this->decisionMap[$packageId]; + if ($previousDecision != 0) { + $literalString = $this->pool->literalToString($literal); + $package = $this->pool->literalToPackage($literal); + throw new SolverBugException( + "Trying to decide $literalString on level $level, even though $package was previously decided as ".(int) $previousDecision."." + ); + } + + if ($literal > 0) { + $this->decisionMap[$packageId] = $level; + } else { + $this->decisionMap[$packageId] = -$level; + } + } } diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index e726e0961..30afdb3fd 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -275,7 +275,7 @@ class Solver { $level++; - $this->decisions->decide($literal, $level, $rule, true); + $this->decisions->decide($literal, $level, $rule); while (true) { $rule = $this->propagate($level); From 76f8642feb2d54d8d46a2e07c17ec3c538a4de6f Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 6 Jun 2012 15:50:29 +0200 Subject: [PATCH 3/6] Remove duplicate function from decisions --- src/Composer/DependencyResolver/Decisions.php | 10 ---------- src/Composer/DependencyResolver/RuleWatchGraph.php | 2 +- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/Composer/DependencyResolver/Decisions.php b/src/Composer/DependencyResolver/Decisions.php index 77289169b..451cb5ff7 100644 --- a/src/Composer/DependencyResolver/Decisions.php +++ b/src/Composer/DependencyResolver/Decisions.php @@ -46,16 +46,6 @@ class Decisions implements \Iterator, \Countable ); } - public function contain($literal) - { - $packageId = abs($literal); - - return ( - $this->decisionMap[$packageId] > 0 && $literal > 0 || - $this->decisionMap[$packageId] < 0 && $literal < 0 - ); - } - public function satisfy($literal) { $packageId = abs($literal); diff --git a/src/Composer/DependencyResolver/RuleWatchGraph.php b/src/Composer/DependencyResolver/RuleWatchGraph.php index 42c764fa6..72b288e15 100644 --- a/src/Composer/DependencyResolver/RuleWatchGraph.php +++ b/src/Composer/DependencyResolver/RuleWatchGraph.php @@ -94,7 +94,7 @@ class RuleWatchGraph $node = $chain->current(); $otherWatch = $node->getOtherWatch($literal); - if (!$node->getRule()->isDisabled() && !$decisions->contain($otherWatch)) { + if (!$node->getRule()->isDisabled() && !$decisions->satisfy($otherWatch)) { $ruleLiterals = $node->getRule()->getLiterals(); $alternativeLiterals = array_filter($ruleLiterals, function ($ruleLiteral) use ($literal, $otherWatch, $decisions) { From 12abff8b4c8fd3dc33adc341c736c4b03f627918 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 6 Jun 2012 15:55:30 +0200 Subject: [PATCH 4/6] Simplify branch handling code --- src/Composer/DependencyResolver/Solver.php | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index 30afdb3fd..9e5c9645a 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -19,6 +19,9 @@ use Composer\Repository\RepositoryInterface; */ class Solver { + const BRANCH_LITERALS = 0; + const BRANCH_LEVEL = 1; + protected $policy; protected $pool; protected $installed; @@ -245,13 +248,7 @@ class Solver $this->propagateIndex = count($this->decisions); } - while (!empty($this->branches)) { - list($literals, $branchLevel) = $this->branches[count($this->branches) - 1]; - - if ($branchLevel < $level) { - break; - } - + while (!empty($this->branches) && $this->branches[count($this->branches) - 1][self::BRANCH_LEVEL] >= $level) { array_pop($this->branches); } } @@ -596,7 +593,6 @@ class Solver $level = 1; $systemLevel = $level + 1; - $minimizationSteps = 0; $installedPos = 0; while (true) { @@ -754,9 +750,8 @@ class Solver } if ($lastLiteral) { - unset($this->branches[$lastBranchIndex][0][$lastBranchOffset]); - $this->branches[$lastBranchIndex][0] = array_values($this->branches[$lastBranchIndex][0]); - $minimizationSteps++; + unset($this->branches[$lastBranchIndex][self::BRANCH_LITERALS][$lastBranchOffset]); + array_values($this->branches[$lastBranchIndex][self::BRANCH_LITERALS]); $level = $lastLevel; $this->revert($level); From 5bea5974f7a5f70aa85d98ac155bd404c4e90bcb Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Thu, 7 Jun 2012 03:23:23 +0200 Subject: [PATCH 5/6] Improve error messages for solver problems --- src/Composer/DependencyResolver/Problem.php | 27 +++++++++++++++------ src/Composer/DependencyResolver/Rule.php | 2 +- src/Composer/Package/BasePackage.php | 5 ++++ src/Composer/Package/PackageInterface.php | 7 ++++++ 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/Composer/DependencyResolver/Problem.php b/src/Composer/DependencyResolver/Problem.php index 012ebab2f..a1407c41b 100644 --- a/src/Composer/DependencyResolver/Problem.php +++ b/src/Composer/DependencyResolver/Problem.php @@ -66,10 +66,10 @@ class Problem $ext = substr($job['packageName'], 4); $error = extension_loaded($ext) ? 'has the wrong version ('.phpversion($ext).') installed' : 'is missing from your system'; - return 'The requested PHP extension "'.$job['packageName'].'" '.$this->constraintToText($job['constraint']).$error.'.'; + return 'The requested PHP extension '.$job['packageName'].$this->constraintToText($job['constraint']).' '.$error.'.'; } - return 'The requested package "'.$job['packageName'].'" '.$this->constraintToText($job['constraint']).'could not be found.'; + return 'The requested package '.$job['packageName'].$this->constraintToText($job['constraint']).' could not be found.'; } } @@ -115,14 +115,27 @@ class Problem { switch ($job['cmd']) { case 'install': - return 'Installation of package "'.$job['packageName'].'" '.$this->constraintToText($job['constraint']).'was requested. Satisfiable by packages ['.implode(', ', $job['packages']).'].'; + if (!$job['packages']) { + return 'No package found to satisfy install request for '.$job['packageName'].$this->constraintToText($job['constraint']); + } + + return 'Installation request for '.$job['packageName'].$this->constraintToText($job['constraint']).': Satisfiable by ['.$this->getPackageList($job['packages']).'].'; case 'update': - return 'Update of package "'.$job['packageName'].'" '.$this->constraintToText($job['constraint']).'was requested.'; + return 'Update request for '.$job['packageName'].$this->constraintToText($job['constraint']).'.'; case 'remove': - return 'Removal of package "'.$job['packageName'].'" '.$this->constraintToText($job['constraint']).'was requested.'; + return 'Removal request for '.$job['packageName'].$this->constraintToText($job['constraint']).''; } - return 'Job(cmd='.$job['cmd'].', target='.$job['packageName'].', packages=['.implode(', ', $job['packages']).'])'; + return 'Job(cmd='.$job['cmd'].', target='.$job['packageName'].', packages=['.$this->packageList($job['packages']).'])'; + } + + protected function getPackageList($packages) + { + return implode(', ', array_map(function ($package) { + return $package->getPrettyString(); + }, + $packages + )); } /** @@ -133,6 +146,6 @@ class Problem */ protected function constraintToText($constraint) { - return ($constraint) ? 'with constraint '.$constraint.' ' : ''; + return ($constraint) ? ' '.$constraint : ''; } } diff --git a/src/Composer/DependencyResolver/Rule.php b/src/Composer/DependencyResolver/Rule.php index a5414ebd9..92f80a65c 100644 --- a/src/Composer/DependencyResolver/Rule.php +++ b/src/Composer/DependencyResolver/Rule.php @@ -171,7 +171,7 @@ class Rule $package1 = $this->pool->literalToPackage($this->literals[0]); $package2 = $this->pool->literalToPackage($this->literals[1]); - return 'Package "'.$package1.'" conflicts with "'.$package2.'"'; + return 'Package '.$package1->getPrettyString().' conflicts with '.$package2->getPrettyString().'"'; case self::RULE_PACKAGE_REQUIRES: $literals = $this->literals; diff --git a/src/Composer/Package/BasePackage.php b/src/Composer/Package/BasePackage.php index 64f6ea6c2..f3543d1a5 100644 --- a/src/Composer/Package/BasePackage.php +++ b/src/Composer/Package/BasePackage.php @@ -201,6 +201,11 @@ abstract class BasePackage implements PackageInterface return $this->getUniqueName(); } + public function getPrettyString() + { + return $this->getPrettyName().'-'.$this->getPrettyVersion(); + } + public function __clone() { $this->repository = null; diff --git a/src/Composer/Package/PackageInterface.php b/src/Composer/Package/PackageInterface.php index fb435b19b..4d088ac5f 100644 --- a/src/Composer/Package/PackageInterface.php +++ b/src/Composer/Package/PackageInterface.php @@ -356,4 +356,11 @@ interface PackageInterface * @return string */ public function __toString(); + + /** + * Converts the package into a pretty readable string + * + * @return string + */ + public function getPrettyString(); } From a08c2a0b9fd01c280fd66b3083bd41315edf8abd Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Thu, 7 Jun 2012 11:07:09 +0200 Subject: [PATCH 6/6] Corrected altered error message in test --- tests/Composer/Test/DependencyResolver/SolverTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Composer/Test/DependencyResolver/SolverTest.php b/tests/Composer/Test/DependencyResolver/SolverTest.php index 0cec2b50a..df7f32cf4 100644 --- a/tests/Composer/Test/DependencyResolver/SolverTest.php +++ b/tests/Composer/Test/DependencyResolver/SolverTest.php @@ -77,7 +77,7 @@ class SolverTest extends TestCase } catch (SolverProblemsException $e) { $problems = $e->getProblems(); $this->assertEquals(1, count($problems)); - $this->assertEquals('The requested package "b" with constraint == 1.0.0.0 could not be found.', (string) $problems[0]); + $this->assertEquals('The requested package b == 1.0.0.0 could not be found.', (string) $problems[0]); } }