Change PopenBased finders to ProcOpenBased finders (#75)

theofidry · Dec 10, 2022 · 0166da6 · 0166da6
1 parent 05dde06
commit 0166da6
Show file tree

Hide file tree

Showing 19 changed files with 179 additions and 66 deletions.
diff --git a/infection.json5 b/infection.json5
@@ -20,6 +20,12 @@
                 "Fidry\\CpuCoreCounter\\CpuCoreCounter::getDefaultFinders"
             ]
         },
+        "CastString": {
+            "ignore": [
+                // I can't find a case in practice where this would happen
+                "Fidry\\CpuCoreCounter\\Exec\\ProcOpen::execute"
+            ]
+        },
         "FalseValue": {
             "ignore": [
                 // This is from thecodingmachine/safe – leave it alone
@@ -28,6 +34,8 @@
         },
         "FunctionCallRemoval": {
             "ignore": [
+                // I can't find a case in practice where this would happen
+                "Fidry\\CpuCoreCounter\\Exec\\ProcOpen::execute",
                 // This is from thecodingmachine/safe – leave it alone
                 "Fidry\\CpuCoreCounter\\Exec\\ShellExec::execute"
             ]

diff --git a/src/Exec/ProcOpen.php b/src/Exec/ProcOpen.php
@@ -0,0 +1,58 @@
+<?php
+
+/*
+ * This file is part of the Fidry CPUCounter Config package.
+ *
+ * (c) Théo FIDRY <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Fidry\CpuCoreCounter\Exec;
+
+use function fclose;
+use function is_resource;
+use function proc_close;
+use function proc_open;
+use function stream_get_contents;
+
+final class ProcOpen
+{
+    /**
+     * @return array{string, string} STDOUT & STDERR tuple
+     */
+    public static function execute(string $command): ?array
+    {
+        $pipes = [];
+
+        $process = @proc_open(
+            $command,
+            [
+                ['pipe', 'rb'],
+                ['pipe', 'wb'], // stdout
+                ['pipe', 'wb'], // stderr
+            ],
+            $pipes
+        );
+
+        if (!is_resource($process)) {
+            return null;
+        }
+
+        fclose($pipes[0]);
+
+        $stdout = (string) stream_get_contents($pipes[1]);
+        $stderr = (string) stream_get_contents($pipes[2]);
+
+        proc_close($process);
+
+        return [$stdout, $stderr];
+    }
+
+    private function __construct()
+    {
+    }
+}
diff --git a/src/Finder/HwLogicalFinder.php b/src/Finder/HwLogicalFinder.php
@@ -19,7 +19,7 @@
  * @see https://github.com/paratestphp/paratest/blob/c163539818fd96308ca8dc60f46088461e366ed4/src/Runners/PHPUnit/Options.php#L903-L909
  * @see https://opensource.apple.com/source/xnu/xnu-792.2.4/libkern/libkern/sysctl.h.auto.html
  */
-final class HwLogicalFinder extends PopenBasedFinder
+final class HwLogicalFinder extends ProcOpenBasedFinder
 {
     protected function getCommand(): string
     {

diff --git a/src/Finder/HwPhysicalFinder.php b/src/Finder/HwPhysicalFinder.php
@@ -19,7 +19,7 @@
  * @see https://github.com/paratestphp/paratest/blob/c163539818fd96308ca8dc60f46088461e366ed4/src/Runners/PHPUnit/Options.php#L903-L909
  * @see https://opensource.apple.com/source/xnu/xnu-792.2.4/libkern/libkern/sysctl.h.auto.html
  */
-final class HwPhysicalFinder extends PopenBasedFinder
+final class HwPhysicalFinder extends ProcOpenBasedFinder
 {
     protected function getCommand(): string
     {

diff --git a/src/Finder/LinuxyNProcessorFinder.php b/src/Finder/LinuxyNProcessorFinder.php
@@ -18,7 +18,7 @@
  *
  * @see https://twitter.com/freebsdfrau/status/1052016199452700678?s=20&t=M2pHkRqmmna-UF68lfL2hw
  */
-final class LinuxyNProcessorFinder extends PopenBasedFinder
+final class LinuxyNProcessorFinder extends ProcOpenBasedFinder
 {
     protected function getCommand(): string
     {

diff --git a/src/Finder/NProcessorFinder.php b/src/Finder/NProcessorFinder.php
@@ -18,7 +18,7 @@
  *
  * @see https://twitter.com/freebsdfrau/status/1052016199452700678?s=20&t=M2pHkRqmmna-UF68lfL2hw
  */
-final class NProcessorFinder extends PopenBasedFinder
+final class NProcessorFinder extends ProcOpenBasedFinder
 {
     protected function getCommand(): string
     {

diff --git a/src/Finder/PopenBasedFinder.php → src/Finder/ProcOpenBasedFinder.php b/src/Finder/PopenBasedFinder.php → src/Finder/ProcOpenBasedFinder.php
@@ -13,59 +13,50 @@
 
 namespace Fidry\CpuCoreCounter\Finder;
 
-use function fgets;
+use Fidry\CpuCoreCounter\Exec\ProcOpen;
 use function filter_var;
 use function function_exists;
 use function is_int;
-use function is_resource;
-use function pclose;
-use function popen;
 use function sprintf;
 use function strrpos;
 use function substr;
+use function trim;
 use const FILTER_VALIDATE_INT;
 use const PHP_EOL;
 
-abstract class PopenBasedFinder implements CpuCoreFinder
+abstract class ProcOpenBasedFinder implements CpuCoreFinder
 {
     public function diagnose(): string
     {
-        if (!function_exists('popen')) {
-            return 'The function "popen" is not available.';
+        if (!function_exists('proc_open')) {
+            return 'The function "proc_open" is not available.';
         }
 
-        // Redirect the STDERR to the STDOUT since popen cannot capture the
-        // STDERR.
-        // We could use proc_open but this would be a greater difference between
-        // the command we really execute when using the finder and what we will
-        // diagnose.
-        $command = $this->getCommand().' 2>&1';
+        $command = $this->getCommand();
+        $output = ProcOpen::execute($command);
 
-        $process = popen($command, 'rb');
-
-        if (!is_resource($process)) {
+        if (null === $output) {
             return sprintf(
-                'Could not execute the function "popen" with the command "%s".',
+                'Failed to execute the command "%s".',
                 $command
             );
         }
 
-        $processResult = fgets($process);
-        $exitCode = pclose($process);
+        [$stdout, $stderr] = $output;
+        $failed = '' !== trim($stderr);
 
-        return 0 === $exitCode
+        return $failed
             ? sprintf(
-                'Executed the command "%s" and got the following output:%s%s',
+                'Executed the command "%s" which wrote the following output to the STDERR:%s%s',
                 $command,
                 PHP_EOL,
-                $processResult
+                $stderr
             )
             : sprintf(
-                'Executed the command "%s" which exited with a non-success exit code (%d) with the following output:%s%s',
+                'Executed the command "%s" and got the following (STDOUT) output:%s%s',
                 $command,
-                $exitCode,
                 PHP_EOL,
-                $processResult
+                $stdout
             );
     }
 
@@ -74,22 +65,22 @@ public function diagnose(): string
      */
     public function find(): ?int
     {
-        if (!function_exists('popen')) {
+        if (!function_exists('proc_open')) {
             return null;
         }
 
-        $process = popen($this->getCommand(), 'rb');
+        $output = ProcOpen::execute($this->getCommand());
 
-        if (!is_resource($process)) {
+        if (null === $output) {
             return null;
         }
 
-        $processResult = fgets($process);
-        pclose($process);
+        [$stdout, $stderr] = $output;
+        $failed = '' !== trim($stderr);
 
-        return false === $processResult
+        return $failed
             ? null
-            : self::countCpuCores($processResult);
+            : self::countCpuCores($stdout);
     }
 
     public function toString(): string

diff --git a/src/Finder/WindowsWmicLogicalFinder.php b/src/Finder/WindowsWmicLogicalFinder.php
@@ -20,7 +20,7 @@
  *
  * @see https://github.com/paratestphp/paratest/blob/c163539818fd96308ca8dc60f46088461e366ed4/src/Runners/PHPUnit/Options.php#L912-L916
  */
-final class WindowsWmicLogicalFinder extends PopenBasedFinder
+final class WindowsWmicLogicalFinder extends ProcOpenBasedFinder
 {
     /**
      * @return positive-int|null

diff --git a/src/Finder/WindowsWmicPhysicalFinder.php b/src/Finder/WindowsWmicPhysicalFinder.php
@@ -20,7 +20,7 @@
  *
  * @see https://github.com/paratestphp/paratest/blob/c163539818fd96308ca8dc60f46088461e366ed4/src/Runners/PHPUnit/Options.php#L912-L916
  */
-final class WindowsWmicPhysicalFinder extends PopenBasedFinder
+final class WindowsWmicPhysicalFinder extends ProcOpenBasedFinder
 {
     /**
      * @return positive-int|null

diff --git a/tests/Exec/ProcOpenTest.php b/tests/Exec/ProcOpenTest.php
@@ -0,0 +1,56 @@
+<?php
+
+/*
+ * This file is part of the Fidry CPUCounter Config package.
+ *
+ * (c) Théo FIDRY <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Fidry\CpuCoreCounter\Test\Exec;
+
+use Fidry\CpuCoreCounter\Exec\ProcOpen;
+use PHPUnit\Framework\TestCase;
+use const PHP_EOL;
+
+/**
+ * @covers \Fidry\CpuCoreCounter\Exec\ProcOpen
+ *
+ * @internal
+ */
+final class ProcOpenTest extends TestCase
+{
+    public function test_it_can_execute_a_command_writing_output_to_the_stdout(): void
+    {
+        $command = 'echo "Hello world!"';
+
+        $expected = ['Hello world!'.PHP_EOL, ''];
+        $actual = ProcOpen::execute($command);
+
+        self::assertSame($expected, $actual);
+    }
+
+    public function test_it_can_execute_a_command_writing_output_to_the_stderr_instead_of_the_stdout(): void
+    {
+        $command = 'echo "Hello world!" 1>&2';
+
+        $expected = ['', 'Hello world!'.PHP_EOL];
+        $actual = ProcOpen::execute($command);
+
+        self::assertSame($expected, $actual);
+    }
+
+    public function test_it_can_execute_a_command_writing_output_to_the_stdout_instead_of_the_stderr(): void
+    {
+        $command = 'echoerr() { echo "$@" 1>&2; }; echoerr "Hello world!" 2>&1';
+
+        $expected = ['Hello world!'.PHP_EOL, ''];
+        $actual = ProcOpen::execute($command);
+
+        self::assertSame($expected, $actual);
+    }
+}
diff --git a/tests/Finder/DummyPopenBasedFinder.php → tests/Finder/DummyProcOpenBasedFinder.php b/tests/Finder/DummyPopenBasedFinder.php → tests/Finder/DummyProcOpenBasedFinder.php
@@ -13,9 +13,9 @@
 
 namespace Fidry\CpuCoreCounter\Test\Finder;
 
-use Fidry\CpuCoreCounter\Finder\PopenBasedFinder;
+use Fidry\CpuCoreCounter\Finder\ProcOpenBasedFinder;
 
-final class DummyPopenBasedFinder extends PopenBasedFinder
+final class DummyProcOpenBasedFinder extends ProcOpenBasedFinder
 {
     protected function getCommand(): string
     {

diff --git a/tests/Finder/HwLogicalFinderTest.php b/tests/Finder/HwLogicalFinderTest.php
@@ -14,21 +14,21 @@
 namespace Fidry\CpuCoreCounter\Test\Finder;
 
 use Fidry\CpuCoreCounter\Finder\HwLogicalFinder;
-use Fidry\CpuCoreCounter\Finder\PopenBasedFinder;
+use Fidry\CpuCoreCounter\Finder\ProcOpenBasedFinder;
 
 /**
  * @covers \Fidry\CpuCoreCounter\Finder\HwLogicalFinder
  *
  * @internal
  */
-final class HwLogicalFinderTest extends PopenBasedFinderTestCase
+final class HwLogicalFinderTest extends ProcOpenBasedFinderTestCase
 {
     public function test_it_can_describe_itself(): void
     {
         self::assertSame('HwLogicalFinder', $this->getFinder()->toString());
     }
 
-    protected function getFinder(): PopenBasedFinder
+    protected function getFinder(): ProcOpenBasedFinder
     {
         return new HwLogicalFinder();
     }

diff --git a/tests/Finder/HwPhysicalFinderTest.php b/tests/Finder/HwPhysicalFinderTest.php
@@ -14,21 +14,21 @@
 namespace Fidry\CpuCoreCounter\Test\Finder;
 
 use Fidry\CpuCoreCounter\Finder\HwPhysicalFinder;
-use Fidry\CpuCoreCounter\Finder\PopenBasedFinder;
+use Fidry\CpuCoreCounter\Finder\ProcOpenBasedFinder;
 
 /**
  * @covers \Fidry\CpuCoreCounter\Finder\HwPhysicalFinder
  *
  * @internal
  */
-final class HwPhysicalFinderTest extends PopenBasedFinderTestCase
+final class HwPhysicalFinderTest extends ProcOpenBasedFinderTestCase
 {
     public function test_it_can_describe_itself(): void
     {
         self::assertSame('HwPhysicalFinder', $this->getFinder()->toString());
     }
 
-    protected function getFinder(): PopenBasedFinder
+    protected function getFinder(): ProcOpenBasedFinder
     {
         return new HwPhysicalFinder();
     }

diff --git a/tests/Finder/LinuxyNProcessorFinderTest.php b/tests/Finder/LinuxyNProcessorFinderTest.php
@@ -14,21 +14,21 @@
 namespace Fidry\CpuCoreCounter\Test\Finder;
 
 use Fidry\CpuCoreCounter\Finder\LinuxyNProcessorFinder;
-use Fidry\CpuCoreCounter\Finder\PopenBasedFinder;
+use Fidry\CpuCoreCounter\Finder\ProcOpenBasedFinder;
 
 /**
  * @covers \Fidry\CpuCoreCounter\Finder\LinuxyNProcessorFinder
  *
  * @internal
  */
-final class LinuxyNProcessorFinderTest extends PopenBasedFinderTestCase
+final class LinuxyNProcessorFinderTest extends ProcOpenBasedFinderTestCase
 {
     public function test_it_can_describe_itself(): void
     {
         self::assertSame('LinuxyNProcessorFinder', $this->getFinder()->toString());
     }
 
-    protected function getFinder(): PopenBasedFinder
+    protected function getFinder(): ProcOpenBasedFinder
     {
         return new LinuxyNProcessorFinder();
     }