Downloader Dependencies #1120

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
5 participants

ukautz commented Sep 18, 2012

Check for dependencies of downloaders (eg whether git, hg, svn.. binary installed, if package requires so) on upload/install and dies with runtime error, if requirements not satisfied.

Ulrich Kautz added some commits Sep 18, 2012

Ulrich Kautz
Check downloader for requirements (eg git binary) before running inst…
…all / update -> throw runtime exception if not satisfied
src/Composer/Installer.php
+ }
+
+ // add to execute list
+ $executes []= array($localRepo, $operation);
@pborreli

pborreli Sep 18, 2012

Contributor

$executes[] =

@stof

stof Sep 18, 2012

Contributor

Why adding the localRepo here ? It will always be the same.

src/Composer/Installer.php
+ if (in_array($operation->getJobType(), array('install', 'update'))) {
+
+ // assure installation source set
+ $package = $operation->getJobType() == 'install'
@pborreli

pborreli Sep 18, 2012

Contributor

'install' === $operation->getJobType()

src/Composer/Installer.php
+ ? $operation->getPackage() : $operation->getTargetPackage();
+ $this->downloadManager->setInstallationSourceForPackage($package);
+ $source = $package->getInstallationSource();
+ $downloadType = $source == 'dist' ? $package->getDistType() : $package->getSourceType();
@pborreli

pborreli Sep 18, 2012

Contributor

'dist' === $source

+ * @param PackageInterface $package package instance
+ * @param string $error any error will be written into this var if passed by ref
+ *
+ * @return bool
@pborreli

pborreli Sep 18, 2012

Contributor

Boolean

@stof

stof Sep 18, 2012

Contributor

@pborreli no. Composer does not use this rule of the capital B which confuses the IDEs (they don't recognize it as boolean but as a class name).

@pborreli

pborreli Sep 18, 2012

Contributor

ok my bad, removing the others keeping this one for the record

@@ -53,4 +53,14 @@ public function update(PackageInterface $initial, PackageInterface $target, $pat
* @param string $path download path
*/
public function remove(PackageInterface $package, $path);
+
+ /**
+ * Checkes whether interface can be used (eg required binary executables)
@stof

stof Sep 18, 2012

Contributor

typo here. And using interface seems weird to me

src/Composer/Installer.php
@@ -445,7 +446,9 @@ protected function doInstall($localRepo, $installedRepo, $aliases, $devMode = fa
if (!$operations) {
$this->io->write('Nothing to install or update');
}
-
+
@stof

stof Sep 18, 2012

Contributor

Please don't add trailing whitespaces

+ $error = 'Did not find executable "git" in $PATH';
+ return false;
+ }
+ return trim($output) == 'OK';
@pborreli

pborreli Sep 18, 2012

Contributor

'OK' === trim($output)

src/Composer/Installer.php
@@ -445,7 +446,9 @@ protected function doInstall($localRepo, $installedRepo, $aliases, $devMode = fa
if (!$operations) {
$this->io->write('Nothing to install or update');
}
-
+
+ // check operations, build executs
@pborreli

pborreli Sep 18, 2012

Contributor

typo on executs, dunno what you wanna say

@ukautz

ukautz Sep 18, 2012

What i wanted to say: Before, the operations were executed immediately, now i build an array of executions, which will be executed delayed - after each operation/package is checked for availability. Will remove it.. Thx

*
- * @throws InvalidArgumentException if package have no urls to download from
+ * @return void
@stof

stof Sep 18, 2012

Contributor

Please remove the @return void

- * @param PackageInterface $package package instance
- * @param string $targetDir target dir
- * @param bool $preferSource prefer installation from source
+ * @param PackageInterface $package package instance
@stof

stof Sep 18, 2012

Contributor

The $preferSource param is missing

src/Composer/Installer.php
@@ -367,6 +367,7 @@ protected function doInstall($localRepo, $installedRepo, $aliases, $devMode = fa
// force dev packages to be updated if we update or install from a (potentially new) lock
foreach ($localRepo->getPackages() as $package) {
+
@stof

stof Sep 18, 2012

Contributor

you should remove this line

ukautz commented Sep 18, 2012

Thx guys. Fixed.

src/Composer/Installer.php
+ // for install/update check whether downloader available
+ if (in_array($operation->getJobType(), array('install', 'update'))) {
+ // assure installation source set
+ $package = $operation->getJobType() === 'install'
@pborreli

pborreli Sep 18, 2012

Contributor

'install' === $operation->getJobType()

src/Composer/Installer.php
+ ? $operation->getPackage() : $operation->getTargetPackage();
+ $this->downloadManager->setInstallationSourceForPackage($package);
+ $source = $package->getInstallationSource();
+ $downloadType = $source === 'dist' ? $package->getDistType() : $package->getSourceType();
@pborreli

pborreli Sep 18, 2012

Contributor

'dist' === $source

*
- * @param PackageInterface $package package instance
- * @param string $targetDir target dir
+ * @param PackageInterface $package package instance
@stof

stof Sep 18, 2012

Contributor

Please keep the phpdoc aligned (the PHP-CS-Fixer can fix it for you)

src/Composer/Installer.php
@@ -484,6 +485,30 @@ protected function doInstall($localRepo, $installedRepo, $aliases, $devMode = fa
$this->io->write(' - ' . $operation);
}
+
+ // for install/update check whether downloader available
@stof

stof Sep 18, 2012

Contributor

missing is before available

Owner

Seldaek commented Sep 19, 2012

Is there really a huge benefit to this? It adds more code, more stuff to maintain, and I don't know if saving a few seconds in the odd case that you don't have a git/hg/svn binary available warrants it.

ukautz commented Sep 19, 2012

Up to you guys.

IMO any tool should make aware of missing requirements as early as possible. You cannot assume hg, svn and git are always available (don't know about Mac&Win, but on Linux, not even Ubuntu pre-installs them).

Contributor

GromNaN commented Jan 30, 2015

Running these check commands will have a bad impact on composer performance. Having such check sounds interesting to unserstand issues, but useless once the system is configured.

To avoid performance impacts, but get debug information, it would be better to run the check only after an error occured to check if it is due to a missing system requirement of a runtime error.

Owner

Seldaek commented Jan 30, 2015

Well we already do that if a git clone fails and you don't have git for example, to be honest nobody complained about this in years so I think it can safely be closed.

@Seldaek Seldaek closed this Jan 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment