-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Failing test to outline problem in #9393 #11772
base: main
Are you sure you want to change the base?
Conversation
1, | ||
2, | ||
"drupal/core-9.3.1.0", | ||
"drupal/core-10.2.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line here is the difference. We must expect drupal/core-10.2.0.0
to be present but it's removed.
Wonder if instead of changing much here, we can just trim anything from the lockfile - before starting optimizing - that isn't required by anything still required on a root level in composer.json? Would that just make the current solution work? |
I just think the general idea of the The The first step of the The
true , right? Why would it even land in the Pool before optimizing if it's not required anywhere? But maybe that's exactly what you mean by trim anything from the lockfile - before starting optimizing ?
|
So in general the optimizer does have access to the request so that it may use it as input to draw conclusions, if it can be sure something won't possibly get picked by the solver later, it can already remove it. In a sense that's what the part you described does, too. It's not that it leaves all packages in the pool, it draws conclusions on what won't get picked anyway and removes it. Apart from that, yes, I think due to the lock file in a partial update case we get packages in the pool that potentially have no chance of getting installed anyway and could already get removed. |
Yeah what I was saying is that should they even end up being part of the pool in the first place. Aka should they not even get added in the |
Yeah can maybe already try to consider that there instead of in the optimizer. In the end the optimizer is really just a step of pool building too, so it doesn't really matter, just depends what's easier/more understandable. |
Okay, managed to debug exactly on what's happening after way too many hours 馃槄 Here's the exact reason on what's happening in the case described by @Seldaek in #9393 (comment):
drupal/webform
is locked at6.1.8.0
but allowed to be updated/removed because of-W
PoolOptimizer::optimizeImpossiblePackagesAway()
removesdrupal/core
10.2.0.0
becausedrupal/webform
is locked at6.1.8.0
and requiresdrupal/core
in^9.3
.The problem is that
drupal/webform
itself is going to be removed during the update process so actually, its requirements should not be considered and thus thedrupal/core
10.2.0.0
should not get removed.This exact case was actually sort of considered in the
$isUnusedPackage
here:composer/src/Composer/DependencyResolver/PoolOptimizer.php
Line 404 in 8e62977
However, this doesn't work properly because
$this->requireConstraintsPerPackage[$packageName])
actually does containdrupal/webform
because this array is built before any optimizations are even applied. It is required by the the locked packagedrupal/spamaway
2.0.0.0
which would also get removed.So the bug happens because the
$this->requireConstraintsPerPackage
(and probably other things) are used as if this array were to contain a definitive list of what is going to be required. Something that we never know at the stage of the PoolOptimizer as this is what the Solver will do - it's a typical problem of recursive optimizations.So long story short: The current optimization step for locked packages is flawed because it is built on top of false assumptions.