Check for iODBC and unixODBC with pkg-config in PDO_ODBC

[NattyNarwhal]

PDO_ODBC required that these backends had their path specified manually, which was clumsy and contrary to how procedural ODBC checked it. This adds a pkg-config based path to check for these backends that ignores the 'dir' part of the flag, so i.e. --with-pdo-odbc=unixODBC should pick it up from the correct location.

Generic and the special ibm-db2 usecase should be unaffected. The header situation is unfortunately ugly, and has a workaround; this should also be cleaned up.

[NattyNarwhal]

Some nitpicks I've already thought of:

• PHP_PDO_ODBC_CHECK_HEADERS is kinda ugly. It should probably just use the appended CFLAGS instead of checking for files.
  • It's also kind of a mess with the number of headers it checks; how many end up getting used? Procedural ODBC imports a smaller set, but does so by checking known driver (managers) ahead of time and knowing what to pull in with that.
• We're frobbing pkg-config manually; this does kinda break overriding PKG_CONFIG via autoconf magically checking PDO_ODBC_CFLAGS/LIBS, see above. (intl does it too, so maybe it's fine?)
• I don't really know how useful ibm-db2 as a special flavour is here.

[petk]

Then this headers check needs to be moved a bit

diff --git a/ext/pdo_odbc/config.m4 b/ext/pdo_odbc/config.m4
index 6de1a363f5..a3de406213 100644
--- a/ext/pdo_odbc/config.m4
+++ b/ext/pdo_odbc/config.m4
@@ -97,9 +97,6 @@ if test "$PHP_PDO_ODBC" != "no"; then
       AC_MSG_WARN([library dir $PDO_ODBC_LIBDIR does not exist])
     fi
 
-    AS_VAR_IF([php_pdo_odbc_have_header], [yes],,
-      [AC_MSG_ERROR([Cannot find header file(s) for pdo_odbc.])])
-
     PDO_ODBC_INCLUDE="$pdo_odbc_def_cflags -I$PDO_ODBC_INCDIR -DPDO_ODBC_TYPE=\\\"$pdo_odbc_flavour\\\""
     PDO_ODBC_LDFLAGS="$pdo_odbc_def_ldflags -L$PDO_ODBC_LIBDIR -l$pdo_odbc_def_lib"
 
@@ -141,6 +138,9 @@ functions required for PDO support.
   PHP_PDO_ODBC_CHECK_HEADER([sqlunix.h])
   PHP_PDO_ODBC_CHECK_HEADER([udbcext.h])
 
+  AS_VAR_IF([php_pdo_odbc_have_header], [yes],,
+    [AC_MSG_ERROR([Cannot find header file(s) for pdo_odbc.])])
+
   PHP_NEW_EXTENSION(pdo_odbc, pdo_odbc.c odbc_driver.c odbc_stmt.c, $ext_shared,, $PDO_ODBC_INCLUDE)
   PHP_SUBST([PDO_ODBC_SHARED_LIBADD])
   PHP_ADD_EXTENSION_DEP(pdo_odbc, pdo)

If these ODBC libraries have pkg-config/pkgconf integrated then that's fine. I haven't checked this so much in details across the systems out there. This pkg-config would ideally be optional but since existing PHP extensions already use it unconditionally, then I think that's ok. Perhaps also good to push on the distributions a bit to integrate it if they haven't already.

For those headers I was already checking it how it could be done with AC_CHECK_HEADER(S) but the issue is that some special order needs to be done. For example, sqlucode.h needs sqltypes.h and probably some others - sqlunix.h. This AC_CHECK_HEADER does a minor compilation step also if header can be included in the code.

[petk]

Or if that check becomes something like this:

