From 0ff07015a1c0951be489ab46a66c093c714fd14c Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 30 Oct 2019 00:24:25 +0100 Subject: [PATCH] Only load package info from lock file for fixed packages As a result some lock file packages are no longer in the pool, so the former installed map, now present map cannot use package ids anymore Need to revisit some more code later to simplify this, todo notes left --- .../DependencyResolver/LockTransaction.php | 47 +++++++++++++------ .../DependencyResolver/PoolBuilder.php | 18 ++++++- src/Composer/DependencyResolver/Problem.php | 2 +- src/Composer/DependencyResolver/Request.php | 11 +++-- src/Composer/DependencyResolver/Solver.php | 2 +- .../installer/github-issues-4795.test | 4 +- .../Fixtures/installer/solver-problems.test | 2 +- 7 files changed, 61 insertions(+), 25 deletions(-) diff --git a/src/Composer/DependencyResolver/LockTransaction.php b/src/Composer/DependencyResolver/LockTransaction.php index a0aa4cd7e..75b2efb5b 100644 --- a/src/Composer/DependencyResolver/LockTransaction.php +++ b/src/Composer/DependencyResolver/LockTransaction.php @@ -76,9 +76,14 @@ class LockTransaction $package = $this->pool->literalToPackage($literal); // wanted & !present - if ($literal > 0 && !isset($this->presentMap[$package->id])) { + if ($literal > 0 && !isset($this->presentMap[spl_object_hash($package)])) { if (isset($lockMeansUpdateMap[abs($literal)]) && !$package instanceof AliasPackage) { - $operations[] = new Operation\UpdateOperation($lockMeansUpdateMap[abs($literal)], $package, $reason); + // TODO we end up here sometimes because we prefer the remote package now to get up to date metadata + // TODO define some level of identity here for what constitutes an update and what can be ignored? new kind of metadata only update? + $target = $lockMeansUpdateMap[abs($literal)]; + if ($package->getName() !== $target->getName() || $package->getVersion() !== $target->getVersion()) { + $operations[] = new Operation\UpdateOperation($target, $package, $reason); + } // avoid updates to one package from multiple origins $ignoreRemove[$lockMeansUpdateMap[abs($literal)]->id] = true; @@ -98,7 +103,7 @@ class LockTransaction $reason = $decision[Decisions::DECISION_REASON]; $package = $this->pool->literalToPackage($literal); - if ($literal <= 0 && isset($this->presentMap[$package->id]) && !isset($ignoreRemove[$package->id])) { + if ($literal <= 0 && isset($this->presentMap[spl_object_hash($package)]) && !isset($ignoreRemove[$package->id])) { if ($package instanceof AliasPackage) { $operations[] = new Operation\MarkAliasUninstalledOperation($package, $reason); } else { @@ -163,29 +168,41 @@ class LockTransaction { $lockMeansUpdateMap = array(); + $packages = array(); + foreach ($this->decisions as $i => $decision) { $literal = $decision[Decisions::DECISION_LITERAL]; $package = $this->pool->literalToPackage($literal); + if ($literal <= 0 && isset($this->presentMap[spl_object_hash($package)])) { + $packages[spl_object_hash($package)] = $package; + } + } + + // some locked packages are not in the pool and thus, were not decided at all + foreach ($this->presentMap as $package) { + if ($package->id === -1) { + $packages[spl_object_hash($package)] = $package; + } + } + + foreach ($packages as $package) { if ($package instanceof AliasPackage) { continue; } - // !wanted & present - if ($literal <= 0 && isset($this->presentMap[$package->id])) { - // TODO can't we just look at existing rules? - $updates = $this->policy->findUpdatePackages($this->pool, $package); + // TODO can't we just look at existing rules? + $updates = $this->policy->findUpdatePackages($this->pool, $package); - $literals = array($package->id); + $literals = array($package->id); - foreach ($updates as $update) { - $literals[] = $update->id; - } + foreach ($updates as $update) { + $literals[] = $update->id; + } - foreach ($literals as $updateLiteral) { - if ($updateLiteral !== $literal && !isset($lockMeansUpdateMap[$updateLiteral])) { - $lockMeansUpdateMap[$updateLiteral] = $package; - } + foreach ($literals as $updateLiteral) { + if (!isset($lockMeansUpdateMap[$updateLiteral])) { + $lockMeansUpdateMap[$updateLiteral] = $package; } } } diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 5376367e2..39ac5098f 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -59,6 +59,7 @@ class PoolBuilder // TODO can actually use very specific constraint $loadNames[$package->getName()] = null; } + foreach ($request->getJobs() as $job) { switch ($job['cmd']) { case 'install': @@ -67,11 +68,24 @@ class PoolBuilder // also see the solver-problems.test test case $constraint = array_key_exists($job['packageName'], $loadNames) ? null : $job['constraint']; $loadNames[$job['packageName']] = $constraint; - $this->nameConstraints[$job['packageName']] = $constraint ? new MultiConstraint(array($job['constraint']), false) : null; + $this->nameConstraints[$job['packageName']] = $constraint ? new MultiConstraint(array($constraint), false) : null; break; } } + // packages from the locked repository only get loaded if they are explicitly fixed + foreach ($repositories as $key => $repository) { + if ($repository === $request->getLockedRepository()) { + foreach ($repository->getPackages() as $lockedPackage) { + foreach ($request->getFixedPackages() as $package) { + if ($package === $lockedPackage) { + $loadNames += $this->loadPackage($request, $package, $key); + } + } + } + } + } + while (!empty($loadNames)) { $loadIds = array(); foreach ($repositories as $key => $repository) { @@ -86,7 +100,7 @@ class PoolBuilder $newLoadNames = array(); foreach ($repositories as $key => $repository) { - if ($repository instanceof PlatformRepository || $repository instanceof InstalledRepositoryInterface) { + if ($repository instanceof PlatformRepository || $repository instanceof InstalledRepositoryInterface || $repository === $request->getLockedRepository()) { continue; } diff --git a/src/Composer/DependencyResolver/Problem.php b/src/Composer/DependencyResolver/Problem.php index bc3f2de04..c2c3f42c6 100644 --- a/src/Composer/DependencyResolver/Problem.php +++ b/src/Composer/DependencyResolver/Problem.php @@ -68,7 +68,7 @@ class Problem /** * A human readable textual representation of the problem's reasons * - * @param array $installedMap A map of all installed packages + * @param array $installedMap A map of all present packages * @return string */ public function getPrettyString(array $installedMap = array(), array $learnedPool = array()) diff --git a/src/Composer/DependencyResolver/Request.php b/src/Composer/DependencyResolver/Request.php index 54adea9b5..0656cbbae 100644 --- a/src/Composer/DependencyResolver/Request.php +++ b/src/Composer/DependencyResolver/Request.php @@ -85,18 +85,20 @@ class Request return isset($this->fixedPackages[spl_object_hash($package)]); } - public function getPresentMap() + // 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) { $presentMap = array(); if ($this->lockedRepository) { foreach ($this->lockedRepository->getPackages() as $package) { - $presentMap[$package->id] = $package; + $presentMap[$packageIds ? $package->id : spl_object_hash($package)] = $package; } } foreach ($this->fixedPackages as $package) { - $presentMap[$package->id] = $package; + $presentMap[$packageIds ? $package->id : spl_object_hash($package)] = $package; } return $presentMap; @@ -113,7 +115,8 @@ class Request return $unlockableMap; } - public function getLockMap() + public function getLockedRepository() { + return $this->lockedRepository; } } diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index 2abcf5d8a..66f325446 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -218,7 +218,7 @@ class Solver $this->io->writeError(sprintf('Dependency resolution completed in %.3f seconds', microtime(true) - $before), true, IOInterface::VERBOSE); if ($this->problems) { - throw new SolverProblemsException($this->problems, $request->getPresentMap(), $this->learnedPool); + throw new SolverProblemsException($this->problems, $request->getPresentMap(true), $this->learnedPool); } return new LockTransaction($this->policy, $this->pool, $request->getPresentMap(), $request->getUnlockableMap(), $this->decisions); diff --git a/tests/Composer/Test/Fixtures/installer/github-issues-4795.test b/tests/Composer/Test/Fixtures/installer/github-issues-4795.test index 4afac72aa..8e5d17dfe 100644 --- a/tests/Composer/Test/Fixtures/installer/github-issues-4795.test +++ b/tests/Composer/Test/Fixtures/installer/github-issues-4795.test @@ -52,8 +52,10 @@ update b/b --with-dependencies Dependency "a/a" is also a root requirement, but is not explicitly whitelisted. Ignoring. Loading composer repositories with package information Updating dependencies -Nothing to install or update +Nothing to modify in lock file Writing lock file +Installing dependencies from lock file (including require-dev) +Nothing to install, update or remove Generating autoload files --EXPECT-- diff --git a/tests/Composer/Test/Fixtures/installer/solver-problems.test b/tests/Composer/Test/Fixtures/installer/solver-problems.test index 57d3ced92..94a0e3aa3 100644 --- a/tests/Composer/Test/Fixtures/installer/solver-problems.test +++ b/tests/Composer/Test/Fixtures/installer/solver-problems.test @@ -65,7 +65,7 @@ Your requirements could not be resolved to an installable set of packages. - requirer/pkg 1.0.0 requires dependency/pkg 1.0.0 -> no matching package found. Problem 4 - stable-requiree-excluded/pkg is locked to version 1.0.0 and an update of this package was not requested. - - Same name, can only install one of: stable-requiree-excluded/pkg[1.0.0, 1.0.1]. + - Same name, can only install one of: stable-requiree-excluded/pkg[1.0.1, 1.0.0]. - Installation request for stable-requiree-excluded/pkg 1.0.1 -> satisfiable by stable-requiree-excluded/pkg[1.0.1]. Potential causes: