From b5c0e68bc73083164e0877302eaf45cf79c26521 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Tue, 27 Oct 2020 16:41:45 +0100 Subject: [PATCH 1/8] PoolBuilder: test case ensuring versions matching locked constraints get loaded --- .../partial-update-unfixing-locked-deps.test | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test new file mode 100644 index 000000000..6c160855b --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test @@ -0,0 +1,47 @@ +--TEST-- +Unlocking a package also unlocks its dependencies when transitive deps are true. But version constraints from other +locked packages still need to be taking into account for loading all necessary versions of these transitive deps. + +--REQUEST-- +{ + "require": { + "root/req1": "*", + "root/req2": "*" + }, + "locked": [ + {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}}, + {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}}, + {"name": "dep/pkg1", "version": "1.0.0"}, + {"name": "dep/pkg2", "version": "1.0.0"} + ], + "allowList": [ + "dep/pkg2" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + [ + {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}}, + {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}}, + {"name": "dep/pkg1", "version": "1.0.0"}, + {"name": "dep/pkg1", "version": "2.0.0"}, + {"name": "dep/pkg2", "version": "1.0.0"}, + {"name": "dep/pkg2", "version": "1.2.0", "require": {"dep/pkg1": "2.*"}} + ] +] + +--EXPECT-- +[ + "root/req1-1.0.0.0 (locked)", + "root/req2-1.0.0.0 (locked)", + "dep/pkg2-1.0.0.0", + "dep/pkg2-1.2.0.0", + "dep/pkg1-1.0.0.0", + "dep/pkg1-2.0.0.0" +] From 7bc2112f2b5ebb4526fd7c69b509a0c4ac267793 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Tue, 27 Oct 2020 17:02:31 +0100 Subject: [PATCH 2/8] InstallerTest: Add a test for partial updates Needs to take constraints of locked packages into account --- ...ate-keeps-older-dep-if-still-required.test | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required.test diff --git a/tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required.test b/tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required.test new file mode 100644 index 000000000..b2a6c67cf --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required.test @@ -0,0 +1,65 @@ +--TEST-- +Ensure that a partial update of a dependency does not conflict if the only way to proceed is using an old locked version. + +--COMPOSER-- +{ + "repositories": [ + { + "type": "package", + "package": [ + {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}}, + {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}}, + {"name": "dep/pkg1", "version": "1.0.0"}, + {"name": "dep/pkg1", "version": "2.0.0"}, + {"name": "dep/pkg2", "version": "1.0.0"}, + {"name": "dep/pkg2", "version": "1.0.1"}, + {"name": "dep/pkg2", "version": "1.2.0", "require": {"dep/pkg1": "2.*"}} + ] + } + ], + "require": { + "root/req1": "*", + "root/req2": "*" + } +} +--LOCK-- +{ + "packages": [ + {"name": "dep/pkg1", "version": "1.0.0", "type": "library"}, + {"name": "dep/pkg2", "version": "1.0.1", "type": "library"}, + {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}, "type": "library"}, + {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}, "type": "library"} + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": {}, + "prefer-stable": false, + "prefer-lowest": false, + "platform": [], + "platform-dev": [] +} +--RUN-- +update dep/pkg2 --with-dependencies +--EXPECT-LOCK-- +{ + "packages": [ + {"name": "dep/pkg1", "version": "1.0.0", "type": "library"}, + {"name": "dep/pkg2", "version": "1.0.1", "type": "library"}, + {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}, "type": "library"}, + {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}, "type": "library"} + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": [], + "prefer-stable": false, + "prefer-lowest": false, + "platform": [], + "platform-dev": [] +} +--EXPECT-- +Installing dep/pkg1 (1.0.0) +Installing root/req1 (1.0.0) +Installing dep/pkg2 (1.0.1) +Installing root/req2 (1.0.0) From 7dc67fbbad6c6f65d7054c6df816843cacfa16f0 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Tue, 27 Oct 2020 17:11:24 +0100 Subject: [PATCH 3/8] Problem: Update fallback error message for requires without matches --- src/Composer/DependencyResolver/Problem.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Composer/DependencyResolver/Problem.php b/src/Composer/DependencyResolver/Problem.php index 327874b26..0dc5b2f6c 100644 --- a/src/Composer/DependencyResolver/Problem.php +++ b/src/Composer/DependencyResolver/Problem.php @@ -271,7 +271,7 @@ class Problem return array("- Root composer.json requires $packageName".self::constraintToText($constraint) . ', ', 'found '.self::getPackageList($packages, $isVerbose).' in lock file but not in remote repositories, make sure you avoid updating this package to keep the one from lock file.'); } - return array("- Root composer.json requires $packageName".self::constraintToText($constraint) . ', ', 'found '.self::getPackageList($packages, $isVerbose).' but '.(self::hasMultipleNames($packages) ? 'these conflict' : 'it conflicts').' with another require.'); + return array("- Root composer.json requires $packageName".self::constraintToText($constraint) . ', ', 'found '.self::getPackageList($packages, $isVerbose).' but these were not loaded, likely because '.(self::hasMultipleNames($packages) ? 'they conflict' : 'it conflicts').' with another require.'); } // check if the package is found when bypassing stability checks From ea42d13f37335dbc017a6b751a79eee532a897db Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Tue, 27 Oct 2020 21:59:12 +0100 Subject: [PATCH 4/8] PoolBuilderTest: check locked constraints are considered on partial update --- .../poolbuilder/partial-update-unfixing-locked-deps.test | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test index 6c160855b..6175029cc 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test @@ -11,7 +11,7 @@ locked packages still need to be taking into account for loading all necessary v "locked": [ {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}}, {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}}, - {"name": "dep/pkg1", "version": "1.0.0"}, + {"name": "dep/pkg1", "version": "1.0.0", "author": "old"}, {"name": "dep/pkg2", "version": "1.0.0"} ], "allowList": [ @@ -29,7 +29,8 @@ locked packages still need to be taking into account for loading all necessary v [ {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}}, {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}}, - {"name": "dep/pkg1", "version": "1.0.0"}, + {"name": "dep/pkg1", "version": "1.0.0", "author": "new"}, + {"name": "dep/pkg1", "version": "1.0.1"}, {"name": "dep/pkg1", "version": "2.0.0"}, {"name": "dep/pkg2", "version": "1.0.0"}, {"name": "dep/pkg2", "version": "1.2.0", "require": {"dep/pkg1": "2.*"}} @@ -43,5 +44,6 @@ locked packages still need to be taking into account for loading all necessary v "dep/pkg2-1.0.0.0", "dep/pkg2-1.2.0.0", "dep/pkg1-1.0.0.0", + "dep/pkg1-1.0.1.0", "dep/pkg1-2.0.0.0" ] From 7ddd1c64af0c5f1a8daf69131e15a71a6352a176 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Tue, 27 Oct 2020 22:10:15 +0100 Subject: [PATCH 5/8] PoolBuilder: On unlock ensure consider all locked requirements for unlocked package --- .../DependencyResolver/PoolBuilder.php | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index ccd3e9dde..32983e708 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -469,15 +469,6 @@ class PoolBuilder */ private function unlockPackage(Request $request, $name) { - // remove locked package by this name which was already initialized - foreach ($request->getLockedPackages() as $lockedPackage) { - if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) { - if (false !== $index = array_search($lockedPackage, $this->packages, true)) { - $request->unlockPackage($lockedPackage); - $this->removeLoadedPackage($request, $lockedPackage, $index); - } - } - } if ( // if we unfixed a replaced package name, we also need to unfix the replacer itself @@ -489,6 +480,29 @@ class PoolBuilder } unset($this->skippedLoad[$name], $this->loadedPackages[$name], $this->maxExtendedReqs[$name]); + + // remove locked package by this name which was already initialized + foreach ($request->getLockedPackages() as $lockedPackage) { + if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) { + if (false !== $index = array_search($lockedPackage, $this->packages, true)) { + $request->unlockPackage($lockedPackage); + $this->removeLoadedPackage($request, $lockedPackage, $index); + + // make sure that any requirements for this package by other locked or fixed packages are now + // also loaded, as they were previously ignored because the locked (now unlocked) package already + // satisfied their requirements + foreach ($request->getFixedOrLockedPackages() as $fixedOrLockedPackage) { + if ($fixedOrLockedPackage !== $lockedPackage && isset($this->skippedLoad[$fixedOrLockedPackage->getName()])) { + foreach ($fixedOrLockedPackage->getRequires() as $requireLink) { + if ($requireLink->getTarget() === $lockedPackage->getName()) { + $this->markPackageNameForLoading($request, $lockedPackage->getName(), $requireLink->getConstraint()); + } + } + } + } + } + } + } } private function removeLoadedPackage(Request $request, PackageInterface $package, $index) From 63bed408187de4dbe8004e418d83ffcfac9404ca Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Tue, 27 Oct 2020 22:18:05 +0100 Subject: [PATCH 6/8] PoolBuilderTest: Add a dependency which must not be loaded on unlock --- .../poolbuilder/partial-update-unfixing-locked-deps.test | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test index 6175029cc..bdd4aa216 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test @@ -32,6 +32,7 @@ locked packages still need to be taking into account for loading all necessary v {"name": "dep/pkg1", "version": "1.0.0", "author": "new"}, {"name": "dep/pkg1", "version": "1.0.1"}, {"name": "dep/pkg1", "version": "2.0.0"}, + {"name": "dep/pkg1", "version": "3.0.0"}, {"name": "dep/pkg2", "version": "1.0.0"}, {"name": "dep/pkg2", "version": "1.2.0", "require": {"dep/pkg1": "2.*"}} ] From 2d91fbc65a3aa12f718b19d6a686aced8918c94f Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Wed, 28 Oct 2020 13:57:18 +0100 Subject: [PATCH 7/8] PoolBuilder: never mark skipped packages for loading Reorder code in loadPackage to avoid duplicate calls --- .../DependencyResolver/PoolBuilder.php | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 32983e708..93912bd76 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -371,26 +371,22 @@ class PoolBuilder $require = $link->getTarget(); $linkConstraint = $link->getConstraint(); - if ($propagateUpdate) { - // if this is a partial update with transitive dependencies we need to unlock the package we now know is a + // if the required package is loaded as a locked package only and hasn't had its deps analyzed + if (isset($this->skippedLoad[$require])) { + // if we're doing a full update or this is a partial update with transitive deps and we're currently + // looking at a package which needs to be updated 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 ($propagateUpdate && $request->getUpdateAllowTransitiveDependencies()) { if ($request->getUpdateAllowTransitiveRootDependencies() || !$this->isRootRequire($request, $this->skippedLoad[$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; - $this->io->writeError('Dependency "'.$require.'" is also a root requirement. Package has not been listed as an update argument, so keeping locked at old version. Use --with-all-dependencies (-W) to include root dependencies.'); + } elseif (!isset($this->updateAllowWarned[$this->skippedLoad[$require]])) { + $this->updateAllowWarned[$this->skippedLoad[$require]] = true; + $this->io->writeError('Dependency "'.$this->skippedLoad[$require].'" is also a root requirement. Package has not been listed as an update argument, so keeping locked at old version. Use --with-all-dependencies (-W) to include root dependencies.'); } - } else { - $this->markPackageNameForLoading($request, $require, $linkConstraint); } } else { - // 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); - } + $this->markPackageNameForLoading($request, $require, $linkConstraint); } } From db0656eab095d6632281a94106e7294a3ad88bc9 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Fri, 13 Nov 2020 14:17:10 +0100 Subject: [PATCH 8/8] Duplicate partial update unlock but keep old version test with provide keyword --- ...er-dep-if-still-required-with-provide.test | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required-with-provide.test diff --git a/tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required-with-provide.test b/tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required-with-provide.test new file mode 100644 index 000000000..4cae3c87c --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required-with-provide.test @@ -0,0 +1,65 @@ +--TEST-- +Ensure that a partial update of a dependency does not conflict if the only way to proceed is using an old locked version. + +--COMPOSER-- +{ + "repositories": [ + { + "type": "package", + "package": [ + {"name": "root/req1", "version": "1.0.0", "require": {"virtual/pkg1": "1.*"}}, + {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*", "dep/pkg1": "*"}}, + {"name": "dep/pkg1", "version": "1.0.0", "provide": {"virtual/pkg1": "1.0.0"}}, + {"name": "dep/pkg1", "version": "2.0.0"}, + {"name": "dep/pkg2", "version": "1.0.0"}, + {"name": "dep/pkg2", "version": "1.0.1"}, + {"name": "dep/pkg2", "version": "1.2.0", "require": {"virtual/pkg1": "2.*"}} + ] + } + ], + "require": { + "root/req1": "*", + "root/req2": "*" + } +} +--LOCK-- +{ + "packages": [ + {"name": "dep/pkg1", "version": "1.0.0", "type": "library", "provide": {"virtual/pkg1": "1.0.0"}}, + {"name": "dep/pkg2", "version": "1.0.0", "type": "library"}, + {"name": "root/req1", "version": "1.0.0", "require": {"virtual/pkg1": "1.*"}, "type": "library"}, + {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*", "dep/pkg1": "*"}, "type": "library"} + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": {}, + "prefer-stable": false, + "prefer-lowest": false, + "platform": [], + "platform-dev": [] +} +--RUN-- +update dep/pkg2 --with-dependencies +--EXPECT-LOCK-- +{ + "packages": [ + {"name": "dep/pkg1", "version": "1.0.0", "type": "library", "provide": {"virtual/pkg1": "1.0.0"}}, + {"name": "dep/pkg2", "version": "1.0.1", "type": "library"}, + {"name": "root/req1", "version": "1.0.0", "require": {"virtual/pkg1": "1.*"}, "type": "library"}, + {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*", "dep/pkg1": "*"}, "type": "library"} + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": [], + "prefer-stable": false, + "prefer-lowest": false, + "platform": [], + "platform-dev": [] +} +--EXPECT-- +Installing dep/pkg1 (1.0.0) +Installing root/req1 (1.0.0) +Installing dep/pkg2 (1.0.1) +Installing root/req2 (1.0.0)