Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize PoolBuilder to not load replaced targets if not required #11449

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 43 additions & 2 deletions src/Composer/DependencyResolver/PoolBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ class PoolBuilder
* @phpstan-var array<int, array<string, array<string, PackageInterface>>>
*/
private $loadedPerRepo = [];
/**
* @var array[]
* @phpstan-var array<string, bool>
*/
private $optionalPackages = [];
/**
* @var BasePackage[]
*/
Expand Down Expand Up @@ -235,8 +240,9 @@ public function buildPool(array $repositories, Request $request): Pool
}
}

while (!empty($this->packagesToLoad)) {
while ([] !== $this->packagesToLoad || [] !== $this->optionalPackages) {
$this->loadPackagesMarkedForLoading($request, $repositories);
$this->loadOptionalPackages($request);
}

if (\count($this->temporaryConstraints) > 0) {
Expand Down Expand Up @@ -483,7 +489,8 @@ private function loadPackage(Request $request, array $repositories, BasePackage

if ($request->getUpdateAllowTransitiveRootDependencies() || !$skippedRootRequires) {
$this->unlockPackage($request, $repositories, $replace);
$this->markPackageNameForLoading($request, $replace, $link->getConstraint());
// Mark as optional - if no other package requires it, we don't need to load it
$this->markPackageNameForOptionalLoading($replace);
} else {
foreach ($skippedRootRequires as $rootRequire) {
if (!isset($this->updateAllowWarned[$rootRequire])) {
Expand Down Expand Up @@ -639,6 +646,8 @@ private function unlockPackage(Request $request, array $repositories, string $na
// 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
// and if this package is replacing another that is required by a locked or fixed package, ensure
// that we load that replaced package in case an update to this package removes the replacement
foreach ($request->getFixedOrLockedPackages() as $fixedOrLockedPackage) {
if ($fixedOrLockedPackage === $lockedPackage) {
continue;
Expand All @@ -649,13 +658,45 @@ private function unlockPackage(Request $request, array $repositories, string $na
if (isset($requires[$lockedPackage->getName()])) {
$this->markPackageNameForLoading($request, $lockedPackage->getName(), $requires[$lockedPackage->getName()]->getConstraint());
}

foreach ($lockedPackage->getReplaces() as $replace) {
if (isset($requires[$replace->getTarget()], $this->skippedLoad[$replace->getTarget()])) {
$this->unlockPackage($request, $repositories, $replace->getTarget());
// Do not call markPackageNameForOptionalLoading() here, we know that $lockedPackage is already
// part of $this->packages, and we check for $requires[$replace->getTarget()] so we're guaranteed
// to require this package.
$this->markPackageNameForLoading($request, $replace->getTarget(), $replace->getConstraint());
}
}
}
}
}
}
}
}

private function markPackageNameForOptionalLoading(string $name): void
{
$this->optionalPackages[$name] = true;
}

private function loadOptionalPackages(Request $request): void
{
if ([] === $this->optionalPackages) {
return;
}

foreach ($this->packages as $package) {
foreach ($package->getRequires() as $link) {
if (isset($this->optionalPackages[$link->getTarget()])) {
$this->markPackageNameForLoading($request, $link->getTarget(), $link->getConstraint());
}
}
}

$this->optionalPackages = [];
}

/**
* @param RepositoryInterface[] $repositories
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
--TEST--
Ensure that a package gets loaded which was previously skipped due to replacement

--REQUEST--
{
"require": {
"root/dep": "*",
"root/no-update": "*"
},
"locked": [
{"name": "root/dep", "version": "1.1.0", "require": {"replacer/pkg": "1.*"}},
{"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}},
{"name": "root/no-update", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}
],
"allowList": [
"root/dep"
],
"allowTransitiveDepsNoRootRequire": true
}

--FIXED--
[
]

--PACKAGE-REPOS--
[
[
{"name": "root/dep", "version": "1.2.0", "require": {"replacer/pkg": ">=1.1.0"}},
{"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}},
{"name": "replacer/pkg", "version": "1.1.0"},
{"name": "replaced/pkg", "version": "1.0.0"},
{"name": "root/no-update", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}
]
]

--EXPECT--
[
"root/no-update-1.0.0.0 (locked)",
"root/dep-1.2.0.0",
"replaced/pkg-1.0.0.0",
"replacer/pkg-1.1.0.0"
]

--EXPECT-OPTIMIZED--
[
"root/no-update-1.0.0.0 (locked)",
"root/dep-1.2.0.0",
"replaced/pkg-1.0.0.0",
"replacer/pkg-1.1.0.0"
]
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ Check that replacers from additional repositories are loaded when doing a partia
"indirect/replacer-1.0.0.0",
"replacer/package-1.2.0.0",
"replacer/package-1.0.0.0",
"base/package-1.0.0.0",
"shared/dep-1.0.0.0",
"shared/dep-1.2.0.0"
]
Expand All @@ -112,6 +111,5 @@ Check that replacers from additional repositories are loaded when doing a partia
"indirect/replacer-1.0.0.0",
"replacer/package-1.2.0.0",
"replacer/package-1.0.0.0",
"base/package-1.0.0.0",
"shared/dep-1.2.0.0"
]
4 changes: 4 additions & 0 deletions tests/Composer/Test/DependencyResolver/PoolBuilderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,14 @@ public function testPoolBuilder(string $file, string $message, array $expect, ar

$result = $this->getPackageResultSet($pool, $packageIds);

sort($expect);
sort($result);
$this->assertSame($expect, $result, 'Unoptimized pool does not match expected package set');

$optimizer = new PoolOptimizer(new DefaultPolicy());
$result = $this->getPackageResultSet($optimizer->optimize($request, $pool), $packageIds);
sort($expectOptimized);
sort($result);
$this->assertSame($expectOptimized, $result, 'Optimized pool does not match expected package set');

chdir($oldCwd);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
--TEST--
Ensure that a package gets loaded which was previously skipped due to replacement
--COMPOSER--
{
"repositories": [
{
"type": "package",
"package": [
{"name": "root/dep", "version": "1.2.0", "require": {"replacer/pkg": ">=1.1.0"}},
{"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}},
{"name": "replacer/pkg", "version": "1.1.0"},
{"name": "replaced/pkg", "version": "1.0.0"},
{"name": "root/no-update", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}
]
}
],
"require": {
"root/dep": "*",
"root/no-update": "*"
}
}
--LOCK--
{
"packages": [
{"name": "root/dep", "version": "1.1.0", "require": {"replacer/pkg": "1.*"}},
{"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}},
{"name": "root/no-update", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}
],
"packages-dev": [],
"aliases": [],
"minimum-stability": "dev",
"stability-flags": [],
"prefer-stable": false,
"prefer-lowest": false,
"platform": [],
"platform-dev": []
}
--RUN--
update root/dep --with-all-dependencies
--EXPECT--
Installing replacer/pkg (1.1.0)
Installing root/dep (1.2.0)
Installing replaced/pkg (1.0.0)
Installing root/no-update (1.0.0)