Cleanup processes output handling #5569
Conversation
| @@ -72,11 +72,11 @@ protected function extract($file, $path) | |||
| if (self::$hasSystemUnzip && !(class_exists('ZipArchive') && Platform::isWindows())) { | |||
| $command = 'unzip '.ProcessExecutor::escape($file).' -d '.ProcessExecutor::escape($path); | |||
| if (!Platform::isWindows()) { | |||
| $command .= ' && chmod -R u+w ' . ProcessExecutor::escape($path); | |||
| $command .= ' >/dev/null && chmod -R u+w ' . ProcessExecutor::escape($path); | |||
There was a problem hiding this comment.
This is the main point of this patch.
There was a problem hiding this comment.
What about just passing -qq to unzip? Should be cross platform compatible then even.
There was a problem hiding this comment.
is it portable?
There was a problem hiding this comment.
I would expect it to be. But I do not have any Windows machines in close proximity :-(
Let me go check some colleagues around here..
Edit: found a windows machine, but it did not have unzip.
There was a problem hiding this comment.
I just looked on my Windows VM, looks OK
|
LGTM |
nicolas-grekas
force-pushed
the
clean-proc-output
branch
from
05e1121
to
41741e0
Compare
| @@ -70,13 +70,13 @@ protected function extract($file, $path) | |||
| $processError = null; | |||
|
|
|||
| if (self::$hasSystemUnzip && !(class_exists('ZipArchive') && Platform::isWindows())) { | |||
| $command = 'unzip '.ProcessExecutor::escape($file).' -d '.ProcessExecutor::escape($path); | |||
| $command = 'unzip -qq '.ProcessExecutor::escape($file).' -d '.ProcessExecutor::escape($path); | |||
There was a problem hiding this comment.
-qq to discard outputs
|
What about using |
|
|
…las-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [Process] Fix double-fread() when reading unix pipes | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - While looking at the blackfire profile of a `composer install`, I was able to reduce the number of calls to `fread` from 90k to 60k using this patch (and from 60k to <1k with composer/composer#5569 but that's another story). In fact, we should continue reading only if there might be something next, which won"t be the case if the buffer has not been filled. Commits ------- ac17617 [Process] Fix double-fread() when reading unix pipes
…las-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [Process] Fix double-fread() when reading unix pipes | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - While looking at the blackfire profile of a `composer install`, I was able to reduce the number of calls to `fread` from 90k to 60k using this patch (and from 60k to <1k with composer/composer#5569 but that's another story). In fact, we should continue reading only if there might be something next, which won"t be the case if the buffer has not been filled. Commits ------- ac17617 [Process] Fix double-fread() when reading unix pipes
|
ping @Seldaek , this is an easy pick :) |
|
Am I missing something or is this enabling output on all these process calls? Because by default ProcessExecutor will |
|
Nope, it won't echo. `proc_open` does that, but not ProcessExecutor... It
just ignores the output that had been buffered by Process.
|
|
As per https://github.com/composer/composer/blob/master/src/Composer/Util/ProcessExecutor.php#L67 and https://github.com/composer/composer/blob/master/src/Composer/Util/ProcessExecutor.php#L102, no.. $e = new Composer\Util\ProcessExecutor();
$e->execute('echo FOO');
$e = new Composer\Util\ProcessExecutor();
$e->execute('echo BAR', $output);This is basically potentially causing a huge mess of output as far as I can tell. I don't know if you tested it at all.. but anyway we can add the -qq on the zip and rar calls, that sounds fine, it should limit the amount of writes they do and therefore the amount of fread needed. The rest I'd rather not do it unless there is a very good reason, because it won't change the amount of fread the Process component does. |
nicolas-grekas
force-pushed
the
clean-proc-output
branch
from
41741e0
to
f063d28
Compare
|
I did, not enough :) Fixed!
|
|
Thanks :) Looks good now, and here's a good reminder to everyone that there is no such thing as an "easy pick" when it comes to reviewing :p |
I was playing with a blackfire profile done on a composer install and figured out that
freadwas called 90k times. After investigating, it turns out thatunzip's output is ignored by composer, but it's not by the underlying Symfony Process object.Let's pipe its output to
/dev/null(not on windows of course).Meanwhile, some code needed a related cleanup, here it is :)