From 4e3d9899789cd2f80c9b608ff3f1429a0f7dbbf1 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Thu, 30 Jan 2020 21:46:58 +0100 Subject: [PATCH 1/4] A package providing a name should not conflict with a package replacing it Simplified whatProvides, mustMatchName is unused, removed unused function from policy --- src/Composer/DependencyResolver/Pool.php | 31 ++++++++----------- .../DependencyResolver/RuleSetGenerator.php | 4 +-- src/Composer/Package/BasePackage.php | 8 +++-- src/Composer/Package/PackageInterface.php | 4 ++- .../installer/provider-conflicts2.test | 18 ++--------- 5 files changed, 26 insertions(+), 39 deletions(-) diff --git a/src/Composer/DependencyResolver/Pool.php b/src/Composer/DependencyResolver/Pool.php index 1e1100b15..7088b3d50 100644 --- a/src/Composer/DependencyResolver/Pool.php +++ b/src/Composer/DependencyResolver/Pool.php @@ -34,7 +34,6 @@ class Pool implements \Countable protected $packages = array(); protected $packageByName = array(); - protected $packageByExactName = array(); protected $versionParser; protected $providerCache = array(); protected $unacceptableFixedPackages; @@ -54,7 +53,6 @@ class Pool implements \Countable $this->packages[] = $package; $package->id = $id++; - $this->packageByExactName[$package->getName()][$package->id] = $package; foreach ($package->getNames() as $provided) { $this->packageByName[$provided][] = $package; @@ -87,44 +85,41 @@ class Pool implements \Countable * @param string $name The package name to be searched for * @param ConstraintInterface $constraint A constraint that all returned * packages must match or null to return all - * @param bool $mustMatchName Whether the name of returned packages - * must match the given name * @return PackageInterface[] A set of packages */ - public function whatProvides($name, ConstraintInterface $constraint = null, $mustMatchName = false) + public function whatProvides($name, ConstraintInterface $constraint = null, $allowProvide = true) { - $key = ((int) $mustMatchName).$constraint; + $key = ((int) $allowProvide).$constraint; if (isset($this->providerCache[$name][$key])) { return $this->providerCache[$name][$key]; } - return $this->providerCache[$name][$key] = $this->computeWhatProvides($name, $constraint, $mustMatchName); + return $this->providerCache[$name][$key] = $this->computeWhatProvides($name, $constraint, $allowProvide); } /** * @see whatProvides */ - private function computeWhatProvides($name, $constraint, $mustMatchName = false) + private function computeWhatProvides($name, $constraint, $allowProvide = true) { - $candidates = array(); - - if ($mustMatchName) { - if (isset($this->packageByExactName[$name])) { - $candidates = $this->packageByExactName[$name]; - } - } elseif (isset($this->packageByName[$name])) { - $candidates = $this->packageByName[$name]; + if (!isset($this->packageByName[$name])) { + return array(); } $matches = array(); - foreach ($candidates as $candidate) { + foreach ($this->packageByName[$name] as $candidate) { switch ($this->match($candidate, $name, $constraint)) { case self::MATCH_NONE: break; - case self::MATCH: case self::MATCH_PROVIDE: + if ($allowProvide) { + $matches[] = $candidate; + } + break; + + case self::MATCH: case self::MATCH_REPLACE: $matches[] = $candidate; break; diff --git a/src/Composer/DependencyResolver/RuleSetGenerator.php b/src/Composer/DependencyResolver/RuleSetGenerator.php index 717a69d20..ca10702a2 100644 --- a/src/Composer/DependencyResolver/RuleSetGenerator.php +++ b/src/Composer/DependencyResolver/RuleSetGenerator.php @@ -160,7 +160,7 @@ class RuleSetGenerator $this->addedMap[$package->id] = true; $this->addedPackages[] = $package; - foreach ($package->getNames() as $name) { + foreach ($package->getNames(false) as $name) { $this->addedPackagesByNames[$name][] = $package; } @@ -179,7 +179,7 @@ class RuleSetGenerator } $packageName = $package->getName(); - $obsoleteProviders = $this->pool->whatProvides($packageName, null); + $obsoleteProviders = $this->pool->whatProvides($packageName, null, false); foreach ($obsoleteProviders as $provider) { if ($provider === $package) { diff --git a/src/Composer/Package/BasePackage.php b/src/Composer/Package/BasePackage.php index 9ecbcb332..74cf3516e 100644 --- a/src/Composer/Package/BasePackage.php +++ b/src/Composer/Package/BasePackage.php @@ -89,14 +89,16 @@ abstract class BasePackage implements PackageInterface /** * {@inheritDoc} */ - public function getNames() + public function getNames($provides = true) { $names = array( $this->getName() => true, ); - foreach ($this->getProvides() as $link) { - $names[$link->getTarget()] = true; + if ($provides) { + foreach ($this->getProvides() as $link) { + $names[$link->getTarget()] = true; + } } foreach ($this->getReplaces() as $link) { diff --git a/src/Composer/Package/PackageInterface.php b/src/Composer/Package/PackageInterface.php index 9db188358..8f8087134 100644 --- a/src/Composer/Package/PackageInterface.php +++ b/src/Composer/Package/PackageInterface.php @@ -45,9 +45,11 @@ interface PackageInterface * No version or release type information should be included in any of the * names. Provided or replaced package names need to be returned as well. * + * @param bool $provides Whether provided names should be included + * * @return array An array of strings referring to this package */ - public function getNames(); + public function getNames($provides = true); /** * Allows the solver to set an id for this package to refer to it. diff --git a/tests/Composer/Test/Fixtures/installer/provider-conflicts2.test b/tests/Composer/Test/Fixtures/installer/provider-conflicts2.test index 343dab537..912d6c86e 100644 --- a/tests/Composer/Test/Fixtures/installer/provider-conflicts2.test +++ b/tests/Composer/Test/Fixtures/installer/provider-conflicts2.test @@ -1,5 +1,5 @@ --TEST-- -Test that names provided by two dependents cause a conflict +Providers of a replaced name should be installable --COMPOSER-- { "repositories": [ @@ -28,18 +28,6 @@ Test that names provided by two dependents cause a conflict --RUN-- update ---EXPECT-EXIT-CODE-- -2 - ---EXPECT-OUTPUT-- -Loading composer repositories with package information -Updating dependencies -Your requirements could not be resolved to an installable set of packages. - - Problem 1 - - Root composer.json requires provider/pkg * -> satisfiable by provider/pkg[1.0.0]. - - Only one of these can be installed: replacer/pkg 1.0.0, provider/pkg 1.0.0. - - Root composer.json requires replacer/pkg * -> satisfiable by replacer/pkg[1.0.0]. - --EXPECT-- - +Installing provider/pkg (1.0.0) +Installing replacer/pkg (1.0.0) From 80a5fdf398cc417d2a4251de8591c56cf6484861 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Fri, 31 Jan 2020 01:06:09 +0100 Subject: [PATCH 2/4] Remove obsolete rules and their generation The only automatic conflict we have results from packages using the same name either by literally having the same name and being different versions or they replace the same name, so - removed all types of obsolete rules - simplified rule generation significantly - got rid of provide filtering in the pool - fixed some language in error handling --- src/Composer/DependencyResolver/Pool.php | 15 +-- src/Composer/DependencyResolver/Rule.php | 92 ++++++------------- .../DependencyResolver/RuleSetGenerator.php | 59 ++---------- .../installer/provider-conflicts.test | 2 +- 4 files changed, 43 insertions(+), 125 deletions(-) diff --git a/src/Composer/DependencyResolver/Pool.php b/src/Composer/DependencyResolver/Pool.php index 7088b3d50..0ab1b67e4 100644 --- a/src/Composer/DependencyResolver/Pool.php +++ b/src/Composer/DependencyResolver/Pool.php @@ -87,20 +87,20 @@ class Pool implements \Countable * packages must match or null to return all * @return PackageInterface[] A set of packages */ - public function whatProvides($name, ConstraintInterface $constraint = null, $allowProvide = true) + public function whatProvides($name, ConstraintInterface $constraint = null) { - $key = ((int) $allowProvide).$constraint; + $key = (string) $constraint; if (isset($this->providerCache[$name][$key])) { return $this->providerCache[$name][$key]; } - return $this->providerCache[$name][$key] = $this->computeWhatProvides($name, $constraint, $allowProvide); + return $this->providerCache[$name][$key] = $this->computeWhatProvides($name, $constraint); } /** * @see whatProvides */ - private function computeWhatProvides($name, $constraint, $allowProvide = true) + private function computeWhatProvides($name, $constraint) { if (!isset($this->packageByName[$name])) { return array(); @@ -113,14 +113,9 @@ class Pool implements \Countable case self::MATCH_NONE: break; - case self::MATCH_PROVIDE: - if ($allowProvide) { - $matches[] = $candidate; - } - break; - case self::MATCH: case self::MATCH_REPLACE: + case self::MATCH_PROVIDE: $matches[] = $candidate; break; diff --git a/src/Composer/DependencyResolver/Rule.php b/src/Composer/DependencyResolver/Rule.php index fe1afd12e..7885efda3 100644 --- a/src/Composer/DependencyResolver/Rule.php +++ b/src/Composer/DependencyResolver/Rule.php @@ -29,10 +29,7 @@ abstract class Rule const RULE_FIXED = 3; const RULE_PACKAGE_CONFLICT = 6; const RULE_PACKAGE_REQUIRES = 7; - const RULE_PACKAGE_OBSOLETES = 8; - const RULE_INSTALLED_PACKAGE_OBSOLETES = 9; const RULE_PACKAGE_SAME_NAME = 10; - const RULE_PACKAGE_IMPLICIT_OBSOLETES = 11; const RULE_LEARNED = 12; const RULE_PACKAGE_ALIAS = 13; @@ -191,83 +188,50 @@ abstract class Rule return $text; - case self::RULE_PACKAGE_OBSOLETES: - if (count($literals) === 2 && $literals[0] < 0 && $literals[1] < 0) { - $package1 = $pool->literalToPackage($literals[0]); - $package2 = $pool->literalToPackage($literals[1]); - - $replaces1 = $this->getReplacedNames($package1); - $replaces2 = $this->getReplacedNames($package2); - - $reason = null; - if ($conflictingNames = array_values(array_intersect($replaces1, $replaces2))) { - $reason = 'They both replace '.(count($conflictingNames) > 1 ? '['.implode(', ', $conflictingNames).']' : $conflictingNames[0]).' and thus cannot coexist.'; - } elseif (in_array($package1->getName(), $replaces2, true)) { - $reason = $package2->getName().' replaces '.$package1->getName().' and thus cannot coexist with it.'; - } elseif (in_array($package2->getName(), $replaces1, true)) { - $reason = $package1->getName().' replaces '.$package2->getName().' and thus cannot coexist with it.'; - } - - if ($reason) { - if (isset($installedMap[$package1->id]) && !isset($installedMap[$package2->id])) { - // swap vars so the if below passes - $tmp = $package2; - $package2 = $package1; - $package1 = $tmp; - } - if (!isset($installedMap[$package1->id]) && isset($installedMap[$package2->id])) { - return $package1->getPrettyString().' cannot be installed as that would require removing '.$package2->getPrettyString().'. '.$reason; - } - - if (!isset($installedMap[$package1->id]) && !isset($installedMap[$package2->id])) { - return 'Only one of these can be installed: '.$package1->getPrettyString().', '.$package2->getPrettyString().'. '.$reason; - } - } - - return 'Only one of these can be installed: '.$package1->getPrettyString().', '.$package2->getPrettyString().'.'; - } - - return $ruleText; - case self::RULE_INSTALLED_PACKAGE_OBSOLETES: - return $ruleText; case self::RULE_PACKAGE_SAME_NAME: - $replacedNames = null; $packageNames = array(); foreach ($literals as $literal) { $package = $pool->literalToPackage($literal); - $pkgReplaces = $this->getReplacedNames($package); - if ($pkgReplaces) { - if ($replacedNames === null) { - $replacedNames = $this->getReplacedNames($package); + $packageNames[$package->getName()] = true; + } + $replacedName = $this->reasonData; + + if (count($packageNames) > 1) { + $reason = null; + + if (!isset($packageNames[$replacedName])) { + $reason = 'They '.(count($literals) == 2 ? 'both' : 'all').' replace '.$replacedName.' and thus cannot coexist.'; + } else { + $replacerNames = $packageNames; + unset($replacerNames[$replacedName]); + $replacerNames = array_keys($replacerNames); + + if (count($replacerNames) == 1) { + $reason = $replacerNames[0] . ' replaces '; } else { - $replacedNames = array_intersect($replacedNames, $this->getReplacedNames($package)); + $reason = '['.implode(', ', $replacerNames).'] replace '; } + $reason .= $replacedName.' and thus cannot coexist with it.'; } - $packageNames[$package->getName()] = true; - } - if ($replacedNames) { - $replacedNames = array_values(array_intersect(array_keys($packageNames), $replacedNames)); - } - if ($replacedNames && count($packageNames) > 1) { - $replacer = null; + $installedPackages = array(); + $removablePackages = array(); foreach ($literals as $literal) { - $package = $pool->literalToPackage($literal); - if (array_intersect($replacedNames, $this->getReplacedNames($package))) { - $replacer = $package; - break; + if (isset($installedMap[abs($literal)])) { + $installedPackages[] = $pool->literalToPackage($literal); + } else { + $removablePackages[] = $pool->literalToPackage($literal); } } - $replacedNames = count($replacedNames) > 1 ? '['.implode(', ', $replacedNames).']' : $replacedNames[0]; - if ($replacer) { - return 'Only one of these can be installed: ' . $this->formatPackagesUnique($pool, $literals) . '. '.$replacer->getName().' replaces '.$replacedNames.' and thus cannot coexist with it.'; + if ($installedPackages && $removablePackages) { + return $this->formatPackagesUnique($pool, $removablePackages).' cannot be installed as that would require removing '.$this->formatPackagesUnique($pool, $installedPackages).'. '.$reason; } + + return 'Only one of these can be installed: '.$this->formatPackagesUnique($pool, $literals).'. '.$reason; } return 'You can only install one version of a package, so only one of these can be installed: ' . $this->formatPackagesUnique($pool, $literals) . '.'; - case self::RULE_PACKAGE_IMPLICIT_OBSOLETES: - return $ruleText; case self::RULE_LEARNED: if (isset($learnedPool[$this->reasonData])) { $learnedString = ', learned rules:'."\n - "; diff --git a/src/Composer/DependencyResolver/RuleSetGenerator.php b/src/Composer/DependencyResolver/RuleSetGenerator.php index ca10702a2..e6ac7ac31 100644 --- a/src/Composer/DependencyResolver/RuleSetGenerator.php +++ b/src/Composer/DependencyResolver/RuleSetGenerator.php @@ -160,8 +160,12 @@ class RuleSetGenerator $this->addedMap[$package->id] = true; $this->addedPackages[] = $package; - foreach ($package->getNames(false) as $name) { - $this->addedPackagesByNames[$name][] = $package; + if (!$package instanceof AliasPackage) { + foreach ($package->getNames(false) as $name) { + $this->addedPackagesByNames[$name][] = $package; + } + } else { + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRequireRule($package, array($package->getAliasOf()), Rule::RULE_PACKAGE_ALIAS, $package)); } foreach ($package->getRequires() as $link) { @@ -177,29 +181,6 @@ class RuleSetGenerator $workQueue->enqueue($require); } } - - $packageName = $package->getName(); - $obsoleteProviders = $this->pool->whatProvides($packageName, null, false); - - 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)); - } else { - if (!isset($this->conflictsForName[$packageName])) { - $this->conflictsForName[$packageName] = array(); - } - if (!$package instanceof AliasPackage) { - $this->conflictsForName[$packageName][$package->id] = $package; - } - if (!$provider instanceof AliasPackage) { - $this->conflictsForName[$packageName][$provider->id] = $provider; - } - } - } } } @@ -218,39 +199,17 @@ class RuleSetGenerator /** @var PackageInterface $possibleConflict */ foreach ($this->addedPackagesByNames[$link->getTarget()] as $possibleConflict) { - $conflictMatch = $this->pool->match($possibleConflict, $link->getTarget(), $link->getConstraint()); - - if ($conflictMatch === Pool::MATCH || $conflictMatch === Pool::MATCH_REPLACE) { + if ($this->pool->match($possibleConflict, $link->getTarget(), $link->getConstraint())) { $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $possibleConflict, Rule::RULE_PACKAGE_CONFLICT, $link)); } - - } - } - - // check obsoletes and implicit obsoletes of a package - foreach ($package->getReplaces() as $link) { - if (!isset($this->addedPackagesByNames[$link->getTarget()])) { - continue; - } - - /** @var PackageInterface $possibleConflict */ - foreach ($this->addedPackagesByNames[$link->getTarget()] as $provider) { - if ($provider === $package) { - continue; - } - - if (!$this->obsoleteImpossibleForAlias($package, $provider)) { - $reason = Rule::RULE_PACKAGE_OBSOLETES; - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $provider, $reason, $link)); - } } } } - foreach ($this->conflictsForName as $name => $packages) { + foreach ($this->addedPackagesByNames as $name => $packages) { if (count($packages) > 1) { $reason = Rule::RULE_PACKAGE_SAME_NAME; - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createMultiConflictRule($packages, $reason, null)); + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createMultiConflictRule($packages, $reason, $name)); } } } diff --git a/tests/Composer/Test/Fixtures/installer/provider-conflicts.test b/tests/Composer/Test/Fixtures/installer/provider-conflicts.test index 3a9700103..a73dd3dc9 100644 --- a/tests/Composer/Test/Fixtures/installer/provider-conflicts.test +++ b/tests/Composer/Test/Fixtures/installer/provider-conflicts.test @@ -42,7 +42,7 @@ Your requirements could not be resolved to an installable set of packages. Problem 1 - __root__ is present at version 1.2.3 and cannot be modified by Composer - - provider/pkg 1.0.0 cannot be installed as that would require removing __root__ 1.2.3. They both replace root-replaced/transitive-replaced and thus cannot coexist. + - provider/pkg[1.0.0] cannot be installed as that would require removing __root__[1.2.3]. They both replace root-replaced/transitive-replaced and thus cannot coexist. - Root composer.json requires provider/pkg * -> satisfiable by provider/pkg[1.0.0]. --EXPECT-- From 415b36a1a1cf9bae5709018876a43d772498e995 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Fri, 31 Jan 2020 01:11:13 +0100 Subject: [PATCH 3/4] Remove match types from Pool as these are no longer used --- src/Composer/DependencyResolver/Pool.php | 37 +++++++----------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/src/Composer/DependencyResolver/Pool.php b/src/Composer/DependencyResolver/Pool.php index 0ab1b67e4..abf3d70bf 100644 --- a/src/Composer/DependencyResolver/Pool.php +++ b/src/Composer/DependencyResolver/Pool.php @@ -27,11 +27,6 @@ use Composer\Package\PackageInterface; */ class Pool implements \Countable { - const MATCH_NONE = 0; - const MATCH = 1; - const MATCH_PROVIDE = 2; - const MATCH_REPLACE = 3; - protected $packages = array(); protected $packageByName = array(); protected $versionParser; @@ -109,18 +104,8 @@ class Pool implements \Countable $matches = array(); foreach ($this->packageByName[$name] as $candidate) { - switch ($this->match($candidate, $name, $constraint)) { - case self::MATCH_NONE: - break; - - case self::MATCH: - case self::MATCH_REPLACE: - case self::MATCH_PROVIDE: - $matches[] = $candidate; - break; - - default: - throw new \UnexpectedValueException('Unexpected match type'); + if ($this->match($candidate, $name, $constraint)) { + $matches[] = $candidate; } } @@ -154,7 +139,7 @@ class Pool implements \Countable * @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 + * @return bool */ public function match($candidate, $name, ConstraintInterface $constraint = null) { @@ -165,10 +150,10 @@ class Pool implements \Countable $pkgConstraint = new Constraint('==', $candidateVersion); if ($constraint === null || $constraint->matches($pkgConstraint)) { - return self::MATCH; + return true; } - return self::MATCH_NONE; + return false; } $provides = $candidate->getProvides(); @@ -178,28 +163,28 @@ class Pool implements \Countable if (isset($replaces[0]) || isset($provides[0])) { foreach ($provides as $link) { if ($link->getTarget() === $name && ($constraint === null || $constraint->matches($link->getConstraint()))) { - return self::MATCH_PROVIDE; + return true; } } foreach ($replaces as $link) { if ($link->getTarget() === $name && ($constraint === null || $constraint->matches($link->getConstraint()))) { - return self::MATCH_REPLACE; + return true; } } - return self::MATCH_NONE; + return false; } if (isset($provides[$name]) && ($constraint === null || $constraint->matches($provides[$name]->getConstraint()))) { - return self::MATCH_PROVIDE; + return true; } if (isset($replaces[$name]) && ($constraint === null || $constraint->matches($replaces[$name]->getConstraint()))) { - return self::MATCH_REPLACE; + return true; } - return self::MATCH_NONE; + return false; } public function isUnacceptableFixedPackage(PackageInterface $package) From f38e969b026144b0e9fb17b570b9b50bc9d89b74 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Tue, 7 Apr 2020 11:16:13 +0200 Subject: [PATCH 4/4] Update test expected output to different formatting --- .../installer/update-allow-list-require-new-replace.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Composer/Test/Fixtures/installer/update-allow-list-require-new-replace.test b/tests/Composer/Test/Fixtures/installer/update-allow-list-require-new-replace.test index dc6e9aa5f..641ac7f9e 100644 --- a/tests/Composer/Test/Fixtures/installer/update-allow-list-require-new-replace.test +++ b/tests/Composer/Test/Fixtures/installer/update-allow-list-require-new-replace.test @@ -48,7 +48,7 @@ Your requirements could not be resolved to an installable set of packages. Problem 1 - current/dep is locked to version 1.0.0 and an update of this package was not requested. - - new/pkg 1.0.0 cannot be installed as that would require removing current/dep 1.0.0. new/pkg replaces current/dep and thus cannot coexist with it. + - new/pkg[1.0.0] cannot be installed as that would require removing current/dep[1.0.0]. new/pkg replaces current/dep and thus cannot coexist with it. - Root composer.json requires new/pkg 1.* -> satisfiable by new/pkg[1.0.0]. Use the option --with-all-dependencies to allow updates and removals for packages currently locked to specific versions.