From f572c9f03d8f9f395b5da21224910388d6bc7d33 Mon Sep 17 00:00:00 2001 From: Jason Woods Date: Wed, 29 Dec 2021 09:28:41 +0000 Subject: [PATCH 1/3] fix: Do not optimise away packages due to a requirement by a locked package that will be uninstalled Fixes #10394 --- .../DependencyResolver/PoolOptimizer.php | 13 +++++ ...ter-impossible-packages-only-required.test | 54 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required.test diff --git a/src/Composer/DependencyResolver/PoolOptimizer.php b/src/Composer/DependencyResolver/PoolOptimizer.php index bfb10a589269..4aa6ffe324d8 100644 --- a/src/Composer/DependencyResolver/PoolOptimizer.php +++ b/src/Composer/DependencyResolver/PoolOptimizer.php @@ -418,6 +418,19 @@ private function optimizeImpossiblePackagesAway(Request $request, Pool $pool) } foreach ($request->getLockedPackages() as $package) { + // If this locked package is no longer required by root or anything in the pool, it may get uninstalled so do not apply its requirements + // In a case where a requirement WERE to appear in the pool by a package that would not be used, it would've been unlocked and so not filtered still + $ignoreUnusedPackage = false; + foreach ($package->getNames(false) as $packageName) { + if (!isset($this->requireConstraintsPerPackage[$packageName])) { + $ignoreUnusedPackage = true; + } + } + + if ($ignoreUnusedPackage) { + continue; + } + foreach ($package->getRequires() as $link) { $require = $link->getTarget(); if (!isset($packageIndex[$require])) { diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required.test new file mode 100644 index 000000000000..2705f2d4e088 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required.test @@ -0,0 +1,54 @@ +--TEST-- +When filtering packages from the pool that cannot meet the fixed/locked requirements, ensure that the requirements for a package that is not required anywhere is not used to filter, as it will be ultimately be removed. + +--REQUEST-- +{ + "require": { + "first/pkg": "*", + "second/pkg": "1.1.0" + }, + "locked": [ + {"name": "first/pkg", "version": "1.0.0", "require": {"second/pkg": "^1.0"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"third/pkg": "1.0.0"}}, + {"name": "third/pkg", "version": "1.0.0", "require": {"fourth/pkg": "1.0.0"}}, + {"name": "fourth/pkg", "version": "1.0.0"} + ], + "allowList": [ + "first/pkg" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + [ + {"name": "first/pkg", "version": "1.0.0", "require": {"second/pkg": "*"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"third/pkg": "1.0.0"}}, + {"name": "second/pkg", "version": "1.1.0", "require": {"fourth/pkg": "2.0.0"}}, + {"name": "third/pkg", "version": "1.0.0", "require": {"fourth/pkg": "1.0.0"}}, + {"name": "fourth/pkg", "version": "1.0.0"}, + {"name": "fourth/pkg", "version": "2.0.0"} + ] +] + +--EXPECT-- +[ + "third/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "second/pkg-1.1.0.0", + "fourth/pkg-1.0.0.0", + "fourth/pkg-2.0.0.0" +] + +--EXPECT-OPTIMIZED-- +[ + "third/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "second/pkg-1.1.0.0", + "fourth/pkg-1.0.0.0", + "fourth/pkg-2.0.0.0" +] From ae7e8f3f28a9688966238c88ef0ab3b6e3f5886b Mon Sep 17 00:00:00 2001 From: Jason Woods Date: Thu, 30 Dec 2021 11:20:27 +0000 Subject: [PATCH 2/3] test: Added additional tests for impossible package pool optimisation, including scenarios not yet optimised --- ...sible-packages-only-required-provides.test | 58 ++++++++++++++++++ ...sible-packages-only-required-replaces.test | 57 ++++++++++++++++++ ...ter-impossible-packages-only-required.test | 1 + .../filter-impossible-packages-provides.test | 59 +++++++++++++++++++ .../filter-impossible-packages-replaces.test | 58 ++++++++++++++++++ .../filter-impossible-packages.test | 1 + 6 files changed, 234 insertions(+) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required-provides.test create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required-replaces.test create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-provides.test create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-replaces.test diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required-provides.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required-provides.test new file mode 100644 index 000000000000..1d90c7c661b1 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required-provides.test @@ -0,0 +1,58 @@ +--TEST-- +When filtering packages from the pool that cannot meet the fixed/locked requirements, ensure that the requirements for a package that is not required anywhere is not used to filter, as it will be ultimately be removed. +(Variant where the requirement is on a provided package, the locked third/pkg must not restrict the provider) +(NOTE: We are not optimising this scenario currently) + +--REQUEST-- +{ + "require": { + "first/pkg": "*", + "second/pkg": "1.1.0", + "provider/pkg": "*" + }, + "locked": [ + {"name": "first/pkg", "version": "1.0.0", "require": {"second/pkg": "^1.0"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"third/pkg": "1.0.0"}}, + {"name": "third/pkg", "version": "1.0.0", "require": {"provided/pkg": "1.0.0"}}, + {"name": "provider/pkg", "version": "1.0.0", "provide": {"provided/pkg": "1.0.0"}} + ], + "allowList": [ + "first/pkg", + "provider/pkg" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + [ + {"name": "first/pkg", "version": "1.0.0", "require": {"second/pkg": "*"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"third/pkg": "1.0.0"}}, + {"name": "second/pkg", "version": "1.1.0", "require": {"provided/pkg": "2.0.0"}}, + {"name": "third/pkg", "version": "1.0.0", "require": {"provided/pkg": "1.0.0"}}, + {"name": "provider/pkg", "version": "1.0.0", "provide": {"provided/pkg": "1.0.0"}}, + {"name": "provider/pkg", "version": "2.0.0", "provide": {"provided/pkg": "2.0.0"}} + ] +] + +--EXPECT-- +[ + "third/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "provider/pkg-1.0.0.0", + "provider/pkg-2.0.0.0", + "second/pkg-1.1.0.0" +] + +--EXPECT-OPTIMIZED-- +[ + "third/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "provider/pkg-1.0.0.0", + "provider/pkg-2.0.0.0", + "second/pkg-1.1.0.0" +] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required-replaces.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required-replaces.test new file mode 100644 index 000000000000..d31a8b5060b3 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required-replaces.test @@ -0,0 +1,57 @@ +--TEST-- +When filtering packages from the pool that cannot meet the fixed/locked requirements, ensure that the requirements for a package that is not required anywhere is not used to filter, as it will be ultimately be removed. +(Variant where the requirement is on a replaced package, the locked third/pkg must not restrict the replacer) +(NOTE: We are not optimising this scenario currently) + +--REQUEST-- +{ + "require": { + "first/pkg": "*", + "second/pkg": "1.1.0", + "replacer/pkg": "*" + }, + "locked": [ + {"name": "first/pkg", "version": "1.0.0", "require": {"second/pkg": "^1.0"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"third/pkg": "1.0.0"}}, + {"name": "third/pkg", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}, + {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}} + ], + "allowList": [ + "first/pkg" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + [ + {"name": "first/pkg", "version": "1.0.0", "require": {"second/pkg": "*"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"third/pkg": "1.0.0"}}, + {"name": "second/pkg", "version": "1.1.0", "require": {"replaced/pkg": "2.0.0"}}, + {"name": "third/pkg", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}, + {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}}, + {"name": "replacer/pkg", "version": "2.0.0", "replace": {"replaced/pkg": "2.0.0"}} + ] +] + +--EXPECT-- +[ + "third/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "second/pkg-1.1.0.0", + "replacer/pkg-1.0.0.0", + "replacer/pkg-2.0.0.0" +] + +--EXPECT-OPTIMIZED-- +[ + "third/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "second/pkg-1.1.0.0", + "replacer/pkg-1.0.0.0", + "replacer/pkg-2.0.0.0" +] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required.test index 2705f2d4e088..ef09d14d06ff 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required.test +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-only-required.test @@ -1,5 +1,6 @@ --TEST-- When filtering packages from the pool that cannot meet the fixed/locked requirements, ensure that the requirements for a package that is not required anywhere is not used to filter, as it will be ultimately be removed. +(The locked third/pkg is not required by any package so will be removed, so should not restrict the versions of fourth/pkg) --REQUEST-- { diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-provides.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-provides.test new file mode 100644 index 000000000000..be8b63b508f1 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-provides.test @@ -0,0 +1,59 @@ +--TEST-- +Do not load packages into the pool that cannot meet the fixed/locked requirements, when a loose requirement is encountered during update +(Variant where requirement is on a provided package, the locked second package should restrict the provider to those that provide < 3.0.0) +(NOTE: We are not optimising this scenario currently) + +--REQUEST-- +{ + "require": { + "first/pkg": "*", + "second/pkg": "*", + "provider/pkg": "*" + }, + "locked": [ + {"name": "first/pkg", "version": "1.0.0", "require": {"provided/pkg": "1.0.0"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"provided/pkg": "< 3.0.0"}}, + {"name": "provider/pkg", "version": "1.0.0", "provide": {"provided/pkg": "1.0.0"}} + ], + "allowList": [ + "first/pkg", + "provider/pkg" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + [ + {"name": "first/pkg", "version": "1.0.0", "require": {"provided/pkg": "1.0.0"}}, + {"name": "first/pkg", "version": "1.1.0", "require": {"provided/pkg": "2.0.0"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"provided/pkg": "*"}}, + {"name": "provider/pkg", "version": "1.0.0", "provide": {"provided/pkg": "1.0.0"}}, + {"name": "provider/pkg", "version": "1.1.0", "provide": {"provided/pkg": "2.0.0"}}, + {"name": "provider/pkg", "version": "1.2.0", "provide": {"provided/pkg": "3.0.0"}} + ] +] + +--EXPECT-- +[ + "second/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "first/pkg-1.1.0.0", + "provider/pkg-1.0.0.0", + "provider/pkg-1.1.0.0", + "provider/pkg-1.2.0.0" +] + +--EXPECT-OPTIMIZED-- +[ + "second/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "first/pkg-1.1.0.0", + "provider/pkg-1.0.0.0", + "provider/pkg-1.1.0.0", + "provider/pkg-1.2.0.0" +] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-replaces.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-replaces.test new file mode 100644 index 000000000000..f01cd8921507 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-replaces.test @@ -0,0 +1,58 @@ +--TEST-- +Do not load packages into the pool that cannot meet the fixed/locked requirements, when a loose requirement is encountered during update +(Variant where requirement is on a replaced package, the locked second package should restrict the replacer to those that replace < 3.0.0) +(NOTE: We are not optimising this scenario currently) + +--REQUEST-- +{ + "require": { + "first/pkg": "*", + "second/pkg": "*", + "replacer/pkg": "*" + }, + "locked": [ + {"name": "first/pkg", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"replaced/pkg": "< 3.0.0"}}, + {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}} + ], + "allowList": [ + "first/pkg" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + [ + {"name": "first/pkg", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}, + {"name": "first/pkg", "version": "1.1.0", "require": {"replaced/pkg": "2.0.0"}}, + {"name": "second/pkg", "version": "1.0.0", "require": {"replaced/pkg": "*"}}, + {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}}, + {"name": "replacer/pkg", "version": "1.1.0", "replace": {"replaced/pkg": "2.0.0"}}, + {"name": "replacer/pkg", "version": "1.2.0", "replace": {"replaced/pkg": "3.0.0"}} + ] +] + +--EXPECT-- +[ + "second/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "first/pkg-1.1.0.0", + "replacer/pkg-1.0.0.0", + "replacer/pkg-1.1.0.0", + "replacer/pkg-1.2.0.0" +] + +--EXPECT-OPTIMIZED-- +[ + "second/pkg-1.0.0.0 (locked)", + "first/pkg-1.0.0.0", + "first/pkg-1.1.0.0", + "replacer/pkg-1.0.0.0", + "replacer/pkg-1.1.0.0", + "replacer/pkg-1.2.0.0" +] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages.test index 3dfea1ec44e2..edbb8b6d0650 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages.test +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages.test @@ -1,5 +1,6 @@ --TEST-- Do not load packages into the pool that cannot meet the fixed/locked requirements, when a loose requirement is encountered during update +(The locked root/req package should restrict dep/dep to only 2.* versions) --REQUEST-- { From 6540b46ab4ebb8f0439831c1bfeeab5d511c4e25 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Fri, 31 Dec 2021 11:43:16 +0100 Subject: [PATCH 3/3] Add test checking that a locked replacer with only requires on its replaced name does filter its dependencies, and fix logic accordingly --- .../DependencyResolver/PoolOptimizer.php | 9 +-- ...r-impossible-packages-locked-replacer.test | 60 +++++++++++++++++++ 2 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-locked-replacer.test diff --git a/src/Composer/DependencyResolver/PoolOptimizer.php b/src/Composer/DependencyResolver/PoolOptimizer.php index 4aa6ffe324d8..841a2e9c63fe 100644 --- a/src/Composer/DependencyResolver/PoolOptimizer.php +++ b/src/Composer/DependencyResolver/PoolOptimizer.php @@ -420,14 +420,15 @@ private function optimizeImpossiblePackagesAway(Request $request, Pool $pool) foreach ($request->getLockedPackages() as $package) { // If this locked package is no longer required by root or anything in the pool, it may get uninstalled so do not apply its requirements // In a case where a requirement WERE to appear in the pool by a package that would not be used, it would've been unlocked and so not filtered still - $ignoreUnusedPackage = false; + $isUnusedPackage = true; foreach ($package->getNames(false) as $packageName) { - if (!isset($this->requireConstraintsPerPackage[$packageName])) { - $ignoreUnusedPackage = true; + if (isset($this->requireConstraintsPerPackage[$packageName])) { + $isUnusedPackage = false; + break; } } - if ($ignoreUnusedPackage) { + if ($isUnusedPackage) { continue; } diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-locked-replacer.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-locked-replacer.test new file mode 100644 index 000000000000..d72d8610543f --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-locked-replacer.test @@ -0,0 +1,60 @@ +--TEST-- +Do not load packages into the pool that cannot meet the fixed/locked requirements, when a loose requirement is encountered during update +(The locked replacer/pkg should restrict dependencies even though it is only referenced by its replaced name) + +--REQUEST-- +{ + "require": { + "first/pkg": "*", + "second/pkg": "*", + "replacer/dep": "*" + }, + "locked": [ + {"name": "first/pkg", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}, + {"name": "second/pkg", "version": "1.0.0"}, + {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}, "require": {"replacer/dep": "1.*"}}, + {"name": "replacer/dep", "version": "1.0.0"} + ], + "allowList": [ + "second/pkg", + "replacer/dep" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + [ + {"name": "first/pkg", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}, + {"name": "first/pkg", "version": "1.1.0", "require": {"replaced/pkg": "2.0.0"}}, + {"name": "second/pkg", "version": "1.0.0"}, + {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}, "require": {"replacer/dep": "1.*"}}, + {"name": "replacer/pkg", "version": "1.1.0", "replace": {"replaced/pkg": "2.0.0"}, "require": {"replacer/dep": "1.*"}}, + {"name": "replacer/pkg", "version": "1.2.0", "replace": {"replaced/pkg": "3.0.0"}, "require": {"replacer/dep": "1.*"}}, + {"name": "replacer/dep", "version": "1.0.0"}, + {"name": "replacer/dep", "version": "1.0.1"}, + {"name": "replacer/dep", "version": "2.0.0"} + ] +] + +--EXPECT-- +[ + "first/pkg-1.0.0.0 (locked)", + "replacer/pkg-1.0.0.0 (locked)", + "second/pkg-1.0.0.0", + "replacer/dep-1.0.0.0", + "replacer/dep-1.0.1.0", + "replacer/dep-2.0.0.0" +] + +--EXPECT-OPTIMIZED-- +[ + "first/pkg-1.0.0.0 (locked)", + "replacer/pkg-1.0.0.0 (locked)", + "second/pkg-1.0.0.0", + "replacer/dep-1.0.1.0" +]