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

derickr · 2018-02-02T15:48:02Z

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

…tension

derickr

I'm wondering whether we should display the configuration summary for the MongoDB extension. That makes sense for the shared object version, but less so for the in-php-src-tree build where it almost implies it's for all of PHP

derickr · 2018-02-02T15:48:21Z

config.m4

+  AC_MSG_CHECKING([for PHP_CONFIG binary])
+  AC_CHECK_PROG(PHP_CONFIG_BINARY, [${PHP_CONFIG}], [yes], [no])
+  AC_MSG_CHECKING([supported PHP versions])
+  if test x"$PHP_CONFIG_BINARY" == x"yes"; then


The PHP binary doesn't always exist... so we need to check for it.

When was PHP_CONFIG introduced? Just curious where you found it missing.

Note that the leading "x" for equality checks is only necessary if an operand is potentially an empty string. In this case, AC_CHECK_PROG() will either assign "yes" or "no", so you're safe comparing the variable to "yes" directly.

derickr · 2018-02-02T15:49:28Z

config.m4

-    m4_include(scripts/build/autotools/CheckSSL.m4)
+    sinclude(scripts/build/autotools/m4/pkg.m4)
+    sinclude(scripts/build/autotools/CheckHost.m4)
+    sinclude(scripts/build/autotools/CheckSSL.m4)


sinclude works like m4_include, but doesn't bail out if it can't find the file. As this happens during ./buildconf, we can't make use of shell magic so this seems the only (ugly) way: we just try them both.

How is this different than deciding to call PHP_ADD_SOURCES() or PHP_ADD_SOURCES_X() based on the value of $ext_shared? Does that solution not apply here because m4_include() is evaluated earlier to generate the configure script, long before we'd be able to check the value of $ext_shared?

I tried using m4_include(PHP_EXT_DIR(mongodb)[scripts/build/autotools/m4/pkg.m4]) but that results in:

/usr/bin/m4:config.m4:314: cannot open `""scripts/build/autotools/m4/pkg.m4': No such file or directory

Is PHP_EXT_DIR(mongodb) only an empty string when building a shared extension? I found it declared in phpize.m4 as an empty string. There is a more substantial definition in configure.ac, but I assume that's for built-in extensions.

derickr · 2018-02-02T15:49:50Z

config.m4

+    else
+      prefixdir=${srcdir}/ext/mongodb
+    fi
+    AC_CREATE_STDINT_H([$prefixdir/src/libbson/src/bson/bson-stdint.h])


We need to create files in the right directory.

Since this is after PHP_NEW_EXTENSION() is called, can we use $ext_srcdir here and avoid the conditional?

derickr · 2018-02-02T15:50:19Z

config.m4

  fi

+  EXTRA_LIBS="$EXTRA_LIBS $MONGODB_SHARED_LIBADD"


Without this, extra libs like snappy or zlib aren't passed on to PHP.

Is this ignored for shared builds?

I see bits of EXTRA_LIBS in PHP core but it's oddly absent from any extension config.m4 files. Given that many of those link extra libraries and can also be built-in, is there a reason for that?

We discussed why EXTRA_LIBS is needed. Long-term, let's see if we can use PHP_ADD_LIBPATH().

jmikola · 2018-02-06T05:22:31Z

config.m4

+  AC_MSG_CHECKING([for PHP_CONFIG binary])
+  AC_CHECK_PROG(PHP_CONFIG_BINARY, [${PHP_CONFIG}], [yes], [no])
+  AC_MSG_CHECKING([supported PHP versions])
+  if test x"$PHP_CONFIG_BINARY" == x"yes"; then


When was PHP_CONFIG introduced? Just curious where you found it missing.

Note that the leading "x" for equality checks is only necessary if an operand is potentially an empty string. In this case, AC_CHECK_PROG() will either assign "yes" or "no", so you're safe comparing the variable to "yes" directly.

jmikola · 2018-02-06T05:24:17Z

config.m4

+      AC_MSG_ERROR([not supported. Need a PHP version >= 5.5.0 (found $PHP_MONGODB_FOUND_VERSION)])
+    fi
+  else
+    AC_MSG_RESULT([undeterminate, continuing, but need a PHP version >= 5.5.0])


"indeterminate" would be more common to use here.

jmikola · 2018-02-06T05:40:19Z

config.m4

-    m4_include(scripts/build/autotools/CheckSSL.m4)
+    sinclude(scripts/build/autotools/m4/pkg.m4)
+    sinclude(scripts/build/autotools/CheckHost.m4)
+    sinclude(scripts/build/autotools/CheckSSL.m4)


How is this different than deciding to call PHP_ADD_SOURCES() or PHP_ADD_SOURCES_X() based on the value of $ext_shared? Does that solution not apply here because m4_include() is evaluated earlier to generate the configure script, long before we'd be able to check the value of $ext_shared?

I tried using m4_include(PHP_EXT_DIR(mongodb)[scripts/build/autotools/m4/pkg.m4]) but that results in:

/usr/bin/m4:config.m4:314: cannot open `""scripts/build/autotools/m4/pkg.m4': No such file or directory

