Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if GNU patch is used #426

Closed
wants to merge 1 commit into from
Closed

Check if GNU patch is used #426

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Plugin/Patches.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,14 @@ protected function getAndApplyPatch(RemoteFilesystem $downloader, $install_path,
// differences between how patch works on windows and unix.
$patch_options = '--no-backup-if-mismatch';
if (PHP_OS_FAMILY == 'BSD') {
Copy link
Collaborator

@danepowell danepowell Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work reliably because PHP_OS_FAMILY reflect the build target for the running PHP version, not the current hosting environment. In my case on MacOS Ventura, for whatever reason, PHP_OS_FAMILY is Linux. See https://github.com/loophp/phposinfo, but I don't know that we want to introduce a new dependency just for this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you're saying is the PHP documentation is wrong: When it says "The operating system family PHP was built for" what they actually mean is "The operating system family PHP was built on"? If so, uhg..

So maybe looking at patch --version is correct after all, although all the logic needed there might not be pretty.

Copy link
Collaborator

@danepowell danepowell Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the only semi-reliable way to detect runtime OS is php_uname, but even that will fall back to displaying the build OS in some situations. I found the phposinfo library to do a pretty good job of compensating for this by using a combination of php_uname, PHP_OS_FAMILY, and shell commands to verify the OS version.

It's obviously better to avoid any conditional logic based on the OS in the first place.

$patchVersion = '';
$this->executor->execute('patch --version', $patchVersion);
if (strpos($patchBinaryVersion, 'GNU patch') === false) {
throw new \Exception(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here should be for non-GNU versions of patch to actually work! :) Please review the PR at #402 where we try to get the non-GNU patch working (with a pretty trivial fix).

"Please make sure to have GNU patch installed, see "
. "https://github.com/cweagans/composer-patches/wiki/Install-%22patch%22-binary"
);
}
$patch_options = '--posix --batch';
}
foreach ($patch_levels as $patch_level) {
Expand Down