Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHPC-2382: Allow static builds with php-src #1555

Merged
merged 7 commits into from
May 22, 2024

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented May 14, 2024

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

Prefer PHP_VERSION_ID for static builds, and fall back to invoking php-config otherwise.

mongocrypt.h previously used a path relative to the PHPC project root, which is not compatible with static builds. The current path is relative to src/, which is explicitly defined as an include path in config.m4.

Related: #1549

Prefer PHP_VERSION_ID for static builds, and fall back to invoking php-config otherwise.

mongocrypt.h previously used a path relative to the PHPC project root, which is not compatible with static builds. The current path is relative to src/, which is explicitly defined as an include path in config.m4.
@jmikola jmikola requested a review from alcaeus May 14, 2024 00:17
@jmikola
Copy link
Member Author

jmikola commented May 14, 2024

I manually tested this with a build of php-8.3.7; however, I'd like to test it with an Alpine container before merging to ensure there's no regression (re: PHPC-2195).

jmikola added 5 commits May 21, 2024 14:43
PlatformFlags.m4 modifies CPPFLAGS in order to make checks in subsequent M4 scripts consistent with PHP_MONGODB_BUNDLED_CFLAGS. Restoring its original value after executing all M4 scripts will avoid unintended side effects on a static PHP build.
STD_CFLAGS was never appended to PHP_MONGODB_BUNDLED_CFLAGS after these M4 scripts were run, so it's safe to assume they had no effect and can be removed entirely.

For CheckCompiler.m4, bumping the GCC requirement to 4.6 (still quite old) will ensure compatibility for libmongoc's BEGIN_IGNORE_DEPRECATIONS macro.
This comment should have been removed in d8d30e5
@@ -18,12 +18,4 @@ AS_IF([test "$os_darwin" = "yes"],[
dnl https://opensource.apple.com/source/Libc/Libc-1439.40.11/gen/compat.5.auto.html
CPPFLAGS="$CPPFLAGS -D_DARWIN_C_SOURCE"
PHP_MONGODB_BUNDLED_CFLAGS="$PHP_MONGODB_BUNDLED_CFLAGS -D_DARWIN_C_SOURCE"

dnl Ignore OpenSSL deprecation warnings on OSX
AX_CHECK_COMPILE_FLAG([-Wno-deprecated-declarations], [STD_CFLAGS="$STD_CFLAGS -Wno-deprecated-declarations"])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the AX_CHECK_COMPILE_FLAG calls in this file seemed safe to remove since the modified $STD_CFLAGS never actually got used. $PHP_MONGODB_BUNDLED_CFLAGS is set before any of these M4 scripts are executed.

This line in particular was based on old behavior in libmongoc, which still persists in the current CMake config. I opened CDRIVER-5576 to report that, as it's likely obsolete. The fact that this code never ran and we received no reports would support that hypothesis.

#error Does not support deprecation warning pragmas
#endif
#endif
])], [], [STD_CFLAGS="$STD_CFLAGS -Wno-deprecated-declarations"])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to PlatformFlags.m4, this would have had no effect. The fact that we never received a bug report about this suggests it's fine to just require GCC 4.6+.

@@ -350,6 +359,9 @@ if test "$PHP_MONGODB" != "no"; then
m4_include(PHP_MONGODB_BASEDIR/scripts/autotools/libmongocrypt/Endian.m4)
m4_include(PHP_MONGODB_BASEDIR/scripts/autotools/libmongocrypt/Version.m4)

dnl Restore CPPFLAGS once all M4 scripts have executed
CPPFLAGS="$old_CPPFLAGS"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual change proposed and validated in #1549 (comment). I've just reduced the scope to the M4 files instead of the entire if block.

@@ -110,13 +118,11 @@ if test "$PHP_MONGODB" != "no"; then
AC_MSG_ERROR(code coverage is not supported for static builds)
fi

COVERAGE_CFLAGS="--coverage -g"
COVERAGE_LDFLAGS="--coverage"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable had no purpose beyond injection into $MONGODB_SHARED_LIBADD.

fi

MAINTAINER_CFLAGS="$_MAINTAINER_CFLAGS"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasons for these names are lost to time, but since I'm now initializing $PHP_MONGODB_DEV_CFLAGS and appending to it directly we can do without this.


AC_MSG_RESULT($PHP_MONGODB_PHP_VERSION)

if test "$PHP_MONGODB_PHP_VERSION_ID" -lt "70400"; then
AC_MSG_ERROR([not supported. Need a PHP version >= 7.4.0 (found $PHP_MONGODB_PHP_VERSION)])
fi

PHP_MONGODB_STD_CFLAGS=""
PHP_MONGODB_DEV_CFLAGS=""
PHP_MONGODB_COVERAGE_CFLAGS=""
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using prefixes and initializing these vars ensures we won't inherit state during a static build (admittedly unlikely).

else
PHP_MONGODB_PHP_VERSION="${PHP_VERSION}"
PHP_MONGODB_PHP_VERSION_ID="${PHP_VERSION_ID}"
fi
Copy link
Member Author

@jmikola jmikola May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I successfully ran ./configure on Alpine using the php:8.2-alpine Docker image. Referred back to instructions in #1477 (comment).

This still works, although it did reveal a bug in the build config output where we print "Bundled ()" when libmongocrypt isn't configured. Opened PHPC-2386 to track that.

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, thank you for cleaning up some old cruft. I tested this on MacOS and confirmed that it still builds as expected.

Note that the Windows builds for PHP 7.4 are currently failing, which we'll have to investigate.

config.m4 Outdated Show resolved Hide resolved
@jmikola
Copy link
Member Author

jmikola commented May 22, 2024

Note that the Windows builds for PHP 7.4 are currently failing, which we'll have to investigate.

This is related to GitHub removing some older toolsets from the windows-2022 image:

I'll look into whether we can quickly work around that by using an older build image for 7.4.

Co-authored-by: Andreas Braun <[email protected]>
@jmikola jmikola merged commit 21d46fe into mongodb:v1.19 May 22, 2024
38 of 51 checks passed
@jmikola jmikola deleted the 1.19-phpc-2382 branch May 22, 2024 13:46
This was referenced May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants