From bd4ba36fa9abad52639f0e78b41efc2cc13c5fc8 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Thu, 14 Oct 2021 14:49:04 +0200 Subject: [PATCH 1/7] Prevent auto-unlocked path repo packages from also unlocking their transitive deps when -w/-W is used --- .../DependencyResolver/PoolBuilder.php | 37 ++++++++++++------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index f05a62fc4..b421af96a 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -102,6 +102,16 @@ class PoolBuilder /** @var array> */ private $skippedLoad = array(); + /** + * Keeps a list of dependencies which are locked but were auto-unlocked as they are path repositories + * + * This half-unlocked state means the package itself will update but the UPDATE_LISTED_WITH_TRANSITIVE_DEPS* + * flags will not apply until the package really gets unlocked in some other way than being a path repo + * + * @var array + */ + private $pathRepoUnlocked = array(); + /** * Keeps a list of dependencies which are root requirements, and as such * have already their maximum required range loaded and can not be @@ -155,6 +165,17 @@ class PoolBuilder foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) { if (!$this->isUpdateAllowed($lockedPackage)) { + // Path repo packages are never loaded from lock, to force them to always remain in sync + // unless symlinking is disabled in which case we probably should rather treat them like + // regular packages + if ($lockedPackage->getDistType() === 'path') { + $transportOptions = $lockedPackage->getTransportOptions(); + if (!isset($transportOptions['symlink']) || $transportOptions['symlink'] !== false) { + $this->pathRepoUnlocked[$lockedPackage->getName()] = true; + continue; + } + } + $request->lockPackage($lockedPackage); $this->skippedLoad[$lockedPackage->getName()][] = $lockedPackage; // remember which packages we skipped loading remote content for in this partial update @@ -360,7 +381,7 @@ class PoolBuilder } foreach ($result['packages'] as $package) { $this->loadedPerRepo[$repoIndex][$package->getName()][$package->getVersion()] = $package; - $this->loadPackage($request, $package); + $this->loadPackage($request, $package, !isset($this->pathRepoUnlocked[$package->getName()])); } } } @@ -369,7 +390,7 @@ class PoolBuilder * @param bool $propagateUpdate * @return void */ - private function loadPackage(Request $request, BasePackage $package, $propagateUpdate = true) + private function loadPackage(Request $request, BasePackage $package, $propagateUpdate) { $index = $this->indexCounter++; $this->packages[$index] = $package; @@ -526,16 +547,6 @@ class PoolBuilder */ private function isUpdateAllowed(BasePackage $package) { - // Path repo packages are never loaded from lock, to force them to always remain in sync - // unless symlinking is disabled in which case we probably should rather treat them like - // regular packages - if ($package->getDistType() === 'path') { - $transportOptions = $package->getTransportOptions(); - if (!isset($transportOptions['symlink']) || $transportOptions['symlink'] !== false) { - return true; - } - } - foreach ($this->updateAllowList as $pattern => $void) { $patternRegexp = BasePackage::packageNameToRegexp($pattern); if (preg_match($patternRegexp, $package->getName())) { @@ -604,7 +615,7 @@ class PoolBuilder } } - unset($this->skippedLoad[$name], $this->loadedPackages[$name], $this->maxExtendedReqs[$name]); + unset($this->skippedLoad[$name], $this->loadedPackages[$name], $this->maxExtendedReqs[$name], $this->pathRepoUnlocked[$name]); // remove locked package by this name which was already initialized foreach ($request->getLockedPackages() as $lockedPackage) { From 0ae5a6ef594610b89c606a49123e7381e0bd6650 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Wed, 27 Oct 2021 21:28:09 +0200 Subject: [PATCH 2/7] Add test verifying unfixing behavior of path repo packages --- .../mirrored-path-repo/composer.json | 8 ++ ...-always-but-not-their-transitive-deps.test | 79 +++++++++++++++++++ .../symlinked-path-repo/composer.json | 8 ++ .../DependencyResolver/PoolBuilderTest.php | 15 +++- 4 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/mirrored-path-repo/composer.json create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo/composer.json diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/mirrored-path-repo/composer.json b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/mirrored-path-repo/composer.json new file mode 100644 index 000000000..5e09733ec --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/mirrored-path-repo/composer.json @@ -0,0 +1,8 @@ +{ + "name": "mirrored/path-pkg", + "version": "2.0.0", + "require": { + "mirrored/transitive": "2.*", + "mirrored/transitive2": "2.*" + } +} diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test new file mode 100644 index 000000000..8e7c8068f --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test @@ -0,0 +1,79 @@ +--TEST-- +Partially updating one root requirement with transitive deps fully updates transitive deps, and always updates symlinked path repos, but not the transitive deps of the path repos. + +--REQUEST-- +{ + "require": { + "root/update": "*", + "symlinked/path-pkg": "*", + "mirrored/path-pkg": "*" + }, + "locked": [ + {"name": "root/update", "version": "1.0.1", "require": {"symlinked/transitive2": ">=1.0.1", "mirrored/transitive2": ">=1.0.1"}}, + {"name": "symlinked/transitive", "version": "1.0.0"}, + {"name": "symlinked/transitive2", "version": "1.0.0"}, + {"name": "mirrored/transitive", "version": "1.0.0"}, + {"name": "mirrored/transitive2", "version": "1.0.0"}, + { + "name": "symlinked/path-pkg", + "version": "1.0.0", + "require": { + "symlinked/transitive": "1.*", + "symlinked/transitive2": "1.*" + }, + "dist": {"type": "path", "url": "./symlinked-path-repo", "reference": "abcd"}, "transport-options": {} + }, + { + "name": "mirrored/path-pkg", + "version": "1.0.0", + "require": { + "mirrored/transitive": "1.*", + "mirrored/transitive2": "1.*" + }, + "dist": {"type": "path", "url": "./mirrored-path-repo", "reference": "abcd"}, "transport-options": {"symlink": false} + } + ], + "allowList": [ + "root/update" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + {"type": "path", "url": "./symlinked-path-repo"}, + {"type": "path", "url": "./mirrored-path-repo", "options": {"symlink": false}}, + [ + {"name": "root/update", "version": "1.0.4", "require": {"symlinked/transitive2": ">=1.0.1", "mirrored/transitive2": ">=1.0.1"}}, + {"name": "symlinked/transitive", "version": "1.0.0"}, + {"name": "symlinked/transitive", "version": "1.0.1"}, + {"name": "symlinked/transitive", "version": "2.0.2"}, + {"name": "symlinked/transitive2", "version": "1.0.0"}, + {"name": "symlinked/transitive2", "version": "1.0.3"}, + {"name": "symlinked/transitive2", "version": "2.0.4"}, + {"name": "mirrored/transitive", "version": "1.0.0"}, + {"name": "mirrored/transitive", "version": "1.0.5"}, + {"name": "mirrored/transitive", "version": "2.0.6"}, + {"name": "mirrored/transitive2", "version": "1.0.0"}, + {"name": "mirrored/transitive2", "version": "1.0.7"}, + {"name": "mirrored/transitive2", "version": "2.0.8"} + ] +] + +--EXPECT-- +[ + "symlinked/transitive-1.0.0.0 (locked)", + "mirrored/transitive-1.0.0.0 (locked)", + "mirrored/path-pkg-1.0.0.0 (locked)", + "symlinked/path-pkg-2.0.0.0", + "root/update-1.0.4.0", + "symlinked/transitive2-1.0.3.0", + "symlinked/transitive2-2.0.4.0", + "mirrored/transitive2-1.0.0.0", + "mirrored/transitive2-1.0.7.0", + "mirrored/transitive2-2.0.8.0" +] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo/composer.json b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo/composer.json new file mode 100644 index 000000000..4f4ff14e5 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo/composer.json @@ -0,0 +1,8 @@ +{ + "name": "symlinked/path-pkg", + "version": "2.0.0", + "require": { + "symlinked/transitive": "2.*", + "symlinked/transitive2": "2.*" + } +} diff --git a/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php index 3eb0ebc41..6be0d3016 100644 --- a/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php +++ b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php @@ -15,6 +15,7 @@ namespace Composer\Test\DependencyResolver; use Composer\DependencyResolver\DefaultPolicy; use Composer\DependencyResolver\Pool; use Composer\DependencyResolver\PoolOptimizer; +use Composer\Config; use Composer\IO\NullIO; use Composer\Repository\ArrayRepository; use Composer\Repository\FilterRepository; @@ -25,6 +26,7 @@ use Composer\Package\AliasPackage; use Composer\Json\JsonFile; use Composer\Package\Loader\ArrayLoader; use Composer\Package\Version\VersionParser; +use Composer\Repository\RepositoryFactory; use Composer\Repository\RepositorySet; use Composer\Test\TestCase; @@ -57,7 +59,7 @@ class PoolBuilderTest extends TestCase $rootAliases[$index]['alias_normalized'] = $parser->normalize($alias['alias']); } - $loader = new ArrayLoader(); + $loader = new ArrayLoader(null, true); $packageIds = array(); $loadPackage = function ($data) use ($loader, &$packageIds) { /** @var ?int $id */ @@ -79,8 +81,17 @@ class PoolBuilderTest extends TestCase return $pkg; }; + $oldCwd = getcwd(); + chdir(__DIR__.'/Fixtures/poolbuilder/'); + $repositorySet = new RepositorySet($minimumStability, $stabilityFlags, $rootAliases, $rootReferences); foreach ($packageRepos as $packages) { + if (isset($packages['type'])) { + $repo = RepositoryFactory::createRepo(new NullIO, new Config(false), $packages); + $repositorySet->addRepository($repo); + continue; + } + $repo = new ArrayRepository(); if (isset($packages['canonical']) || isset($packages['only']) || isset($packages['exclude'])) { $options = $packages; @@ -142,6 +153,8 @@ class PoolBuilderTest extends TestCase $result[] = $pool->packageById($i); } + chdir($oldCwd); + return array_map(function (BasePackage $package) use ($packageIds) { if ($id = array_search($package, $packageIds, true)) { return $id; From d0c1e6cb058fe0adaa6fe135bc0a1005cfebd535 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Mon, 8 Nov 2021 14:21:59 +0100 Subject: [PATCH 3/7] Add InstallerTest for path repo symlink unfixing --- ...tial-update-with-symlinked-path-repos.test | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 tests/Composer/Test/Fixtures/installer/partial-update-with-symlinked-path-repos.test diff --git a/tests/Composer/Test/Fixtures/installer/partial-update-with-symlinked-path-repos.test b/tests/Composer/Test/Fixtures/installer/partial-update-with-symlinked-path-repos.test new file mode 100644 index 000000000..79a6b8b3c --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/partial-update-with-symlinked-path-repos.test @@ -0,0 +1,91 @@ +--TEST-- +Partially updating one root requirement with transitive deps fully updates transitive deps, and always updates symlinked path repos, but not the transitive deps of the path repos. + +--COMPOSER-- +{ + "repositories": [ + {"type": "path", "url": "./DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo"}, + {"type": "path", "url": "./DependencyResolver/Fixtures/poolbuilder/mirrored-path-repo", "options": {"symlink": false}}, + { + "type": "package", + "package": [ + {"name": "root/update", "version": "1.0.4", "require": {"symlinked/transitive2": ">=1.0.1", "mirrored/transitive2": ">=1.0.1"}}, + {"name": "symlinked/transitive", "version": "1.0.0"}, + {"name": "symlinked/transitive", "version": "1.0.1"}, + {"name": "symlinked/transitive", "version": "2.0.3"}, + {"name": "symlinked/transitive2", "version": "1.0.0"}, + {"name": "symlinked/transitive2", "version": "1.0.3"}, + {"name": "symlinked/transitive2", "version": "2.0.3"}, + {"name": "mirrored/transitive", "version": "1.0.0"}, + {"name": "mirrored/transitive", "version": "1.0.5"}, + {"name": "mirrored/transitive2", "version": "1.0.0"}, + {"name": "mirrored/transitive2", "version": "1.0.7"} + ] + } + ], + "require": { + "root/update": "*", + "symlinked/path-pkg": "*", + "mirrored/path-pkg": "*" + } +} +--LOCK-- +{ + "packages": [ + {"name": "root/update", "version": "1.0.1", "require": {"symlinked/transitive2": ">=1.0.1", "mirrored/transitive2": ">=1.0.1"}}, + {"name": "symlinked/transitive", "version": "1.0.0"}, + {"name": "symlinked/transitive2", "version": "1.0.0"}, + {"name": "mirrored/transitive", "version": "1.0.0"}, + {"name": "mirrored/transitive2", "version": "1.0.0"}, + { + "name": "symlinked/path-pkg", + "version": "1.0.0", + "require": { + "symlinked/transitive": "1.*", + "symlinked/transitive2": "1.*" + }, + "dist": {"type": "path", "url": "./symlinked-path-repo", "reference": "abcd"}, "transport-options": {} + }, + { + "name": "mirrored/path-pkg", + "version": "1.0.0", + "require": { + "mirrored/transitive": "1.*", + "mirrored/transitive2": "1.*" + }, + "dist": {"type": "path", "url": "./mirrored-path-repo", "reference": "abcd"}, "transport-options": {"symlink": false} + } + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": { + }, + "prefer-stable": false, + "prefer-lowest": false, + "platform": [], + "platform-dev": [] +} +--INSTALLED-- +[ + { "name": "a/old", "version": "0.9.0" }, + { "name": "b/unstable", "version": "1.1.0-alpha", "require": {"f/dependency": "1.*"} }, + { "name": "c/uptodate", "version": "2.0.0" }, + { "name": "f/dependency", "version": "1.0.0" } +] +--RUN-- +update --with-all-dependencies root/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 symlinked/path-pkg * -> satisfiable by symlinked/path-pkg[2.0.0]. + - symlinked/path-pkg 2.0.0 requires symlinked/transitive 2.* -> found symlinked/transitive[2.0.3] but the package is fixed to 1.0.0 (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. + +--EXPECT-- From fcda22659ab9471858c0340ff6cc624d22866d30 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Mon, 8 Nov 2021 14:27:05 +0100 Subject: [PATCH 4/7] Update deprecation baseline for 8.1 --- tests/deprecations-8.1.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/deprecations-8.1.json b/tests/deprecations-8.1.json index a987d98f0..0982624a2 100644 --- a/tests/deprecations-8.1.json +++ b/tests/deprecations-8.1.json @@ -77,12 +77,12 @@ { "location": "Composer\\Test\\InstallerTest::testIntegrationWithRawPool", "message": "preg_match(): Passing null to parameter #4 ($flags) of type int is deprecated", - "count": 1784 + "count": 1800 }, { "location": "Composer\\Test\\InstallerTest::testIntegrationWithPoolOptimizer", "message": "preg_match(): Passing null to parameter #4 ($flags) of type int is deprecated", - "count": 1784 + "count": 1800 }, { "location": "Composer\\Test\\Package\\Archiver\\ArchivableFilesFinderTest::testManualExcludes", From b8caf4b214562aceb6406da23b1c18ae0ea52573 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Tue, 9 Nov 2021 15:20:11 +0100 Subject: [PATCH 5/7] Test path repo dependencies are updated in transitive partial updates (#7) --- ...th-repo-replacer-with-transitive-deps.test | 82 +++++++++++++++++++ .../composer.json | 12 +++ .../composer.json | 9 ++ 3 files changed, 103 insertions(+) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repo-replacer-with-transitive-deps.test create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo-replacer/composer.json create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo-with-replaced-deps/composer.json diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repo-replacer-with-transitive-deps.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repo-replacer-with-transitive-deps.test new file mode 100644 index 000000000..54cb48b83 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repo-replacer-with-transitive-deps.test @@ -0,0 +1,82 @@ +--TEST-- +Partially updating with deps a root requirement which depends on packages in a symlinked path repo should load all available versions for the path repo packages' dependencies. + +--REQUEST-- +{ + "require": { + "root/update": "*", + "symlinked/transitive3": "*", + "symlinked/transitive5": "*" + }, + "locked": [ + {"name": "root/update", "version": "1.0.1", "require": {"symlinked/path-pkg": ">=1.0.1", "symlinked/replaced-pkg": "*"}}, + {"name": "symlinked/transitive", "version": "1.0.0"}, + {"name": "symlinked/transitive3", "version": "1.0.0", "replace": {"symlinked/transitive3-replaced": "1.0.0"}}, + { + "name": "symlinked/path-pkg", + "version": "1.0.0", + "require": { + "symlinked/transitive": "1.*", + "symlinked/transitive3-replaced": "1.*", + "symlinked/transitive5-replaced": "1.*" + }, + "dist": {"type": "path", "url": "./symlinked-path-repo-with-replaced-deps", "reference": "abcd"}, "transport-options": {} + }, + {"name": "symlinked/transitive4", "version": "1.0.0"}, + {"name": "symlinked/transitive5", "version": "1.0.0", "replace": {"symlinked/transitive5-replaced": "1.0.0"}}, + { + "name": "symlinked/path-pkg-replace", + "version": "1.0.0", + "require": { + "symlinked/transitive3-replaced": "1.*", + "symlinked/transitive4": "1.*", + "symlinked/transitive5-replaced": "1.*" + }, + "replace": { + "symlinked/replaced-pkg": "1.0.0" + }, + "dist": {"type": "path", "url": "./symlinked-path-repo-replacer", "reference": "abcd"}, "transport-options": {} + } + ], + "allowList": [ + "root/update" + ], + "allowTransitiveDeps": true +} + +--FIXED-- +[ +] + +--PACKAGE-REPOS-- +[ + {"type": "path", "url": "./symlinked-path-repo-with-replaced-deps"}, + {"type": "path", "url": "./symlinked-path-repo-replacer"}, + [ + {"name": "root/update", "version": "1.0.4", "require": {"symlinked/path-pkg": ">=1.0.1", "symlinked/replaced-pkg": "*"}}, + {"name": "symlinked/transitive", "version": "1.0.0"}, + {"name": "symlinked/transitive", "version": "2.0.2"}, + {"name": "symlinked/transitive3", "version": "1.0.0", "replace": {"symlinked/transitive3-replaced": "1.0.0"}}, + {"name": "symlinked/transitive3", "version": "1.0.3", "replace": {"symlinked/transitive3-replaced": "1.0.3"}}, + {"name": "symlinked/transitive3", "version": "2.0.4", "replace": {"symlinked/transitive3-replaced": "2.0.4"}}, + {"name": "symlinked/transitive4", "version": "1.0.0"}, + {"name": "symlinked/transitive4", "version": "2.0.2"}, + {"name": "symlinked/transitive5", "version": "1.0.0", "replace": {"symlinked/transitive5-replaced": "1.0.0"}}, + {"name": "symlinked/transitive5", "version": "1.0.3", "replace": {"symlinked/transitive5-replaced": "1.0.3"}}, + {"name": "symlinked/transitive5", "version": "2.0.4", "replace": {"symlinked/transitive5-replaced": "2.0.4"}} + ] +] + +--EXPECT-- +[ + "symlinked/transitive-1.0.0.0", + "symlinked/transitive-2.0.2.0", + "symlinked/path-pkg-2.0.0.0", + "root/update-1.0.4.0", + "symlinked/transitive3-1.0.3.0", + "symlinked/transitive3-2.0.4.0", + "symlinked/transitive4-1.0.0.0", + "symlinked/transitive4-2.0.2.0", + "symlinked/transitive5-1.0.3.0", + "symlinked/transitive5-2.0.4.0" +] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo-replacer/composer.json b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo-replacer/composer.json new file mode 100644 index 000000000..af3d798d7 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo-replacer/composer.json @@ -0,0 +1,12 @@ +{ + "name": "symlinked/path-pkg-replace", + "version": "2.0.0", + "require": { + "symlinked/transitive3-replaced": ">=1.0.3", + "symlinked/transitive4": "2.*", + "symlinked/transitive5-replaced": "2.*" + }, + "replace": { + "symlinked/replaced-pkg": "2.0.0" + } +} diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo-with-replaced-deps/composer.json b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo-with-replaced-deps/composer.json new file mode 100644 index 000000000..9737f9d81 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/symlinked-path-repo-with-replaced-deps/composer.json @@ -0,0 +1,9 @@ +{ + "name": "symlinked/path-pkg", + "version": "2.0.0", + "require": { + "symlinked/transitive": "2.*", + "symlinked/transitive3-replaced": "2.*", + "symlinked/transitive5-replaced": ">=1.0.3" + } +} From 4352f23962e98a2c7e6be5d36781b516e60d6ac9 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Fri, 12 Nov 2021 23:25:06 +0100 Subject: [PATCH 6/7] Fix implementation & tweak test --- .../DependencyResolver/PoolBuilder.php | 56 +++++++++++++------ ...th-repo-replacer-with-transitive-deps.test | 12 ++-- ...-always-but-not-their-transitive-deps.test | 12 ++++ .../DependencyResolver/PoolBuilderTest.php | 4 +- 4 files changed, 60 insertions(+), 24 deletions(-) diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index b421af96a..0a81bdde9 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -165,9 +165,18 @@ class PoolBuilder foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) { if (!$this->isUpdateAllowed($lockedPackage)) { + $lockedName = $lockedPackage->getName(); + + // remember which packages we skipped loading remote content for in this partial update + $this->skippedLoad[$lockedPackage->getName()][] = $lockedPackage; + foreach ($lockedPackage->getReplaces() as $link) { + $this->skippedLoad[$link->getTarget()][] = $lockedPackage; + } + // Path repo packages are never loaded from lock, to force them to always remain in sync // unless symlinking is disabled in which case we probably should rather treat them like - // regular packages + // regular packages. We mark them specially so they can be reloaded fully including update propagation + // if they do get unlocked, but by default they are unlocked without update propagation. if ($lockedPackage->getDistType() === 'path') { $transportOptions = $lockedPackage->getTransportOptions(); if (!isset($transportOptions['symlink']) || $transportOptions['symlink'] !== false) { @@ -177,11 +186,6 @@ class PoolBuilder } $request->lockPackage($lockedPackage); - $this->skippedLoad[$lockedPackage->getName()][] = $lockedPackage; - // remember which packages we skipped loading remote content for in this partial update - foreach ($lockedPackage->getReplaces() as $link) { - $this->skippedLoad[$link->getTarget()][] = $lockedPackage; - } } } } @@ -204,7 +208,7 @@ class PoolBuilder || $package->getRepository() instanceof PlatformRepository || StabilityFilter::isPackageAcceptable($this->acceptableStabilities, $this->stabilityFlags, $package->getNames(), $package->getStability()) ) { - $this->loadPackage($request, $package, false); + $this->loadPackage($request, $repositories, $package, false); } else { $this->unacceptableFixedOrLockedPackages[] = $package; } @@ -354,7 +358,7 @@ class PoolBuilder * @param RepositoryInterface[] $repositories * @return void */ - private function loadPackagesMarkedForLoading(Request $request, $repositories) + private function loadPackagesMarkedForLoading(Request $request, array $repositories) { foreach ($this->packagesToLoad as $name => $constraint) { $this->loadedPackages[$name] = $constraint; @@ -381,16 +385,17 @@ class PoolBuilder } foreach ($result['packages'] as $package) { $this->loadedPerRepo[$repoIndex][$package->getName()][$package->getVersion()] = $package; - $this->loadPackage($request, $package, !isset($this->pathRepoUnlocked[$package->getName()])); + $this->loadPackage($request, $repositories, $package, !isset($this->pathRepoUnlocked[$package->getName()])); } } } /** * @param bool $propagateUpdate + * @param RepositoryInterface[] $repositories * @return void */ - private function loadPackage(Request $request, BasePackage $package, $propagateUpdate) + private function loadPackage(Request $request, array $repositories, BasePackage $package, $propagateUpdate) { $index = $this->indexCounter++; $this->packages[$index] = $package; @@ -445,7 +450,7 @@ class PoolBuilder $skippedRootRequires = $this->getSkippedRootRequires($request, $require); if ($request->getUpdateAllowTransitiveRootDependencies() || !$skippedRootRequires) { - $this->unlockPackage($request, $require); + $this->unlockPackage($request, $repositories, $require); $this->markPackageNameForLoading($request, $require, $linkConstraint); } else { foreach ($skippedRootRequires as $rootRequire) { @@ -470,7 +475,7 @@ class PoolBuilder $skippedRootRequires = $this->getSkippedRootRequires($request, $replace); if ($request->getUpdateAllowTransitiveRootDependencies() || !$skippedRootRequires) { - $this->unlockPackage($request, $replace); + $this->unlockPackage($request, $repositories, $replace); $this->markPackageNameForLoading($request, $replace, $link->getConstraint()); } else { foreach ($skippedRootRequires as $rootRequire) { @@ -588,10 +593,11 @@ class PoolBuilder * Reverts the decision to use a locked package if a partial update with transitive dependencies * found that this package actually needs to be updated * + * @param RepositoryInterface[] $repositories * @param string $name * @return void */ - private function unlockPackage(Request $request, $name) + private function unlockPackage(Request $request, array $repositories, $name) { foreach ($this->skippedLoad[$name] as $packageOrReplacer) { // if we unfixed a replaced package name, we also need to unfix the replacer itself @@ -599,7 +605,7 @@ class PoolBuilder if ($packageOrReplacer->getName() !== $name && isset($this->skippedLoad[$packageOrReplacer->getName()])) { $replacerName = $packageOrReplacer->getName(); if ($request->getUpdateAllowTransitiveRootDependencies() || (!$this->isRootRequire($request, $name) && !$this->isRootRequire($request, $replacerName))) { - $this->unlockPackage($request, $replacerName); + $this->unlockPackage($request, $repositories, $replacerName); if ($this->isRootRequire($request, $replacerName)) { $this->markPackageNameForLoading($request, $replacerName, new MatchAllConstraint); @@ -615,6 +621,14 @@ class PoolBuilder } } + if (isset($this->pathRepoUnlocked[$name])) { + foreach ($this->packages as $index => $package) { + if ($package->getName() === $name) { + $this->removeLoadedPackage($request, $repositories, $package, $index, false); + } + } + } + unset($this->skippedLoad[$name], $this->loadedPackages[$name], $this->maxExtendedReqs[$name], $this->pathRepoUnlocked[$name]); // remove locked package by this name which was already initialized @@ -622,7 +636,7 @@ class PoolBuilder if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) { if (false !== $index = array_search($lockedPackage, $this->packages, true)) { $request->unlockPackage($lockedPackage); - $this->removeLoadedPackage($request, $lockedPackage, $index); + $this->removeLoadedPackage($request, $repositories, $lockedPackage, $index, true); // 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 @@ -645,15 +659,23 @@ class PoolBuilder } /** + * @param RepositoryInterface[] $repositories * @param int $index + * @param bool $unlock * @return void */ - private function removeLoadedPackage(Request $request, BasePackage $package, $index) + private function removeLoadedPackage(Request $request, array $repositories, BasePackage $package, $index, $unlock) { + $repoIndex = array_search($package->getRepository(), $repositories, true); + + unset($this->loadedPerRepo[$repoIndex][$package->getName()][$package->getVersion()]); unset($this->packages[$index]); if (isset($this->aliasMap[spl_object_hash($package)])) { foreach ($this->aliasMap[spl_object_hash($package)] as $aliasIndex => $aliasPackage) { - $request->unlockPackage($aliasPackage); + if ($unlock) { + $request->unlockPackage($aliasPackage); + } + unset($this->loadedPerRepo[$repoIndex][$aliasPackage->getName()][$aliasPackage->getVersion()]); unset($this->packages[$aliasIndex]); } unset($this->aliasMap[spl_object_hash($package)]); diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repo-replacer-with-transitive-deps.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repo-replacer-with-transitive-deps.test index 54cb48b83..4fc0f4a7b 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repo-replacer-with-transitive-deps.test +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repo-replacer-with-transitive-deps.test @@ -6,7 +6,8 @@ Partially updating with deps a root requirement which depends on packages in a s "require": { "root/update": "*", "symlinked/transitive3": "*", - "symlinked/transitive5": "*" + "symlinked/transitive5": "*", + "symlinked/path-pkg-replace": "*" }, "locked": [ {"name": "root/update", "version": "1.0.1", "require": {"symlinked/path-pkg": ">=1.0.1", "symlinked/replaced-pkg": "*"}}, @@ -69,14 +70,15 @@ Partially updating with deps a root requirement which depends on packages in a s --EXPECT-- [ - "symlinked/transitive-1.0.0.0", - "symlinked/transitive-2.0.2.0", - "symlinked/path-pkg-2.0.0.0", "root/update-1.0.4.0", + "symlinked/path-pkg-2.0.0.0", + "symlinked/path-pkg-replace-2.0.0.0", + "symlinked/transitive-2.0.2.0", + "symlinked/transitive3-1.0.0.0", "symlinked/transitive3-1.0.3.0", "symlinked/transitive3-2.0.4.0", - "symlinked/transitive4-1.0.0.0", "symlinked/transitive4-2.0.2.0", + "symlinked/transitive5-1.0.0.0", "symlinked/transitive5-1.0.3.0", "symlinked/transitive5-2.0.4.0" ] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test index 8e7c8068f..10f2d7fd5 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test @@ -77,3 +77,15 @@ Partially updating one root requirement with transitive deps fully updates trans "mirrored/transitive2-1.0.7.0", "mirrored/transitive2-2.0.8.0" ] + +--EXPECT-OPTIMIZED-- +[ + "symlinked/transitive-1.0.0.0 (locked)", + "mirrored/transitive-1.0.0.0 (locked)", + "mirrored/path-pkg-1.0.0.0 (locked)", + "symlinked/path-pkg-2.0.0.0", + "root/update-1.0.4.0", + "symlinked/transitive2-2.0.4.0", + "mirrored/transitive2-1.0.7.0", + "mirrored/transitive2-2.0.8.0" +] diff --git a/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php index 6be0d3016..58517af28 100644 --- a/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php +++ b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php @@ -140,6 +140,8 @@ class PoolBuilderTest extends TestCase $optimizer = new PoolOptimizer(new DefaultPolicy()); $result = $this->getPackageResultSet($optimizer->optimize($request, $pool), $packageIds); $this->assertSame($expectOptimized, $result, 'Optimized pool does not match expected package set'); + + chdir($oldCwd); } /** @@ -153,8 +155,6 @@ class PoolBuilderTest extends TestCase $result[] = $pool->packageById($i); } - chdir($oldCwd); - return array_map(function (BasePackage $package) use ($packageIds) { if ($id = array_search($package, $packageIds, true)) { return $id; From a6fc1ab663c08240c324b3b8ee463d16c4ed75f8 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Thu, 25 Nov 2021 15:18:24 +0100 Subject: [PATCH 7/7] Fix feedback --- src/Composer/DependencyResolver/PoolBuilder.php | 12 +++--------- .../partial-update-with-symlinked-path-repos.test | 9 ++------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 0a81bdde9..5d9f2e325 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -165,8 +165,6 @@ class PoolBuilder foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) { if (!$this->isUpdateAllowed($lockedPackage)) { - $lockedName = $lockedPackage->getName(); - // remember which packages we skipped loading remote content for in this partial update $this->skippedLoad[$lockedPackage->getName()][] = $lockedPackage; foreach ($lockedPackage->getReplaces() as $link) { @@ -624,7 +622,7 @@ class PoolBuilder if (isset($this->pathRepoUnlocked[$name])) { foreach ($this->packages as $index => $package) { if ($package->getName() === $name) { - $this->removeLoadedPackage($request, $repositories, $package, $index, false); + $this->removeLoadedPackage($request, $repositories, $package, $index); } } } @@ -636,7 +634,7 @@ class PoolBuilder if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) { if (false !== $index = array_search($lockedPackage, $this->packages, true)) { $request->unlockPackage($lockedPackage); - $this->removeLoadedPackage($request, $repositories, $lockedPackage, $index, true); + $this->removeLoadedPackage($request, $repositories, $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 @@ -661,10 +659,9 @@ class PoolBuilder /** * @param RepositoryInterface[] $repositories * @param int $index - * @param bool $unlock * @return void */ - private function removeLoadedPackage(Request $request, array $repositories, BasePackage $package, $index, $unlock) + private function removeLoadedPackage(Request $request, array $repositories, BasePackage $package, $index) { $repoIndex = array_search($package->getRepository(), $repositories, true); @@ -672,9 +669,6 @@ class PoolBuilder unset($this->packages[$index]); if (isset($this->aliasMap[spl_object_hash($package)])) { foreach ($this->aliasMap[spl_object_hash($package)] as $aliasIndex => $aliasPackage) { - if ($unlock) { - $request->unlockPackage($aliasPackage); - } unset($this->loadedPerRepo[$repoIndex][$aliasPackage->getName()][$aliasPackage->getVersion()]); unset($this->packages[$aliasIndex]); } diff --git a/tests/Composer/Test/Fixtures/installer/partial-update-with-symlinked-path-repos.test b/tests/Composer/Test/Fixtures/installer/partial-update-with-symlinked-path-repos.test index 79a6b8b3c..2ea842613 100644 --- a/tests/Composer/Test/Fixtures/installer/partial-update-with-symlinked-path-repos.test +++ b/tests/Composer/Test/Fixtures/installer/partial-update-with-symlinked-path-repos.test @@ -29,6 +29,7 @@ Partially updating one root requirement with transitive deps fully updates trans "mirrored/path-pkg": "*" } } + --LOCK-- { "packages": [ @@ -66,13 +67,7 @@ Partially updating one root requirement with transitive deps fully updates trans "platform": [], "platform-dev": [] } ---INSTALLED-- -[ - { "name": "a/old", "version": "0.9.0" }, - { "name": "b/unstable", "version": "1.1.0-alpha", "require": {"f/dependency": "1.*"} }, - { "name": "c/uptodate", "version": "2.0.0" }, - { "name": "f/dependency", "version": "1.0.0" } -] + --RUN-- update --with-all-dependencies root/update