From dc88236c0795dd05368332970876bebea0581039 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Sun, 25 Oct 2020 13:49:31 +0100 Subject: [PATCH 1/4] Make sure operations are executed in batches, including downloads, when a plugin is present which modifies downloads, fixes #9333 --- .../Installer/InstallationManager.php | 158 +++++++++++------- 1 file changed, 97 insertions(+), 61 deletions(-) diff --git a/src/Composer/Installer/InstallationManager.php b/src/Composer/Installer/InstallationManager.php index a4c919b57..dc3711aac 100644 --- a/src/Composer/Installer/InstallationManager.php +++ b/src/Composer/Installer/InstallationManager.php @@ -180,7 +180,6 @@ class InstallationManager */ public function execute(RepositoryInterface $repo, array $operations, $devMode = true, $runScripts = true) { - $promises = array(); $cleanupPromises = array(); $loop = $this->loop; @@ -234,61 +233,14 @@ class InstallationManager } try { - foreach ($operations as $index => $operation) { - $opType = $operation->getOperationType(); - - // ignoring alias ops as they don't need to execute anything at this stage - if (!in_array($opType, array('update', 'install', 'uninstall'))) { - continue; - } - - if ($opType === 'update') { - $package = $operation->getTargetPackage(); - $initialPackage = $operation->getInitialPackage(); - } else { - $package = $operation->getPackage(); - $initialPackage = null; - } - $installer = $this->getInstaller($package->getType()); - - $cleanupPromises[$index] = function () use ($opType, $installer, $package, $initialPackage) { - // avoid calling cleanup if the download was not even initialized for a package - // as without installation source configured nothing will work - if (!$package->getInstallationSource()) { - return; - } - - return $installer->cleanup($opType, $package, $initialPackage); - }; - - if ($opType !== 'uninstall') { - $promise = $installer->download($package, $initialPackage); - if ($promise) { - $promises[] = $promise; - } - } - } - - // execute all downloads first - if (!empty($promises)) { - $progress = null; - if ($this->outputProgress && $this->io instanceof ConsoleIO && !$this->io->isDebug() && count($promises) > 1) { - $progress = $this->io->getProgressBar(); - } - $this->loop->wait($promises, $progress); - if ($progress) { - $progress->clear(); - } - } - - // execute operations in batches to make sure every plugin is installed in the - // right order and activated before the packages depending on it are installed + // execute operations in batches to make sure download-modifying-plugins are installed + // before the other packages get downloaded $batches = array(); $batch = array(); foreach ($operations as $index => $operation) { if (in_array($operation->getOperationType(), array('update', 'install'), true)) { $package = $operation->getOperationType() === 'update' ? $operation->getTargetPackage() : $operation->getPackage(); - if ($package->getType() === 'composer-plugin' || $package->getType() === 'composer-installer') { + if ($package->getType() === 'composer-plugin' && $extra = $package->getExtra() && isset($extra['plugin-modifies-downloads']) && $extra['plugin-modifies-downloads'] === true) { if ($batch) { $batches[] = $batch; } @@ -306,7 +258,7 @@ class InstallationManager } foreach ($batches as $batch) { - $this->executeBatch($repo, $batch, $cleanupPromises, $devMode, $runScripts, $operations); + $this->downloadAndExecuteBatch($repo, $batch, $cleanupPromises, $devMode, $runScripts, $operations); } } catch (\Exception $e) { $runCleanup(); @@ -334,12 +286,91 @@ class InstallationManager $repo->write($devMode, $this); } + /** + * @param array $operations List of operations to execute in this batch + * @param array $allOperations Complete list of operations to be executed in the install job, used for event listeners + */ + private function downloadAndExecuteBatch(RepositoryInterface $repo, array $operations, array &$cleanupPromises, $devMode, $runScripts, array $allOperations) + { + $promises = array(); + + foreach ($operations as $index => $operation) { + $opType = $operation->getOperationType(); + + // ignoring alias ops as they don't need to execute anything at this stage + if (!in_array($opType, array('update', 'install', 'uninstall'))) { + continue; + } + + if ($opType === 'update') { + $package = $operation->getTargetPackage(); + $initialPackage = $operation->getInitialPackage(); + } else { + $package = $operation->getPackage(); + $initialPackage = null; + } + $installer = $this->getInstaller($package->getType()); + + $cleanupPromises[$index] = function () use ($opType, $installer, $package, $initialPackage) { + // avoid calling cleanup if the download was not even initialized for a package + // as without installation source configured nothing will work + if (!$package->getInstallationSource()) { + return; + } + + return $installer->cleanup($opType, $package, $initialPackage); + }; + + if ($opType !== 'uninstall') { + $promise = $installer->download($package, $initialPackage); + if ($promise) { + $promises[] = $promise; + } + } + } + + // execute all downloads first + if (count($promises)) { + $this->waitOnPromises($promises); + } + + // execute operations in batches to make sure every plugin is installed in the + // right order and activated before the packages depending on it are installed + $batches = array(); + $batch = array(); + foreach ($operations as $index => $operation) { + if (in_array($operation->getOperationType(), array('update', 'install'), true)) { + $package = $operation->getOperationType() === 'update' ? $operation->getTargetPackage() : $operation->getPackage(); + if ($package->getType() === 'composer-plugin' || $package->getType() === 'composer-installer') { + if ($batch) { + $batches[] = $batch; + } + $batches[] = array($index => $operation); + $batch = array(); + + continue; + } + } + $batch[$index] = $operation; + } + + if ($batch) { + $batches[] = $batch; + } + + foreach ($batches as $batch) { + $this->executeBatch($repo, $batch, $cleanupPromises, $devMode, $runScripts, $allOperations); + } + } + /** * @param array $operations List of operations to execute in this batch * @param array $allOperations Complete list of operations to be executed in the install job, used for event listeners */ private function executeBatch(RepositoryInterface $repo, array $operations, array $cleanupPromises, $devMode, $runScripts, array $allOperations) { + $promises = array(); + foreach ($operations as $index => $operation) { $opType = $operation->getOperationType(); @@ -397,15 +428,20 @@ class InstallationManager } // execute all prepare => installs/updates/removes => cleanup steps - if (!empty($promises)) { - $progress = null; - if ($this->outputProgress && $this->io instanceof ConsoleIO && !$this->io->isDebug() && count($promises) > 1) { - $progress = $this->io->getProgressBar(); - } - $this->loop->wait($promises, $progress); - if ($progress) { - $progress->clear(); - } + if (count($promises)) { + $this->waitOnPromises($promises); + } + } + + private function waitOnPromises(array $promises) + { + $progress = null; + if ($this->outputProgress && $this->io instanceof ConsoleIO && !$this->io->isDebug() && count($promises) > 1) { + $progress = $this->io->getProgressBar(); + } + $this->loop->wait($promises, $progress); + if ($progress) { + $progress->clear(); } } From 2d4e1e0dce257bde27fe6342ad7cadda2c8b549d Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Sun, 25 Oct 2020 14:06:45 +0100 Subject: [PATCH 2/4] Make sure Transaction sorts operations correctly to begin with --- .../DependencyResolver/Transaction.php | 29 ++++++++++++++++++- .../Installer/InstallationManager.php | 2 +- .../DependencyResolver/TransactionTest.php | 26 +++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/Composer/DependencyResolver/Transaction.php b/src/Composer/DependencyResolver/Transaction.php index 4b67b674e..09c3bc6f5 100644 --- a/src/Composer/DependencyResolver/Transaction.php +++ b/src/Composer/DependencyResolver/Transaction.php @@ -248,6 +248,9 @@ class Transaction */ private function movePluginsToFront(array $operations) { + $dlModyingPluginsNoDeps = array(); + $dlModyingPluginsWithDeps = array(); + $dlModyingPluginRequires = array(); $pluginsNoDeps = array(); $pluginsWithDeps = array(); $pluginRequires = array(); @@ -261,6 +264,30 @@ class Transaction continue; } + $isDownloadsModifyingPlugin = $package->getType() === 'composer-plugin' && ($extra = $package->getExtra()) && isset($extra['plugin-modifies-downloads']) && $extra['plugin-modifies-downloads'] === true; + + // is this a downloads modifying plugin or a dependency of one? + if ($isDownloadsModifyingPlugin || count(array_intersect($package->getNames(), $dlModyingPluginRequires))) { + // get the package's requires, but filter out any platform requirements + $requires = array_filter(array_keys($package->getRequires()), function ($req) { + return !PlatformRepository::isPlatformPackage($req); + }); + + // is this a plugin with no meaningful dependencies? + if ($isPlugin && !count($requires)) { + // plugins with no dependencies go to the very front + array_unshift($dlModyingPluginsNoDeps, $op); + } else { + // capture the requirements for this package so those packages will be moved up as well + $dlModyingPluginRequires = array_merge($dlModyingPluginRequires, $requires); + // move the operation to the front + array_unshift($dlModyingPluginsWithDeps, $op); + } + + unset($operations[$idx]); + continue; + } + // is this package a plugin? $isPlugin = $package->getType() === 'composer-plugin' || $package->getType() === 'composer-installer'; @@ -286,7 +313,7 @@ class Transaction } } - return array_merge($pluginsNoDeps, $pluginsWithDeps, $operations); + return array_merge($dlModyingPluginsNoDeps, $dlModyingPluginsWithDeps, $pluginsNoDeps, $pluginsWithDeps, $operations); } /** diff --git a/src/Composer/Installer/InstallationManager.php b/src/Composer/Installer/InstallationManager.php index dc3711aac..374e37e2d 100644 --- a/src/Composer/Installer/InstallationManager.php +++ b/src/Composer/Installer/InstallationManager.php @@ -240,7 +240,7 @@ class InstallationManager foreach ($operations as $index => $operation) { if (in_array($operation->getOperationType(), array('update', 'install'), true)) { $package = $operation->getOperationType() === 'update' ? $operation->getTargetPackage() : $operation->getPackage(); - if ($package->getType() === 'composer-plugin' && $extra = $package->getExtra() && isset($extra['plugin-modifies-downloads']) && $extra['plugin-modifies-downloads'] === true) { + if ($package->getType() === 'composer-plugin' && ($extra = $package->getExtra()) && isset($extra['plugin-modifies-downloads']) && $extra['plugin-modifies-downloads'] === true) { if ($batch) { $batches[] = $batch; } diff --git a/tests/Composer/Test/DependencyResolver/TransactionTest.php b/tests/Composer/Test/DependencyResolver/TransactionTest.php index 8599a5da6..39354c006 100644 --- a/tests/Composer/Test/DependencyResolver/TransactionTest.php +++ b/tests/Composer/Test/DependencyResolver/TransactionTest.php @@ -42,8 +42,28 @@ class TransactionTest extends TestCase $packageG = $this->getPackage('g/g', '1.0.0'), $packageA0first = $this->getPackage('a0/first', '1.2.3'), $packageFalias2 = $this->getAliasPackage($packageF, 'dev-bar'), + $plugin = $this->getPackage('x/plugin', '1.0.0'), + $plugin2Dep = $this->getPackage('x/plugin2-dep', '1.0.0'), + $plugin2 = $this->getPackage('x/plugin2', '1.0.0'), + $dlModifyingPlugin = $this->getPackage('x/downloads-modifying', '1.0.0'), + $dlModifyingPlugin2Dep = $this->getPackage('x/downloads-modifying2-dep', '1.0.0'), + $dlModifyingPlugin2 = $this->getPackage('x/downloads-modifying2', '1.0.0'), ); + $plugin->setType('composer-installer'); + foreach (array($plugin2, $dlModifyingPlugin, $dlModifyingPlugin2) as $pluginPackage) { + $pluginPackage->setType('composer-plugin'); + } + + $plugin2->setRequires(array( + 'x/plugin2-dep' => new Link('x/plugin2', 'x/plugin2-dep', $this->getVersionConstraint('=', '1.0.0'), Link::TYPE_REQUIRE), + )); + $dlModifyingPlugin2->setRequires(array( + 'x/downloads-modifying2-dep' => new Link('x/downloads-modifying2', 'x/downloads-modifying2-dep', $this->getVersionConstraint('=', '1.0.0'), Link::TYPE_REQUIRE), + )); + $dlModifyingPlugin->setExtra(array('plugin-modifies-downloads' => true)); + $dlModifyingPlugin2->setExtra(array('plugin-modifies-downloads' => true)); + $packageD->setRequires(array( 'f/f' => new Link('d/d', 'f/f', $this->getVersionConstraint('>', '0.2'), Link::TYPE_REQUIRE), 'g/provider' => new Link('d/d', 'g/provider', $this->getVersionConstraint('>', '0.2'), Link::TYPE_REQUIRE), @@ -54,6 +74,12 @@ class TransactionTest extends TestCase array('job' => 'uninstall', 'package' => $packageC), array('job' => 'uninstall', 'package' => $packageE), array('job' => 'markAliasUninstalled', 'package' => $packageEalias), + array('job' => 'install', 'package' => $dlModifyingPlugin), + array('job' => 'install', 'package' => $dlModifyingPlugin2Dep), + array('job' => 'install', 'package' => $dlModifyingPlugin2), + array('job' => 'install', 'package' => $plugin), + array('job' => 'install', 'package' => $plugin2Dep), + array('job' => 'install', 'package' => $plugin2), array('job' => 'install', 'package' => $packageA0first), array('job' => 'update', 'from' => $packageB, 'to' => $packageBnew), array('job' => 'install', 'package' => $packageG), From e770cb4dcf0c77b1d5bbbe145e5dc98aba3a78d1 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Sun, 25 Oct 2020 14:11:02 +0100 Subject: [PATCH 3/4] Fix var name --- src/Composer/DependencyResolver/Transaction.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Composer/DependencyResolver/Transaction.php b/src/Composer/DependencyResolver/Transaction.php index 09c3bc6f5..ecbeb87ac 100644 --- a/src/Composer/DependencyResolver/Transaction.php +++ b/src/Composer/DependencyResolver/Transaction.php @@ -274,7 +274,7 @@ class Transaction }); // is this a plugin with no meaningful dependencies? - if ($isPlugin && !count($requires)) { + if ($isDownloadsModifyingPlugin && !count($requires)) { // plugins with no dependencies go to the very front array_unshift($dlModyingPluginsNoDeps, $op); } else { From 769ce48289b79a3f55a792cae09b2a9011909524 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Sun, 25 Oct 2020 21:25:56 +0100 Subject: [PATCH 4/4] Fix typo --- src/Composer/DependencyResolver/Transaction.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Composer/DependencyResolver/Transaction.php b/src/Composer/DependencyResolver/Transaction.php index ecbeb87ac..3d662166a 100644 --- a/src/Composer/DependencyResolver/Transaction.php +++ b/src/Composer/DependencyResolver/Transaction.php @@ -248,9 +248,9 @@ class Transaction */ private function movePluginsToFront(array $operations) { - $dlModyingPluginsNoDeps = array(); - $dlModyingPluginsWithDeps = array(); - $dlModyingPluginRequires = array(); + $dlModifyingPluginsNoDeps = array(); + $dlModifyingPluginsWithDeps = array(); + $dlModifyingPluginRequires = array(); $pluginsNoDeps = array(); $pluginsWithDeps = array(); $pluginRequires = array(); @@ -267,7 +267,7 @@ class Transaction $isDownloadsModifyingPlugin = $package->getType() === 'composer-plugin' && ($extra = $package->getExtra()) && isset($extra['plugin-modifies-downloads']) && $extra['plugin-modifies-downloads'] === true; // is this a downloads modifying plugin or a dependency of one? - if ($isDownloadsModifyingPlugin || count(array_intersect($package->getNames(), $dlModyingPluginRequires))) { + if ($isDownloadsModifyingPlugin || count(array_intersect($package->getNames(), $dlModifyingPluginRequires))) { // get the package's requires, but filter out any platform requirements $requires = array_filter(array_keys($package->getRequires()), function ($req) { return !PlatformRepository::isPlatformPackage($req); @@ -276,12 +276,12 @@ class Transaction // is this a plugin with no meaningful dependencies? if ($isDownloadsModifyingPlugin && !count($requires)) { // plugins with no dependencies go to the very front - array_unshift($dlModyingPluginsNoDeps, $op); + array_unshift($dlModifyingPluginsNoDeps, $op); } else { // capture the requirements for this package so those packages will be moved up as well - $dlModyingPluginRequires = array_merge($dlModyingPluginRequires, $requires); + $dlModifyingPluginRequires = array_merge($dlModifyingPluginRequires, $requires); // move the operation to the front - array_unshift($dlModyingPluginsWithDeps, $op); + array_unshift($dlModifyingPluginsWithDeps, $op); } unset($operations[$idx]); @@ -313,7 +313,7 @@ class Transaction } } - return array_merge($dlModyingPluginsNoDeps, $dlModyingPluginsWithDeps, $pluginsNoDeps, $pluginsWithDeps, $operations); + return array_merge($dlModifyingPluginsNoDeps, $dlModifyingPluginsWithDeps, $pluginsNoDeps, $pluginsWithDeps, $operations); } /**