Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix PoolOptimizer should consider disjunctive MultiConstraints #10579

Merged
merged 8 commits into from Mar 12, 2022
Merged

Fix PoolOptimizer should consider disjunctive MultiConstraints #10579

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b23177b
Failing test case
Toflar Feb 28, 2022
fa0eae9
Quickfix, needs more investigation
Toflar Feb 28, 2022
073e115
Only disjunctive multi constraints are of interest
Toflar Feb 28, 2022
160e1f4
More test coverage
Toflar Mar 2, 2022
8263c96
CS
Toflar Mar 2, 2022
abbe9c9
CS
Toflar Mar 2, 2022
9169458
Added a note and removed useless test case
Toflar Mar 2, 2022
51c22aa
Simplify code
Seldaek Mar 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
70 changes: 64 additions & 6 deletions src/Composer/DependencyResolver/PoolOptimizer.php
View file
Open in desktop
Expand Up @@ -110,21 +110,18 @@ private function prepare(Request $request, Pool $pool)

// Extract requested package requirements
foreach ($request->getRequires() as $require => $constraint) {
$constraint = Intervals::compactConstraint($constraint);
$this->requireConstraintsPerPackage[$require][(string) $constraint] = $constraint;
$this->extractRequireConstraintsPerPackage($require, $constraint);
}

// First pass over all packages to extract information and mark package constraints irremovable
foreach ($pool->getPackages() as $package) {
// Extract package requirements
foreach ($package->getRequires() as $link) {
$constraint = Intervals::compactConstraint($link->getConstraint());
$this->requireConstraintsPerPackage[$link->getTarget()][(string) $constraint] = $constraint;
$this->extractRequireConstraintsPerPackage($link->getTarget(), $link->getConstraint());
}
// Extract package conflicts
foreach ($package->getConflicts() as $link) {
$constraint = Intervals::compactConstraint($link->getConstraint());
$this->conflictConstraintsPerPackage[$link->getTarget()][(string) $constraint] = $constraint;
$this->extractConflictConstraintsPerPackage($link->getTarget(), $link->getConstraint());
}

// Keep track of alias packages for every package so if either the alias or aliased is kept
Expand Down Expand Up @@ -453,4 +450,65 @@ private function optimizeImpossiblePackagesAway(Request $request, Pool $pool)
}
}
}

/**
* Disjunctive require constraints need to be considered in their own group. E.g. "^2.14 || ^3.3" needs to generate
* two require constraint groups in order for us to keep the best matching package for "^2.14" AND "^3.3" as otherwise, we'd
* only keep either one which can cause trouble (e.g. when using --prefer-lowest).
*
* @param string $package
* @param ConstraintInterface $constraint
* @return void
*/
private function extractRequireConstraintsPerPackage($package, ConstraintInterface $constraint)
{
foreach ($this->expandDisjunctiveMultiConstraints($constraint) as $expanded) {
$this->requireConstraintsPerPackage[$package][(string) $expanded] = $expanded;
}
}

/**
* Disjunctive conflict constraints need to be considered in their own group. E.g. "^2.14 || ^3.3" needs to generate
* two conflict constraint groups in order for us to keep the best matching package for "^2.14" AND "^3.3" as otherwise, we'd
* only keep either one which can cause trouble (e.g. when using --prefer-lowest).
*
* @param string $package
* @param ConstraintInterface $constraint
* @return void
*/
private function extractConflictConstraintsPerPackage($package, ConstraintInterface $constraint)
{
foreach ($this->expandDisjunctiveMultiConstraints($constraint) as $expanded) {
$this->conflictConstraintsPerPackage[$package][(string) $expanded] = $expanded;
}
}

/**
*
* @param ConstraintInterface $constraint
* @return ConstraintInterface[]
*/
private function expandDisjunctiveMultiConstraints(ConstraintInterface $constraint)
{
$expanded = array();
$constraint = Intervals::compactConstraint($constraint);

if ($constraint instanceof MultiConstraint && $constraint->isDisjunctive()) {
foreach ($constraint->getConstraints() as $sub) {
// TODO: Do we need to call ourselves recursively? Is it even possible?
// Tried to write a testcase in PoolOptimizerTest::testNestedDisjunctiveMultiConstraints() but
// couldn't find an example where Intervals::compactConstraint() leaves a nested disjunctive MultiConstraint
// untouched.
// $expanded = array_merge($expanded, $this->expandDisjunctiveMultiConstraints($sub));
$expanded[] = $sub;
}

return $expanded;
}

// Regular constraints and conjunctive MultiConstraints
$expanded[] = $constraint;

return $expanded;
}
}
10 changes: 9 additions & 1 deletion tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/basic-prefer-highest.test
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@ Test filters irrelevant package "package/b" in version 1.0.0
"name": "package/a",
"version": "1.0.0",
"require": {
"package/b": "^1.0"
"package/b": ">=1.0 <1.1 || ^1.2"
}
},
{
Expand All @@ -25,6 +25,10 @@ Test filters irrelevant package "package/b" in version 1.0.0
{
"name": "package/b",
"version": "1.0.1"
},
{
"name": "package/b",
"version": "1.2.0"
}
]

