Migrate ext/gd to pkg-config

[nikic]

• --with-webp-dir becomes --with-webp
• --with-jpeg-dir becomes --with-jpeg
• --with-png-dir is removed. libpng is required.
• --with-zlib-dir is removed. zlib is required. (This option is still used by some other exts.)
• --with-xpm-dir becomes --with-xpm.

[nikic]

@cmb69 You might want to double-check this.

[carusogabriel]

ping @petk

[cmb69]

@nikic $GD_JPEG is always empty here, so JPEG support would never be available (same for WebP and XPM). A possible solution would be:

 ext/gd/config.m4 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ext/gd/config.m4 b/ext/gd/config.m4
index bb55c5fd94..9eb593f3b1 100644
--- a/ext/gd/config.m4
+++ b/ext/gd/config.m4
@@ -54,7 +54,7 @@ AC_DEFUN([PHP_GD_WEBP],[
 
 AC_DEFUN([PHP_GD_JPEG],[
   if test "$PHP_JPEG" != "no"; then
-    PKG_CHECK_MODULES([JPEG], [libjpeg])
+    PKG_CHECK_MODULES([JPEG], [libjpeg], [JPEG_FOUND=true])
     PHP_EVAL_LIBLINE($JPEG_LIBS, GD_SHARED_LIBADD)
     PHP_EVAL_INCLINE($JPEG_CFLAGS)
   fi
@@ -146,7 +146,7 @@ dnl enable the support in bundled GD library
     GDLIB_CFLAGS="$GDLIB_CFLAGS -DHAVE_LIBWEBP"
   fi
 
-  if test -n "$GD_JPEG"; then
+  if test -n "$JPEG_FOUND"; then
     AC_DEFINE(HAVE_GD_JPG, 1, [ ])
     GDLIB_CFLAGS="$GDLIB_CFLAGS -DHAVE_LIBJPEG"
   fi

I wonder, though, why we have an extra check here. It seems to me that we could define HAVE_GD_JPG and set GDLIB_CFLAGS right away after PKG_CHECK_MODULES([JPEG], [libjpeg]).

[nikic]

Thanks for pointing this out. I've changed this to set the defines directly in the checking functions. I've also replaced the GDLIB_CFLAGS with AC_DEFINEs. While these are only strictly necessary for the libgd build, I think that handling them via config.h is easier.

I've also dropped the feature checks in case an external libgd is used. In that case features are detected in a different way, by looking at symbols in the library. Whether libjpeg etc are present when PHP is compiled should not be relevant in that case.

[cmb69]

While these are only strictly necessary for the libgd build, I think that handling them via config.h is easier.

ACK. However, ext/gd/libgd/gd_webp.c is not ready for that, since it includes gd.h (which includes config.h) only if HAVE_LIBWEBP is defined. So we'd also need something like:

 ext/gd/libgd/gd_webp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ext/gd/libgd/gd_webp.c b/ext/gd/libgd/gd_webp.c
index 51295797f5..bcd8008eab 100644
--- a/ext/gd/libgd/gd_webp.c
+++ b/ext/gd/libgd/gd_webp.c
@@ -1,10 +1,11 @@
-#ifdef HAVE_LIBWEBP
 #include <stdio.h>
 #include <math.h>
 #include <string.h>
 #include <stdlib.h>
 #include "gd.h"
 #include "gdhelpers.h"
+
+#ifdef HAVE_LIBWEBP
 #include "webp/decode.h"
 #include "webp/encode.h"

BTW: is there a particular reason why our Travis builds don't use --with-webp?

[nikic]

I've added these changes and also added --with-webp on Travis. Let's see if it runs into any issues.

[nikic]

I dropped this, because it does not appear to be in use.

[cmb69]

LGTM. Thanks for working on this!

[nikic]

Merged as 19d8a6b, UPGRADING added in 48ca2c0.

[cmb69]

I think we should also switch to pkg-config for system libgd (it is supported as of libgd 2.1.0, which we require anyway). Not sure about the option names, though. Maybe --enable-gd and --with-external-gd?

[nikic]

@cmb69 That sounds reasonable to me. I used --with-external-pcre for the PCRE case, so that would be consistent.

[cmb69]

@nikic Thanks! I'll submit a respective PR as soon as possible.