AC_DEFUN([PHP_PDO_ODBC_CHECK_HEADER],
[AC_MSG_CHECKING([for $1])
  AC_PREPROC_IFELSE([AC_LANG_PROGRAM([#include <$1>], [])], [
    php_pdo_odbc_have_header=yes
    AC_DEFINE_UNQUOTED(AS_TR_CPP([HAVE_$1]), [1],
      [Define to 1 if you have the <$1> header file.])
      AC_MSG_RESULT([yes])
  ], [AC_MSG_RESULT([no])])
])

then the CFLAGS can be used.

[NattyNarwhal]

unixODBC and iODBC upstreams ship .pc files. Procedural ODBC already uses pkg-config unconditionally for iODBC, but offers manually setting it for unixODBC. Since autoconf does allow overriding the flags that pkg-config would set on it (well, except for invoking pkg-config manually), I think using PKG_CHECK_MODULES should be OK.

I tried using AC_CHECK_HEADERS, but it didn't seem to want to pick up CFLAGS; autoconf documentation implies it should be used with system headers only. You could probably abuse pushing/popping, but your proposal makes more sense to me and would remove the manual invocation of pkg-config.

[NattyNarwhal]

Hmm, your proposed fix doesn't work. I think AC_CHECK_HEADERS and friends rely on CPPFLAGS instead of CFLAGS as they want just the preprocessor flags. Could modify it, but then GH-14939 would be pretty handy right now...

[petk]

Ah, the AC_PREPROC_IFELSE is a preprocessor check only, yes. Nice catch. This AC_CHECK_HEADER not using the CFLAGS is something buggy indeed. Then we have to fix some checks also elsewhere.

[petk]

Ah, the AC_PREPROC_IFELSE is a preprocessor check only, yes. Nice catch. This AC_CHECK_HEADER not using the CFLAGS is something buggy indeed. Then we have to fix some checks also elsewhere.

Tested: AC_CHECK_HEADER and CFLAGS work ok. There is over-escaped double quote in there somewhere.

[NattyNarwhal]

Unfortunately, I think those are load bearing escapes; seems they're needed so cpp embeds them as strings:

In file included from /Users/calvin/src/php-src/ext/pdo_odbc/pdo_odbc.c:28:
ext/pdo_odbc/pdo_odbc_arginfo.h:10:44: error: use of undeclared identifier 'unixODBC'
        REGISTER_STRING_CONSTANT("PDO_ODBC_TYPE", PDO_ODBC_TYPE, CONST_PERSISTENT);
                                                  ^
<command line>:3:23: note: expanded from macro 'PDO_ODBC_TYPE'
#define PDO_ODBC_TYPE unixODBC
                      ^
/Users/calvin/src/php-src/ext/pdo_odbc/pdo_odbc.c:129:54: error: expected ')'
        php_info_print_table_row(2, "PDO Driver for ODBC (" PDO_ODBC_TYPE ")" , "enabled");
                                                            ^
<command line>:3:23: note: expanded from macro 'PDO_ODBC_TYPE'
#define PDO_ODBC_TYPE unixODBC
                      ^
/Users/calvin/src/php-src/ext/pdo_odbc/pdo_odbc.c:129:26: note: to match this '('
        php_info_print_table_row(2, "PDO Driver for ODBC (" PDO_ODBC_TYPE ")" , "enabled");
                                ^
2 errors generated.
/bin/sh /Users/calvin/src/php-src/libtool --silent --preserve-dup-deps --tag=CC --mode=compile cc -Iext/random/ -I/Users/calvin/src/php-src/ext/random/ -I/Users/calvin/src/php-src/main -I/Users/calvin/src/php-src -I/Users/calvin/src/php-src/ext/date/lib -I/opt/local/include -I/Users/calvin/src/php-src/TSRM -I/Users/calvin/src/php-src/Zend  -D_GNU_SOURCE  -fno-common -Wstrict-prototypes -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -g -ffp-contract=off -fvisibility=hidden -O0 -DZEND_SIGNALS   -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 -c /Users/calvin/src/php-src/ext/random/engine_xoshiro256starstar.c -o ext/random/engine_xoshiro256starstar.lo  -MMD -MF ext/random/engine_xoshiro256starstar.dep -MT ext/random/engine_xoshiro256starstar.lo
make: *** [ext/pdo_odbc/pdo_odbc.lo] Error 1
make: *** Waiting for unfinished jobs....
/Users/calvin/src/php-src/ext/pdo_odbc/odbc_driver.c:399:29: error: expected ')'
                        ZVAL_STRING(val, "ODBC-" PDO_ODBC_TYPE);
                                                 ^
<command line>:3:23: note: expanded from macro 'PDO_ODBC_TYPE'
#define PDO_ODBC_TYPE unixODBC
                      ^
/Users/calvin/src/php-src/ext/pdo_odbc/odbc_driver.c:399:4: note: to match this '('
                        ZVAL_STRING(val, "ODBC-" PDO_ODBC_TYPE);
                        ^
/Users/calvin/src/php-src/Zend/zend_API.h:956:20: note: expanded from macro 'ZVAL_STRING'
                const char *_s = (s);                                   \
                                 ^
1 error generated.
make: *** [ext/pdo_odbc/odbc_driver.lo] Error 1

[petk]

Then something like this might be simpler to simply avoid those quotes handling in the shell:

diff --git a/ext/pdo_odbc/config.m4 b/ext/pdo_odbc/config.m4
index 7e2e53b594..dc28444812 100644
--- a/ext/pdo_odbc/config.m4
+++ b/ext/pdo_odbc/config.m4
@@ -75,7 +75,6 @@ if test "$PHP_PDO_ODBC" != "no"; then
     PKG_CHECK_MODULES([PDO_ODBC], [$pdo_odbc_pkgconfig_module])
     PHP_EVAL_INCLINE([$PDO_ODBC_CFLAGS])
     PHP_EVAL_LIBLINE([$PDO_ODBC_LIBS], [PDO_ODBC_SHARED_LIBADD])
-    PDO_ODBC_INCLUDE="$PDO_ODBC_CFLAGS -DPDO_ODBC_TYPE=\\\"$pdo_odbc_flavour\\\""
   else
     if test -n "$pdo_odbc_dir"; then
       PDO_ODBC_INCDIR="$pdo_odbc_dir/include"
@@ -93,7 +92,7 @@ if test "$PHP_PDO_ODBC" != "no"; then
       AC_MSG_WARN([library dir $PDO_ODBC_LIBDIR does not exist])
     fi
 
-    PDO_ODBC_INCLUDE="$pdo_odbc_def_cflags -I$PDO_ODBC_INCDIR -DPDO_ODBC_TYPE=\\\"$pdo_odbc_flavour\\\""
+    PDO_ODBC_CFLAGS="$pdo_odbc_def_cflags -I$PDO_ODBC_INCDIR"
     PDO_ODBC_LDFLAGS="$pdo_odbc_def_ldflags -L$PDO_ODBC_LIBDIR -l$pdo_odbc_def_lib"
 
     PHP_EVAL_LIBLINE([$PDO_ODBC_LDFLAGS], [PDO_ODBC_SHARED_LIBADD])
@@ -117,7 +116,7 @@ functions required for PDO support.
   fi
 
   OLD_CPPFLAGS="$CPPFLAGS"
-  CPPFLAGS="$CPPFLAGS $PDO_ODBC_INCLUDE"
+  CPPFLAGS="$CPPFLAGS $PDO_ODBC_CFLAGS"
   PHP_PDO_ODBC_CHECK_HEADER([cli0cli.h])
   PHP_PDO_ODBC_CHECK_HEADER([cli0core.h])
   PHP_PDO_ODBC_CHECK_HEADER([cli0defs.h])
@@ -140,7 +139,10 @@ functions required for PDO support.
   AS_VAR_IF([php_pdo_odbc_have_header], [yes],,
     [AC_MSG_ERROR([Cannot find header file(s) for pdo_odbc.])])
 
-  PHP_NEW_EXTENSION(pdo_odbc, pdo_odbc.c odbc_driver.c odbc_stmt.c, $ext_shared,, $PDO_ODBC_INCLUDE)
+  AC_DEFINE_UNQUOTED([PDO_ODBC_TYPE], ["$pdo_odbc_flavour"],
+    [Define to the ODBC driver or driver manager value.])
+
+  PHP_NEW_EXTENSION(pdo_odbc, pdo_odbc.c odbc_driver.c odbc_stmt.c, $ext_shared,, $PDO_ODBC_CFLAGS)
   PHP_SUBST([PDO_ODBC_SHARED_LIBADD])
   PHP_ADD_EXTENSION_DEP(pdo_odbc, pdo)

[petk]

Probably the CS and quotes could be further synced here, specially in that PHP_CHECK_LIBRARY, but it works anyway, so all good to go then.

Thanks @NattyNarwhal for the pkg-config integration. That is one step better and simpler.