report if libmaxminddb is bundled or system - #289
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughBundled builds now define Changeslibmaxminddb labeling
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change only labels the library source in PHP info output and presents no unresolved merge risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)
✨ 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. A rabbit checks the build flag bright Comment |
|
Perhaps version is irrelevant when bundled, so an alternative report is Or |
horgh
left a comment
There was a problem hiding this comment.
Good idea, thank you! I have a few comments.
| maxminddb_sources="$maxminddb_sources libmaxminddb/src/maxminddb.c libmaxminddb/src/data-pool.c" | ||
|
|
||
| AC_DEFINE([HAVE_LIBMAXMINDDB_BUNDLED], [1], [Use bundled or system libmaxminddb]) | ||
| PHP_SUBST([HAVE_LIBMAXMINDDB_BUNDLED]) |
There was a problem hiding this comment.
Is this line needed? I think it might not do anything useful.
| CFLAGS="$CFLAGS -fvisibility=hidden -UHAVE_CONFIG_H -DHAVE_CONFIG_H=0 -DMMDB_UINT128_USING_MODE=0 -DMMDB_UINT128_IS_BYTE_ARRAY=1" | ||
|
|
||
| maxminddb_sources="$maxminddb_sources libmaxminddb/src/maxminddb.c libmaxminddb/src/data-pool.c" | ||
|
|
There was a problem hiding this comment.
I think we need this in ext/config.w32 too.
|
|
||
| maxminddb_sources="$maxminddb_sources libmaxminddb/src/maxminddb.c libmaxminddb/src/data-pool.c" | ||
|
|
||
| AC_DEFINE([HAVE_LIBMAXMINDDB_BUNDLED], [1], [Use bundled or system libmaxminddb]) |
There was a problem hiding this comment.
Maybe "Defined when the bundled libmaxminddb is compiled into the extension"?
| #ifdef HAVE_LIBMAXMINDDB_BUNDLED | ||
| 2, "libmaxminddb bundled library version", MMDB_lib_version()); | ||
| #else | ||
| 2, "libmaxminddb system library version", MMDB_lib_version()); |
There was a problem hiding this comment.
I wonder if adding a new row would be a good option, e.g. something like
#ifdef HAVE_LIBMAXMINDDB_BUNDLED
const char *lib_source = "bundled";
#else
const char *lib_source = "system";
#endif
php_info_print_table_row(2, "libmaxminddb library", lib_source);And we could keep the current version one as is, which is still useful.
Alternatively we could have the current line appear like libmaxminddb library version 1.xxx (bundled) and libmaxminddb library version 1.xxx.
If you think keeping your proposed way is superior, that seems okay too though.
As version 1.14.0 now has an option to use the system or bundled library, it could be useful to report it.
Summary by CodeRabbit