feat: add --enable-apcu option to toggle APCu support#92
Conversation
📝 WalkthroughWalkthrough
ChangesAPCu Configure Support
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config.m4`:
- Around line 138-147: When APCu was explicitly requested (PHP_APCU == "yes")
the configure script currently prints "not found" and continues; change the
logic in the APCu detection block so that after testing for
"$phpincludedir/ext/apcu/apc_serializer.h" you still set apc_inc_path and
AC_DEFINE(HAVE_APCU_SUPPORT,1,...) when found, but if the header is missing and
PHP_APCU equals "yes" call AC_MSG_ERROR with a clear failure message (e.g.,
"APCu requested but header apc_serializer.h not found in
$phpincludedir/ext/apcu") so configure aborts; if PHP_APCU is "auto" or unset
keep the existing AC_MSG_RESULT([not found]) behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if test "$PHP_APCU" != "no"; then | ||
| AC_MSG_CHECKING([for APCu includes]) | ||
| if test -f "$phpincludedir/ext/apcu/apc_serializer.h"; then | ||
| apc_inc_path="$phpincludedir" | ||
| AC_MSG_RESULT([APCu in $apc_inc_path]) | ||
| AC_DEFINE(HAVE_APCU_SUPPORT, 1, [Whether to enable APCu support]) | ||
| else | ||
| AC_MSG_RESULT([not found]) | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Error when APCu is explicitly enabled but the header is not found.
When a user explicitly enables APCu with --enable-apcu=yes, but the required header file is not found, the configure script should fail with an error instead of silently continuing. Currently, the script only prints "not found" and proceeds without defining HAVE_APCU_SUPPORT, which violates the user's explicit request.
The standard autoconf pattern distinguishes between:
--enable-apcu=auto(or default): Try to detect, continue if not found--enable-apcu=yes(explicit): Require APCu, fail if not found--disable-apcuor--enable-apcu=no: Skip detection
🔧 Proposed fix to error on explicit enable
if test "$PHP_APCU" != "no"; then
AC_MSG_CHECKING([for APCu includes])
if test -f "$phpincludedir/ext/apcu/apc_serializer.h"; then
apc_inc_path="$phpincludedir"
AC_MSG_RESULT([APCu in $apc_inc_path])
AC_DEFINE(HAVE_APCU_SUPPORT, 1, [Whether to enable APCu support])
else
- AC_MSG_RESULT([not found])
+ AC_MSG_RESULT([not found])
+ if test "$PHP_APCU" = "yes"; then
+ AC_MSG_ERROR([APCu support explicitly requested but APCu headers not found])
+ fi
fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if test "$PHP_APCU" != "no"; then | |
| AC_MSG_CHECKING([for APCu includes]) | |
| if test -f "$phpincludedir/ext/apcu/apc_serializer.h"; then | |
| apc_inc_path="$phpincludedir" | |
| AC_MSG_RESULT([APCu in $apc_inc_path]) | |
| AC_DEFINE(HAVE_APCU_SUPPORT, 1, [Whether to enable APCu support]) | |
| else | |
| AC_MSG_RESULT([not found]) | |
| fi | |
| fi | |
| if test "$PHP_APCU" != "no"; then | |
| AC_MSG_CHECKING([for APCu includes]) | |
| if test -f "$phpincludedir/ext/apcu/apc_serializer.h"; then | |
| apc_inc_path="$phpincludedir" | |
| AC_MSG_RESULT([APCu in $apc_inc_path]) | |
| AC_DEFINE(HAVE_APCU_SUPPORT, 1, [Whether to enable APCu support]) | |
| else | |
| AC_MSG_RESULT([not found]) | |
| if test "$PHP_APCU" = "yes"; then | |
| AC_MSG_ERROR([APCu support explicitly requested but APCu headers not found]) | |
| fi | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config.m4` around lines 138 - 147, When APCu was explicitly requested
(PHP_APCU == "yes") the configure script currently prints "not found" and
continues; change the logic in the APCu detection block so that after testing
for "$phpincludedir/ext/apcu/apc_serializer.h" you still set apc_inc_path and
AC_DEFINE(HAVE_APCU_SUPPORT,1,...) when found, but if the header is missing and
PHP_APCU equals "yes" call AC_MSG_ERROR with a clear failure message (e.g.,
"APCu requested but header apc_serializer.h not found in
$phpincludedir/ext/apcu") so configure aborts; if PHP_APCU is "auto" or unset
keep the existing AC_MSG_RESULT([not found]) behavior.
1 participant
Summary by CodeRabbit
--enable-apcuconfigure option to allow flexible APCu support enablement during build configuration.