hugo
/
composer
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix solver problem exceptions with unexpected contradictory "Conclusions"

Browse Source 
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 451bab1c2c 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.
main
Nils Adermann 5 years ago
parent 27bdda1fd3
commit 6b2edeae56
4 changed files with 177 additions and 1 deletions
Show Stats Download Patch File Download Diff File

12
src/Composer/DependencyResolver/Decisions.php
Unescape Escape View File

@ -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;
}
}

2
src/Composer/DependencyResolver/Solver.php
Unescape Escape View File

@ -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;

64
tests/Composer/Test/DependencyResolver/SolverTest.php
Unescape Escape View File

@ -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);

100
tests/Composer/Test/Fixtures/installer/update-requiring-decision-reverts-and-learning-positive-literals.test
Unescape Escape View File

@ -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)
Write Preview
Loading…
Cancel
Save