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
Load only explicitly named package data #3994
Conversation
| @@ -96,6 +96,151 @@ public function setRootAliases(array $rootAliases) | |||
| $this->rootAliases = $rootAliases; | |||
| } | |||
|
|
|||
| public function ensureLoaded($pool, array $constrainedNames) | |||
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.
$pool should be typehinted
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.
I don't mean to leave any of this code as-is, this was a POC to see if this is doable, I'll rewrite all of this anyway.
| $packages = $this->loadName($pool, $packageSpec['name']); | ||
|
|
||
| foreach ($packages as $package) { | ||
| foreach ($package->getRequires() as $link) { |
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.
shouldn't this be handled in the Pool to ensure that dependencies can be provided by other repositories ?
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.
please read my reply :(
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.
and what about dev requirements for the root package ?
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.
@naderman your replies don't appear in the diff of the PR until I reload the diff 😄
|
Something looks broken. Tests are not passing |
|
Yes because the current list of loaded packages doesn't actually contain everything that is needed. |
|
Again this is really just a FYI PR that I was discussing with jordi which I didn't want to just live on my disk, no need to discuss this yet. |
|
current composer: with this patch which still loads full provider files, and only reduces the data after it is downloaded: So pretty confident this can make a significant performance impact if implemented properly. |
caf91b4
to
ec5416f
Compare
| return $this->rootAliases; | ||
| } | ||
|
|
||
| public function loadRecursively(array $packageNames, $acceptableCallback) |
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.
please add some phpdoc to document that the second argument is a callable, as we cannot typehint it
|
Alright looking good, it reduces memory footprint a little and runtime as well since we process less files so that means less network time too which is nice. A good side effect though is that it moves all the loading ahead of the solver, so there won't be confusion anymore as to what it's doing between downloading two files, now they should download quick and then solving starts. |
|
@Seldaek @naderman After this was merged I've started to get errors when using private (Satis) and public packages together. Reverting composer to a version right before this change (b025d09) fixes this. |
|
Do you have capital letters in the name of packages in your Satis repo ? |
|
Nope, no capital letters in package names. There are capital letters in some version names though (branch names) - not sure if that matters. |
|
nope, it should not |
|
Well the issue seems to be that for some reason some packages are not being loaded. Since this is multiple repositories I figure there is some issue in that section of the code, that tries to load names it found as deps in one repo in another. |
|
Would be great if we had a test for this ^_^ Locally just using a satis repo works fine. |
|
Going to revert this, while I debug, as this may take a while. |
This will significantly reduce the amount of loaded metadata for the solver. This should improve network time as well as solver complexity.