From 6b2edeae56d83b84ba60957aee18c314257d98fa Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Sun, 3 Feb 2019 01:53:17 +0100 Subject: [PATCH] Fix solver problem exceptions with unexpected contradictory "Conclusions" This 5 character fix comes with a solver test as well as a functional installer test essentially verifying the same thing. The solver test is more useful when working on the solver. But the functional test is less likely to be accidentally modified incorrectly during refactoring, as every single package, version and link in the rather complex test scenario is essential, and a modified version of the test may very well still result in a successful installation but no longer verify the bug described below. Background: In commit 451bab1c2cd58e05af6e21639b829408ad023463 from May 19, 2012 I refactored literals from complex objects into pure integers to reduce memory consumption. The absolute value of an integer literal is the id of the package it refers to in the package pool. The sign indicates whether the package should be installed (positive) or removed (negative), So a major part of the refactoring was swapping this call: $literal->getPackageId() For this: abs($literal) Unintentionally in line 554/523 I incorrectly applied this change to the line: $this->literalFromId(-$literal->getPackageId()); It was converted to: -abs($literal); The function literalFromId used to create a new literal object. By using the abs() function this change essentially forces the resulting literal to be negative, while the minus sign previously inverted the literal, so positive into negative and vice versa. This particular line is in a function meant to analyze a conflicting decision during dependency resolution and to draw a conclusion from it, then revert the state of the solver to an earlier position, and attempt to solve the rest of the rules again with this new "learned" conclusion. Because of this bug these conclusions could only ever occur in the negative, e.g. "don't install package X". This is by far the most likely scenario when the solver reaches this particular line, but there are exceptions. If you experienced a solver problem description that contained a statement like "Conclusion: don't install vendor/package 1.2.3" which directly contradicted other statements listed as part of the problem, this could likely have been the cause. --- src/Composer/DependencyResolver/Decisions.php | 12 +++ src/Composer/DependencyResolver/Solver.php | 2 +- .../Test/DependencyResolver/SolverTest.php | 64 +++++++++++ ...everts-and-learning-positive-literals.test | 100 ++++++++++++++++++ 4 files changed, 177 insertions(+), 1 deletion(-) create mode 100644 tests/Composer/Test/Fixtures/installer/update-requiring-decision-reverts-and-learning-positive-literals.test diff --git a/src/Composer/DependencyResolver/Decisions.php b/src/Composer/DependencyResolver/Decisions.php index a9808e60e..86b62c3d3 100644 --- a/src/Composer/DependencyResolver/Decisions.php +++ b/src/Composer/DependencyResolver/Decisions.php @@ -196,4 +196,16 @@ class Decisions implements \Iterator, \Countable $this->decisionMap[$packageId] = -$level; } } + + public function __toString() + { + $decisionMap = $this->decisionMap; + ksort($decisionMap); + $str = '['; + foreach ($decisionMap as $packageId => $level) { + $str .= $packageId.':'.$level.','; + } + $str .= ']'; + return $str; + } } diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index 1ed35ad9c..c40789c1f 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -470,7 +470,7 @@ class Solver unset($seen[abs($literal)]); if ($num && 0 === --$num) { - $learnedLiterals[0] = -abs($literal); + $learnedLiterals[0] = -$literal; if (!$l1num) { break 2; diff --git a/tests/Composer/Test/DependencyResolver/SolverTest.php b/tests/Composer/Test/DependencyResolver/SolverTest.php index 24147e6ad..7094f412d 100644 --- a/tests/Composer/Test/DependencyResolver/SolverTest.php +++ b/tests/Composer/Test/DependencyResolver/SolverTest.php @@ -838,6 +838,70 @@ class SolverTest extends TestCase )); } + /** + * Tests for a bug introduced in commit 451bab1c2cd58e05af6e21639b829408ad023463 Solver.php line 554/523 + * + * Every package and link in this test matters, only a combination this complex will run into the situation in which + * a negatively decided literal will need to be learned inverted as a positive assertion. + * + * In particular in this case the goal is to first have the solver decide X 2.0 should not be installed to later + * decide to learn that X 2.0 must be installed and revert decisions to retry solving with this new assumption. + */ + public function testLearnPositiveLiteral() + { + $this->repo->addPackage($packageA = $this->getPackage('A', '1.0')); + $this->repo->addPackage($packageB = $this->getPackage('B', '1.0')); + $this->repo->addPackage($packageC1 = $this->getPackage('C', '1.0')); + $this->repo->addPackage($packageC2 = $this->getPackage('C', '2.0')); + $this->repo->addPackage($packageD = $this->getPackage('D', '1.0')); + $this->repo->addPackage($packageE = $this->getPackage('E', '1.0')); + $this->repo->addPackage($packageF1 = $this->getPackage('F', '1.0')); + $this->repo->addPackage($packageF2 = $this->getPackage('F', '2.0')); + $this->repo->addPackage($packageG1 = $this->getPackage('G', '1.0')); + $this->repo->addPackage($packageG2 = $this->getPackage('G', '2.0')); + $this->repo->addPackage($packageG3 = $this->getPackage('G', '3.0')); + + $packageA->setRequires(array( + 'b' => new Link('A', 'B', $this->getVersionConstraint('==', '1.0'), 'requires'), + 'c' => new Link('A', 'C', $this->getVersionConstraint('>=', '1.0'), 'requires'), + 'd' => new Link('A', 'D', $this->getVersionConstraint('==', '1.0'), 'requires'), + )); + + $packageB->setRequires(array( + 'e' => new Link('B', 'E', $this->getVersionConstraint('==', '1.0'), 'requires'), + )); + + $packageC1->setRequires(array( + 'f' => new Link('C', 'F', $this->getVersionConstraint('==', '1.0'), 'requires'), + )); + $packageC2->setRequires(array( + 'f' => new Link('C', 'F', $this->getVersionConstraint('==', '1.0'), 'requires'), + 'g' => new Link('C', 'G', $this->getVersionConstraint('>=', '1.0'), 'requires'), + )); + + $packageD->setRequires(array( + 'f' => new Link('D', 'F', $this->getVersionConstraint('>=', '1.0'), 'requires'), + )); + + $packageE->setRequires(array( + 'g' => new Link('E', 'G', $this->getVersionConstraint('<=', '2.0'), 'requires'), + )); + + $this->reposComplete(); + + $this->request->install('A'); + + $this->checkSolverResult(array( + array('job' => 'install', 'package' => $packageF1), + array('job' => 'install', 'package' => $packageD), + array('job' => 'install', 'package' => $packageG2), + array('job' => 'install', 'package' => $packageC2), + array('job' => 'install', 'package' => $packageE), + array('job' => 'install', 'package' => $packageB), + array('job' => 'install', 'package' => $packageA), + )); + } + protected function reposComplete() { $this->pool->addRepository($this->repoInstalled); diff --git a/tests/Composer/Test/Fixtures/installer/update-requiring-decision-reverts-and-learning-positive-literals.test b/tests/Composer/Test/Fixtures/installer/update-requiring-decision-reverts-and-learning-positive-literals.test new file mode 100644 index 000000000..3f5667823 --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/update-requiring-decision-reverts-and-learning-positive-literals.test @@ -0,0 +1,100 @@ +--TEST-- +Update a project which requires decision reverts and learning a positive literal to arrive at a correct solution. + +Tests for solver regression in commit 451bab1c2cd58e05af6e21639b829408ad023463. See also SolverTest testLearnPositiveLiteral +--COMPOSER-- +{ + "repositories": [ + { + "type": "package", + "package": [ + { + "name": "spryker-feature/product", + "require": { + "spryker-feature/spryker-core": "1.0.0", + "spryker-shop/product-search-widget": ">=1.0.0", + "spryker/product-category-filter-gui": "1.0.0" + }, + "version": "1.0.0" + }, + { + "name": "spryker-feature/spryker-core", + "version": "1.0.0", + "require": { + "spryker/store": "1.0.0" + } + }, + { + "name": "spryker/store", + "version": "1.0.0", + "require": { + "spryker/kernel": "<=2.0.0" + } + }, + { + "name": "spryker-shop/product-search-widget", + "version": "1.0.0", + "require": { + "spryker/catalog": "1.0.0" + } + }, + { + "name": "spryker-shop/product-search-widget", + "version": "2.0.0", + "require": { + "spryker/catalog": "1.0.0", + "spryker/kernel": ">=1.0.0" + } + }, + { + "name": "spryker/product-category-filter-gui", + "version": "1.0.0", + "require": { + "spryker/catalog": ">=1.0.0" + } + }, + { + "name": "spryker/catalog", + "version": "1.0.0", + "require": { } + }, + { + "name": "spryker/catalog", + "version": "2.0.0", + "require": { } + }, + + { + "name": "spryker/kernel", + "version": "1.0.0", + "require": { } + }, + { + "name": "spryker/kernel", + "version": "2.0.0", + "require": { + } + }, + { + "name": "spryker/kernel", + "version": "3.0.0", + "require": { } + } + ] + } + ], + "require": { + "spryker-feature/product": "1.0.0" + } +} +--RUN-- +update +--EXPECT-- +Installing spryker/catalog (1.0.0) +Installing spryker/product-category-filter-gui (1.0.0) +Installing spryker/kernel (2.0.0) +Installing spryker-shop/product-search-widget (2.0.0) +Installing spryker/store (1.0.0) +Installing spryker-feature/spryker-core (1.0.0) +Installing spryker-feature/product (1.0.0) +