From 73e24ea9fbcf07c0626513e63bbc8c0607b21e4f Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Thu, 17 Sep 2020 12:08:04 +0200 Subject: [PATCH 1/4] Partial updates should remove all unused dependencies Instead of marking locked packages as fixed, we change the pool builder to load only the locked version and treat it like a fixed package, but removing the actual request fix, makes the solver treat it as a regular optional dependency. As a consequence locked packages may be removed on a partial update of another package, but they cannot be updated. --- .../DependencyResolver/LockTransaction.php | 1 + .../DependencyResolver/PoolBuilder.php | 13 ++++-- .../installer/remove-deletes-unused-deps.test | 46 +++++++++++++++++++ ...ing-if-removal-requires-update-of-dep.test | 39 ++++++++++++++++ 4 files changed, 94 insertions(+), 5 deletions(-) create mode 100644 tests/Composer/Test/Fixtures/installer/remove-deletes-unused-deps.test create mode 100644 tests/Composer/Test/Fixtures/installer/remove-does-nothing-if-removal-requires-update-of-dep.test diff --git a/src/Composer/DependencyResolver/LockTransaction.php b/src/Composer/DependencyResolver/LockTransaction.php index fbe543522..3b0255304 100644 --- a/src/Composer/DependencyResolver/LockTransaction.php +++ b/src/Composer/DependencyResolver/LockTransaction.php @@ -62,6 +62,7 @@ class LockTransaction extends Transaction if ($literal > 0) { $package = $pool->literalToPackage($literal); + $this->resultPackages['all'][] = $package; if (!isset($this->unlockableMap[$package->id])) { $this->resultPackages['non-dev'][] = $package; diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 15995a9cd..a245a50c3 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -123,13 +123,16 @@ class PoolBuilder public function buildPool(array $repositories, Request $request) { + $singleVersionPackages = $request->getFixedPackages(); + if ($request->getUpdateAllowList()) { $this->updateAllowList = $request->getUpdateAllowList(); $this->warnAboutNonMatchingUpdateAllowList($request); foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) { if (!$this->isUpdateAllowed($lockedPackage)) { - $request->fixPackage($lockedPackage); + //$request->fixPackage($lockedPackage); + $singleVersionPackages[] = $lockedPackage; $lockedName = $lockedPackage->getName(); // remember which packages we skipped loading remote content for in this partial update $this->skippedLoad[$lockedName] = $lockedName; @@ -140,7 +143,7 @@ class PoolBuilder } } - foreach ($request->getFixedPackages() as $package) { + foreach ($singleVersionPackages as $package) { // using MatchAllConstraint here because fixed packages do not need to retrigger // loading any packages $this->loadedPackages[$package->getName()] = new MatchAllConstraint(); @@ -345,7 +348,7 @@ class PoolBuilder // apply to if (isset($this->rootReferences[$name])) { // do not modify the references on already locked packages - if (!$request->isFixedPackage($package)) { + if ($request->getLockedRepository() !== $package->getRepository() && !$request->isFixedPackage($package)) { $package->setSourceDistReferences($this->rootReferences[$name]); } } @@ -473,7 +476,7 @@ class PoolBuilder foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) { if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) { if (false !== $index = array_search($lockedPackage, $this->packages, true)) { - $request->unfixPackage($lockedPackage); + //$request->unfixPackage($lockedPackage); $this->removeLoadedPackage($request, $lockedPackage, $index); } } @@ -496,7 +499,7 @@ class PoolBuilder unset($this->packages[$index]); if (isset($this->aliasMap[spl_object_hash($package)])) { foreach ($this->aliasMap[spl_object_hash($package)] as $aliasIndex => $aliasPackage) { - $request->unfixPackage($aliasPackage); + //$request->unfixPackage($aliasPackage); unset($this->packages[$aliasIndex]); } unset($this->aliasMap[spl_object_hash($package)]); diff --git a/tests/Composer/Test/Fixtures/installer/remove-deletes-unused-deps.test b/tests/Composer/Test/Fixtures/installer/remove-deletes-unused-deps.test new file mode 100644 index 000000000..8a81d02e9 --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/remove-deletes-unused-deps.test @@ -0,0 +1,46 @@ +--TEST-- +Removing a package deletes unused dependencies and does not update other dependencies +--COMPOSER-- +{ + "repositories": [ + { + "type": "package", + "package": [ + { "name": "root/a", "version": "1.0.0", "require": { "dep/c": "*", "dep/d": "*"} }, + { "name": "remove/b", "version": "1.0.0", "require": { "dep/c": "*", "dep/e": "*"} }, + { "name": "dep/c", "version": "1.0.0" }, + { "name": "dep/c", "version": "1.2.0" }, + { "name": "dep/d", "version": "1.0.0" }, + { "name": "dep/d", "version": "1.2.0" }, + { "name": "dep/e", "version": "1.0.0" }, + { "name": "unrelated/f", "version": "1.0.0" } + ] + } + ], + "require": { + "root/a": "*" + } +} +--LOCK-- +{ + "packages": [ + { "name": "root/a", "version": "1.0.0", "require": { "dep/c": "*", "dep/d": "*"} }, + { "name": "remove/b", "version": "1.0.0", "require": { "dep/c": "*", "dep/e": "*"} }, + { "name": "dep/c", "version": "1.0.0" }, + { "name": "dep/d", "version": "1.0.0" }, + { "name": "dep/e", "version": "1.0.0" }, + { "name": "unrelated/f", "version": "1.0.0" } + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": [], + "prefer-stable": false, + "prefer-lowest": false +} +--RUN-- +update remove/b +--EXPECT-- +Installing dep/d (1.0.0) +Installing dep/c (1.0.0) +Installing root/a (1.0.0) diff --git a/tests/Composer/Test/Fixtures/installer/remove-does-nothing-if-removal-requires-update-of-dep.test b/tests/Composer/Test/Fixtures/installer/remove-does-nothing-if-removal-requires-update-of-dep.test new file mode 100644 index 000000000..5df46f118 --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/remove-does-nothing-if-removal-requires-update-of-dep.test @@ -0,0 +1,39 @@ +--TEST-- +Removing a package has no effect if another package would require an update in order to find a correct set of dependencies without the removed package +--COMPOSER-- +{ + "repositories": [ + { + "type": "package", + "package": [ + { "name": "root/a", "version": "1.0.0", "require": { "remove/b": "*", "dep/c": "*"} }, + { "name": "remove/b", "version": "1.0.0", "require": { "dep/c": "*"} }, + { "name": "dep/c", "version": "1.0.0" }, + { "name": "dep/c", "version": "1.2.0", "replace": { "remove/b": "1.0.0"} } + ] + } + ], + "require": { + "root/a": "*" + } +} +--LOCK-- +{ + "packages": [ + { "name": "root/a", "version": "1.0.0", "require": { "remove/b": "*", "dep/c": "*"} }, + { "name": "remove/b", "version": "1.0.0", "require": { "dep/c": "*"} }, + { "name": "dep/c", "version": "1.0.0" } + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": [], + "prefer-stable": false, + "prefer-lowest": false +} +--RUN-- +update remove/b +--EXPECT-- +Installing dep/c (1.0.0) +Installing remove/b (1.0.0) +Installing root/a (1.0.0) From 74fb313c3916a6e84738edc3f159c915c827c46f Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 23 Sep 2020 16:52:30 +0200 Subject: [PATCH 2/4] Separate locked packages from fixed packages in request Locked packages are basically like removable fixed packages, so we still only load one version, but we do not require their installation unless something the user needs requires their use. So they automatically get removed if they are no longer needed on any update. --- src/Composer/DependencyResolver/Pool.php | 10 +-- .../DependencyResolver/PoolBuilder.php | 62 +++++++++---------- src/Composer/DependencyResolver/Problem.php | 14 ++--- src/Composer/DependencyResolver/Request.php | 45 ++++++++++---- src/Composer/DependencyResolver/Rule.php | 57 ++++++++++------- .../DependencyResolver/RuleSetGenerator.php | 5 +- src/Composer/DependencyResolver/Solver.php | 2 +- src/Composer/Installer.php | 6 +- .../installer/alias-solver-problems2.test | 3 - ...update-allow-list-require-new-replace.test | 17 +---- ...pendencies-require-new-replace-mutual.test | 1 + 11 files changed, 117 insertions(+), 105 deletions(-) diff --git a/src/Composer/DependencyResolver/Pool.php b/src/Composer/DependencyResolver/Pool.php index d407f0a3e..511d2b427 100644 --- a/src/Composer/DependencyResolver/Pool.php +++ b/src/Composer/DependencyResolver/Pool.php @@ -31,13 +31,13 @@ class Pool implements \Countable protected $packageByName = array(); protected $versionParser; protected $providerCache = array(); - protected $unacceptableFixedPackages; + protected $unacceptableFixedOrLockedPackages; - public function __construct(array $packages = array(), array $unacceptableFixedPackages = array()) + public function __construct(array $packages = array(), array $unacceptableFixedOrLockedPackages = array()) { $this->versionParser = new VersionParser; $this->setPackages($packages); - $this->unacceptableFixedPackages = $unacceptableFixedPackages; + $this->unacceptableFixedOrLockedPackages = $unacceptableFixedOrLockedPackages; } private function setPackages(array $packages) @@ -181,9 +181,9 @@ class Pool implements \Countable return false; } - public function isUnacceptableFixedPackage(PackageInterface $package) + public function isUnacceptableFixedOrLockedPackage(PackageInterface $package) { - return \in_array($package, $this->unacceptableFixedPackages, true); + return \in_array($package, $this->unacceptableFixedOrLockedPackages, true); } public function __toString() diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index a245a50c3..2d2cc291a 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -81,7 +81,7 @@ class PoolBuilder /** * @psalm-var list */ - private $unacceptableFixedPackages = array(); + private $unacceptableFixedOrLockedPackages = array(); private $updateAllowList = array(); private $skippedLoad = array(); @@ -90,7 +90,7 @@ class PoolBuilder * have already their maximum required range loaded and can not be * extended by markPackageNameForLoading * - * Packages get cleared from this list if they get unfixed as in that case + * Packages get cleared from this list if they get unlocked as in that case * we need to actually load them */ private $maxExtendedReqs = array(); @@ -123,16 +123,14 @@ class PoolBuilder public function buildPool(array $repositories, Request $request) { - $singleVersionPackages = $request->getFixedPackages(); - if ($request->getUpdateAllowList()) { $this->updateAllowList = $request->getUpdateAllowList(); $this->warnAboutNonMatchingUpdateAllowList($request); foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) { if (!$this->isUpdateAllowed($lockedPackage)) { - //$request->fixPackage($lockedPackage); - $singleVersionPackages[] = $lockedPackage; + $request->lockPackage($lockedPackage); + $fixedOrLockedPackages[] = $lockedPackage; $lockedName = $lockedPackage->getName(); // remember which packages we skipped loading remote content for in this partial update $this->skippedLoad[$lockedName] = $lockedName; @@ -143,7 +141,7 @@ class PoolBuilder } } - foreach ($singleVersionPackages as $package) { + foreach ($request->getFixedOrLockedPackages() as $package) { // using MatchAllConstraint here because fixed packages do not need to retrigger // loading any packages $this->loadedPackages[$package->getName()] = new MatchAllConstraint(); @@ -163,12 +161,12 @@ class PoolBuilder ) { $this->loadPackage($request, $package, false); } else { - $this->unacceptableFixedPackages[] = $package; + $this->unacceptableFixedOrLockedPackages[] = $package; } } foreach ($request->getRequires() as $packageName => $constraint) { - // fixed packages have already been added, so if a root require needs one of them, no need to do anything + // fixed and locked packages have already been added, so if a root require needs one of them, no need to do anything if (isset($this->loadedPackages[$packageName])) { continue; } @@ -222,21 +220,21 @@ class PoolBuilder $this->rootAliases, $this->rootReferences, $this->packages, - $this->unacceptableFixedPackages + $this->unacceptableFixedOrLockedPackages ); $this->eventDispatcher->dispatch($prePoolCreateEvent->getName(), $prePoolCreateEvent); $this->packages = $prePoolCreateEvent->getPackages(); - $this->unacceptableFixedPackages = $prePoolCreateEvent->getUnacceptableFixedPackages(); + $this->unacceptableFixedOrLockedPackages = $prePoolCreateEvent->getUnacceptableFixedPackages(); } - $pool = new Pool($this->packages, $this->unacceptableFixedPackages); + $pool = new Pool($this->packages, $this->unacceptableFixedOrLockedPackages); $this->aliasMap = array(); $this->packagesToLoad = array(); $this->loadedPackages = array(); $this->loadedPerRepo = array(); $this->packages = array(); - $this->unacceptableFixedPackages = array(); + $this->unacceptableFixedOrLockedPackages = array(); $this->maxExtendedReqs = array(); $this->skippedLoad = array(); $this->indexCounter = 0; @@ -253,7 +251,7 @@ class PoolBuilder return; } - // Root require (which was not unfixed) already loaded the maximum range so no + // Root require (which was not unlocked) already loaded the maximum range so no // need to check anything here if (isset($this->maxExtendedReqs[$name])) { return; @@ -261,7 +259,7 @@ class PoolBuilder // Root requires can not be overruled by dependencies so there is no point in // extending the loaded constraint for those. - // This is triggered when loading a root require which was fixed but got unfixed, then + // This is triggered when loading a root require which was locked but got unlocked, then // we make sure that we load at most the intervals covered by the root constraint. $rootRequires = $request->getRequires(); if (isset($rootRequires[$name]) && !Intervals::isSubsetOf($constraint, $rootRequires[$name])) { @@ -314,7 +312,7 @@ class PoolBuilder break; } - // these repos have their packages fixed if they need to be loaded so we + // these repos have their packages fixed or locked if they need to be loaded so we // never need to load anything else from them if ($repository instanceof PlatformRepository || $repository === $request->getLockedRepository()) { continue; @@ -347,14 +345,14 @@ class PoolBuilder // right version. It'd be more work to figure out which versions and which aliases of those versions this may // apply to if (isset($this->rootReferences[$name])) { - // do not modify the references on already locked packages - if ($request->getLockedRepository() !== $package->getRepository() && !$request->isFixedPackage($package)) { + // do not modify the references on already locked or fixed packages + if (!$request->isLockedPackage($package) && !$request->isFixedPackage($package)) { $package->setSourceDistReferences($this->rootReferences[$name]); } } - // if propogateUpdate is false we are loading a fixed package, root aliases do not apply as they are manually - // loaded as separate packages in this case + // if propogateUpdate is false we are loading a fixed or locked package, root aliases do not apply as they are + // manually loaded as separate packages in this case if ($propagateUpdate && isset($this->rootAliases[$name][$package->getVersion()])) { $alias = $this->rootAliases[$name][$package->getVersion()]; if ($package instanceof AliasPackage) { @@ -375,11 +373,11 @@ class PoolBuilder $linkConstraint = $link->getConstraint(); if ($propagateUpdate) { - // if this is a partial update with transitive dependencies we need to unfix the package we now know is a + // if this is a partial update with transitive dependencies we need to unlock the package we now know is a // dependency of another package which we are trying to update, and then attempt to load it again if ($request->getUpdateAllowTransitiveDependencies() && isset($this->skippedLoad[$require])) { if ($request->getUpdateAllowTransitiveRootDependencies() || !$this->isRootRequire($request, $this->skippedLoad[$require])) { - $this->unfixPackage($request, $require); + $this->unlockPackage($request, $require); $this->markPackageNameForLoading($request, $require, $linkConstraint); } elseif (!$request->getUpdateAllowTransitiveRootDependencies() && $this->isRootRequire($request, $require) && !isset($this->updateAllowWarned[$require])) { $this->updateAllowWarned[$require] = true; @@ -389,7 +387,7 @@ class PoolBuilder $this->markPackageNameForLoading($request, $require, $linkConstraint); } } else { - // We also need to load the requirements of a fixed package + // We also need to load the requirements of a locked package // unless it was skipped if (!isset($this->skippedLoad[$require])) { $this->markPackageNameForLoading($request, $require, $linkConstraint); @@ -397,14 +395,14 @@ class PoolBuilder } } - // if we're doing a partial update with deps we also need to unfix packages which are being replaced in case they - // are currently locked and thus prevent this updateable package from being installable/updateable + // if we're doing a partial update with deps we also need to unlock packages which are being replaced in case + // they are currently locked and thus prevent this updateable package from being installable/updateable if ($propagateUpdate && $request->getUpdateAllowTransitiveDependencies()) { foreach ($package->getReplaces() as $link) { $replace = $link->getTarget(); if (isset($this->loadedPackages[$replace], $this->skippedLoad[$replace])) { if ($request->getUpdateAllowTransitiveRootDependencies() || !$this->isRootRequire($request, $this->skippedLoad[$replace])) { - $this->unfixPackage($request, $replace); + $this->unlockPackage($request, $replace); $this->markPackageNameForLoading($request, $replace, $link->getConstraint()); } elseif (!$request->getUpdateAllowTransitiveRootDependencies() && $this->isRootRequire($request, $replace) && !isset($this->updateAllowWarned[$replace])) { $this->updateAllowWarned[$replace] = true; @@ -467,16 +465,16 @@ class PoolBuilder } /** - * Reverts the decision to use a fixed package from lock file if a partial update with transitive dependencies + * Reverts the decision to use a locked package if a partial update with transitive dependencies * found that this package actually needs to be updated */ - private function unfixPackage(Request $request, $name) + private function unlockPackage(Request $request, $name) { // remove locked package by this name which was already initialized - foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) { + foreach ($request->getLockedPackages() as $lockedPackage) { if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) { if (false !== $index = array_search($lockedPackage, $this->packages, true)) { - //$request->unfixPackage($lockedPackage); + $request->unlockPackage($lockedPackage); $this->removeLoadedPackage($request, $lockedPackage, $index); } } @@ -488,7 +486,7 @@ class PoolBuilder // as long as it was not unfixed yet && isset($this->skippedLoad[$this->skippedLoad[$name]]) ) { - $this->unfixPackage($request, $this->skippedLoad[$name]); + $this->unlockPackage($request, $this->skippedLoad[$name]); } unset($this->skippedLoad[$name], $this->loadedPackages[$name], $this->maxExtendedReqs[$name]); @@ -499,7 +497,7 @@ class PoolBuilder unset($this->packages[$index]); if (isset($this->aliasMap[spl_object_hash($package)])) { foreach ($this->aliasMap[spl_object_hash($package)] as $aliasIndex => $aliasPackage) { - //$request->unfixPackage($aliasPackage); + $request->unlockPackage($aliasPackage); unset($this->packages[$aliasIndex]); } unset($this->aliasMap[spl_object_hash($package)]); diff --git a/src/Composer/DependencyResolver/Problem.php b/src/Composer/DependencyResolver/Problem.php index 1ddd16268..327874b26 100644 --- a/src/Composer/DependencyResolver/Problem.php +++ b/src/Composer/DependencyResolver/Problem.php @@ -229,11 +229,11 @@ class Problem return array("- Root composer.json requires linked library ".$packageName.self::constraintToText($constraint).' but ', 'it has the wrong version installed or is missing from your system, make sure to load the extension providing it.'); } - $fixedPackage = null; - foreach ($request->getFixedPackages() as $package) { + $lockedPackage = null; + foreach ($request->getLockedPackages() as $package) { if ($package->getName() === $packageName) { - $fixedPackage = $package; - if ($pool->isUnacceptableFixedPackage($package)) { + $lockedPackage = $package; + if ($pool->isUnacceptableFixedOrLockedPackage($package)) { return array("- ", $package->getPrettyName().' is fixed to '.$package->getPrettyVersion().' (lock file version) by a partial update but that version is rejected by your minimum-stability. Make sure you list it as an argument for the update command.'); } break; @@ -253,13 +253,13 @@ class Problem } } - if ($fixedPackage) { - $fixedConstraint = new Constraint('==', $fixedPackage->getVersion()); + if ($lockedPackage) { + $fixedConstraint = new Constraint('==', $lockedPackage->getVersion()); $filtered = array_filter($packages, function ($p) use ($fixedConstraint) { return $fixedConstraint->matches(new Constraint('==', $p->getVersion())); }); if (0 === count($filtered)) { - return array("- Root composer.json requires $packageName".self::constraintToText($constraint) . ', ', 'found '.self::getPackageList($packages, $isVerbose).' but the package is fixed to '.$fixedPackage->getPrettyVersion().' (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.'); + return array("- Root composer.json requires $packageName".self::constraintToText($constraint) . ', ', 'found '.self::getPackageList($packages, $isVerbose).' but the package is fixed to '.$lockedPackage->getPrettyVersion().' (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.'); } } diff --git a/src/Composer/DependencyResolver/Request.php b/src/Composer/DependencyResolver/Request.php index c0f30ed0e..af3141cec 100644 --- a/src/Composer/DependencyResolver/Request.php +++ b/src/Composer/DependencyResolver/Request.php @@ -44,7 +44,7 @@ class Request protected $lockedRepository; protected $requires = array(); protected $fixedPackages = array(); - protected $unlockables = array(); + protected $lockedPackages = array(); protected $updateAllowList = array(); protected $updateAllowTransitiveDependencies = false; @@ -71,18 +71,22 @@ class Request * * @param bool $lockable if set to false, the package will not be written to the lock file */ - public function fixPackage(PackageInterface $package, $lockable = true) + public function fixPackage(PackageInterface $package) { $this->fixedPackages[spl_object_hash($package)] = $package; - - if (!$lockable) { - $this->unlockables[spl_object_hash($package)] = $package; - } } - public function unfixPackage(PackageInterface $package) + /** + * Mark an existing package as installed but removable + */ + public function lockPackage(PackageInterface $package) { - unset($this->fixedPackages[spl_object_hash($package)], $this->unlockables[spl_object_hash($package)]); + $this->lockedPackages[spl_object_hash($package)] = $package; + } + + public function unlockPackage(PackageInterface $package) + { + unset($this->lockedPackages[spl_object_hash($package)]); } public function setUpdateAllowList($updateAllowList, $updateAllowTransitiveDependencies) @@ -121,6 +125,21 @@ class Request return isset($this->fixedPackages[spl_object_hash($package)]); } + public function getLockedPackages() + { + return $this->lockedPackages; + } + + public function isLockedPackage(PackageInterface $package) + { + return isset($this->lockedPackages[spl_object_hash($package)]); + } + + public function getFixedOrLockedPackages() + { + return array_merge($this->fixedPackages, $this->lockedPackages); + } + // TODO look into removing the packageIds option, the only place true is used is for the installed map in the solver problems // some locked packages may not be in the pool so they have a package->id of -1 public function getPresentMap($packageIds = false) @@ -140,15 +159,15 @@ class Request return $presentMap; } - public function getUnlockableMap() + public function getFixedPackagesMap() { - $unlockableMap = array(); + $fixedPackagesMap = array(); - foreach ($this->unlockables as $package) { - $unlockableMap[$package->id] = $package; + foreach ($this->fixedPackages as $package) { + $fixedPackagesMap[$package->id] = $package; } - return $unlockableMap; + return $fixedPackagesMap; } public function getLockedRepository() diff --git a/src/Composer/DependencyResolver/Rule.php b/src/Composer/DependencyResolver/Rule.php index 5469470c3..2f57acd5f 100644 --- a/src/Composer/DependencyResolver/Rule.php +++ b/src/Composer/DependencyResolver/Rule.php @@ -125,23 +125,25 @@ abstract class Rule public function isCausedByLock(RepositorySet $repositorySet, Request $request, Pool $pool) { - if ($this->getReason() === self::RULE_FIXED && $this->reasonData['lockable']) { - return true; - } - if ($this->getReason() === self::RULE_PACKAGE_REQUIRES) { if (PlatformRepository::isPlatformPackage($this->reasonData->getTarget())) { return false; } - foreach ($request->getFixedPackages() as $package) { - if ($package->getName() === $this->reasonData->getTarget()) { - if ($pool->isUnacceptableFixedPackage($package)) { - return true; + if ($request->getLockedRepository()) { + foreach ($request->getLockedRepository()->getPackages() as $package) { + if ($package->getName() === $this->reasonData->getTarget()) { + if ($pool->isUnacceptableFixedOrLockedPackage($package)) { + return true; + } + if (!$this->reasonData->getConstraint()->matches(new Constraint('=', $package->getVersion()))) { + return true; + } + // required package was locked but has been unlocked and still matches + if (!$request->isLockedPackage($package)) { + return true; + } + break; } - if (!$this->reasonData->getConstraint()->matches(new Constraint('=', $package->getVersion()))) { - return true; - } - break; } } } @@ -150,15 +152,17 @@ abstract class Rule if (PlatformRepository::isPlatformPackage($this->reasonData['packageName'])) { return false; } - foreach ($request->getFixedPackages() as $package) { - if ($package->getName() === $this->reasonData['packageName']) { - if ($pool->isUnacceptableFixedPackage($package)) { - return true; + if ($request->getLockedRepository()) { + foreach ($request->getLockedRepository()->getPackages() as $package) { + if ($package->getName() === $this->reasonData['packageName']) { + if ($pool->isUnacceptableFixedOrLockedPackage($package)) { + return true; + } + if (!$this->reasonData['constraint']->matches(new Constraint('=', $package->getVersion()))) { + return true; + } + break; } - if (!$this->reasonData['constraint']->matches(new Constraint('=', $package->getVersion()))) { - return true; - } - break; } } } @@ -180,13 +184,20 @@ abstract class Rule return 'No package found to satisfy root composer.json require '.$packageName.($constraint ? ' '.$constraint->getPrettyString() : ''); } + $packagesNonAlias = array_values(array_filter($packages, function ($p) { + return !($p instanceof AliasPackage); + })); + if (count($packagesNonAlias) === 1) { + $package = $packagesNonAlias[0]; + if (!($package instanceof AliasPackage) && $request->isLockedPackage($package)) { + return $package->getPrettyName().' is locked to version '.$package->getPrettyVersion()." and an update of this package was not requested."; + } + } + return 'Root composer.json requires '.$packageName.($constraint ? ' '.$constraint->getPrettyString() : '').' -> satisfiable by '.$this->formatPackagesUnique($pool, $packages, $isVerbose).'.'; case self::RULE_FIXED: $package = $this->deduplicateDefaultBranchAlias($this->reasonData['package']); - if ($this->reasonData['lockable']) { - return $package->getPrettyName().' is locked to version '.$package->getPrettyVersion().' and an update of this package was not requested.'; - } return $package->getPrettyName().' is present at version '.$package->getPrettyVersion() . ' and cannot be modified by Composer'; diff --git a/src/Composer/DependencyResolver/RuleSetGenerator.php b/src/Composer/DependencyResolver/RuleSetGenerator.php index 3daee5845..ba3264857 100644 --- a/src/Composer/DependencyResolver/RuleSetGenerator.php +++ b/src/Composer/DependencyResolver/RuleSetGenerator.php @@ -223,12 +223,10 @@ class RuleSetGenerator protected function addRulesForRequest(Request $request, $ignorePlatformReqs) { - $unlockableMap = $request->getUnlockableMap(); - foreach ($request->getFixedPackages() as $package) { if ($package->id == -1) { // fixed package was not added to the pool as it did not pass the stability requirements, this is fine - if ($this->pool->isUnacceptableFixedPackage($package)) { + if ($this->pool->isUnacceptableFixedOrLockedPackage($package)) { continue; } @@ -240,7 +238,6 @@ class RuleSetGenerator $rule = $this->createInstallOneOfRule(array($package), Rule::RULE_FIXED, array( 'package' => $package, - 'lockable' => !isset($unlockableMap[$package->id]), )); $this->addRule(RuleSet::TYPE_REQUEST, $rule); } diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index 431ce2302..92444eaac 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -215,7 +215,7 @@ class Solver throw new SolverProblemsException($this->problems, $this->learnedPool); } - return new LockTransaction($this->pool, $request->getPresentMap(), $request->getUnlockableMap(), $this->decisions); + return new LockTransaction($this->pool, $request->getPresentMap(), $request->getFixedPackagesMap(), $this->decisions); } /** diff --git a/src/Composer/Installer.php b/src/Composer/Installer.php index 8ed6f8af8..b0aa17046 100644 --- a/src/Composer/Installer.php +++ b/src/Composer/Installer.php @@ -821,9 +821,9 @@ class Installer { $request = new Request($lockedRepository); - $request->fixPackage($rootPackage, false); + $request->fixPackage($rootPackage); if ($rootPackage instanceof RootAliasPackage) { - $request->fixPackage($rootPackage->getAliasOf(), false); + $request->fixPackage($rootPackage->getAliasOf()); } $fixedPackages = $platformRepo->getPackages(); @@ -841,7 +841,7 @@ class Installer || !isset($provided[$package->getName()]) || !$provided[$package->getName()]->getConstraint()->matches(new Constraint('=', $package->getVersion())) ) { - $request->fixPackage($package, false); + $request->fixPackage($package); } } diff --git a/tests/Composer/Test/Fixtures/installer/alias-solver-problems2.test b/tests/Composer/Test/Fixtures/installer/alias-solver-problems2.test index d206764f2..df9634689 100644 --- a/tests/Composer/Test/Fixtures/installer/alias-solver-problems2.test +++ b/tests/Composer/Test/Fixtures/installer/alias-solver-problems2.test @@ -43,9 +43,6 @@ Updating dependencies Your requirements could not be resolved to an installable set of packages. Problem 1 - - locked/pkg is locked to version dev-master and an update of this package was not requested. - - locked/pkg dev-master requires locked/dependency 1.0.0 -> found locked/dependency[1.0.0] in lock file but not in remote repositories, make sure you avoid updating this package to keep the one from lock file. - Problem 2 - locked/pkg dev-master requires locked/dependency 1.0.0 -> found locked/dependency[1.0.0] in lock file but not in remote repositories, make sure you avoid updating this package to keep the one from lock file. - locked/pkg is locked to version dev-master and an update of this package was not requested. 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 60f899ba1..163471fa6 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 @@ -1,5 +1,5 @@ --TEST-- -If a new requirement cannot be installed on a partial update due to replace, there should be a suggestion to use --with-all-dependencies +A partial update for a new package yet to be installed should remove another dependency it replaces if that allows installation. --COMPOSER-- { "repositories": [ @@ -39,17 +39,6 @@ If a new requirement cannot be installed on a partial update due to replace, the } --RUN-- update new/pkg ---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 - - 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. - - Root composer.json requires new/pkg 1.* -> satisfiable by new/pkg[1.0.0]. - -Use the option --with-all-dependencies to allow upgrades, downgrades and removals for packages currently locked to specific versions. --EXPECT-- +Removing current/dep (1.0.0) +Installing new/pkg (1.0.0) diff --git a/tests/Composer/Test/Fixtures/installer/update-allow-list-with-dependencies-require-new-replace-mutual.test b/tests/Composer/Test/Fixtures/installer/update-allow-list-with-dependencies-require-new-replace-mutual.test index 0cb5ad97f..a850748c8 100644 --- a/tests/Composer/Test/Fixtures/installer/update-allow-list-with-dependencies-require-new-replace-mutual.test +++ b/tests/Composer/Test/Fixtures/installer/update-allow-list-with-dependencies-require-new-replace-mutual.test @@ -45,6 +45,7 @@ Require a new package in the composer.json and updating with its name as an argu --RUN-- update new/pkg --with-dependencies --EXPECT-- +Removing current/dep-provide (1.0.0) Removing current/dep (1.0.0) Installing new/pkg (1.0.0) Installing new/pkg-provide (1.0.0) From fdde9e593343111d8f78e66901851ac427b3bae1 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 14 Oct 2020 10:57:23 +0200 Subject: [PATCH 3/4] On composer install we fix locked packages, but consider them locked for error reporting --- src/Composer/DependencyResolver/Request.php | 12 +++++++++++- src/Composer/DependencyResolver/Rule.php | 4 ++++ src/Composer/Installer.php | 3 +-- .../Fixtures/installer/partial-update-from-lock.test | 5 ++--- .../root-alias-change-with-circular-dep.test | 2 -- 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/Composer/DependencyResolver/Request.php b/src/Composer/DependencyResolver/Request.php index af3141cec..618f1b784 100644 --- a/src/Composer/DependencyResolver/Request.php +++ b/src/Composer/DependencyResolver/Request.php @@ -45,6 +45,7 @@ class Request protected $requires = array(); protected $fixedPackages = array(); protected $lockedPackages = array(); + protected $fixedLockedPackages = array(); protected $updateAllowList = array(); protected $updateAllowTransitiveDependencies = false; @@ -84,6 +85,15 @@ class Request $this->lockedPackages[spl_object_hash($package)] = $package; } + /** + * Mark a package fixed, but also keep track it is from the lock file (needed for composer install error reporting) + */ + public function fixLockedPackage(PackageInterface $package) + { + $this->fixedPackages[spl_object_hash($package)] = $package; + $this->fixedLockedPackages[spl_object_hash($package)] = $package; + } + public function unlockPackage(PackageInterface $package) { unset($this->lockedPackages[spl_object_hash($package)]); @@ -132,7 +142,7 @@ class Request public function isLockedPackage(PackageInterface $package) { - return isset($this->lockedPackages[spl_object_hash($package)]); + return isset($this->lockedPackages[spl_object_hash($package)]) || isset($this->fixedLockedPackages[spl_object_hash($package)]); } public function getFixedOrLockedPackages() diff --git a/src/Composer/DependencyResolver/Rule.php b/src/Composer/DependencyResolver/Rule.php index 2f57acd5f..371c2ab05 100644 --- a/src/Composer/DependencyResolver/Rule.php +++ b/src/Composer/DependencyResolver/Rule.php @@ -199,6 +199,10 @@ abstract class Rule case self::RULE_FIXED: $package = $this->deduplicateDefaultBranchAlias($this->reasonData['package']); + if ($request->isLockedPackage($package)) { + return $package->getPrettyName().' is locked to version '.$package->getPrettyVersion().' and an update of this package was not requested.'; + } + return $package->getPrettyName().' is present at version '.$package->getPrettyVersion() . ' and cannot be modified by Composer'; case self::RULE_PACKAGE_CONFLICT: diff --git a/src/Composer/Installer.php b/src/Composer/Installer.php index b0aa17046..7e83bf136 100644 --- a/src/Composer/Installer.php +++ b/src/Composer/Installer.php @@ -633,7 +633,7 @@ class Installer } foreach ($lockedRepository->getPackages() as $package) { - $request->fixPackage($package); + $request->fixLockedPackage($package); } foreach ($this->locker->getPlatformRequirements($this->devMode) as $link) { @@ -651,7 +651,6 @@ class Installer // installing the locked packages on this platform resulted in lock modifying operations, there wasn't a conflict, but the lock file as-is seems to not work on this system if (0 !== count($lockTransaction->getOperations())) { $this->io->writeError('Your lock file cannot be installed on this system without changes. Please run composer update.', true, IOInterface::QUIET); - // TODO actually display operations to explain what happened? return 1; } } catch (SolverProblemsException $e) { diff --git a/tests/Composer/Test/Fixtures/installer/partial-update-from-lock.test b/tests/Composer/Test/Fixtures/installer/partial-update-from-lock.test index bce29a21f..d176b02d3 100644 --- a/tests/Composer/Test/Fixtures/installer/partial-update-from-lock.test +++ b/tests/Composer/Test/Fixtures/installer/partial-update-from-lock.test @@ -21,7 +21,8 @@ Partial update from lock file should update everything to the state of the lock, "require": { "a/old": "*", "b/unstable": "*", - "c/uptodate": "*" + "c/uptodate": "*", + "e/newreq": "*" } } --LOCK-- @@ -60,7 +61,6 @@ update b/unstable { "name": "a/old", "version": "1.0.0", "type": "library" }, { "name": "b/unstable", "version": "1.0.0", "type": "library", "require": {"f/dependency": "1.*"} }, { "name": "c/uptodate", "version": "1.0.0", "type": "library" }, - { "name": "d/removed", "version": "1.0.0", "type": "library" }, { "name": "e/newreq", "version": "1.0.0", "type": "library" }, { "name": "f/dependency", "version": "1.0.0", "type": "library" } ], @@ -77,5 +77,4 @@ update b/unstable Upgrading a/old (0.9.0 => 1.0.0) Downgrading b/unstable (1.1.0-alpha => 1.0.0) Downgrading c/uptodate (2.0.0 => 1.0.0) -Installing d/removed (1.0.0) Installing e/newreq (1.0.0) diff --git a/tests/Composer/Test/Fixtures/installer/root-alias-change-with-circular-dep.test b/tests/Composer/Test/Fixtures/installer/root-alias-change-with-circular-dep.test index 5cd80b27b..28e4ac4bf 100644 --- a/tests/Composer/Test/Fixtures/installer/root-alias-change-with-circular-dep.test +++ b/tests/Composer/Test/Fixtures/installer/root-alias-change-with-circular-dep.test @@ -62,6 +62,4 @@ Your lock file does not contain a compatible set of packages. Please run compose - b/requirer is locked to version 1.0.0 and an update of this package was not requested. - b/requirer 1.0.0 requires root/pkg ^1 -> found root/pkg[2.x-dev] but it does not match the constraint. -Use the option --with-all-dependencies to allow upgrades, downgrades and removals for packages currently locked to specific versions. - --EXPECT-- From 8a2bae82ab2689286c704427db043ec4edda7aa8 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 14 Oct 2020 11:42:16 +0200 Subject: [PATCH 4/4] Improve docblocks on fixed/locked/fixedLocked packages in request Also fixes two small code review issues --- .../DependencyResolver/PoolBuilder.php | 1 - src/Composer/DependencyResolver/Request.php | 20 +++++++++++++++---- src/Composer/DependencyResolver/Rule.php | 2 +- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 2d2cc291a..83c0ca08f 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -130,7 +130,6 @@ class PoolBuilder foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) { if (!$this->isUpdateAllowed($lockedPackage)) { $request->lockPackage($lockedPackage); - $fixedOrLockedPackages[] = $lockedPackage; $lockedName = $lockedPackage->getName(); // remember which packages we skipped loading remote content for in this partial update $this->skippedLoad[$lockedName] = $lockedName; diff --git a/src/Composer/DependencyResolver/Request.php b/src/Composer/DependencyResolver/Request.php index 618f1b784..01753ee55 100644 --- a/src/Composer/DependencyResolver/Request.php +++ b/src/Composer/DependencyResolver/Request.php @@ -68,9 +68,10 @@ class Request } /** - * Mark an existing package as being installed and having to remain installed + * Mark a package as currently present and having to remain installed * - * @param bool $lockable if set to false, the package will not be written to the lock file + * This is used for platform packages which cannot be modified by Composer. A rule enforcing their installation is + * generated for dependency resolution. Partial updates with dependencies cannot in any way modify these packages. */ public function fixPackage(PackageInterface $package) { @@ -78,7 +79,14 @@ class Request } /** - * Mark an existing package as installed but removable + * Mark a package as locked to a specific version but removable + * + * This is used for lock file packages which need to be treated similar to fixed packages by the pool builder in + * that by default they should really only have the currently present version loaded and no remote alternatives. + * + * However unlike fixed packages there will not be a special rule enforcing their installation for the solver, so + * if nothing requires these packages they will be removed. Additionally in a partial update these packages can be + * unlocked, meaning other versions can be installed if explicitly requested as part of the update. */ public function lockPackage(PackageInterface $package) { @@ -86,7 +94,11 @@ class Request } /** - * Mark a package fixed, but also keep track it is from the lock file (needed for composer install error reporting) + * Marks a locked package fixed. So it's treated irremovable like a platform package. + * + * This is necessary for the composer install step which verifies the lock file integrity and should not allow + * removal of any packages. At the same time lock packages there cannot simply be marked fixed, as error reporting + * would then report them as platform packages, so this still marks them as locked packages at the same time. */ public function fixLockedPackage(PackageInterface $package) { diff --git a/src/Composer/DependencyResolver/Rule.php b/src/Composer/DependencyResolver/Rule.php index 371c2ab05..37bb462c8 100644 --- a/src/Composer/DependencyResolver/Rule.php +++ b/src/Composer/DependencyResolver/Rule.php @@ -189,7 +189,7 @@ abstract class Rule })); if (count($packagesNonAlias) === 1) { $package = $packagesNonAlias[0]; - if (!($package instanceof AliasPackage) && $request->isLockedPackage($package)) { + if ($request->isLockedPackage($package)) { return $package->getPrettyName().' is locked to version '.$package->getPrettyVersion()." and an update of this package was not requested."; } }