Expand All @@ -41,6 +45,10 @@ Test filters irrelevant package "package/b" in version 1.0.0
{
"name": "package/b",
"version": "1.0.1"
},
{
"name": "package/b",
"version": "1.2.0"
}
]

55 changes: 55 additions & 0 deletions tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/complex-prefer-lowest.test
View file
Open in desktop
@@ -0,0 +1,55 @@
--TEST--
Test keeps package "package/b" in version 2.2.0 because for prefer-lowest either one might be relevant

--REQUEST--
{
"require": {
"package/a": "^1.0"
},
"preferLowest": true
}


--POOL-BEFORE--
[
{
"name": "package/a",
"version": "1.0.0",
"require": {
"package/b": "^1.0 || ^2.2"
}
},
{
"name": "package/b",
"version": "1.0.0"
},
{
"name": "package/b",
"version": "1.0.1"
},
{
"name": "package/b",
"version": "2.2.0"
}
]


--POOL-AFTER--
[
{
"name": "package/a",
"version": "1.0.0",
"require": {
"package/b": "^1.0"
}
},
{
"name": "package/b",
"version": "1.0.0"
},
{
"name": "package/b",
"version": "2.2.0"
}
]

78 changes: 78 additions & 0 deletions tests/Composer/Test/DependencyResolver/PoolOptimizerTest.php
View file
Open in desktop
Expand Up @@ -19,14 +19,92 @@
use Composer\Json\JsonFile;
use Composer\Package\AliasPackage;
use Composer\Package\BasePackage;
use Composer\Package\Link;
use Composer\Package\Loader\ArrayLoader;
use Composer\Package\RootPackage;
use Composer\Package\Version\VersionParser;
use Composer\Pcre\Preg;
use Composer\Repository\LockArrayRepository;
use Composer\Semver\Constraint\Constraint;
use Composer\Semver\Constraint\MatchAllConstraint;
use Composer\Semver\Constraint\MultiConstraint;
use Composer\Test\TestCase;

class PoolOptimizerTest extends TestCase
{
public function testNestedDisjunctiveMultiConstraints()
{
$requirer = new RootPackage('package/a', '1.0.0.0', '1.0.0');

$requirer->setRequires(array(
'package/b' => new Link('package/a', 'package/b', new MultiConstraint( // Not possible with the version parser but this represents (^2.5 || (~1.2.3 || ^4.0))
array(
new MultiConstraint( // ^2.5
array(
new Constraint('>=', '2.5.0.0-dev'),
new Constraint('<', '3.0.0.0-dev'),
),
true // conjunctive
),
new MultiConstraint( // ~1.2.3 || ^4.0
array(
new MultiConstraint( // ~1.2.3
array(
new Constraint('>=', '1.2.3.0-dev'),
new Constraint('<', '1.3.0.0-dev'),
),
true // conjunctive
),
new MultiConstraint( // ^4.0
array(
new Constraint('>=', '4.0.0.0-dev'),
new Constraint('<', '5.0.0.0-dev'),
),
true // conjunctive
),
),
false // disjunctive
),
),
false // disjunctive
)
)
));


$packagesBefore = array(
$requirer,
$this->loadPackage(array('name' => 'package/b', 'version' => '1.2.3')),
$this->loadPackage(array('name' => 'package/b', 'version' => '1.2.4')),
$this->loadPackage(array('name' => 'package/b', 'version' => '2.5.0')),
$this->loadPackage(array('name' => 'package/b', 'version' => '2.5.1')),
$this->loadPackage(array('name' => 'package/b', 'version' => '4.0.0')),
$this->loadPackage(array('name' => 'package/b', 'version' => '4.0.1')),
);

$request = new Request(new LockArrayRepository());
$request->requireName('package/a');

$pool = new Pool($packagesBefore);
$poolOptimizer = new PoolOptimizer(new DefaultPolicy(true, true)); // --prefer-lowest
$optimizedPool = $poolOptimizer->optimize($request, $pool);

$this->assertSame(array(
'package/a@1.0.0.0',
'package/b@1.2.3.0',
'package/b@2.5.0.0',
'package/b@4.0.0.0',
), $this->reducePackagesInfoForComparison($optimizedPool->getPackages()));


$this->assertSame(array(
'1.2.4.0' => '1.2.4',
'2.5.1.0' => '2.5.1',
'4.0.1.0' => '4.0.1',
), $optimizedPool->getRemovedVersions('package/b', new MatchAllConstraint()));
}


/**
* @dataProvider provideIntegrationTests
* @param mixed[] $requestData
Expand Down