Re-organized PoolOptimizer - reverting #9620 to fix #9393 #11766
Conversation
|
We certainly also need to add a reproducer test for #9393 - I'll try to work on that. |
| 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; |
There was a problem hiding this comment.
@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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😇
There was a problem hiding this comment.
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.
|
See #11772 for the test. |
Imho this should fix #9393 by reverting #9620.
The problem of #9620 is that it marks a package for removal although this has been previously marked to be kept. This causes the bug described in #9393. While debugging, I noticed that understanding the logic in the
PoolOptimizeris not as easy because it's designed toThis was fine because it was the way it was initially designed when I first contributed the optimizer in #9261 which only consisted of
optimizeByIdenticalDependencies().However, #9393 introduced a second optimization step (
optimizeImpossiblePackagesAway()) in which additional packages are marked for removal even if the first step marked them as to be kept - causing the mentioned bug.So what I did in this PR is invert the logic:
@driskell - maybe you could try to re-implement your logic of #9620 on top of this PR? Basically instead of
markPackageForRemoval()they should not even be marked as required to keep.