From cc7632489d3d2728862b3d3d5053aafbd543bdf3 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 20 Jun 2012 19:04:21 +0200 Subject: [PATCH] Make problem report messages more readable Added pretty strings to constraints --- src/Composer/DependencyResolver/Pool.php | 13 ++++ src/Composer/DependencyResolver/Problem.php | 44 +++++++++---- src/Composer/DependencyResolver/Rule.php | 27 +++++--- .../DependencyResolver/RuleSetGenerator.php | 10 +-- src/Composer/DependencyResolver/Solver.php | 3 +- .../SolverProblemsException.php | 13 ++-- src/Composer/Package/BasePackage.php | 2 +- src/Composer/Package/Link.php | 6 ++ .../LinkConstraintInterface.php | 2 + .../LinkConstraint/MultiConstraint.php | 14 +++++ .../LinkConstraint/SpecificConstraint.php | 16 +++++ .../Package/Version/VersionParser.php | 9 ++- .../Test/DependencyResolver/SolverTest.php | 62 ++++++++++++++++++- tests/Composer/Test/TestCase.php | 6 +- 14 files changed, 187 insertions(+), 40 deletions(-) diff --git a/src/Composer/DependencyResolver/Pool.php b/src/Composer/DependencyResolver/Pool.php index 7c5c4ed81..7c7c1fca4 100644 --- a/src/Composer/DependencyResolver/Pool.php +++ b/src/Composer/DependencyResolver/Pool.php @@ -163,4 +163,17 @@ class Pool { return ($literal > 0 ? '+' : '-') . $this->literalToPackage($literal); } + + public function literalToPrettyString($literal, $installedMap) + { + $package = $this->literalToPackage($literal); + + if (isset($installedMap[$package->getId()])) { + $prefix = ($literal > 0 ? 'keep' : 'remove'); + } else { + $prefix = ($literal > 0 ? 'install' : 'don\'t install'); + } + + return $prefix.' '.$package->getPrettyString(); + } } diff --git a/src/Composer/DependencyResolver/Problem.php b/src/Composer/DependencyResolver/Problem.php index a1407c41b..bab6a7390 100644 --- a/src/Composer/DependencyResolver/Problem.php +++ b/src/Composer/DependencyResolver/Problem.php @@ -19,11 +19,19 @@ namespace Composer\DependencyResolver; */ class Problem { + /** + * A map containing the id of each rule part of this problem as a key + * @var array + */ + protected $reasonSeen; + /** * A set of reasons for the problem, each is a rule or a job and a rule * @var array */ - protected $reasons; + protected $reasons = array(); + + protected $section = 0; /** * Add a rule as a reason @@ -50,12 +58,16 @@ class Problem /** * A human readable textual representation of the problem's reasons + * + * @param array $installedMap A map of all installed packages */ - public function __toString() + public function getPrettyString(array $installedMap = array()) { - if (count($this->reasons) === 1) { - reset($this->reasons); - $reason = current($this->reasons); + $reasons = call_user_func_array('array_merge', array_reverse($this->reasons)); + + if (count($reasons) === 1) { + reset($reasons); + $reason = current($reasons); $rule = $reason['rule']; $job = $reason['job']; @@ -73,9 +85,9 @@ class Problem } } - $messages = array("Problem caused by:"); + $messages = array(); - foreach ($this->reasons as $reason) { + foreach ($reasons as $reason) { $rule = $reason['rule']; $job = $reason['job']; @@ -84,12 +96,12 @@ class Problem $messages[] = $this->jobToText($job); } elseif ($rule) { if ($rule instanceof Rule) { - $messages[] = $rule->toHumanReadableString(); + $messages[] = $rule->getPrettyString($installedMap); } } } - return implode("\n\t\t\t- ", $messages); + return "\n - ".implode("\n - ", $messages); } /** @@ -100,11 +112,17 @@ class Problem */ protected function addReason($id, $reason) { - if (!isset($this->reasons[$id])) { - $this->reasons[$id] = $reason; + if (!isset($this->reasonSeen[$id])) { + $this->reasonSeen[$id] = true; + $this->reasons[$this->section][] = $reason; } } + public function nextSection() + { + $this->section++; + } + /** * Turns a job into a human readable description * @@ -119,7 +137,7 @@ class Problem 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']).'].'; + return 'Installation request for '.$job['packageName'].$this->constraintToText($job['constraint']).' -> satisfiable by '.$this->getPackageList($job['packages']).'.'; case 'update': return 'Update request for '.$job['packageName'].$this->constraintToText($job['constraint']).'.'; case 'remove': @@ -146,6 +164,6 @@ class Problem */ protected function constraintToText($constraint) { - return ($constraint) ? ' '.$constraint : ''; + return ($constraint) ? ' '.$constraint->getPrettyString() : ''; } } diff --git a/src/Composer/DependencyResolver/Rule.php b/src/Composer/DependencyResolver/Rule.php index 92f80a65c..2b645ce94 100644 --- a/src/Composer/DependencyResolver/Rule.php +++ b/src/Composer/DependencyResolver/Rule.php @@ -147,14 +147,14 @@ class Rule return 1 === count($this->literals); } - public function toHumanReadableString() + public function getPrettyString(array $installedMap = array()) { $ruleText = ''; foreach ($this->literals as $i => $literal) { if ($i != 0) { $ruleText .= '|'; } - $ruleText .= $this->pool->literalToString($literal); + $ruleText .= $this->pool->literalToPrettyString($literal, $installedMap); } switch ($this->reason) { @@ -171,7 +171,7 @@ class Rule $package1 = $this->pool->literalToPackage($this->literals[0]); $package2 = $this->pool->literalToPackage($this->literals[1]); - return 'Package '.$package1->getPrettyString().' conflicts with '.$package2->getPrettyString().'"'; + return $package1->getPrettyString().' conflicts with '.$package2->getPrettyString().'.'; case self::RULE_PACKAGE_REQUIRES: $literals = $this->literals; @@ -183,11 +183,15 @@ class Rule $requires[] = $this->pool->literalToPackage($literal); } - $text = 'Package "'.$sourcePackage.'" contains the rule '.$this->reasonData.'. '; + $text = $this->reasonData->getPrettyString($sourcePackage); if ($requires) { - $text .= 'Any of these packages satisfy the dependency: '.implode(', ', $requires).'.'; + $requireText = array(); + foreach ($requires as $require) { + $requireText[] = $require->getPrettyString(); + } + $text .= ' -> satisfiable by '.implode(', ', $requireText).'.'; } else { - $text .= 'No package satisfies this dependency.'; + $text .= ' -> no matching package found.'; } return $text; @@ -197,11 +201,18 @@ class Rule case self::RULE_INSTALLED_PACKAGE_OBSOLETES: return $ruleText; case self::RULE_PACKAGE_SAME_NAME: - return $ruleText; + $text = "Can only install one of: "; + + $packages = array(); + foreach ($this->literals as $i => $literal) { + $packages[] = $this->pool->literalToPackage($literal)->getPrettyString(); + } + + return $text.implode(', ', $packages).'.'; case self::RULE_PACKAGE_IMPLICIT_OBSOLETES: return $ruleText; case self::RULE_LEARNED: - return 'learned: '.$ruleText; + return 'Conclusion: '.$ruleText; case self::RULE_PACKAGE_ALIAS: return $ruleText; } diff --git a/src/Composer/DependencyResolver/RuleSetGenerator.php b/src/Composer/DependencyResolver/RuleSetGenerator.php index 95ca88ada..343653ac3 100644 --- a/src/Composer/DependencyResolver/RuleSetGenerator.php +++ b/src/Composer/DependencyResolver/RuleSetGenerator.php @@ -157,7 +157,7 @@ class RuleSetGenerator foreach ($package->getRequires() as $link) { $possibleRequires = $this->pool->whatProvides($link->getTarget(), $link->getConstraint()); - $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createRequireRule($package, $possibleRequires, Rule::RULE_PACKAGE_REQUIRES, (string) $link)); + $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createRequireRule($package, $possibleRequires, Rule::RULE_PACKAGE_REQUIRES, $link)); foreach ($possibleRequires as $require) { $workQueue->enqueue($require); @@ -168,7 +168,7 @@ class RuleSetGenerator $possibleConflicts = $this->pool->whatProvides($link->getTarget(), $link->getConstraint()); foreach ($possibleConflicts as $conflict) { - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createConflictRule($package, $conflict, Rule::RULE_PACKAGE_CONFLICT, (string) $link)); + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createConflictRule($package, $conflict, Rule::RULE_PACKAGE_CONFLICT, $link)); } } @@ -185,7 +185,7 @@ class RuleSetGenerator if (!$this->obsoleteImpossibleForAlias($package, $provider)) { $reason = ($isInstalled) ? Rule::RULE_INSTALLED_PACKAGE_OBSOLETES : Rule::RULE_PACKAGE_OBSOLETES; - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createConflictRule($package, $provider, $reason, (string) $link)); + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createConflictRule($package, $provider, $reason, $link)); } } } @@ -198,10 +198,10 @@ class RuleSetGenerator } if (($package instanceof AliasPackage) && $package->getAliasOf() === $provider) { - $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createRequireRule($package, array($provider), Rule::RULE_PACKAGE_ALIAS, (string) $package)); + $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createRequireRule($package, array($provider), Rule::RULE_PACKAGE_ALIAS, $package)); } elseif (!$this->obsoleteImpossibleForAlias($package, $provider)) { $reason = ($package->getName() == $provider->getName()) ? Rule::RULE_PACKAGE_SAME_NAME : Rule::RULE_PACKAGE_IMPLICIT_OBSOLETES; - $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createConflictRule($package, $provider, $reason, (string) $package)); + $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createConflictRule($package, $provider, $reason, $package)); } } } diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index 9e5c9645a..1b6f9ef3d 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -182,7 +182,7 @@ class Solver } if ($this->problems) { - throw new SolverProblemsException($this->problems); + throw new SolverProblemsException($this->problems, $this->installedMap); } $transaction = new Transaction($this->policy, $this->pool, $this->installedMap, $this->decisions); @@ -457,6 +457,7 @@ class Solver return; } + $problem->nextSection(); $problem->addRule($conflictRule); } diff --git a/src/Composer/DependencyResolver/SolverProblemsException.php b/src/Composer/DependencyResolver/SolverProblemsException.php index d558aa122..9d1ec7f86 100644 --- a/src/Composer/DependencyResolver/SolverProblemsException.php +++ b/src/Composer/DependencyResolver/SolverProblemsException.php @@ -18,23 +18,24 @@ namespace Composer\DependencyResolver; class SolverProblemsException extends \RuntimeException { protected $problems; + protected $installedMap; - public function __construct(array $problems) + public function __construct(array $problems, array $installedMap) { $this->problems = $problems; + $this->installedMap = $installedMap; parent::__construct($this->createMessage()); } protected function createMessage() { - $messages = array(); - - foreach ($this->problems as $problem) { - $messages[] = (string) $problem; + $text = "\n"; + foreach ($this->problems as $i => $problem) { + $text .= " Problem ".($i+1).$problem->getPrettyString($this->installedMap)."\n"; } - return "\n\tProblems:\n\t\t- ".implode("\n\t\t- ", $messages); + return $text; } public function getProblems() diff --git a/src/Composer/Package/BasePackage.php b/src/Composer/Package/BasePackage.php index f3543d1a5..a3ae853f3 100644 --- a/src/Composer/Package/BasePackage.php +++ b/src/Composer/Package/BasePackage.php @@ -203,7 +203,7 @@ abstract class BasePackage implements PackageInterface public function getPrettyString() { - return $this->getPrettyName().'-'.$this->getPrettyVersion(); + return $this->getPrettyName().' '.$this->getPrettyVersion(); } public function __clone() diff --git a/src/Composer/Package/Link.php b/src/Composer/Package/Link.php index 348bbced7..da6373144 100644 --- a/src/Composer/Package/Link.php +++ b/src/Composer/Package/Link.php @@ -13,6 +13,7 @@ namespace Composer\Package; use Composer\Package\LinkConstraint\LinkConstraintInterface; +use Composer\Package\PackageInterface; /** * Represents a link between two packages, represented by their names @@ -71,4 +72,9 @@ class Link { return $this->source.' '.$this->description.' '.$this->target.' ('.$this->constraint.')'; } + + public function getPrettyString(PackageInterface $sourcePackage) + { + return $sourcePackage->getPrettyString().' '.$this->description.' '.$this->target.' '.$this->constraint->getPrettyString().''; + } } diff --git a/src/Composer/Package/LinkConstraint/LinkConstraintInterface.php b/src/Composer/Package/LinkConstraint/LinkConstraintInterface.php index 44867dbc9..199c9fc42 100644 --- a/src/Composer/Package/LinkConstraint/LinkConstraintInterface.php +++ b/src/Composer/Package/LinkConstraint/LinkConstraintInterface.php @@ -20,5 +20,7 @@ namespace Composer\Package\LinkConstraint; interface LinkConstraintInterface { public function matches(LinkConstraintInterface $provider); + public function setPrettyString($prettyString); + public function getPrettyString(); public function __toString(); } diff --git a/src/Composer/Package/LinkConstraint/MultiConstraint.php b/src/Composer/Package/LinkConstraint/MultiConstraint.php index db3af2a39..3b9a88323 100644 --- a/src/Composer/Package/LinkConstraint/MultiConstraint.php +++ b/src/Composer/Package/LinkConstraint/MultiConstraint.php @@ -20,6 +20,7 @@ namespace Composer\Package\LinkConstraint; class MultiConstraint implements LinkConstraintInterface { protected $constraints; + protected $prettyString; /** * Sets operator and version to compare a package with @@ -42,6 +43,19 @@ class MultiConstraint implements LinkConstraintInterface return true; } + public function setPrettyString($prettyString) + { + $this->prettyString = $prettyString; + } + + public function getPrettyString() + { + if ($this->prettyString) { + return $this->prettyString; + } + return $this->__toString(); + } + public function __toString() { $constraints = array(); diff --git a/src/Composer/Package/LinkConstraint/SpecificConstraint.php b/src/Composer/Package/LinkConstraint/SpecificConstraint.php index e21048b68..b58929b39 100644 --- a/src/Composer/Package/LinkConstraint/SpecificConstraint.php +++ b/src/Composer/Package/LinkConstraint/SpecificConstraint.php @@ -19,6 +19,8 @@ namespace Composer\Package\LinkConstraint; */ abstract class SpecificConstraint implements LinkConstraintInterface { + protected $prettyString; + public function matches(LinkConstraintInterface $provider) { if ($provider instanceof MultiConstraint) { @@ -31,7 +33,21 @@ abstract class SpecificConstraint implements LinkConstraintInterface return true; } + public function setPrettyString($prettyString) + { + $this->prettyString = $prettyString; + } + + public function getPrettyString() + { + if ($this->prettyString) { + return $this->prettyString; + } + return $this->__toString(); + } + // implementations must implement a method of this format: // not declared abstract here because type hinting violates parameter coherence (TODO right word?) // public function matchSpecific( $provider); + } diff --git a/src/Composer/Package/Version/VersionParser.php b/src/Composer/Package/Version/VersionParser.php index 891f16dd0..917b772c9 100644 --- a/src/Composer/Package/Version/VersionParser.php +++ b/src/Composer/Package/Version/VersionParser.php @@ -164,6 +164,8 @@ class VersionParser */ public function parseConstraints($constraints) { + $prettyConstraint = $constraints; + if (preg_match('{^([^,\s]*?)@('.implode('|', array_keys(BasePackage::$stabilities)).')$}i', $constraints, $match)) { $constraints = empty($match[1]) ? '*' : $match[1]; } @@ -184,10 +186,13 @@ class VersionParser } if (1 === count($constraintObjects)) { - return $constraintObjects[0]; + $constraint = $constraintObjects[0]; + } else { + $constraint = new MultiConstraint($constraintObjects); } - return new MultiConstraint($constraintObjects); + $constraint->setPrettyString($prettyConstraint); + return $constraint; } private function parseConstraint($constraint) diff --git a/tests/Composer/Test/DependencyResolver/SolverTest.php b/tests/Composer/Test/DependencyResolver/SolverTest.php index 7638e9509..b9f7dc1e9 100644 --- a/tests/Composer/Test/DependencyResolver/SolverTest.php +++ b/tests/Composer/Test/DependencyResolver/SolverTest.php @@ -66,7 +66,7 @@ class SolverTest extends TestCase $this->repo->addPackage($this->getPackage('A', '1.0')); $this->reposComplete(); - $this->request->install('B', $this->getVersionConstraint('=', '1')); + $this->request->install('B', $this->getVersionConstraint('==', '1')); try { $transaction = $this->solver->solve($this->request); @@ -74,7 +74,7 @@ class SolverTest extends TestCase } catch (SolverProblemsException $e) { $problems = $e->getProblems(); $this->assertEquals(1, count($problems)); - $this->assertEquals('The requested package b == 1.0.0.0 could not be found.', (string) $problems[0]); + $this->assertEquals('The requested package b == 1 could not be found.', $problems[0]->getPrettyString()); } } @@ -641,7 +641,13 @@ class SolverTest extends TestCase } catch (SolverProblemsException $e) { $problems = $e->getProblems(); $this->assertEquals(1, count($problems)); - // TODO assert problem properties + + $msg = "\n"; + $msg .= " Problem 1\n"; + $msg .= " - Installation request for a -> satisfiable by A 1.0.\n"; + $msg .= " - B 1.0 conflicts with A 1.0.\n"; + $msg .= " - Installation request for b -> satisfiable by B 1.0.\n"; + $this->assertEquals($msg, $e->getMessage()); } } @@ -665,6 +671,56 @@ class SolverTest extends TestCase $problems = $e->getProblems(); $this->assertEquals(1, count($problems)); // TODO assert problem properties + + $msg = "\n"; + $msg .= " Problem 1\n"; + $msg .= " - Installation request for a -> satisfiable by A 1.0.\n"; + $msg .= " - A 1.0 requires b >= 2.0 -> no matching package found.\n"; + $this->assertEquals($msg, $e->getMessage()); + } + } + + public function testRequireMismatchException() + { + $this->repo->addPackage($packageA = $this->getPackage('A', '1.0')); + $this->repo->addPackage($packageB = $this->getPackage('B', '1.0')); + $this->repo->addPackage($packageB2 = $this->getPackage('B', '0.9')); + $this->repo->addPackage($packageC = $this->getPackage('C', '1.0')); + $this->repo->addPackage($packageD = $this->getPackage('D', '1.0')); + + $packageA->setRequires(array( + new Link('A', 'B', $this->getVersionConstraint('>=', '1.0'), 'requires'), + )); + $packageB->setRequires(array( + new Link('B', 'C', $this->getVersionConstraint('>=', '1.0'), 'requires'), + )); + $packageC->setRequires(array( + new Link('C', 'D', $this->getVersionConstraint('>=', '1.0'), 'requires'), + )); + $packageD->setRequires(array( + new Link('D', 'B', $this->getVersionConstraint('<', '1.0'), 'requires'), + )); + + $this->reposComplete(); + + $this->request->install('A'); + + try { + $transaction = $this->solver->solve($this->request); + $this->fail('Unsolvable conflict did not result in exception.'); + } catch (SolverProblemsException $e) { + $problems = $e->getProblems(); + $this->assertEquals(1, count($problems)); + + $msg = "\n"; + $msg .= " Problem 1\n"; + $msg .= " - C 1.0 requires d >= 1.0 -> satisfiable by D 1.0.\n"; + $msg .= " - D 1.0 requires b < 1.0 -> satisfiable by B 0.9.\n"; + $msg .= " - B 1.0 requires c >= 1.0 -> satisfiable by C 1.0.\n"; + $msg .= " - Can only install one of: B 0.9, B 1.0.\n"; + $msg .= " - A 1.0 requires b >= 1.0 -> satisfiable by B 1.0.\n"; + $msg .= " - Installation request for a -> satisfiable by A 1.0.\n"; + $this->assertEquals($msg, $e->getMessage()); } } diff --git a/tests/Composer/Test/TestCase.php b/tests/Composer/Test/TestCase.php index a544bdfa9..0e13a7921 100644 --- a/tests/Composer/Test/TestCase.php +++ b/tests/Composer/Test/TestCase.php @@ -33,10 +33,14 @@ abstract class TestCase extends \PHPUnit_Framework_TestCase protected function getVersionConstraint($operator, $version) { - return new VersionConstraint( + $constraint = new VersionConstraint( $operator, self::getVersionParser()->normalize($version) ); + + $constraint->setPrettyString($operator.' '.$version); + + return $constraint; } protected function getPackage($name, $version)