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.

Sign up for GitHub

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

Jump to bottom

Re-organized PoolOptimizer - reverting #9620 to fix #9393 #11766

Closed
wants to merge 1 commit into from
Closed

Re-organized PoolOptimizer - reverting #9620 to fix #9393 #11766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
108 changes: 11 additions & 97 deletions src/Composer/DependencyResolver/PoolOptimizer.php
View file
Open in desktop
Expand Up @@ -48,11 +48,6 @@ class PoolOptimizer
*/
private $conflictConstraintsPerPackage = [];

/**
* @var array<int, true>
*/
private $packagesToRemove = [];

/**
* @var array<int, BasePackage[]>
*/
Expand All @@ -74,8 +69,6 @@ public function optimize(Request $request, Pool $pool): Pool

$this->optimizeByIdenticalDependencies($request, $pool);

$this->optimizeImpossiblePackagesAway($request, $pool);

$optimizedPool = $this->applyRemovalsToPool($pool);

// No need to run this recursively at the moment
Expand All @@ -86,7 +79,6 @@ public function optimize(Request $request, Pool $pool): Pool
$this->irremovablePackages = [];
$this->requireConstraintsPerPackage = [];
$this->conflictConstraintsPerPackage = [];
$this->packagesToRemove = [];
$this->aliasesPerPackage = [];
$this->removedVersionsByPackage = [];

Expand Down Expand Up @@ -145,6 +137,11 @@ private function prepare(Request $request, Pool $pool): void