Is PHP_EXT_DIR(mongodb) only an empty string when building a shared extension? I found it declared in phpize.m4 as an empty string. There is a more substantial definition in configure.ac, but I assume that's for built-in extensions.

jmikola · 2018-02-06T05:42:28Z

config.m4

-    m4_include(src/libbson/build/autotools/m4/ac_compile_check_sizeof.m4)
-    m4_include(src/libbson/build/autotools/m4/ac_create_stdint_h.m4)
-    AC_CREATE_STDINT_H([$srcdir/src/libbson/src/bson/bson-stdint.h])
+    sinclude(src/libbson/build/autotools/CheckAtomics.m4)


Side question: once we sinclude() a file, is an include within that file relative to the file's own path? I wonder if we could clean up a lot of this by having a single gateway include that then includes all of these general m4 files.

jmikola · 2018-02-06T05:45:17Z

config.m4

+    else
+      prefixdir=${srcdir}/ext/mongodb
+    fi
+    AC_CREATE_STDINT_H([$prefixdir/src/libbson/src/bson/bson-stdint.h])


Since this is after PHP_NEW_EXTENSION() is called, can we use $ext_srcdir here and avoid the conditional?

jmikola · 2018-02-06T05:45:39Z

config.m4

+      prefixdir=${srcdir}
+    else
+      prefixdir=${srcdir}/ext/mongodb
+    fi


Same question here regarding $ext_srcdir.

jmikola

Changes look good. Up to you if you'd like this added for 1.4.0 or would rather it sit until we import libmongoc Autotools in 1.5.0.

Either way, I think we should add a blurb in CONTRIBUTING.md that instructs how to easily build PHP from source with the extension built-in (similar to our Windows build example).

jmikola · 2018-05-15T13:44:53Z

Superseded by #830.

PHPC-759: Ensure that the driver can be compiled as a built-in PHP ex…

1c04a9c

…tension

derickr requested review from jmikola and kvwalker February 2, 2018 15:48

derickr commented Feb 2, 2018

View reviewed changes

jmikola reviewed Feb 6, 2018

View reviewed changes

jmikola approved these changes Feb 6, 2018

View reviewed changes

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

wysow mentioned this pull request May 9, 2018

Not static compilation #441

Closed

jmikola mentioned this pull request May 10, 2018

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

Merged

jmikola closed this May 15, 2018

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

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

Uh oh!

Conversation

derickr commented Feb 2, 2018 • edited by jmikola Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Feb 6, 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!

jmikola Feb 6, 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!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

jmikola commented May 15, 2018

Uh oh!

Uh oh!

derickr commented Feb 2, 2018 •

edited by jmikola

Loading

jmikola Feb 6, 2018 •

edited

Loading

jmikola Feb 6, 2018 •

edited

Loading