From a7577cdfdd3b0459ac28d85b08ecec5b6f8f9ce2 Mon Sep 17 00:00:00 2001 From: tal7aouy Date: Sat, 23 Nov 2024 13:23:16 +0100 Subject: [PATCH 1/2] enhance-unix-build-cleanup --- src/Building/UnixBuild.php | 43 ++++++++++- test/integration/Building/UnixBuildTest.php | 83 ++++++++++++++++++++- 2 files changed, 121 insertions(+), 5 deletions(-) diff --git a/src/Building/UnixBuild.php b/src/Building/UnixBuild.php index 384c5474..3d7c4376 100644 --- a/src/Building/UnixBuild.php +++ b/src/Building/UnixBuild.php @@ -12,6 +12,7 @@ use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Process\Process as SymfonyProcess; +use Throwable; use function count; use function file_exists; use function implode; @@ -32,6 +33,8 @@ public function __invoke( OutputInterface $output, PhpizePath|null $phpizePath, ): BinaryFile { + $this->cleanup($downloadedPackage, $output); + $outputCallback = null; if ($output->isVerbose()) { /** @var callable(SymfonyProcess::ERR|SymfonyProcess::OUT, string):void $outputCallback */ @@ -77,7 +80,8 @@ public function __invoke( $expectedSoFile, )); - return BinaryFile::fromFileWithSha256Checksum($expectedSoFile); + $binaryFile = BinaryFile::fromFileWithSha256Checksum($expectedSoFile); + return $binaryFile; } /** @param callable(SymfonyProcess::ERR|SymfonyProcess::OUT, string): void|null $outputCallback */ @@ -151,4 +155,41 @@ private function make( $outputCallback, ); } + private function cleanup( + DownloadedPackage $downloadedPackage, + OutputInterface $output + ): void { + if (! file_exists($downloadedPackage->extractedSourcePath . '/Makefile')) { + return; + } + + try { + // Run make clean first + Process::run( + ['make', 'clean'], + $downloadedPackage->extractedSourcePath, + self::CONFIGURE_TIMEOUT_SECS, + null + ); + + // Then run phpize --clean + Process::run( + ['phpize', '--clean'], + $downloadedPackage->extractedSourcePath, + self::PHPIZE_TIMEOUT_SECS, + null + ); + + if ($output->isVerbose()) { + $output->writeln('Build files cleaned up'); + } + } catch (Throwable $e) { + if ($output->isVerbose()) { + $output->writeln(sprintf( + 'Warning: Failed to clean up build files: %s', + $e->getMessage() + )); + } + } + } } diff --git a/test/integration/Building/UnixBuildTest.php b/test/integration/Building/UnixBuildTest.php index ce82c942..91d378ae 100644 --- a/test/integration/Building/UnixBuildTest.php +++ b/test/integration/Building/UnixBuildTest.php @@ -18,8 +18,8 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\Console\Output\BufferedOutput; +use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Process\Process; - use function dirname; #[CoversClass(UnixBuild::class)] @@ -27,6 +27,7 @@ final class UnixBuildTest extends TestCase { private const TEST_EXTENSION_PATH = __DIR__ . '/../../assets/pie_test_ext'; + public function testUnixBuildCanBuildExtension(): void { if (Platform::isWindows()) { @@ -76,8 +77,8 @@ public function testUnixBuildCanBuildExtension(): void ->getExitCode(), ); - (new Process(['make', 'clean'], $downloadedPackage->extractedSourcePath))->mustRun(); - (new Process(['phpize', '--clean'], $downloadedPackage->extractedSourcePath))->mustRun(); + self::assertFileDoesNotExist($downloadedPackage->extractedSourcePath . '/Makefile'); + self::assertFileDoesNotExist($downloadedPackage->extractedSourcePath . '/configure'); } public function testUnixBuildWillThrowExceptionWhenExpectedBinaryNameMismatches(): void @@ -120,7 +121,6 @@ public function testUnixBuildWillThrowExceptionWhenExpectedBinaryNameMismatches( (new Process(['phpize', '--clean'], $downloadedPackage->extractedSourcePath))->mustRun(); } } - public function testUnixBuildCanBuildExtensionWithBuildPath(): void { if (Platform::isWindows()) { @@ -173,4 +173,79 @@ public function testUnixBuildCanBuildExtensionWithBuildPath(): void (new Process(['make', 'clean'], $downloadedPackage->extractedSourcePath))->mustRun(); (new Process(['phpize', '--clean'], $downloadedPackage->extractedSourcePath))->mustRun(); } + + + public function testCleanupHandlesNonExistentMakefileGracefully(): void + { + if (Platform::isWindows()) { + self::markTestSkipped('Unix build test cannot be run on Windows'); + } + + $output = new BufferedOutput(); + $downloadedPackage = DownloadedPackage::fromPackageAndExtractedPath( + new Package( + $this->createMock(CompletePackage::class), + ExtensionType::PhpModule, + ExtensionName::normaliseFromString('pie_test_ext'), + 'pie_test_ext', + '0.1.0', + null, + [], + true, + true, + null, + ), + self::TEST_EXTENSION_PATH, + ); + + $unixBuilder = new UnixBuild(); + + // Create a ReflectionMethod to test the private cleanup method + $cleanupMethod = new \ReflectionMethod($unixBuilder, 'cleanup'); + $cleanupMethod->setAccessible(true); + + // Should not throw any exception + $cleanupMethod->invoke($unixBuilder, $downloadedPackage, $output); + + $outputString = $output->fetch(); + self::assertStringNotContainsString('Build files cleaned up', $outputString); + } + + public function testVerboseOutputShowsCleanupMessages(): void + { + if (Platform::isWindows()) { + self::markTestSkipped('Unix build test cannot be run on Windows'); + } + + $output = new BufferedOutput(); + $output->setVerbosity(OutputInterface::VERBOSITY_VERBOSE); + + $downloadedPackage = DownloadedPackage::fromPackageAndExtractedPath( + new Package( + $this->createMock(CompletePackage::class), + ExtensionType::PhpModule, + ExtensionName::normaliseFromString('pie_test_ext'), + 'pie_test_ext', + '0.1.0', + null, + [ConfigureOption::fromComposerJsonDefinition(['name' => 'enable-pie_test_ext'])], + true, + true, + null, + ), + self::TEST_EXTENSION_PATH, + ); + + $unixBuilder = new UnixBuild(); + $builtBinary = $unixBuilder->__invoke( + $downloadedPackage, + TargetPlatform::fromPhpBinaryPath(PhpBinaryPath::fromCurrentProcess(), null), + ['--enable-pie_test_ext'], + $output, + null, + ); + + $outputString = $output->fetch(); + self::assertStringContainsString('Build files cleaned up', $outputString); + } } From eaabc96f65d0e7821b60c78ef664ba21e84083b5 Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Tue, 26 Nov 2024 10:41:57 +0000 Subject: [PATCH 2/2] Simplify UnixBuild to use phpize --clean, and use the PhpizePath VO --- src/Building/UnixBuild.php | 73 +++++++++++---------- test/integration/Building/UnixBuildTest.php | 36 ++++++---- 2 files changed, 62 insertions(+), 47 deletions(-) diff --git a/src/Building/UnixBuild.php b/src/Building/UnixBuild.php index 3d7c4376..6e7c9942 100644 --- a/src/Building/UnixBuild.php +++ b/src/Building/UnixBuild.php @@ -12,7 +12,6 @@ use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Process\Process as SymfonyProcess; -use Throwable; use function count; use function file_exists; use function implode; @@ -33,8 +32,6 @@ public function __invoke( OutputInterface $output, PhpizePath|null $phpizePath, ): BinaryFile { - $this->cleanup($downloadedPackage, $output); - $outputCallback = null; if ($output->isVerbose()) { /** @var callable(SymfonyProcess::ERR|SymfonyProcess::OUT, string):void $outputCallback */ @@ -48,8 +45,18 @@ public function __invoke( }; } + $phpizePath ??= PhpizePath::guessFrom($targetPlatform->phpBinaryPath); + + /** + * Call a cleanup first; most of the time, we expect to be changing a + * version (e.g. upgrade, downgrade), in which case the source is + * already clean anyway; however, sometimes we want to rebuild the + * current ext, so this will perform a clean first + */ + $this->cleanup($phpizePath, $downloadedPackage, $output, $outputCallback); + $this->phpize( - $phpizePath ?? PhpizePath::guessFrom($targetPlatform->phpBinaryPath), + $phpizePath, $downloadedPackage, $output, $outputCallback, @@ -80,8 +87,7 @@ public function __invoke( $expectedSoFile, )); - $binaryFile = BinaryFile::fromFileWithSha256Checksum($expectedSoFile); - return $binaryFile; + return BinaryFile::fromFileWithSha256Checksum($expectedSoFile); } /** @param callable(SymfonyProcess::ERR|SymfonyProcess::OUT, string): void|null $outputCallback */ @@ -155,41 +161,40 @@ private function make( $outputCallback, ); } + + /** @param callable(SymfonyProcess::ERR|SymfonyProcess::OUT, string): void|null $outputCallback */ private function cleanup( + PhpizePath $phpize, DownloadedPackage $downloadedPackage, - OutputInterface $output + OutputInterface $output, + callable|null $outputCallback, ): void { - if (! file_exists($downloadedPackage->extractedSourcePath . '/Makefile')) { + /** + * A basic, but fallible check to see if we should clean first. This + * should work "most" of the time, unless someone has removed the + * configure script manually... + */ + if (! file_exists($downloadedPackage->extractedSourcePath . '/configure')) { + if ($output->isVerbose()) { + $output->writeln('Skipping phpize --clean, configure does not exist'); + } + return; } - try { - // Run make clean first - Process::run( - ['make', 'clean'], - $downloadedPackage->extractedSourcePath, - self::CONFIGURE_TIMEOUT_SECS, - null - ); - - // Then run phpize --clean - Process::run( - ['phpize', '--clean'], - $downloadedPackage->extractedSourcePath, - self::PHPIZE_TIMEOUT_SECS, - null - ); + $phpizeCleanCommand = [$phpize->phpizeBinaryPath, '--clean']; - if ($output->isVerbose()) { - $output->writeln('Build files cleaned up'); - } - } catch (Throwable $e) { - if ($output->isVerbose()) { - $output->writeln(sprintf( - 'Warning: Failed to clean up build files: %s', - $e->getMessage() - )); - } + if ($output->isVerbose()) { + $output->writeln('Running phpize --clean step using: ' . implode(' ', $phpizeCleanCommand) . ''); } + + Process::run( + $phpizeCleanCommand, + $downloadedPackage->extractedSourcePath, + self::PHPIZE_TIMEOUT_SECS, + $outputCallback, + ); + + $output->writeln('Build files cleaned up.'); } } diff --git a/test/integration/Building/UnixBuildTest.php b/test/integration/Building/UnixBuildTest.php index 91d378ae..41bffb72 100644 --- a/test/integration/Building/UnixBuildTest.php +++ b/test/integration/Building/UnixBuildTest.php @@ -20,6 +20,7 @@ use Symfony\Component\Console\Output\BufferedOutput; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Process\Process; + use function dirname; #[CoversClass(UnixBuild::class)] @@ -27,7 +28,6 @@ final class UnixBuildTest extends TestCase { private const TEST_EXTENSION_PATH = __DIR__ . '/../../assets/pie_test_ext'; - public function testUnixBuildCanBuildExtension(): void { if (Platform::isWindows()) { @@ -77,8 +77,8 @@ public function testUnixBuildCanBuildExtension(): void ->getExitCode(), ); - self::assertFileDoesNotExist($downloadedPackage->extractedSourcePath . '/Makefile'); - self::assertFileDoesNotExist($downloadedPackage->extractedSourcePath . '/configure'); + (new Process(['make', 'clean'], $downloadedPackage->extractedSourcePath))->mustRun(); + (new Process(['phpize', '--clean'], $downloadedPackage->extractedSourcePath))->mustRun(); } public function testUnixBuildWillThrowExceptionWhenExpectedBinaryNameMismatches(): void @@ -121,6 +121,7 @@ public function testUnixBuildWillThrowExceptionWhenExpectedBinaryNameMismatches( (new Process(['phpize', '--clean'], $downloadedPackage->extractedSourcePath))->mustRun(); } } + public function testUnixBuildCanBuildExtensionWithBuildPath(): void { if (Platform::isWindows()) { @@ -174,14 +175,18 @@ public function testUnixBuildCanBuildExtensionWithBuildPath(): void (new Process(['phpize', '--clean'], $downloadedPackage->extractedSourcePath))->mustRun(); } - - public function testCleanupHandlesNonExistentMakefileGracefully(): void + public function testCleanupDoesNotCleanWhenConfigureIsMissing(): void { if (Platform::isWindows()) { self::markTestSkipped('Unix build test cannot be run on Windows'); } + (new Process(['phpize', '--clean'], self::TEST_EXTENSION_PATH))->mustRun(); + self::assertFileDoesNotExist(self::TEST_EXTENSION_PATH . '/configure'); + $output = new BufferedOutput(); + $output->setVerbosity(OutputInterface::VERBOSITY_VERBOSE); + $downloadedPackage = DownloadedPackage::fromPackageAndExtractedPath( new Package( $this->createMock(CompletePackage::class), @@ -199,15 +204,16 @@ public function testCleanupHandlesNonExistentMakefileGracefully(): void ); $unixBuilder = new UnixBuild(); - - // Create a ReflectionMethod to test the private cleanup method - $cleanupMethod = new \ReflectionMethod($unixBuilder, 'cleanup'); - $cleanupMethod->setAccessible(true); - - // Should not throw any exception - $cleanupMethod->invoke($unixBuilder, $downloadedPackage, $output); + $unixBuilder->__invoke( + $downloadedPackage, + TargetPlatform::fromPhpBinaryPath(PhpBinaryPath::fromCurrentProcess(), null), + ['--enable-pie_test_ext'], + $output, + null, + ); $outputString = $output->fetch(); + self::assertStringContainsString('Skipping phpize --clean, configure does not exist', $outputString); self::assertStringNotContainsString('Build files cleaned up', $outputString); } @@ -217,6 +223,9 @@ public function testVerboseOutputShowsCleanupMessages(): void self::markTestSkipped('Unix build test cannot be run on Windows'); } + (new Process(['phpize'], self::TEST_EXTENSION_PATH))->mustRun(); + self::assertFileExists(self::TEST_EXTENSION_PATH . '/configure'); + $output = new BufferedOutput(); $output->setVerbosity(OutputInterface::VERBOSITY_VERBOSE); @@ -237,7 +246,7 @@ public function testVerboseOutputShowsCleanupMessages(): void ); $unixBuilder = new UnixBuild(); - $builtBinary = $unixBuilder->__invoke( + $unixBuilder->__invoke( $downloadedPackage, TargetPlatform::fromPhpBinaryPath(PhpBinaryPath::fromCurrentProcess(), null), ['--enable-pie_test_ext'], @@ -246,6 +255,7 @@ public function testVerboseOutputShowsCleanupMessages(): void ); $outputString = $output->fetch(); + self::assertStringContainsString('Running phpize --clean step', $outputString); self::assertStringContainsString('Build files cleaned up', $outputString); } }