private function markPackageIrremovable(BasePackage $package): void
{
// Already marked to keep
if (isset($this->irremovablePackages[$package->id])) {
return;
}

$this->irremovablePackages[$package->id] = true;
if ($package instanceof AliasPackage) {
// recursing here so aliasesPerPackage for the aliasOf can be checked
Expand All @@ -166,7 +163,7 @@ private function applyRemovalsToPool(Pool $pool): Pool
$packages = [];
$removedVersions = [];
foreach ($pool->getPackages() as $package) {
if (!isset($this->packagesToRemove[$package->id])) {
if (isset($this->irremovablePackages[$package->id])) {
$packages[] = $package;
} else {
$removedVersions[$package->getName()][$package->getVersion()] = $package->getPrettyVersion();
Expand All @@ -191,8 +188,6 @@ private function optimizeByIdenticalDependencies(Request $request, Pool $pool):
continue;
}

$this->markPackageForRemoval($package->id);

$dependencyHash = $this->calculateDependencyHash($package);

foreach ($package->getNames(false) as $packageName) {
Expand Down Expand Up @@ -240,7 +235,7 @@ private function optimizeByIdenticalDependencies(Request $request, Pool $pool):
foreach ($constraintGroup as $packages) {
// Only one package in this constraint group has the same requirements, we're not allowed to remove that package
if (1 === \count($packages)) {
$this->keepPackage($packages[0], $identicalDefinitionsPerPackage, $packageIdenticalDefinitionLookup);
$this->keepPackageWithinDependencyGroups($packages[0], $identicalDefinitionsPerPackage, $packageIdenticalDefinitionLookup);
continue;
}

Expand All @@ -253,7 +248,7 @@ private function optimizeByIdenticalDependencies(Request $request, Pool $pool):
}

foreach ($this->policy->selectPreferredPackages($pool, $literals) as $preferredLiteral) {
$this->keepPackage($pool->literalToPackage($preferredLiteral), $identicalDefinitionsPerPackage, $packageIdenticalDefinitionLookup);
$this->keepPackageWithinDependencyGroups($pool->literalToPackage($preferredLiteral), $identicalDefinitionsPerPackage, $packageIdenticalDefinitionLookup);
}
}
}
Expand Down Expand Up @@ -300,33 +295,18 @@ private function calculateDependencyHash(BasePackage $package): string
return $hash;
}

private function markPackageForRemoval(int $id): void
{
// We are not allowed to remove packages if they have been marked as irremovable
if (isset($this->irremovablePackages[$id])) {
throw new \LogicException('Attempted removing a package which was previously marked irremovable');
}

$this->packagesToRemove[$id] = true;
}

/**
* @param array<string, array<string, array<string, list<BasePackage>>>> $identicalDefinitionsPerPackage
* @param array<int, array<string, array{groupHash: string, dependencyHash: string}>> $packageIdenticalDefinitionLookup
*/
private function keepPackage(BasePackage $package, array $identicalDefinitionsPerPackage, array $packageIdenticalDefinitionLookup): void
private function keepPackageWithinDependencyGroups(BasePackage $package, array $identicalDefinitionsPerPackage, array $packageIdenticalDefinitionLookup): void
{
// Already marked to keep
if (!isset($this->packagesToRemove[$package->id])) {
return;
}

unset($this->packagesToRemove[$package->id]);
$this->markPackageIrremovable($package);

if ($package instanceof AliasPackage) {
// recursing here so aliasesPerPackage for the aliasOf can be checked
// and all its aliases marked to be kept as well
$this->keepPackage($package->getAliasOf(), $identicalDefinitionsPerPackage, $packageIdenticalDefinitionLookup);
$this->keepPackageWithinDependencyGroups($package->getAliasOf(), $identicalDefinitionsPerPackage, $packageIdenticalDefinitionLookup);
}

// record all the versions of the package group so we can list them later in Problem output
Expand All @@ -345,8 +325,6 @@ private function keepPackage(BasePackage $package, array $identicalDefinitionsPe

if (isset($this->aliasesPerPackage[$package->id])) {
foreach ($this->aliasesPerPackage[$package->id] as $aliasPackage) {
unset($this->packagesToRemove[$aliasPackage->id]);

// record all the versions of the package group so we can list them later in Problem output
foreach ($aliasPackage->getNames(false) as $name) {
if (isset($packageIdenticalDefinitionLookup[$aliasPackage->id][$name])) {
Expand All @@ -364,70 +342,6 @@ private function keepPackage(BasePackage $package, array $identicalDefinitionsPe
}
}

/**
* Use the list of locked packages to constrain the loaded packages
* This will reduce packages with significant numbers of historical versions to a smaller number
* and reduce the resulting rule set that is generated
*/
private function optimizeImpossiblePackagesAway(Request $request, Pool $pool): void
{
if (count($request->getLockedPackages()) === 0) {
return;
}

$packageIndex = [];

foreach ($pool->getPackages() as $package) {
$id = $package->id;

// Do not remove irremovable packages
if (isset($this->irremovablePackages[$id])) {
continue;
}
// Do not remove a package aliased by another package, nor aliases
if (isset($this->aliasesPerPackage[$id]) || $package instanceof AliasPackage) {
continue;
}
// Do not remove locked packages
if ($request->isFixedPackage($package) || $request->isLockedPackage($package)) {
continue;
}

$packageIndex[$package->getName()][$package->id] = $package;
}

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
$isUnusedPackage = true;
Copy link
Contributor

@driskell driskell Dec 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Toflar It's a weird one, I think originally I did miss this. I found the update: #10405

So something perhaps went wrong at #10405. Was test coverage perhaps with a gap?

In #9393 it looks like it would need this code - to not apply the constraints from a package if it's no longer required. Perhaps when it got moved to the optimisation step and started using requireConstraintsPerPackage it changed something.

I can see if I can find some time soon to relook at this in this location. But I think it's this specific 7 lines of code that is failing us 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth dropping it out for now. I think requireConstraintsPerPackage is including requirements of each package added to the pool so if multiple versions got added of something and one of those versions references a package that would still be removed it likely then optimises away. One for a separate PR with a better test.

Is it possible to generate a failing test for this for future attempts?

I think at a guess it needs a package in the pool with two versions, one of those that would be installed, the other that references a require on a package that is going to be removed. Then that package that is going to be removed has a constraint that can't be met.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to generate a failing test for this for future attempts?

Yeah, I think I know exactly why this is happening. I just need some time to come up with a reproducer test but I do think we can also keep the existing locked packages optimizer, I just have to sit down and work on it 😇

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've described the issue here: #9393 (comment) - relying on requireConstraintsPerPackage is exactly the problem because that does never contain the packages that will be required but more than that. Hence, removing packages based on that can always fail.

foreach ($package->getNames(false) as $packageName) {
if (isset($this->requireConstraintsPerPackage[$packageName])) {
$isUnusedPackage = false;
break;
}
}

if ($isUnusedPackage) {
continue;
}

foreach ($package->getRequires() as $link) {
$require = $link->getTarget();
if (!isset($packageIndex[$require])) {
continue;
}

$linkConstraint = $link->getConstraint();
foreach ($packageIndex[$require] as $id => $requiredPkg) {
if (false === CompilingMatcher::match($linkConstraint, Constraint::OP_EQ, $requiredPkg->getVersion())) {
$this->markPackageForRemoval($id);
unset($packageIndex[$require][$id]);
}
}
}
}
}

/**
* Disjunctive require constraints need to be considered in their own group. E.g. "^2.14 || ^3.3" needs to generate
* two require constraint groups in order for us to keep the best matching package for "^2.14" AND "^3.3" as otherwise, we'd
Expand Down
3 changes: 2 additions & 1 deletion ...t/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages-locked-replacer.test
View file
Open in desktop
Expand Up @@ -56,5 +56,6 @@ Do not load packages into the pool that cannot meet the fixed/locked requirement
"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"
"replacer/dep-1.0.1.0",
"replacer/dep-2.0.0.0"
]
1 change: 1 addition & 0 deletions tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/filter-impossible-packages.test
View file
Open in desktop
Expand Up @@ -51,5 +51,6 @@ Do not load packages into the pool that cannot meet the fixed/locked requirement
[
2,
"some/pkg-1.0.4.0",
"dep/dep-1.0.2.0",
"dep/dep-2.0.1.0"
]
3 changes: 2 additions & 1 deletion ...s/poolbuilder/partial-update-unfixes-path-repos-always-but-not-their-transitive-deps.test
View file
Open in desktop
Expand Up @@ -86,5 +86,6 @@ Partially updating one root requirement with transitive deps fully updates trans
"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-1.0.7.0",
"mirrored/transitive2-2.0.8.0"
]
3 changes: 2 additions & 1 deletion ...ser/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test
View file
Open in desktop
Expand Up @@ -55,5 +55,6 @@ locked packages still need to be taking into account for loading all necessary v
"root/req2-1.0.0.0 (locked)",
"dep/pkg2-1.0.0.0",
"dep/pkg2-1.2.0.0",
"dep/pkg1-1.0.1.0"
"dep/pkg1-1.0.1.0",
"dep/pkg1-2.0.0.0"
]