Support for ProxySQL compilation on macOS (Darwin/Apple Silicon)#5308
Conversation
Detailed changes: - Defined UNAME_S in root Makefile to ensure correct platform guards. - Isolated Darwin-specific OpenSSL detection and version checks. - Enabled ICU support for PostgreSQL on Darwin. - Added Darwin guards for MariaDB client zlib patches. - Restored original Linux behavior for all build scripts. - Added new Darwin-specific patches for MariaDB client.
- Add mysql-8.4.0-clang17.patch to fix AppleClang 17 template strictness errors - Update test/deps/Makefile to automatically apply patch on Darwin - Enforce sequential build (-j1) for MySQL connectors on macOS to prevent race conditions - Patch fixes 9 template keyword errors in MySQL serialization headers
- Support OPENSSL_ROOT_DIR environment variable for better Homebrew integration - Fix OpenSSL library path on Darwin (use lib instead of lib64) - Improve OpenSSL version extraction using grep/sed for better compatibility - Add TAP test build instructions to BUILD-MACOS.md
📝 WalkthroughWalkthroughAdds comprehensive macOS (Darwin) build support: OS-aware Makefile logic, Homebrew/OpenSSL discovery and validation, Darwin-specific dependency build rules and patches, platform-specific source guards/includes, updated macOS build docs and INSTALL macOS steps. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello @renecannao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive support for compiling ProxySQL on macOS, specifically targeting Darwin/Apple Silicon architectures. It addresses numerous platform-specific challenges, ranging from core code compatibility and build system adjustments to dependency management and test stability. The changes ensure that ProxySQL can be successfully built and run on macOS, significantly expanding its supported environments and improving the development experience for Mac users. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for compiling ProxySQL on macOS (Darwin/Apple Silicon). The changes span across core compatibility fixes, build system infrastructure improvements, and test dependency stability. Key updates include platform-specific header and type adjustments, enhanced OpenSSL detection and linking logic for Homebrew installations, and Makefile modifications to handle macOS conventions for dynamic libraries and linker flags. Additionally, a new documentation guide (doc/BUILD-MACOS.md) has been added, providing step-by-step instructions for building on macOS, significantly improving the developer experience for this platform. The changes are well-structured and address various platform-specific challenges effectively.
| --- libs/mysql/serialization/archive.h 2024-04-10 08:26:28 | ||
| +++ libs/mysql/serialization/archive.h 2026-01-21 00:07:15 | ||
| @@ -81,14 +81,15 @@ | ||
| /// @note To be implemented in Archive_derived_type | ||
| template <typename Type> | ||
| static std::size_t get_size(Type &&arg) { | ||
| - return Archive_derived_type::template get_size(std::forward<Type>(arg)); | ||
| + return Archive_derived_type::template get_size<Type>( | ||
| + std::forward<Type>(arg)); | ||
| } | ||
|
|
||
| /// @brief Returns archive size - size of data written to the archive | ||
| /// @return archive size - size of data written to the archive | ||
| /// @note To be implemented in Archive_derived_type | ||
| inline std::size_t get_size_written() const { | ||
| - return Archive_derived_type::template get_size_written(); | ||
| + return Archive_derived_type::get_size_written(); | ||
| } | ||
|
|
||
| /// @brief Function returns maximum size of the Type | ||
| --- libs/mysql/serialization/serializer_impl.hpp 2024-04-10 08:26:28 | ||
| +++ libs/mysql/serialization/serializer_impl.hpp 2026-01-21 00:07:15 | ||
| @@ -51,8 +51,8 @@ | ||
| Serializer<Serializer_derived_type, Archive_type>::get_size_field_def( | ||
| Field_id_type field_id, | ||
| const Field_definition<Field_type, field_size_defined> &field_definition) { | ||
| - return Serializer_derived_type::template get_size_field_def(field_id, | ||
| - field_definition); | ||
| + return Serializer_derived_type::template get_size_field_def< | ||
| + Field_type, field_size_defined>(field_id, field_definition); | ||
| } | ||
|
|
||
| template <class Serializer_derived_type, class Archive_type> | ||
| @@ -61,8 +61,8 @@ | ||
| Serializer<Serializer_derived_type, Archive_type>::get_size_serializable( | ||
| Field_id_type field_id, const Serializable_concrete_type &serializable, | ||
| bool skip_id) { | ||
| - return Serializer_derived_type::template get_size_serializable( | ||
| - field_id, serializable, skip_id); | ||
| + return Serializer_derived_type::template get_size_serializable< | ||
| + Serializable_concrete_type>(field_id, serializable, skip_id); | ||
| } | ||
|
|
||
| template <class Serializer_derived_type, class Archive_type> | ||
| @@ -83,8 +83,8 @@ | ||
|
|
||
| template <class Serializer_derived_type, class Archive_type> | ||
| template <typename T> | ||
| -Serializer<Serializer_derived_type, Archive_type> | ||
| - &Serializer<Serializer_derived_type, Archive_type>::operator>>(T &arg) { | ||
| +Serializer<Serializer_derived_type, Archive_type> & | ||
| +Serializer<Serializer_derived_type, Archive_type>::operator>>(T &arg) { | ||
| Field_id_type field_id = serialization_format_version; | ||
| // passing 0 as serializable_end_pos | ||
| decode_serializable(m_level, field_id, 0, arg, false); | ||
| @@ -101,12 +101,11 @@ | ||
| void Serializer<Serializer_derived_type, Archive_type>:: | ||
| encode_serializable_fields(const Serializable_type &serializable, | ||
| Level_type level) { | ||
| - auto process_serializable = | ||
| - [ this, level ](const auto &field, auto field_id) -> auto { | ||
| + auto process_serializable = [this, level](const auto &field, | ||
| + auto field_id) -> auto { | ||
| this->encode_serializable(level, field_id, field, false); | ||
| }; | ||
| - auto process_field = | ||
| - [ this, level ](const auto &field, auto field_id) -> auto { | ||
| + auto process_field = [this, level](const auto &field, auto field_id) -> auto { | ||
| this->encode_field(level, field_id, field); | ||
| ++field_id; | ||
| }; | ||
| @@ -139,13 +138,13 @@ | ||
| decode_serializable_fields(Serializable_type &serializable, | ||
| Level_type level, | ||
| std::size_t serializable_end_pos) { | ||
| - auto process_serializable = [ this, level, serializable_end_pos ]( | ||
| - auto &field, auto field_id) -> auto { | ||
| + auto process_serializable = [this, level, serializable_end_pos]( | ||
| + auto &field, auto field_id) -> auto { | ||
| this->decode_serializable(level, field_id, serializable_end_pos, field, | ||
| false); | ||
| }; | ||
| - auto process_field = [ this, level, serializable_end_pos ]( | ||
| - auto &field, auto field_id) -> auto { | ||
| + auto process_field = [this, level, serializable_end_pos]( | ||
| + auto &field, auto field_id) -> auto { | ||
| this->decode_field(level, field_id, serializable_end_pos, field); | ||
|
|
||
| ++field_id; | ||
| --- libs/mysql/serialization/serializer_default_impl.hpp 2024-04-10 08:26:28 | ||
| +++ libs/mysql/serialization/serializer_default_impl.hpp 2026-01-21 00:11:38 | ||
| @@ -205,8 +205,8 @@ | ||
| template <class Field_type, Field_size field_size_defined, typename Enabler> | ||
| void Serializer_default<Archive_concrete_type>::encode_field( | ||
| const Field_type &field, Serializer_array_tag) { | ||
| - using value_type = std::remove_reference_t<decltype( | ||
| - *std::begin(std::declval<Field_type &>()))>; | ||
| + using value_type = std::remove_reference_t<decltype(*std::begin( | ||
| + std::declval<Field_type &>()))>; | ||
| for (const auto &internal_field : field) { | ||
| // we use default size for internal fields (0) | ||
| encode_field<value_type, 0>(internal_field); | ||
| @@ -219,8 +219,8 @@ | ||
| template <class Field_type, Field_size field_size_defined, typename Enabler> | ||
| void Serializer_default<Archive_concrete_type>::decode_field( | ||
| Field_type &field, Serializer_array_tag) { | ||
| - using value_type = std::remove_reference_t<decltype( | ||
| - *std::begin(std::declval<Field_type &>()))>; | ||
| + using value_type = std::remove_reference_t<decltype(*std::begin( | ||
| + std::declval<Field_type &>()))>; | ||
| for (auto &internal_field : field) { | ||
| // we use default size for internal fields (0) | ||
| decode_field<value_type, 0>(internal_field); | ||
| @@ -233,7 +233,8 @@ | ||
| template <class Field_type, Field_size field_size_defined, typename Enabler> | ||
| std::size_t Serializer_default<Archive_concrete_type>::get_field_size( | ||
| const Field_type &field) { | ||
| - return Archive_concrete_type::template get_size( | ||
| + return Archive_concrete_type::template get_size< | ||
| + Field_wrapper<const Field_type, field_size_defined>>( | ||
| Field_wrapper<const Field_type, field_size_defined>(field)); | ||
| } | ||
|
|
||
| @@ -304,8 +305,8 @@ | ||
| std::size_t Serializer_default<Archive_concrete_type>::get_field_size( | ||
| const Field_type &field, Serializer_array_tag) { | ||
| std::size_t field_size = 0; | ||
| - using value_type = std::remove_reference_t<decltype( | ||
| - *std::begin(std::declval<Field_type &>()))>; | ||
| + using value_type = std::remove_reference_t<decltype(*std::begin( | ||
| + std::declval<Field_type &>()))>; | ||
| for (const auto &internal_field : field) { | ||
| field_size += get_field_size<value_type, 0>(internal_field); | ||
| } | ||
| @@ -397,7 +398,7 @@ | ||
| } | ||
| }; | ||
| auto func_f = [&last_non_ignorable_field_id]( | ||
| - const auto &field, auto processed_field_id) -> auto { | ||
| + const auto &field, auto processed_field_id) -> auto { | ||
| if (field.run_encode_predicate() && field.is_field_ignorable() == false) { | ||
| last_non_ignorable_field_id = processed_field_id + 1; | ||
| } | ||
| @@ -473,8 +474,8 @@ | ||
| std::size_t calculated_size = 0; | ||
| bool is_provided = field_definition.run_encode_predicate(); | ||
| if (is_provided) { | ||
| - auto size_id_type = Archive_concrete_type::template get_size( | ||
| - create_varlen_field_wrapper(field_id)); | ||
| + auto size_id_type = Archive_concrete_type::template get_size< | ||
| + Field_wrapper<Field_id_type, 0>>(create_varlen_field_wrapper(field_id)); | ||
| calculated_size = get_field_size<Field_type, field_size_defined>( | ||
| field_definition.get_ref()) + | ||
| size_id_type; | ||
| @@ -489,18 +490,19 @@ | ||
| bool skip_id) { | ||
| std::size_t serializable_overhead_type = 0; | ||
| if (skip_id == false) { | ||
| - serializable_overhead_type = Archive_concrete_type::template get_size( | ||
| - create_varlen_field_wrapper(field_id)); | ||
| + serializable_overhead_type = Archive_concrete_type::template get_size< | ||
| + Field_wrapper<Field_id_type, 0>>(create_varlen_field_wrapper(field_id)); | ||
| } | ||
| auto serializable_size = serializable.template get_size_internal<Base_type>(); | ||
| - auto serializable_overhead_size = Archive_concrete_type::template get_size( | ||
| + auto serializable_overhead_size = Archive_concrete_type::template get_size< | ||
| + Field_wrapper<decltype(serializable_size), 0>>( | ||
| create_varlen_field_wrapper(serializable_size)); | ||
|
|
||
| Field_id_type last_non_ignorable_field_id = | ||
| find_last_non_ignorable_field_id(serializable); | ||
|
|
||
| auto serializable_overhead_last_non_ignorable_field_id = | ||
| - Archive_concrete_type::template get_size( | ||
| + Archive_concrete_type::template get_size<Field_wrapper<Field_id_type, 0>>( | ||
| create_varlen_field_wrapper(last_non_ignorable_field_id)); | ||
| return serializable_overhead_type + serializable_overhead_size + | ||
| serializable_overhead_last_non_ignorable_field_id + serializable_size; |
There was a problem hiding this comment.
This patch addresses template strictness errors in MySQL 8.4 serialization headers when compiled with modern Clang (version 17 or newer). The changes involve explicitly specifying template arguments for get_size and get_size_serializable methods, and adjusting lambda capture lists and decltype usage to conform to stricter C++20 rules. This is a critical correctness fix for building on macOS with up-to-date compilers.
| #ifdef __APPLE__ | ||
| #include <machine/endian.h> | ||
| #include <libkern/OSByteOrder.h> | ||
|
|
||
| #define htobe16(x) OSSwapHostToBigInt16(x) | ||
| #define htole16(x) OSSwapHostToLittleInt16(x) | ||
| #define be16toh(x) OSSwapBigToHostInt16(x) | ||
| #define le16toh(x) OSSwapLittleToHostInt16(x) | ||
|
|
||
| #define htobe32(x) OSSwapHostToBigInt32(x) | ||
| #define htole32(x) OSSwapHostToLittleInt32(x) | ||
| #define be32toh(x) OSSwapBigToHostInt32(x) | ||
| #define le32toh(x) OSSwapLittleToHostInt32(x) | ||
|
|
||
| #define htobe64(x) OSSwapHostToBigInt64(x) | ||
| #define htole64(x) OSSwapHostToLittleInt64(x) | ||
| #define be64toh(x) OSSwapBigToHostInt64(x) | ||
| #define le64toh(x) OSSwapLittleToHostInt64(x) | ||
|
|
||
| #define mempcpy(dest, src, n) ((char *)memcpy(dest, src, n) + (n)) | ||
| #else | ||
| #include <endian.h> | ||
| #endif |
There was a problem hiding this comment.
This #ifdef __APPLE__ block correctly provides macOS-specific definitions for byte order conversion macros (htobe16, htole16, etc.) and mempcpy. These are essential for cross-platform compatibility, as macOS uses libkern/OSByteOrder.h and different function names compared to Linux's <endian.h> for these operations. This is a critical correctness fix for building on Apple platforms.
| unsigned long val; | ||
| opt->get(cmd_opt)->getULong(val); | ||
| cur_val = val; |
There was a problem hiding this comment.
Modifying update_ulong_var_if_set to use an intermediate unsigned long val for opt->get(cmd_opt)->getULong(val) and then assigning it to cur_val is a crucial correctness fix. The getULong method expects an unsigned long&, but cur_val is a uint64_t&. This ensures type compatibility and prevents potential undefined behavior or data loss on platforms where uint64_t and unsigned long might have different sizes.
| cd mysql-connector-c-8.4.0 && mysql_dir=$$(ls -1d mysql-8.4.*/) && \ | ||
| sed -i '' 's/# ifndef fdopen/# if !defined(fdopen) \&\& !defined(__APPLE__)/' $${mysql_dir}extra/zlib/zlib-1.2.13/zutil.h | ||
| cd mysql-connector-c-8.4.0 && ln -fsh $$(ls -1d mysql-8.4.*/) mysql-connector-c | ||
| cd mysql-connector-c-8.4.0/mysql-connector-c && patch -p0 --batch < ../../mysql-8.4.0-clang17.patch |
There was a problem hiding this comment.
Applying mysql-8.4.0-clang17.patch is a critical fix for MySQL 8.4 connector on macOS with modern Clang. This patch resolves template strictness errors in serialization headers, ensuring compatibility with C++20 and newer compilers.
| cd mysql-connector-c && mysql_dir=$$(ls -1d mysql-5.7.*/) && \ | ||
| sed -i '' 's/CMAKE_POLICY(SET CMP0018 OLD)/CMAKE_POLICY(SET CMP0018 NEW)/' $${mysql_dir}CMakeLists.txt && \ | ||
| sed -i '' 's/CMAKE_POLICY(SET CMP0022 OLD)/CMAKE_POLICY(SET CMP0022 NEW)/' $${mysql_dir}CMakeLists.txt && \ | ||
| sed -i '' 's/CMAKE_POLICY(SET CMP0045 OLD)/CMAKE_POLICY(SET CMP0045 NEW)/' $${mysql_dir}CMakeLists.txt && \ | ||
| sed -i '' 's/CMAKE_POLICY(SET CMP0042 OLD)/CMAKE_POLICY(SET CMP0042 NEW)/' $${mysql_dir}CMakeLists.txt && \ | ||
| sed -i '' 's/CMAKE_POLICY(SET CMP0075 OLD)/CMAKE_POLICY(SET CMP0075 NEW)/' $${mysql_dir}CMakeLists.txt && \ | ||
| sed -i '' 's/# ifndef fdopen/# if !defined(fdopen) \&\& !defined(__APPLE__)/' $${mysql_dir}extra/zlib/zlib-1.2.13/zutil.h |
There was a problem hiding this comment.
The sed commands to update CMAKE_POLICY settings in CMakeLists.txt are critical for building MySQL 5.7 connector on macOS with modern Clang. These policies address strictness issues in template serialization headers, ensuring compatibility with C++20 and newer compilers.
| cd mysql-connector-c-8.4.0 && tar -zxf mysql-*.tar.gz | ||
| ifeq ($(UNAME_S),Darwin) | ||
| cd mysql-connector-c-8.4.0 && mysql_dir=$$(ls -1d mysql-8.4.*/) && \ | ||
| sed -i '' 's/# ifndef fdopen/# if !defined(fdopen) \&\& !defined(__APPLE__)/' $${mysql_dir}extra/zlib/zlib-1.2.13/zutil.h |
There was a problem hiding this comment.
The sed command to modify zutil.h is a targeted fix for a specific compilation issue related to fdopen on macOS. This ensures that the zlib component within the MySQL 8.4 connector builds correctly on Darwin.
| ifeq ($(UNAME_S),Darwin) | ||
| SSL_LDIR := $(CUSTOM_OPENSSL_PATH)/lib | ||
| else | ||
| SSL_LDIR := $(CUSTOM_OPENSSL_PATH)/lib64 | ||
| endif |
There was a problem hiding this comment.
Changing SSL_LDIR from lib64 to lib for Darwin is a necessary platform-specific adjustment. macOS typically installs libraries in /lib or /usr/local/lib, not /lib64.
| cd mysql-connector-c-8.4.0/mysql-connector-c && cmake . -DFORCE_INSOURCE_BUILD=1 -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_POLICY_VERSION_MINIMUM=3.5 \ | ||
| -DWITHOUT_SERVER=ON -DDOWNLOAD_BOOST=1 -DWITH_BOOST=./mysql-server/downloads/ -DWITH_UNIT_TESTS=OFF \ | ||
| -DCMAKE_CXX_FLAGS_RELWITHDEBINFO="-O0 -ggdb -DNDEBUG -fPIC" -DWITH_SSL=$$(brew --prefix openssl@3) -DOPENSSL_ROOT_DIR=$$(brew --prefix openssl@3) |
There was a problem hiding this comment.
Specifying -DWITH_SSL=$$(brew --prefix openssl@3) -DOPENSSL_ROOT_DIR=$$(brew --prefix openssl@3) for the MySQL 8.4 connector cmake command on Darwin is essential. This explicitly tells cmake where to find the Homebrew-installed OpenSSL 3, ensuring correct linking.
| -DCMAKE_CXX_FLAGS_RELWITHDEBINFO="-O0 -ggdb -DNDEBUG -fPIC" | ||
| endif | ||
| ifeq ($(UNAME_S),Darwin) | ||
| cd mysql-connector-c-8.4.0/mysql-connector-c && CC=${CC} CXX=${CXX} ${MAKE} -j1 |
There was a problem hiding this comment.
Enforcing -j1 (sequential build) for MySQL 8.4 connector on Darwin is a good practice if intermittent race conditions are observed during parallel builds. This prioritizes build reliability over speed for this specific dependency on macOS.
| ifeq ($(UNAME_S),Darwin) | ||
| LIB_SSL_PATH := $(shell find $(SSL_LDIR) -name "libssl.dylib" 2>/dev/null | head -n 1) | ||
| ifeq ($(LIB_SSL_PATH),) | ||
| LIB_SSL_PATH := $(shell find $(SSL_LDIR) -name "libssl.a" 2>/dev/null | head -n 1) | ||
| endif | ||
| LIB_CRYPTO_PATH := $(shell find $(SSL_LDIR) -name "libcrypto.dylib" 2>/dev/null | head -n 1) | ||
| ifeq ($(LIB_CRYPTO_PATH),) | ||
| LIB_CRYPTO_PATH := $(shell find $(SSL_LDIR) -name "libcrypto.a" 2>/dev/null | head -n 1) | ||
| endif | ||
| else | ||
| LIB_SSL_PATH := $(shell find $(SSL_LDIR) -name "libssl.so*" 2>/dev/null | head -n 1) | ||
| ifeq ($(LIB_SSL_PATH),) | ||
| LIB_SSL_PATH := $(shell find $(SSL_LDIR) -name "libssl.a" 2>/dev/null | head -n 1) | ||
| endif | ||
| LIB_CRYPTO_PATH := $(shell find $(SSL_LDIR) -name "libcrypto.so*" 2>/dev/null | head -n 1) | ||
| ifeq ($(LIB_CRYPTO_PATH),) | ||
| LIB_CRYPTO_PATH := $(shell find $(SSL_LDIR) -name "libcrypto.a" 2>/dev/null | head -n 1) | ||
| endif | ||
| endif |
There was a problem hiding this comment.
This conditional block correctly adjusts the search for OpenSSL library files (libssl.dylib, libcrypto.dylib, .a archives) for Darwin. This is essential for linking against OpenSSL on macOS, where dynamic libraries typically use the .dylib extension instead of .so.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for compiling ProxySQL on macOS (Darwin/Apple Silicon), addressing platform-specific differences in headers, build tools, and dependencies.
Changes:
- Added platform-specific fixes for headers, types, and library detection across core ProxySQL code
- Enhanced build system with Darwin-aware OpenSSL detection and dependency management
- Created MySQL 8.4 Clang 17 compatibility patch for modern compiler strictness
- Added comprehensive macOS build documentation
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/deps/mysql-8.4.0-clang17.patch | Template strictness fixes for MySQL 8.4 serialization headers with Clang 17/C++20 |
| test/deps/Makefile | Darwin-specific build configuration for MySQL and MariaDB connectors, including sequential builds to prevent race conditions |
| src/Makefile | Added OpenSSL libraries and PostgreSQL dependencies for Darwin linking |
| lib/sha256crypt.cpp | Darwin-specific endianness macros and mempcpy implementation |
| lib/proxysql_utils.cpp | Conditional inclusion of Linux-specific close_range header |
| lib/ProxySQL_GloVars.cpp | Type compatibility fix for unsigned long/uint64_t conversion |
| lib/ProxySQL_Admin_Stats.cpp | Added format specifier handling for unsigned long on Darwin |
| lib/ProxySQL_Admin.cpp | Conditional compilation for IDLE_THREADS-specific variables |
| lib/PgSQL_Thread.cpp | Conditional compilation for IDLE_THREADS feature guards |
| lib/PgSQL_Monitor.cpp | Fixed ULONG_LONG_MAX to standard ULLONG_MAX |
| lib/Base_Thread.cpp | Conditional compilation for PgSQL thread copy_cmd_matcher |
| include/proxysql_utils.h | Platform-specific conjunction template handling for Darwin |
| include/proxysql_glovars.hpp | Added OpenSSL header include |
| include/makefiles_vars.mk | Defensive check for /etc/os-release existence on Darwin |
| common_mk/openssl_version_check.mk | Darwin-specific OpenSSL version detection from headers |
| common_mk/openssl_flags.mk | Comprehensive Darwin support for OpenSSL library detection (.dylib/.a) |
| Makefile | Darwin-specific user/group checks and version extraction using sed instead of grep -P |
| deps/Makefile | Darwin build configuration for curl, MariaDB, PostgreSQL, and libusual dependencies |
| deps/mariadb-client-library/zutil.h.patch | Fix for zlib fdopen macro conflict on Darwin |
| deps/mariadb-client-library/zutil.c.patch | K&R to ANSI C function signature conversion for zlib |
| doc/BUILD-MACOS.md | Comprehensive macOS build guide with Homebrew setup |
| INSTALL.md | Updated macOS dependencies and environment variable setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ifeq ($(UNAME_S),Darwin) | ||
| cd curl/curl && patch configure < ../configure.patch | ||
| endif | ||
| cd curl/curl && sed -i '' 's/as_fn_error \$$? "one or more libs available at link-time are not available run-time/echo "Skipped check: one or more libs available at link-time are not available run-time/' configure |
There was a problem hiding this comment.
The sed command uses an inline replacement to skip a runtime library check in the curl configure script. While this works on Darwin, it would be more maintainable to use a dedicated patch file similar to how other dependencies are handled, especially since this appears to be a workaround for a macOS-specific issue with dynamic library path checking. This would make the change more explicit and easier to understand.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@doc/BUILD-MACOS.md`:
- Around line 23-27: The docs hardcode the Homebrew prefix (/opt/homebrew) for
PATH, PKG_CONFIG_PATH, and OPENSSL_ROOT_DIR which breaks on Intel macs; update
the instructions in BUILD-MACOS.md to compute the prefix dynamically (use `brew
--prefix`) and interpolate that value for PATH, PKG_CONFIG_PATH and
OPENSSL_ROOT_DIR so the same snippet works on both Intel and Apple Silicon
installations.
In `@INSTALL.md`:
- Around line 191-200: The INSTALL.md snippet hardcodes the Apple Silicon
Homebrew prefix (/opt/homebrew) which breaks on Intel Macs; update the
instructions to use a portable Homebrew prefix by referencing the result of brew
--prefix (e.g., use brew --prefix or brew --prefix openssl@3) when setting PATH,
PKG_CONFIG_PATH and OPENSSL_ROOT_DIR so both /opt/homebrew and /usr/local
installations work across architectures; locate the three export lines (PATH,
PKG_CONFIG_PATH, OPENSSL_ROOT_DIR) in the macOS build notes and replace the
hardcoded /opt/homebrew paths with commands that derive the prefix dynamically.
♻️ Duplicate comments (10)
lib/ProxySQL_GloVars.cpp (1)
343-345: LGTM: type-safe extraction before assigning to uint64_t.lib/PgSQL_Monitor.cpp (1)
15-15: Portability improvement with<climits>andULLONG_MAX.
Good move for standard macro availability across platforms.Also applies to: 1824-1824
deps/mariadb-client-library/zutil.h.patch (1)
7-10: fdopen macro removal is reasonable.
Please confirm no supported toolchains still rely on the old fallback.lib/sha256crypt.cpp (1)
4-26: No new concerns beyond prior review.deps/mariadb-client-library/zutil.c.patch (1)
7-33: Already covered in prior review.deps/Makefile (4)
122-126: Covered in prior review (Darwin curl configure adjustments).
193-201: Covered in prior review (Darwin MariaDB patches/CMake flags).
318-322: Covered in prior review (Darwin PostgreSQL ICU/OpenSSL wiring).
332-336: Covered in prior review (Darwin libusual OpenSSL flags).Makefile (1)
91-96: Covered in prior review (Linux-only getent checks).
🧹 Nitpick comments (5)
lib/ProxySQL_Admin_Stats.cpp (1)
491-495: Prefer PRIu64 to avoid truncation on ILP32/LLP64.
Castinguint64_ttounsigned longand formatting with%lucan truncate whereunsigned longis 32-bit;PRIu64keeps it portable.♻️ Portable formatting change
-} else if constexpr (std::is_same_v<T, uint64_t>) { - sprintf(buf, "%lu", (unsigned long)val); +} else if constexpr (std::is_same_v<T, uint64_t>) { + sprintf(buf, "%" PRIu64, val);+#include <cinttypes>include/proxysql_utils.h (1)
29-36: Guardstd::conjunctionbehind C++17 availability.The Apple-only branch assumes
std::conjunctionexists. If any macOS build path still uses C++11/14 (LEGACY_BUILD or older toolchains), this will fail at compile time. Consider gating onCXX17(or a feature-test macro) and falling back to the local implementation.♻️ Suggested guard
-#if defined(__APPLE__) -using std::conjunction; -#elif defined(CXX17) +#if defined(__APPLE__) && defined(CXX17) +using std::conjunction; +#elif defined(CXX17) template<class...> struct conjunction : std::true_type { }; template<class B1> struct std::conjunction<B1> : B1 { }; template<class B1, class... Bn> struct std::conjunction<B1, Bn...> : std::conditional<bool(B1::value), std::conjunction<Bn...>, B1>::type {}; `#else` template<class...> struct conjunction : std::true_type { }; template<class B1> struct conjunction<B1> : B1 { }; template<class B1, class... Bn> struct conjunction<B1, Bn...> : std::conditional<bool(B1::value), conjunction<Bn...>, B1>::type {}; `#endif` // CXX17common_mk/openssl_version_check.mk (2)
6-42: Darwin path lacks informational echo present in non-Darwin path.The non-Darwin path (line 44) includes
@echo "Checking OpenSSL version..."for user feedback, but the Darwin path does not. Consider adding this for consistency.Add informational echo for Darwin
check_openssl_version: ifeq ($(UNAME_S),Darwin) + `@echo` "Checking OpenSSL version..." `@if` [ -n "$(CUSTOM_OPENSSL_PATH)" ]; then \
21-26: Version comparison methods are inconsistent across paths.The Darwin path (lines 21, 36) and non-Darwin custom path (line 64) use manual major/minor/patch comparison, while the non-Darwin pkg-config path (line 78) uses
sort -V. This inconsistency could cause different behavior for version strings with suffixes (e.g.,3.0.0-fips).The manual comparison uses
cut -d'.' -f3for patch, which would fail to parse correctly if the patch component contains non-numeric suffixes.Consider normalizing the comparison approach, or stripping suffixes before comparison:
- patch=$$(echo $$version_number | cut -d'.' -f3); \ + patch=$$(echo $$version_number | cut -d'.' -f3 | sed 's/[^0-9].*//'); \Also applies to: 36-41, 78-81
common_mk/openssl_flags.mk (1)
38-56: Library search logic is duplicated but correct.The Darwin-specific search for
.dylibfiles (falling back to.a) is correct. The duplication between the non-custom (lines 38-56) and custom (lines 65-83) paths is understandable given Makefile constraints, though a GNU Makedefinecould reduce this if desired.Consider extracting the library search logic into a reusable macro:
# Example of a define-based approach (optional) define find_ssl_libs_darwin LIB_SSL_PATH := $$(shell find $(1) -name "libssl.dylib" 2>/dev/null | head -n 1) ifeq ($$(LIB_SSL_PATH),) LIB_SSL_PATH := $$(shell find $(1) -name "libssl.a" 2>/dev/null | head -n 1) endif # ... similar for LIB_CRYPTO_PATH endefAlso applies to: 65-83
| ```bash | ||
| export PATH="/opt/homebrew/bin:$PATH" | ||
| export PKG_CONFIG_PATH="/opt/homebrew/opt/openssl@3/lib/pkgconfig:$PKG_CONFIG_PATH" | ||
| export OPENSSL_ROOT_DIR="/opt/homebrew/opt/openssl@3" | ||
| ``` |
There was a problem hiding this comment.
Use Homebrew prefix to avoid Intel/Apple path mismatch.
The guide targets both Intel and Apple Silicon, but hardcodes /opt/homebrew. On Intel, Homebrew usually lives in /usr/local. Suggest using brew --prefix to make this portable.
✏️ Suggested doc update
-export PATH="/opt/homebrew/bin:$PATH"
-export PKG_CONFIG_PATH="/opt/homebrew/opt/openssl@3/lib/pkgconfig:$PKG_CONFIG_PATH"
-export OPENSSL_ROOT_DIR="/opt/homebrew/opt/openssl@3"
+HOMEBREW_PREFIX="$(brew --prefix)"
+export PATH="$HOMEBREW_PREFIX/bin:$PATH"
+export PKG_CONFIG_PATH="$HOMEBREW_PREFIX/opt/openssl@3/lib/pkgconfig:$PKG_CONFIG_PATH"
+export OPENSSL_ROOT_DIR="$HOMEBREW_PREFIX/opt/openssl@3"📝 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.
| ```bash | |
| export PATH="/opt/homebrew/bin:$PATH" | |
| export PKG_CONFIG_PATH="/opt/homebrew/opt/openssl@3/lib/pkgconfig:$PKG_CONFIG_PATH" | |
| export OPENSSL_ROOT_DIR="/opt/homebrew/opt/openssl@3" | |
| ``` |
🤖 Prompt for AI Agents
In `@doc/BUILD-MACOS.md` around lines 23 - 27, The docs hardcode the Homebrew
prefix (/opt/homebrew) for PATH, PKG_CONFIG_PATH, and OPENSSL_ROOT_DIR which
breaks on Intel macs; update the instructions in BUILD-MACOS.md to compute the
prefix dynamically (use `brew --prefix`) and interpolate that value for PATH,
PKG_CONFIG_PATH and OPENSSL_ROOT_DIR so the same snippet works on both Intel and
Apple Silicon installations.
| brew install automake bzip2 cmake make git gpatch gnutls openssl@3 icu4c pkg-config libiconv zlib | ||
| ``` | ||
|
|
||
| To compile on macOS, you must set the following environment variables to ensure the build system can find OpenSSL and Homebrew dependencies: | ||
|
|
||
| ```bash | ||
| export PATH="/opt/homebrew/bin:$PATH" | ||
| export PKG_CONFIG_PATH="/opt/homebrew/opt/openssl@3/lib/pkgconfig:$PKG_CONFIG_PATH" | ||
| export OPENSSL_ROOT_DIR="/opt/homebrew/opt/openssl@3" | ||
| make # or make debug |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the current state of INSTALL.md around lines 191-200
wc -l INSTALL.mdRepository: sysown/proxysql
Length of output: 73
🏁 Script executed:
# Read the relevant section
sed -n '185,205p' INSTALL.mdRepository: sysown/proxysql
Length of output: 929
🌐 Web query:
Homebrew prefix paths Apple Silicon Intel macOS /opt/homebrew /usr/local
💡 Result:
Short answer
- Default Homebrew prefixes:
- Apple Silicon (ARM/M1/M2/M3): /opt/homebrew. [1]
- Intel macOS (x86_64): /usr/local. [1][2]
Why it matters
- Prebuilt bottles (binary packages) are produced for the default prefix; installing outside it can force source builds or fail. [2]
- Using different prefixes lets ARM and Intel installs coexist (useful with Rosetta). [3]
Common commands
- Show the prefix Homebrew is using: brew --prefix. [1]
- Show which brew will run: which brew or command -v brew.
- Run ARM or Intel installer under a specific arch (Rosetta) if needed: arch -x86_64 /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)". [1]
PATH / shell note
- After install the installer prints “Next steps” (you must add the /opt/homebrew or /usr/local paths to your shell PATH or run brew shellenv). Example for Apple Silicon: eval "$(brew shellenv)" or add export PATH="/opt/homebrew/bin:$PATH". [1]
If you see errors
- “Cannot install in Homebrew on ARM processor in Intel default prefix (/usr/local)” means you’re running ARM macOS but Homebrew is in the Intel prefix; reinstall or install a separate ARM Homebrew in /opt/homebrew. [4]
Sources
[1] Homebrew installation docs (default prefixes and installer behavior).
[2] Homebrew FAQ (why use the default prefix; bottles).
[3] Discussion/why /opt/homebrew was chosen (Apple StackExchange summary).
[4] Example error + community guidance (StackOverflow).
Document both Homebrew prefixes (Apple Silicon vs Intel).
The current snippet only shows the /opt/homebrew path used by Apple Silicon Macs. Intel Mac users would need /usr/local instead. Using brew --prefix in the documentation provides a portable solution that works across architectures.
📝 Suggested doc tweak
-export PATH="/opt/homebrew/bin:$PATH"
-export PKG_CONFIG_PATH="/opt/homebrew/opt/openssl@3/lib/pkgconfig:$PKG_CONFIG_PATH"
-export OPENSSL_ROOT_DIR="/opt/homebrew/opt/openssl@3"
+export HOMEBREW_PREFIX="$(brew --prefix)"
+export PATH="$HOMEBREW_PREFIX/bin:$PATH"
+export PKG_CONFIG_PATH="$HOMEBREW_PREFIX/opt/openssl@3/lib/pkgconfig:$PKG_CONFIG_PATH"
+export OPENSSL_ROOT_DIR="$HOMEBREW_PREFIX/opt/openssl@3"📝 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.
| brew install automake bzip2 cmake make git gpatch gnutls openssl@3 icu4c pkg-config libiconv zlib | |
| ``` | |
| To compile on macOS, you must set the following environment variables to ensure the build system can find OpenSSL and Homebrew dependencies: | |
| ```bash | |
| export PATH="/opt/homebrew/bin:$PATH" | |
| export PKG_CONFIG_PATH="/opt/homebrew/opt/openssl@3/lib/pkgconfig:$PKG_CONFIG_PATH" | |
| export OPENSSL_ROOT_DIR="/opt/homebrew/opt/openssl@3" | |
| make # or make debug | |
| brew install automake bzip2 cmake make git gpatch gnutls openssl@3 icu4c pkg-config libiconv zlib |
🤖 Prompt for AI Agents
In `@INSTALL.md` around lines 191 - 200, The INSTALL.md snippet hardcodes the
Apple Silicon Homebrew prefix (/opt/homebrew) which breaks on Intel Macs; update
the instructions to use a portable Homebrew prefix by referencing the result of
brew --prefix (e.g., use brew --prefix or brew --prefix openssl@3) when setting
PATH, PKG_CONFIG_PATH and OPENSSL_ROOT_DIR so both /opt/homebrew and /usr/local
installations work across architectures; locate the three export lines (PATH,
PKG_CONFIG_PATH, OPENSSL_ROOT_DIR) in the macOS build notes and replace the
hardcoded /opt/homebrew paths with commands that derive the prefix dynamically.
- Convert patch format from git-style (diff --git) to unified diff (diff -ruN) - Update line numbers in patch contexts for PostgreSQL compatibility - This makes the patches more portable and easier to apply across different environments
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
deps/postgresql/get_result_from_pgconn.patch (1)
9-23:⚠️ Potential issue | 🟡 MinorFix inaccurate docstring and clarify result-pointer lifetime constraints.
The inline comment describes this as "Get error result from PGconn" but
conn->resultholds any result type, not just errors. Correct this to match the more accurate header comment: "Get PGresult directly from PGconn."Additionally, while the
constqualifier andPGASYNC_IDLEcheck provide some safety, the returned pointer refers to internal libpq state that may become stale if the caller invokes other libpq operations on the same connection. The existing WARNING comment about not releasing the result is important but incomplete—consider expanding the header documentation to clarify the lifetime constraint: the pointer is valid only until the next libpq call on the same connection.deps/postgresql/handle_row_data.patch (2)
8-12:⚠️ Potential issue | 🟡 MinorNull-parameter check returns the same code as "delegate to PQisBusy", masking errors.
Returning
1for nullconn/resultis indistinguishable from a legitimate "not handled" case. A caller has no way to tell whether the function declined to process the message or was given invalid arguments. Consider returning a distinct error code (e.g.,-2or a dedicated enum) or at minimum adding an assertion/log.
158-164:⚠️ Potential issue | 🟠 Major
result->datais a dangling pointer intoconn->inBuffer— document the lifetime contract.After
conn->inStartis advanced (line 163), the memory region pointed to byresult->datais marked as consumed. A subsequentpqReadDataor buffer compaction will invalidate this pointer. The caller must copy or fully consumeresult->databefore any further libpq I/O call.At minimum, add a comment at the declaration site (
PSresult.datainlibpq-fe.h) and in the function's doc-block warning thatdatais only valid until the nextPQconsumeInput/PQgetResult/etc. call. If ProxySQL's usage pattern guarantees immediate consumption, a brief comment confirming that invariant would also help future maintainers.
🧹 Nitpick comments (6)
deps/postgresql/get_result_from_pgconn.patch (1)
32-33: Header warning could also mention pointer lifetime.The "DO NOT RELEASE THIS RESULT" warning is good, but callers also need to know that the pointer becomes invalid after any subsequent libpq call on the same connection. Consider expanding:
📝 Suggested documentation improvement
-/* Get PGresult directly from PGconn. WARNING: DO NOT RELEASE THIS RESULT */ +/* Get PGresult directly from PGconn. + * WARNING: The returned pointer is borrowed — DO NOT call PQclear() on it, + * and do not use it after any further libpq operation on the same conn. */deps/postgresql/handle_row_data.patch (5)
116-120: Inconsistent indentation and brace alignment in the buffer-availability check.This block mixes tabs and spaces and the closing brace doesn't align with the opening
if. The rest of the function uses tab-based indentation. This hurts readability in a safety-critical code path.🪄 Proposed formatting fix
- msgLength -= 4; - avail = conn->inEnd - conn->inCursor; - if (avail < msgLength) { - if ((conn->inCursor+(size_t)msgLength) <= (size_t)conn->inBufSize) - return EOF; - return 1; - } + msgLength -= 4; + avail = conn->inEnd - conn->inCursor; + if (avail < msgLength) + { + if ((conn->inCursor + (size_t) msgLength) <= (size_t) conn->inBufSize) + return EOF; + return 1; + }
105-109: Line 108:VALID_LONG_MESSAGE_TYPE(id)is always true here — dead code.At this point
idis guaranteed to be'D'(checked on line 94).'D'(DataRow) is a valid long message type in PostgreSQL, soVALID_LONG_MESSAGE_TYPE('D')is always true and themsgLength > 30000path can never triggerreturn 1. Consider removing the check to avoid misleading future readers, or add a comment explaining it's kept as defensive belt-and-suspenders.
84-85: Return value1is overloaded for both "normal delegation" and "protocol error" — callers can't distinguish them.Throughout
psHandleRowData,return 1is used for:
- Normal delegation: not BUSY (line 85), not a
'D'message (line 95), first packet (line 124).- Error/anomaly: malformed length (line 106), oversized non-long message (line 109), field-count mismatch (line 141), truncated field data (lines 138, 148, 154).
All these cases tell the caller "call PQisBusy", which is fine as a fallback but makes it impossible to detect and log malformed messages. Consider using a distinct negative return code for protocol errors (e.g.,
-2) alongside the documented0/1/-1set, or at least add inline comments clarifying which returns are "expected delegation" vs "error bailout."Also applies to: 94-95, 105-109, 137-141, 147-148, 153-154
161-161: RedundantasyncStatusassignment — already guaranteedPGASYNC_BUSY.Line 84 ensures
asyncStatus == PGASYNC_BUSYat function entry, and nothing in this function changes it before line 161. The assignment is a no-op. If it's intentional defensive coding, a comment would clarify intent; otherwise, it can be removed.
176-186: Clean upPSresult: remove commented-out field, add doc comment fordatalifetime.
- The
//int fieldcount;(line 185) is dead code in a public header — remove it.- The struct comment on line 177 is incomplete (
PSresult --with nothing after the dash).- Most importantly, document the lifetime of
data(valid only until the next libpq I/O call) to guard against misuse.🪄 Suggested cleanup
-/* ---------------- - * PSresult -- - * ---------------- - */ -typedef struct psResult -{ - char id; - int len; - const char* data; - //int fieldcount; -} PSresult; +/* ---------------- + * PSresult -- lightweight result container for ProxySQL row-data handling. + * 'data' points into PGconn's internal buffer and is only valid until the + * next libpq I/O call (e.g. PQconsumeInput, PQgetResult). + * ---------------- + */ +typedef struct psResult +{ + char id; + int len; + const char *data; +} PSresult;
|
Detailed List of Changes
1. Core ProxySQL Compatibility
include/proxysql_utils.h,lib/proxysql_utils.cpp, and various thread-related files (PgSQL_Thread.cpp,Base_Thread.cpp, etc.) to handle platform-specific differences in headers and system types.sha256crypt.cpp.lib/ProxySQL_GloVars.cppfor platform parity.2. Build System Infrastructure
common_mk/openssl_flags.mkandopenssl_version_check.mkto:OPENSSL_ROOT_DIRis set.lib64tolibon Darwin).grep/sedpattern matching to extract OpenSSL version strings.Makefileandsrc/MakefilewithDarwinguards to handle ICU linking, dynamic library extensions (.dylib), and correct linker flags.deps/Makefileto support building core dependencies (like MariaDB client) on macOS, including patches forzlibto preventfdopenconflicts.3. Test Dependency Stability (MySQL 8.4)
test/deps/mysql-8.4.0-clang17.patch) to resolve template strictness errors in MySQL 8.4 serialization headers encountered with modern Clang.test/deps/Makefileto automatically apply the MySQL compatibility patch.-j1) for MySQL connectors on Darwin to prevent intermittent race conditions during the generation of collation tables (uca9dump).4. Documentation
doc/BUILD-MACOS.mdwith a detailed step-by-step guide on environment setup (Homebrew, dependencies) and the build process.INSTALL.md.Commits Summary
62bc15fdImprove macOS build system OpenSSL detection and documentationbd16436eFix MySQL 8.4 build on macOS with C++20 compatibility patchd10cdd3dUpdated macOS build instructions in INSTALL.md and added doc/BUILD-MACOS.mdd43ae6e1Surgical fixes for macOS compatibility: headers, types, and Makefile linkingfdb8dfb1Build system: Darwin-specific fixes and strict platform paritySummary by CodeRabbit
New Features
Bug Fixes
Chores