Fix detection of image formats in system gd

[orlitzky]

We currently get false positives when checking for, say, AVIF support in the system libgd: #12019

I replaced the existing link tests with run tests of a little gd program that seems to do the trick.

[petk]

Very good find. Thanks, @orlitzky

Yes, the libgd seems to need some patches from the php-src or some further code adjustments at least. Because with the bundled libgd all of these seem to work ok. I can't think of the better way at the moment so let's go with this. Seems good.

Also, let's add this to PHP-8.1 so we have current maintained branches all fixed.

I'll check for more soon...

[orlitzky]

Thanks! Cross-compilation makes it past ./configure now.

I was wondering why upstream libgd might have left those stub functions enabled. It looks intentional, so... maybe it's to avoid changing the API/ABI based on build-time choices?

This PR reveals some more test failures (in addition the remaining few in #11252) when the system gd lacks PNG support.They're easy to fix though and I'll work on it when this is done.

[petk]

There is this constant HAVE_GD_JPG which was inconsistently named in the past. Perhaps something like this instead of renaming all occurrences to HAVE_GD_JPEG out there:

--- a/ext/gd/config.m4
+++ b/ext/gd/config.m4
@@ -140,9 +140,9 @@ AC_DEFUN([PHP_GD_JISX0208],[
 
 
 dnl Build and run a program to determine if GD has support for the given
-dnl format. The sole argument is the proper-noun-capitalized name of the
+dnl format. The first argument is the proper-noun-capitalized name of the
 dnl format -- basically the word Foo in gdImageCreateFromFoo -- such as
-dnl Png. If support for format Foo exists, HAVE_GD_FOO will be defined
+dnl Png. If support for format Foo exists, 2nd argument will be defined
 dnl to 1. The reason for this charade is that gd defines "junk" versions
 dnl of each gdImageCreateFromFoo function even when it does not support
 dnl the Foo format. Those junk functions display a warning but eventually
@@ -171,7 +171,7 @@ int main(int argc, char** argv) {
   return 0;
 } ]])],[
     AC_MSG_RESULT([yes])
-    AC_DEFINE(HAVE_GD_[]m4_toupper($1), 1, [ ])
+    AC_DEFINE([$2], 1, [ ])
   ],[
     AC_MSG_RESULT([no])
   ],[
@@ -182,13 +182,13 @@ int main(int argc, char** argv) {
 ])
 
 AC_DEFUN([PHP_GD_CHECK_VERSION],[
-  PHP_GD_CHECK_FORMAT([Png])
-  PHP_GD_CHECK_FORMAT([Avif])
-  PHP_GD_CHECK_FORMAT([Webp])
-  PHP_GD_CHECK_FORMAT([Jpeg])
-  PHP_GD_CHECK_FORMAT([Xpm])
-  PHP_GD_CHECK_FORMAT([Bmp])
-  PHP_GD_CHECK_FORMAT([Tga])
+  PHP_GD_CHECK_FORMAT([Png],[HAVE_GD_PNG])
+  PHP_GD_CHECK_FORMAT([Avif],[HAVE_GD_AVIF])
+  PHP_GD_CHECK_FORMAT([Webp],[HAVE_GD_WEBP])
+  PHP_GD_CHECK_FORMAT([Jpeg],[HAVE_GD_JPG])
+  PHP_GD_CHECK_FORMAT([Xpm],[HAVE_GD_XPM])
+  PHP_GD_CHECK_FORMAT([Bmp],[HAVE_GD_BMP])
+  PHP_GD_CHECK_FORMAT([Tga],[HAVE_GD_TGA])
   PHP_CHECK_LIBRARY(gd, gdFontCacheShutdown,           [AC_DEFINE(HAVE_GD_FREETYPE,          1, [ ])], [], [ $GD_SHARED_LIBADD ])
   PHP_CHECK_LIBRARY(gd, gdVersionString,               [AC_DEFINE(HAVE_GD_LIBVERSION,        1, [ ])], [], [ $GD_SHARED_LIBADD ])
   PHP_CHECK_LIBRARY(gd, gdImageGetInterpolationMethod, [AC_DEFINE(HAVE_GD_GET_INTERPOLATION, 1, [ ])], [], [ $GD_SHARED_LIBADD ])

[orlitzky]

There is this constant HAVE_GD_JPG which was inconsistently named in the past. Perhaps something like this instead of renaming all occurrences to HAVE_GD_JPEG out there:

Hi, sorry for the delay, I never got the email notification for this comment and it took me a few months to independently wonder what ever happened to this PR.

I've implemented your suggestion. Renaming the constant was unintentional.

[petk]

Looks good to me. I'll recheck it in the following days and move forward with merging this soon...

[petk]

Hello, looks ok now, yes. I'll only add this change into the PR if that's fine with you and merge this in the following day(s) to master branch:

diff --git a/ext/gd/config.m4 b/ext/gd/config.m4
index c427a29f31..c453c0f38d 100644
--- a/ext/gd/config.m4
+++ b/ext/gd/config.m4
@@ -153,7 +153,7 @@ AC_DEFUN([PHP_GD_CHECK_FORMAT],[
   LIBS="${LIBS} ${GD_SHARED_LIBADD}"
   AC_MSG_CHECKING([for working gdImageCreateFrom$1 in libgd])
   AC_LANG_PUSH([C])
-  AC_RUN_IFELSE([AC_LANG_SOURCE([[
+  AC_RUN_IFELSE([AC_LANG_SOURCE([
 #include <stdio.h>
 #include <unistd.h>
 #include <gd.h>
@@ -166,11 +166,13 @@ void exit1(int priority, const char *format, va_list args) {
 /* Override the default gd_error_method with one that
    actually causes the program to return an error. */
 int main(int argc, char** argv) {
-  FILE* f = NULL;
+  m4_if([$1],[Xpm],
+  [char* f = "test.xpm"],
+  [FILE* f = NULL]);
   gdSetErrorMethod(exit1);
   gdImagePtr p = gdImageCreateFrom$1(f);
   return 0;
-} ]])],[
+}])],[
     AC_MSG_RESULT([yes])
     AC_DEFINE($2, 1, [Does gdImageCreateFrom$1 work?])
   ],[

Because the gdImageCreateFromXpm expects char * type, otherwise warning is emitted:

warning: passing argument 1 of ‘gdImageCreateFromXpm’ from incompatible pointer type [-Wincompatible-pointer-types]
   13 |             gdImagePtr p = gdImageCreateFromXpm(f);
      |                                                 ^
      |                                                 |
      |                                                 FILE *

and somehow test waits for the user input with certain build configurations. Other functions are OK.

[orlitzky]

Yeah good catch, I didn't notice the different argument type. Thanks again.

[petk]

Merged to master via 85e5635 Thank you @orlitzky 🎉

[orlitzky]

No problem, thanks for your help with this.

[todeveni]

Also, let's add this to PHP-8.1 so we have current maintained branches all fixed.

Will this be backported to maintained branches or is it PHP 8.4 only?

[petk]

I guess we could add it to PHP-8.2. I was a bit worried for the existing builds out there if it might cause issues. 🤔