PHPC-759: Ensure that the driver can be compiled as a built-in PHP extension #830

jmikola · 2018-05-10T22:42:11Z

https://jira.mongodb.org/browse/PHPC-759

This will supersede #749

jmikola · 2018-05-11T21:09:12Z

scripts/build/autotools/libbson/FindDependencies.m4

-           [AC_MSG_ERROR([libbson requires pthreads on non-Windows platforms.])])
+AX_PTHREAD([
+  PHP_EVAL_INCLINE([$PTHREAD_CFLAGS])
+  PHP_EVAL_LIBLINE([$PTHREAD_LIBS],[MONGODB_SHARED_LIBADD])


This is still problematic for static builds, as pthread doesn't get linked to PHP. I believe this is similar to what @derickr brought up in https://github.com/mongodb/mongo-php-driver/pull/749/files#r166189048, although I'm still curious how other extensions manage to link libraries for shared/static builds in a portable manner.

@sgolemon: can you chime in?

This is already done by PHP for shared and static builds.

jmikola · 2018-05-14T22:00:26Z

config.m4

-        AC_MSG_RESULT(version $LIBBSON_VER found)
+        PHP_MONGODB_BSON_CFLAGS=`$PKG_CONFIG libbson-1.0 --cflags`
+        PHP_MONGODB_BSON_LIBS=`$PKG_CONFIG libbson-1.0 --libs`
+        PHP_MONGODB_BSON_VERSION=`$PKG_CONFIG libbson-1.0 --modversion`


These names were changed for consistency with other CheckWhatever.m4 files and the PHP version check up top. I'm all for more "PHP_MONGODB" prefixing.

jmikola · 2018-05-14T22:01:29Z

config.m4

      else
        AC_MSG_ERROR(system libbson must be upgraded to version >= 1.9.0)
      fi
    else
      AC_MSG_ERROR(pkgconfig and libbson must be installed)
    fi
-    PHP_EVAL_INCLINE($LIBBSON_INC)
-    PHP_EVAL_LIBLINE($LIBBSON_LIB, MONGODB_SHARED_LIBADD)
+    PHP_MONGODB_CFLAGS="$PHP_MONGODB_CFLAGS $PHP_MONGODB_BSON_CFLAGS"


On the off chance that pkg-config suggests CFLAGS that aren't just include directories, this ensures they get added. I imagine it's unlikely that libbson and the other libraries we use will do so, but I think this is the correct approach given the limitations of PHP_EVAL_INCLINE.

I do think it's OK to continue using PHP_EVAL_LIBLINE, since is less limited. We just require a single work-around for pthreads, which I mention below.

AFAIK, PHP_EVAL_INCLINE also makes sure there are no duplicates - not sure if that could be a problem here.

(Not important, if this is broken, people will tell us)

PHP_EVAL_INCLINE calls PHP_ADD_INCLUDE for each -I path, which calls both PHP_EXPAND_PATH (expanding to an absolute path) and PHP_RUN_ONCE to ensure each path is only added once.

Nearly all of these instances obtain CFLAGS from pkg-config being called for a unique library, so I'm comfortable trusting that their CFLAGS can be added as-is. The only overlap IMO is libmongoc and libbson. Since libmongoc's libmongoc-1.0.pc file depends on libbson-1.0, it ends up including libbson's pkg-config output in its own. To address that, I suppose we could skip merging in libbson's own pkg-config output and just rely on libmongoc. I'd propose making this change today.

One real exception is AX_PTHREAD, which is likely going to return -pthread (ignored by PHP_EVAL_INCLINE entirely) instead of an -I path. For that, I think we certainly need to just append to PHP_MONGODB_CFLAGS.

One other thought. There are more pkg-config options than I realized. Per pkg-config(1), we can request -I CFLAGS and "other" CFLAGS independently, which would let us rely on PHP_EVAL_INCLINE and defer to PHP_MONGODB_CFLAGS when necessary. Similarly, we can requires -L and -l libs independently from "other" linker arguments (which might require manual appends instead of PHP_EVAL_LIBLINE). While this is trivial to add for libbson and libmongoc, since we call pkg-config directly, we also know by looking at their package files that it would have no functional impact. It could be helpful for other libraries (e.g. CheckSSL.m4), but those are all using the pkg.m4 macro at the moment and would require some significant refactoring. I don't think there's a pressing need for this, but it does seem like it'd be the "correct" approach. If it sounds worthwhile, I can make a separate ticket to track the change and we can triage it for some future release.

I wouldn't worry about it then. I'd defer making a ticket until somebody notices it.

Created PHPC-1190.

jmikola · 2018-05-14T22:05:22Z

config.m4

+        AC_MSG_ERROR([m4 could not include $1: No such file or directory])
+      fi
+      m4_builtin([sinclude],[$1])
+      m4_builtin([sinclude],[ext/mongodb/][$1])


I think we can assume that the extension will be build from this path, since it's the extension's name. That said, users need to manually copy/symlink in order to build statically so I suppose they could inadvertently place us under a different path. If so, the conditional a few lines up isn't going to point out that the ext/FOO path is the problem.

Let me know if you'd like a more helpful error for that case. It may be as simple as adding "ext/mongodb" to the error message, or conditionally printing a special message if $enable_static=yes.

I don't think we should do this, as we don't want to promote compiling the driver as a static extension.

jmikola · 2018-05-14T22:05:55Z

config.m4

+    _include([scripts/build/autotools/libmongoc/Versions.m4])
+    _include([scripts/build/autotools/libmongoc/WeakSymbols.m4])
+
+    m4_popdef([_include])


Just cleaning up after ourselves so not to pollute the global M4 namespace. This should also be flexible in the event that some other extension (or PHP) defines an _include macro.

Or also prefix it with php_mongodb_ ?

Currently there is no risk of conflicting with an _include macro defined outside of this push/pop scope. The only risk would be if one of the M4 files we include defined its own _include macro without also using push/pop and overwrote our own.

PHP_MONGODB_ADD_SOURCES and friends can't use push/pop (reasonably) since we define them in an included file and call them later on in config.m4. Prefixing makes sense there to be a good citizen, but I don't think prefixing _include would add anything here (apart from verbosity).

jmikola · 2018-05-14T22:08:34Z

config.m4

-      src/libbson/src/bson/bson-version.h
-      src/libmongoc/src/mongoc/mongoc-config.h
-      src/libmongoc/src/mongoc/mongoc-version.h
+      ${ac_config_dir}/src/libbson/src/bson/bson-config.h


In my testing, I wasn't able to use PHP_EXT_SRCDIR(mongodb) inside of AC_CONFIG_FILES, as it seems to care about its literal arguments to complain if it's invoked twice with the same parameters.

jmikola · 2018-05-14T22:11:02Z

config.m4

@@ -325,10 +348,17 @@ if test "$PHP_MONGODB" != "no"; then
  PHP_ADD_BUILD_DIR(PHP_EXT_BUILDDIR(mongodb)[/src/MongoDB/Monitoring/])
  PHP_ADD_BUILD_DIR(PHP_EXT_BUILDDIR(mongodb)[/src/contrib/])

+  dnl Necessary to ensure that static builds include "-pthread" when linking
+  if test "$ext_shared" != "yes"; then
+    EXTRA_LDFLAGS_PROGRAM="$EXTRA_LDFLAGS_PROGRAM $EXTRA_LDFLAGS"


I took a look at the Makefile produced when building PHP statically and found that EXTRA_LDFLAGS was ignored when making the actual SAPI binaries. I'm not sure why that is the case, but EXTRA_LDFLAGS_PROGRAM is actually included in the linking for all SAPI binaries.

We're adding $EXTRA_LDFLAGS here because that's going to be set when detecting pthreads (see my next comment in FindDependencies.m4).

jmikola · 2018-05-14T22:11:36Z

config.m4

@@ -325,10 +348,17 @@ if test "$PHP_MONGODB" != "no"; then
  PHP_ADD_BUILD_DIR(PHP_EXT_BUILDDIR(mongodb)[/src/MongoDB/Monitoring/])
  PHP_ADD_BUILD_DIR(PHP_EXT_BUILDDIR(mongodb)[/src/contrib/])

+  dnl Necessary to ensure that static builds include "-pthread" when linking


If you decide that --coverage is a relevant option for static builds, I can amend this comment. Otherwise, I believe this only applies to -pthread.

As I said in the other PR: I don't think it's relevant.

jmikola · 2018-05-14T22:15:37Z

config.m4

-AC_CONFIG_COMMANDS_POST([echo "
+  AC_CONFIG_COMMANDS_POST([
+    if test "$enable_static" = "no"; then
+      echo "


AC_CONFIG_COMMANDS_POST emits no immediate output in the configure file and instead queues up output for the end of configure generation (after config.status). Therefore, any logic needs to go in its body.

I opted not to use $ext_shared here since this won't actually be executed anywhere near our extension's own configuration and I was worried that the logic might use the $ext_shared value from the very last extension that was configured. $enable_static seemed more reliable as it's always no for phpize shared builds and it appears to always be yes when building PHP itself.

jmikola · 2018-05-14T22:21:18Z

scripts/build/autotools/libbson/FindDependencies.m4

+
+  # PTHREAD_CFLAGS may come back as "-pthread", which should also be used when
+  # linking. We can trust PHP_EVAL_LIBLINE to ignore other values.
+  PHP_EVAL_LIBLINE([$PTHREAD_CFLAGS],[MONGODB_SHARED_LIBADD])


This is an odd edge case.

PHP_EVAL_LIBLINE has a special case where if it sees -pthread (instead of the usual -lfoo) and the build is static, it adds it to EXTRA_LDFLAGS instead of MONGODB_SHARED_LIBADD. For whatever reason, AX_PTHREAD's own tests doesn't determine that -pthread is required as a link flag, so it only reports it in CFLAGS. Without this, we'll get symbol errors when linking libbson object files in static builds.

Note: this goes hand in hand with our logic in config.m4 to ensure EXTRA_LDFLAGS is merged into EXTRA_LDFLAGS_PROGRAM for static builds.

jmikola · 2018-05-14T22:24:07Z

scripts/build/autotools/libmongoc/FindDependencies.m4

-dnl AX_PTHREAD
+# Check for pthreads. libmongoc's original FindDependencies.m4 script did not
+# require pthreads, but it does appear to be necessary on non-Windows platforms
+# based on mongoc-openssl.c and mongoc-thread-private.h.


I'm not sure why libmongoc always considered pthreads optional, but they are certainly assumed to exist in these files. It's possible that this was an edge case and the dependency was satisfied by also linking libbson successfully.

In any event, I think this is correct and I'm not going to both libmongoc about it since the M4 file in question no longer exists in their repository.

jmikola · 2018-05-14T22:25:19Z

scripts/build/autotools/libmongoc/FindDependencies.m4

+
+  # PTHREAD_CFLAGS may come back as "-pthread", which should also be used when
+  # linking. We can trust PHP_EVAL_LIBLINE to ignore other values.
+  PHP_EVAL_LIBLINE([$PTHREAD_CFLAGS],[MONGODB_SHARED_LIBADD])


Note: PHP uses PHP_RUN_ONCE when adding -pthread, so we don't end up with duplicates from the libbson check done earlier. That said, I don't think it'd be problematic even if we did.

jmikola · 2018-05-14T22:26:46Z

@derickr: I wasn't able to perform a static build locally with PHP 5.5 due to some token errors that appeared unrelated to our extension. Happy to discuss that tomorrow, but I'd appreciate it if you could test this locally with 5.x and 7.x anyway to ensure everything is on the up and up.

derickr

I tried a static build with PHP 5.5, and it works fine:

[GIT: PHP-5.5]
derick@singlemalt:~/dev/php/php-src.git $ ./sapi/cli/php -n -m | grep mongodb
mongodb

derickr · 2018-05-15T09:43:51Z

config.m4

+  PHP_MONGODB_PHP_VERSION=$PHP_VERSION
+  PHP_MONGODB_PHP_VERSION_ID=$PHP_VERSION_ID
+
+  if test -z "$PHP_MONGODB_PHP_VERSION"; then


How can it sometimes be set, and sometimes not?

test -z checks if the string has non-zero length. For shared builds, PHP_VERSION is never set so PHP_MONGODB_PHP_VERSION will be initialized as an empty string.

derickr · 2018-05-15T09:44:16Z

config.m4

+
+  AC_MSG_RESULT($PHP_MONGODB_PHP_VERSION)
+  if test "$PHP_MONGODB_PHP_VERSION_ID" -lt "50500"; then
+    AC_MSG_ERROR([not supported. Need a PHP version >= 5.5.0 (found $PHP_MONGODB_PHP_VERSION)])


Do we currently want a check to make sure it's also lower than 7.3?

I don't think so. In the case of 7.3, we know there's a BC break for iteration that leads to a compile-time error, but jumping between earlier 7.x releases didn't require changes IIRC. I'm fine with that behavior.

Similarly, we don't limit the libbson/libmongoc shared library versions with an upper bound -- provided they satisfy 1.0 ABI. I realize they're more strict about BC than PHP, but it's also possible that behaviors of some of the internal functions we use could change inadvertently :)

derickr · 2018-05-15T09:45:46Z

config.m4

      else
        AC_MSG_ERROR(system libbson must be upgraded to version >= 1.9.0)
      fi
    else
      AC_MSG_ERROR(pkgconfig and libbson must be installed)
    fi
-    PHP_EVAL_INCLINE($LIBBSON_INC)
-    PHP_EVAL_LIBLINE($LIBBSON_LIB, MONGODB_SHARED_LIBADD)
+    PHP_MONGODB_CFLAGS="$PHP_MONGODB_CFLAGS $PHP_MONGODB_BSON_CFLAGS"


AFAIK, PHP_EVAL_INCLINE also makes sure there are no duplicates - not sure if that could be a problem here.

derickr · 2018-05-15T09:48:08Z

config.m4

+        AC_MSG_ERROR([m4 could not include $1: No such file or directory])
+      fi
+      m4_builtin([sinclude],[$1])
+      m4_builtin([sinclude],[ext/mongodb/][$1])


I don't think we should do this, as we don't want to promote compiling the driver as a static extension.

derickr · 2018-05-15T09:48:36Z

config.m4

+    _include([scripts/build/autotools/libmongoc/Versions.m4])
+    _include([scripts/build/autotools/libmongoc/WeakSymbols.m4])
+
+    m4_popdef([_include])


Or also prefix it with php_mongodb_ ?

derickr · 2018-05-15T09:49:44Z

config.m4

@@ -325,10 +348,17 @@ if test "$PHP_MONGODB" != "no"; then
  PHP_ADD_BUILD_DIR(PHP_EXT_BUILDDIR(mongodb)[/src/MongoDB/Monitoring/])
  PHP_ADD_BUILD_DIR(PHP_EXT_BUILDDIR(mongodb)[/src/contrib/])

+  dnl Necessary to ensure that static builds include "-pthread" when linking


As I said in the other PR: I don't think it's relevant.

derickr

derickr

LGTM

derickr · 2018-05-15T13:54:26Z

config.m4

+  PHP_MONGODB_PHP_VERSION=$PHP_VERSION
+  PHP_MONGODB_PHP_VERSION_ID=$PHP_VERSION_ID
+
+  if test -z "$PHP_MONGODB_PHP_VERSION"; then


derickr · 2018-05-15T13:54:34Z

config.m4

+
+  AC_MSG_RESULT($PHP_MONGODB_PHP_VERSION)
+  if test "$PHP_MONGODB_PHP_VERSION_ID" -lt "50500"; then
+    AC_MSG_ERROR([not supported. Need a PHP version >= 5.5.0 (found $PHP_MONGODB_PHP_VERSION)])


derickr · 2018-05-15T13:56:13Z

config.m4

+    _include([scripts/build/autotools/libmongoc/Versions.m4])
+    _include([scripts/build/autotools/libmongoc/WeakSymbols.m4])
+
+    m4_popdef([_include])


derickr · 2018-05-15T13:59:25Z

config.m4

      else
        AC_MSG_ERROR(system libbson must be upgraded to version >= 1.9.0)
      fi
    else
      AC_MSG_ERROR(pkgconfig and libbson must be installed)
    fi
-    PHP_EVAL_INCLINE($LIBBSON_INC)
-    PHP_EVAL_LIBLINE($LIBBSON_LIB, MONGODB_SHARED_LIBADD)
+    PHP_MONGODB_CFLAGS="$PHP_MONGODB_CFLAGS $PHP_MONGODB_BSON_CFLAGS"


(Not important, if this is broken, people will tell us)

derickr

LGTM

derickr · 2018-05-15T14:17:21Z

config.m4

      else
        AC_MSG_ERROR(system libbson must be upgraded to version >= 1.9.0)
      fi
    else
      AC_MSG_ERROR(pkgconfig and libbson must be installed)
    fi
-    PHP_EVAL_INCLINE($LIBBSON_INC)
-    PHP_EVAL_LIBLINE($LIBBSON_LIB, MONGODB_SHARED_LIBADD)
+    PHP_MONGODB_CFLAGS="$PHP_MONGODB_CFLAGS $PHP_MONGODB_BSON_CFLAGS"


I wouldn't worry about it then. I'd defer making a ticket until somebody notices it.

jmikola force-pushed the phpc-759 branch from c4839a5 to 38ef46e Compare May 11, 2018 21:07

jmikola commented May 11, 2018

View reviewed changes

jmikola force-pushed the phpc-759 branch from 38ef46e to 48902e1 Compare May 14, 2018 21:54

jmikola added 6 commits May 14, 2018 17:59

PHPC-759: Fix PHP version check for static builds

94d8c77

PHPC-759: Use flexible M4 includes for static/shared builds

62894c1

PHPC-759: Use absolute path for AC_CONFIG_FILES

d01b08b

Move pthreads CFLAG/LIBS additions to AX_PTHREAD's action-if-found

7d1de0d

PHPC-759: Flexible macros for adding sources, includes, and build paths

dd36d74

Remove empty line

4500d8d

jmikola force-pushed the phpc-759 branch from 48902e1 to b746c97 Compare May 14, 2018 21:59

jmikola requested a review from derickr May 14, 2018 21:59

jmikola changed the title ~~[WIP] PHPC-759: Ensure that the driver can be compiled as a built-in PHP extension~~ PHPC-759: Ensure that the driver can be compiled as a built-in PHP extension May 14, 2018

jmikola added 4 commits May 14, 2018 18:22

PHPC-759: Fix pthread linking for static builds

c0fc8dd

Add CFLAGS directly instead of relying on PHP_EVAL_INCLINE

746f92e

Remove redundant PHP_SUBST(EXTRA_LDFLAGS)

62bb969

This is already done by PHP for shared and static builds.

PHPC-759: Hide extension config summary for static builds

e1827ed

jmikola force-pushed the phpc-759 branch from b746c97 to e1827ed Compare May 14, 2018 22:22

jmikola commented May 14, 2018

View reviewed changes

derickr reviewed May 15, 2018

View reviewed changes

derickr suggested changes May 15, 2018

View reviewed changes

This was referenced May 15, 2018

[WAIT] PHPC-759: Ensure that the driver can be compiled as a built-in PHP extension #749

Closed

Not static compilation #441

Closed

derickr approved these changes May 15, 2018

View reviewed changes

jmikola merged commit e1827ed into mongodb:master May 15, 2018

jmikola added a commit that referenced this pull request May 15, 2018

Merge pull request #830

c78c03f

jmikola deleted the phpc-759 branch May 15, 2018 18:08

PHPC-759: Ensure that the driver can be compiled as a built-in PHP extension #830

PHPC-759: Ensure that the driver can be compiled as a built-in PHP extension #830

Uh oh!

Conversation

jmikola commented May 10, 2018

Uh oh!

jmikola May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola commented May 14, 2018

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jmikola May 11, 2018 •

edited

Loading

jmikola May 15, 2018 •

edited

Loading

derickr May 15, 2018 •

edited

Loading

derickr May 15, 2018 •

edited

Loading

derickr left a comment •

edited

Loading