From e5b948c683e98ed2618431456ad447d0d7c8d209 Mon Sep 17 00:00:00 2001 From: Christophe Coevoet Date: Mon, 9 Jul 2018 22:07:12 +0200 Subject: [PATCH 1/2] Refactor the handling of conflict rules in the solver Conflict rules are not added in the solver based on the packages loaded in the solver by require rules, instead of loading remote metadata for them. This has 2 benefits: - it reduces the number of conflict rules in the solver in case of conflict rules targetting packages which are not required - it fixes the behavior of replaces, which is meant to conflict with all versions of the replaced package, without introducing a performance regression (this behavior was changed when optimizing composer in the past). --- src/Composer/DependencyResolver/Pool.php | 4 +- .../DependencyResolver/RuleSetGenerator.php | 76 ++++++++++++++----- 2 files changed, 57 insertions(+), 23 deletions(-) diff --git a/src/Composer/DependencyResolver/Pool.php b/src/Composer/DependencyResolver/Pool.php index 3dc6d90be..085aaa7bf 100644 --- a/src/Composer/DependencyResolver/Pool.php +++ b/src/Composer/DependencyResolver/Pool.php @@ -317,12 +317,12 @@ class Pool implements \Countable * Checks if the package matches the given constraint directly or through * provided or replaced packages * - * @param array|PackageInterface $candidate + * @param PackageInterface $candidate * @param string $name Name of the package to be matched * @param ConstraintInterface $constraint The constraint to verify * @return int One of the MATCH* constants of this class or 0 if there is no match */ - private function match($candidate, $name, ConstraintInterface $constraint = null, $bypassFilters) + public function match($candidate, $name, ConstraintInterface $constraint = null, $bypassFilters) { $candidateName = $candidate->getName(); $candidateVersion = $candidate->getVersion(); diff --git a/src/Composer/DependencyResolver/RuleSetGenerator.php b/src/Composer/DependencyResolver/RuleSetGenerator.php index c534be958..60617ba43 100644 --- a/src/Composer/DependencyResolver/RuleSetGenerator.php +++ b/src/Composer/DependencyResolver/RuleSetGenerator.php @@ -28,6 +28,9 @@ class RuleSetGenerator protected $installedMap; protected $whitelistedMap; protected $addedMap; + protected $conflictAddedMap; + protected $addedPackages; + protected $addedPackagesByNames; public function __construct(PolicyInterface $policy, Pool $pool) { @@ -185,6 +188,7 @@ class RuleSetGenerator $workQueue->enqueue($package); while (!$workQueue->isEmpty()) { + /** @var PackageInterface $package */ $package = $workQueue->dequeue(); if (isset($this->addedMap[$package->id])) { continue; @@ -192,6 +196,11 @@ class RuleSetGenerator $this->addedMap[$package->id] = true; + $this->addedPackages[] = $package; + foreach ($package->getNames() as $name) { + $this->addedPackagesByNames[$name][] = $package; + } + foreach ($package->getRequires() as $link) { if ($ignorePlatformReqs && preg_match(PlatformRepository::PLATFORM_PACKAGE_REGEX, $link->getTarget())) { continue; @@ -206,11 +215,41 @@ class RuleSetGenerator } } + $packageName = $package->getName(); + $obsoleteProviders = $this->pool->whatProvides($packageName, null); + + foreach ($obsoleteProviders as $provider) { + if ($provider === $package) { + continue; + } + + if (($package instanceof AliasPackage) && $package->getAliasOf() === $provider) { + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRequireRule($package, array($provider), Rule::RULE_PACKAGE_ALIAS, $package)); + } elseif (!$this->obsoleteImpossibleForAlias($package, $provider)) { + $reason = ($packageName == $provider->getName()) ? Rule::RULE_PACKAGE_SAME_NAME : Rule::RULE_PACKAGE_IMPLICIT_OBSOLETES; + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $provider, $reason, $package)); + } + } + } + } + + protected function addConflictRules() + { + /** @var PackageInterface $package */ + foreach ($this->addedPackages as $package) { foreach ($package->getConflicts() as $link) { - $possibleConflicts = $this->pool->whatProvides($link->getTarget(), $link->getConstraint()); + if (!isset($this->addedPackagesByNames[$link->getTarget()])) { + continue; + } + + /** @var PackageInterface $possibleConflict */ + foreach ($this->addedPackagesByNames[$link->getTarget()] as $possibleConflict) { + $conflictMatch = $this->pool->match($possibleConflict, $link->getTarget(), $link->getConstraint(), true); + + if ($conflictMatch === Pool::MATCH || $conflictMatch === Pool::MATCH_REPLACE) { + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $possibleConflict, Rule::RULE_PACKAGE_CONFLICT, $link)); + } - foreach ($possibleConflicts as $conflict) { - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $conflict, Rule::RULE_PACKAGE_CONFLICT, $link)); } } @@ -218,9 +257,12 @@ class RuleSetGenerator $isInstalled = isset($this->installedMap[$package->id]); foreach ($package->getReplaces() as $link) { - $obsoleteProviders = $this->pool->whatProvides($link->getTarget(), $link->getConstraint()); + if (!isset($this->addedPackagesByNames[$link->getTarget()])) { + continue; + } - foreach ($obsoleteProviders as $provider) { + /** @var PackageInterface $possibleConflict */ + foreach ($this->addedPackagesByNames[$link->getTarget()] as $provider) { if ($provider === $package) { continue; } @@ -231,22 +273,6 @@ class RuleSetGenerator } } } - - $packageName = $package->getName(); - $obsoleteProviders = $this->pool->whatProvides($packageName, null); - - foreach ($obsoleteProviders as $provider) { - if ($provider === $package) { - continue; - } - - if (($package instanceof AliasPackage) && $package->getAliasOf() === $provider) { - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRequireRule($package, array($provider), Rule::RULE_PACKAGE_ALIAS, $package)); - } elseif (!$this->obsoleteImpossibleForAlias($package, $provider)) { - $reason = ($packageName == $provider->getName()) ? Rule::RULE_PACKAGE_SAME_NAME : Rule::RULE_PACKAGE_IMPLICIT_OBSOLETES; - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $provider, $reason, $package)); - } - } } } @@ -327,12 +353,20 @@ class RuleSetGenerator $this->pool->setWhitelist($this->whitelistedMap); $this->addedMap = array(); + $this->conflictAddedMap = array(); + $this->addedPackages = array(); + $this->addedPackagesByNames = array(); foreach ($this->installedMap as $package) { $this->addRulesForPackage($package, $ignorePlatformReqs); } $this->addRulesForJobs($ignorePlatformReqs); + $this->addConflictRules(); + + // Remove references to packages + $this->addedPackages = $this->addedPackagesByNames = null; + return $this->rules; } } From 8c3898aa576643fb4238141ad94c2f5a6e1e74ad Mon Sep 17 00:00:00 2001 From: Christophe Coevoet Date: Mon, 9 Jul 2018 22:51:23 +0200 Subject: [PATCH 2/2] Update tests for replace conflicts This reverts the test changes done in b4698568d2 to the original tests added in 1425bb7fc3. --- ...-installed-when-installing-from-lock.test} | 12 +++--- ...aced-packages-should-not-be-installed.test | 24 +++++++++++ ...kages-wrong-version-install-from-lock.test | 41 ------------------- 3 files changed, 29 insertions(+), 48 deletions(-) rename tests/Composer/Test/Fixtures/installer/{replaced-packages-wrong-version-install.test => replaced-packages-should-not-be-installed-when-installing-from-lock.test} (85%) create mode 100644 tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed.test delete mode 100644 tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install-from-lock.test diff --git a/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install.test b/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed-when-installing-from-lock.test similarity index 85% rename from tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install.test rename to tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed-when-installing-from-lock.test index 8971f8069..947d96b28 100644 --- a/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install.test +++ b/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed-when-installing-from-lock.test @@ -1,5 +1,5 @@ --TEST-- -Requiring a replaced package in a version, that is not provided by the replacing package, should install correctly (although that is not a very smart idea) +Requiring a replaced package in a version, that is not provided by the replacing package, should result in a conflict, when installing from lock --COMPOSER-- { "repositories": [ @@ -17,9 +17,7 @@ Requiring a replaced package in a version, that is not provided by the replacing "foo/replaced": "2.0.0" } } ---RUN-- -install ---EXPECT-LOCK-- +--LOCK-- { "packages": [ { "name": "foo/original", "version": "1.0.0", "replace": {"foo/replaced": "1.0.0"}, "type": "library" }, @@ -34,8 +32,8 @@ install "platform": [], "platform-dev": [] } +--RUN-- +install --EXPECT-EXIT-CODE-- -0 +2 --EXPECT-- -Installing foo/original (1.0.0) -Installing foo/replaced (2.0.0) diff --git a/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed.test b/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed.test new file mode 100644 index 000000000..0d1ea7701 --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed.test @@ -0,0 +1,24 @@ +--TEST-- +Requiring a replaced package in a version, that is not provided by the replacing package, should result in a conflict +--COMPOSER-- +{ + "repositories": [ + { + "type": "package", + "package": [ + { "name": "foo/original", "version": "1.0.0", "replace": {"foo/replaced": "1.0.0"} }, + { "name": "foo/replaced", "version": "1.0.0" }, + { "name": "foo/replaced", "version": "2.0.0" } + ] + } + ], + "require": { + "foo/original": "1.0.0", + "foo/replaced": "2.0.0" + } +} +--RUN-- +install +--EXPECT-EXIT-CODE-- +2 +--EXPECT-- diff --git a/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install-from-lock.test b/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install-from-lock.test deleted file mode 100644 index 2721772ae..000000000 --- a/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install-from-lock.test +++ /dev/null @@ -1,41 +0,0 @@ ---TEST-- -Requiring a replaced package in a version, that is not provided by the replacing package, should install correctly (although that is not a very smart idea) also when installing from lock ---COMPOSER-- -{ - "repositories": [ - { - "type": "package", - "package": [ - { "name": "foo/original", "version": "1.0.0", "replace": {"foo/replaced": "1.0.0"} }, - { "name": "foo/replaced", "version": "1.0.0" }, - { "name": "foo/replaced", "version": "2.0.0" } - ] - } - ], - "require": { - "foo/original": "1.0.0", - "foo/replaced": "2.0.0" - } -} ---LOCK-- -{ - "packages": [ - { "name": "foo/original", "version": "1.0.0", "replace": {"foo/replaced": "1.0.0"}, "type": "library" }, - { "name": "foo/replaced", "version": "2.0.0", "type": "library" } - ], - "packages-dev": [], - "aliases": [], - "minimum-stability": "stable", - "stability-flags": {}, - "prefer-stable": false, - "prefer-lowest": false, - "platform": [], - "platform-dev": [] -} ---RUN-- -install ---EXPECT-EXIT-CODE-- -0 ---EXPECT-- -Installing foo/original (1.0.0) -Installing foo/replaced (2.0.0)