Fix macOS compilation and add static zstd linking#5465
Conversation
- Add Homebrew include paths for zstd headers on macOS - Fix pthread_t cast on macOS where it's a pointer type On macOS, pthread_t is defined as _opaque_pthread_t * (a pointer), unlike Linux where it's an integral type. Use reinterpret_cast<uintptr_t> for proper conversion. Related: #5462
- Add zstd v1.5.7 as a static dependency in deps/ - Add zstd build rules to deps/Makefile - Add ZSTD_PATH, ZSTD_IDIR, ZSTD_LDIR to makefiles_paths.mk - Add zstd include path to lib/Makefile and src/Makefile - Link zstd statically in LIBPROXYSQLAR for all platforms - Remove -lzstd from dynamic linking flags (Linux and macOS) - Replace clickhouse-cpp bundled zstd with our static zstd This eliminates the runtime dependency on system/Homebrew zstd and ensures consistent behavior across platforms. Related: #5462
- Add README.md to deps/zstd documenting source and usage - Add comments in deps/Makefile for zstd build rule - Add comments in include/makefiles_paths.mk for ZSTD paths - Update INSTALL.md to note zstd is bundled and statically linked Related: #5462
- Add zstd cleanup to deps/Makefile cleanall target - Add deps/zstd/zstd-*/ to .gitignore for extracted sources
📝 WalkthroughWalkthroughThe pull request introduces zstd as a vendored dependency to ProxySQL with corresponding build infrastructure. Changes include adding a Changes
Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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, 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 significantly improves ProxySQL's build process and cross-platform compatibility by fixing macOS-specific compilation errors and transitioning to static linking for the zstd compression library. It also lays foundational work for future AI-driven capabilities through extensive new documentation and a Retrieval-Augmented Generation (RAG) Proof of Concept, alongside general updates to the project's changelog and contributing guidelines. 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. Ignored Files
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. Footnotes
|
|
There was a problem hiding this comment.
Pull request overview
This PR aims to fix macOS compilation issues and remove runtime zstd dependencies by bundling and statically linking zstd, while also including broad dependency, CI, and documentation updates.
Changes:
- Add Homebrew include paths / macOS-specific fixes and introduce bundled static
zstd. - Extend build tooling (OpenSSL detection/flags) and update several vendored dependency pins/patches.
- Add/refresh substantial documentation and CI workflow orchestration.
Reviewed changes
Copilot reviewed 107 out of 1006 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/LLM_Bridge/TESTING.md | Adds detailed NL2SQL testing guide documentation |
| doc/BUILD-MACOS.md | Adds macOS build instructions via Homebrew |
| doc/ANOMALY_DETECTION/README.md | Adds anomaly detection documentation |
| doc/AI-DOCUMENTATION-FRAMEWORK.md | Adds framework for AI-generated documentation standards |
| deps/zstd/zstd | Adds zstd version pointer/symlink entry |
| deps/zstd/README.md | Documents bundled zstd source/version/build |
| deps/sqlite3/throw.patch | Updates sqlite patch offsets / changes toupper declaration patching |
| deps/sqlite3/sqlite3_pass_exts.patch | Updates sqlite patch offsets |
| deps/sqlite3/sqlite3.c-multiplication-overflow.patch | Removes sqlite overflow patch |
| deps/sqlite3/sqlite3 | Updates sqlite amalgamation version pointer |
| deps/sqlite3/sqlite-vec-source/sqlite-vec.h.tmpl | Adds sqlite-vec header template |
| deps/sqlite3/sqlite-vec-source/sqlite-vec.h | Adds generated sqlite-vec header |
| deps/sqlite3/sqlite-vec-source/README.md | Adds sqlite-vec integration documentation |
| deps/sqlite3/from_unixtime.patch | Updates sqlite from_unixtime patch metadata/offsets |
| deps/sqlite3/README.md | Adds ProxySQL sqlite-vec integration overview |
| deps/postgresql/pqsendpipelinesync.patch | Adds libpq pipeline sync API patch |
| deps/postgresql/postgresql | Adds PostgreSQL version pointer |
| deps/postgresql/handle_row_data.patch | Adds ProxySQL-specific lightweight libpq row handler patches |
| deps/postgresql/get_result_from_pgconn.patch | Adds helper to access PGresult from PGconn |
| deps/postgresql/fmt_err_msg.patch | Adds formatted error verbosity modes to psql/libpq |
| deps/postgresql/bind_fmt_text.patch | Adjusts libpq text parameter length handling |
| deps/mariadb-client-library/zutil.h.patch | Removes custom fdopen override patching |
| deps/mariadb-client-library/zutil.c.patch | Modernizes zlib function prototypes for C compilers |
| deps/mariadb-client-library/mariadb_lib.c.metadata_column_check.patch | Adds integrity check for metadata column-count packet |
| deps/mariadb-client-library/bool_keyword.patch | Renames bool typedef to bbool in MariaDB connector headers/tests |
| deps/libusual/libusual | Adds libusual version pointer |
| deps/libssl/verify-bio_st-match.sh | Removes verification script |
| deps/libssl/openssl | Removes OpenSSL version pointer |
| deps/libssl/curve448.c-multiplication-overflow.patch | Removes OpenSSL overflow patch |
| deps/libssl/a_time.c-multiplication-overflow.patch | Removes OpenSSL overflow patch |
| deps/libssl/README.md | Notes OpenSSL linking behavior change |
| deps/libscram/scram.c.diff | Switches strtok to strtok_r in scram diff |
| deps/libscram/include/scram.h | Adds vendored scram header |
| deps/libscram/include/scram-internal.h | Adds scram internal typedef/header helpers |
| deps/libscram/Makefile | Adds standalone build Makefile for libscram |
| deps/libmicrohttpd/libmicrohttpd | Updates libmicrohttpd version pointer |
| deps/libmicrohttpd/connection.c-snprintf-overflow.patch | Removes libmicrohttpd overflow patch |
| deps/libconfig/libconfig | Updates libconfig version pointer |
| deps/curl/curl | Updates curl version pointer |
| deps/coredumper/includes.patch | Removes coredumper includes patch |
| deps/coredumper/coredumper | Updates coredumper version pointer |
| deps/clickhouse-cpp/wire_format.patch | Removes clickhouse include patch |
| deps/clickhouse-cpp/clickhouse-cpp | Updates clickhouse-cpp version pointer |
| common_mk/openssl_version_check.mk | Adds OpenSSL version checking target logic |
| common_mk/openssl_flags.mk | Adds OpenSSL include/lib path detection logic |
| RELEASE_NOTES_WORKFLOW.md | Adds release notes generation workflow documentation |
| README.md | Adds TOC + updates install links/examples + PGSQL admin examples |
| RAG_POC/sql-examples.md | Adds SQL examples for RAG index querying |
| RAG_POC/schema.sql | Adds SQLite schema for RAG PoC |
| RAG_POC/sample_sqlite.sql | Adds sample SQLite config for ingestion testing |
| RAG_POC/sample_mysql.sql | Adds sample MySQL dataset for ingestion testing |
| RAG_POC/rag_system_prompt.md | Adds agent system prompt for RAG orchestrator |
| RAG_POC/mcp-tools.md | Defines MCP tool contracts for RAG querying |
| RAG_POC/embeddings-design.md | Adds embeddings design blueprint |
| RAG_POC/architecture-runtime-retrieval.md | Adds runtime retrieval architecture blueprint |
| RAG_POC/architecture-data-model.md | Adds data model/ingestion architecture blueprint |
| RAG_POC/Makefile | Adds PoC build Makefile for rag_ingest |
| RAG_POC/EMBEDDING_TEST_PLAN.md | Adds embedding test plan documentation |
| RAG_IMPLEMENTATION_SUMMARY.md | Adds RAG implementation summary document |
| RAG_IMPLEMENTATION_COMPLETE.md | Adds RAG implementation completion document |
| RAG_FILE_SUMMARY.md | Adds RAG file inventory summary document |
| RAG_COMPLETION_SUMMARY.md | Adds RAG completion summary document |
| ProxySQL-3.0.4-Release-Notes.md | Adds ProxySQL 3.0.4 release notes document |
| INSTALL.md | Updates installation instructions + adds macOS section + repo URLs |
| CONTRIBUTING.md | Adds contributing guide |
| .github/workflows/CI-trigger.yml | Adds CI trigger workflow orchestration entrypoint |
| .github/workflows/CI-taptests.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-taptests-ssl.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-taptests-groups.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-taptests-asan.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-shuntest.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-selftests.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-repltests.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-package-build.yml | Updates branch patterns + adds trigger input |
| .github/workflows/CI-maketest.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-codeql.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-builds.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-basictests.yml | Refactors workflow triggering via workflow_run |
| .github/workflows/CI-3p-sqlalchemy.yml | Splits mysql/mariadb/pgsql jobs + workflow_run |
| .github/workflows/CI-3p-postgresql.yml | Adds new third-party PostgreSQL workflow |
| .github/workflows/CI-3p-php-pdo-pgsql.yml | Adds new third-party PHP PDO PGSQL workflow |
| .github/workflows/CI-3p-php-pdo-mysql.yml | Splits mysql/mariadb jobs + workflow_run |
| .github/workflows/CI-3p-pgjdbc.yml | Adds new pgjdbc workflow |
| .github/workflows/CI-3p-mysql-connector-j.yml | Splits mysql/mariadb jobs + workflow_run |
| .github/workflows/CI-3p-mariadb-connector-c.yml | Splits mysql/mariadb jobs + workflow_run |
| .github/workflows/CI-3p-laravel-framework.yml | Splits mysql/mariadb/pgsql jobs + workflow_run |
| .github/workflows/CI-3p-django-framework.yml | Splits mysql/mariadb/pgsql jobs + workflow_run |
| .github/workflows/CI-3p-aiomysql.yml | Adds new aiomysql workflow with mysql/mariadb jobs |
| .github/ISSUE_TEMPLATE/bug_report.md | Adds bug report issue template |
| .aider.conf.yml | Adds repository-local aider configuration |
Comments suppressed due to low confidence (1)
deps/postgresql/pqsendpipelinesync.patch:1
- The header comment says “without flushing the send buffer”, but the implementation calls
pqPipelineFlush(conn). Either update the comment to reflect the actual behavior (flushes conditionally in pipeline mode), or change the implementation if true “no flush” semantics are required by callers.
diff --git src/interfaces/libpq/fe-exec.c src/interfaces/libpq/fe-exec.c
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "OpenSSL header file not found at $$header_path"; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| version_number=$$(grep "# *define OPENSSL_VERSION_STR" $$header_path | sed -E 's/.*"([^"]+)".*/\1/'); \ |
There was a problem hiding this comment.
OPENSSL_VERSION_STR is typically a string like "OpenSSL 3.6.1 11 Feb 2025", so splitting on . yields a non-numeric major (e.g. "OpenSSL 3"), breaking numeric comparisons and potentially failing valid builds. Prefer parsing OPENSSL_VERSION_NUMBER from opensslv.h (hex) or extract only the numeric X.Y.Z portion via a regex before comparing.
| version_number=$$(grep "# *define OPENSSL_VERSION_STR" $$header_path | sed -E 's/.*"([^"]+)".*/\1/'); \ | |
| version_number=$$(grep "# *define OPENSSL_VERSION_STR" $$header_path | sed -E 's/.*OpenSSL ([0-9]+\.[0-9]+\.[0-9]+).*/\1/'); \ |
| major=$$(echo $$version_number | cut -d'.' -f1); \ | ||
| minor=$$(echo $$version_number | cut -d'.' -f2); \ | ||
| patch=$$(echo $$version_number | cut -d'.' -f3); \ | ||
| if [ $$major -gt 3 ] || { [ $$major -eq 3 ] && { [ $$minor -gt 0 ] || { [ $$minor -eq 0 ] && [ $$patch -ge 0 ]; }; }; }; then \ |
There was a problem hiding this comment.
OPENSSL_VERSION_STR is typically a string like "OpenSSL 3.6.1 11 Feb 2025", so splitting on . yields a non-numeric major (e.g. "OpenSSL 3"), breaking numeric comparisons and potentially failing valid builds. Prefer parsing OPENSSL_VERSION_NUMBER from opensslv.h (hex) or extract only the numeric X.Y.Z portion via a regex before comparing.
| ifeq ($(UNAME_S),Darwin) | ||
| SSL_LDIR := $(CUSTOM_OPENSSL_PATH)/lib | ||
| else | ||
| SSL_LDIR := $(CUSTOM_OPENSSL_PATH)/lib64 |
There was a problem hiding this comment.
Defaulting SSL_LDIR to $(CUSTOM_OPENSSL_PATH)/lib64 on non-Darwin breaks common layouts where OpenSSL installs into lib/ (not lib64/). Consider probing both lib64 and lib (or using pkg-config --variable=libdir when available) to reduce build failures for custom OpenSSL installs.
| SSL_LDIR := $(CUSTOM_OPENSSL_PATH)/lib64 | |
| # Prefer lib64 when present, but fall back to lib to support installations | |
| # that use lib/ instead of lib64/. | |
| ifneq ("$(wildcard $(CUSTOM_OPENSSL_PATH)/lib64)","") | |
| SSL_LDIR := $(CUSTOM_OPENSSL_PATH)/lib64 | |
| else | |
| SSL_LDIR := $(CUSTOM_OPENSSL_PATH)/lib | |
| endif |
| pull_request: | ||
| paths-ignore: | ||
| - '.github/**' | ||
| - '**.md' |
There was a problem hiding this comment.
The ignore glob for markdown has a trailing space ('**.md' ), which can prevent GitHub Actions from matching it as intended and cause workflows to run on markdown-only changes. Remove the trailing whitespace so the pattern is exactly **.md.
| - '**.md' | |
| - '**.md' |
| Table of Contents | ||
| ================= | ||
|
|
||
| - [Introduction](#introduction) |
There was a problem hiding this comment.
The ToC links (#introduction, etc.) may not resolve correctly because the headings now include embedded HTML (Introduction<a name="..."></a>), which alters GitHub’s auto-generated heading IDs. To keep stable anchors, consider using plain Markdown headings (e.g. ## Introduction) and place explicit anchors on separate lines (e.g. <a id="introduction"></a>) or rely solely on GitHub’s generated IDs without manual <a name=...> tags.
| /* ProxySQL special handler function */ | ||
| extern int PShandleRowData(PGconn *conn, bool is_first_packet, PSresult* result); | ||
|
|
||
| +/* Send a pipeline sync message without flushing the send buffer */ |
There was a problem hiding this comment.
The header comment says “without flushing the send buffer”, but the implementation calls pqPipelineFlush(conn). Either update the comment to reflect the actual behavior (flushes conditionally in pipeline mode), or change the implementation if true “no flush” semantics are required by callers.
| +/* Send a pipeline sync message without flushing the send buffer */ | |
| +/* Send a pipeline sync message (see implementation for flushing behavior) */ |
| LIBUSUAL_DIR ?= /path/to/libusual | ||
|
|
||
| # OpenSSL dir | ||
| LIBOPENSSL_DIR ?= /path/to/openssl | ||
|
|
||
| # PostgreSQL dir | ||
| POSTGRESQL_DIR ?= /path/to/postgresql | ||
| POSTGRESQL_INCLUDE_DIR = $(POSTGRESQL_DIR)/src/include/ |
There was a problem hiding this comment.
The default include paths are placeholder /path/to/... values, so invoking this Makefile without explicitly setting variables will fail and can break automated dependency builds if it’s wired into deps/Makefile. Prefer deriving defaults from the repository root (relative paths), or fail fast with a clear $(error ...) message when these variables are unset, so builds don’t proceed with invalid include directories.
| LIBUSUAL_DIR ?= /path/to/libusual | |
| # OpenSSL dir | |
| LIBOPENSSL_DIR ?= /path/to/openssl | |
| # PostgreSQL dir | |
| POSTGRESQL_DIR ?= /path/to/postgresql | |
| POSTGRESQL_INCLUDE_DIR = $(POSTGRESQL_DIR)/src/include/ | |
| ifndef LIBUSUAL_DIR | |
| $(error LIBUSUAL_DIR is not set. Please invoke make with LIBUSUAL_DIR=<path to libusual>) | |
| endif | |
| # OpenSSL dir | |
| ifndef LIBOPENSSL_DIR | |
| $(error LIBOPENSSL_DIR is not set. Please invoke make with LIBOPENSSL_DIR=<path to OpenSSL>) | |
| endif | |
| # PostgreSQL dir | |
| ifndef POSTGRESQL_DIR | |
| $(error POSTGRESQL_DIR is not set. Please invoke make with POSTGRESQL_DIR=<path to PostgreSQL source tree>) | |
| endif | |
| POSTGRESQL_INCLUDE_DIR ?= $(POSTGRESQL_DIR)/src/include/ |
| #if !defined(bool) && !defined(bool_defined) && (!defined(HAVE_BOOL) || !defined(__cplusplus)) | ||
| -typedef char bool; /* Ordinary boolean values 0 1 */ | ||
| +typedef char bbool; /* Ordinary boolean values 0 1 */ | ||
| #endif |
There was a problem hiding this comment.
Renaming the typedef from bool to bbool avoids clashing with the C++ keyword, but it can break any C compilation units that relied on this header providing a bool typedef (especially if they don’t include <stdbool.h>). A safer fix is to ensure the typedef never activates under __cplusplus (e.g. tighten the preprocessor condition), or switch C code to <stdbool.h> while keeping C++ builds unaffected—without changing the exposed type name for C consumers.
| #endif | ||
|
|
||
| #define SQLITE_VEC_VERSION "v${VERSION}" | ||
| // TODO rm |
There was a problem hiding this comment.
The // TODO rm marker is committed in the public template and will likely be perpetuated into generated headers or downstream copies. Remove the TODO (or convert it into a meaningful comment) to avoid shipping “temporary” notes in vendored third-party templates.
| // TODO rm | |
| // Build date of this SQLite vector extension, substituted during template expansion. |
| @@ -0,0 +1,63 @@ | |||
| # Compiling ProxySQL on macOS | |||
There was a problem hiding this comment.
The PR title/description focuses on macOS compilation fixes and static zstd linking, but the diff includes many additional, unrelated changes (large RAG PoC documentation set, multiple dependency bumps, extensive CI workflow refactors, new contributor docs). Consider splitting these into separate PRs (or updating the PR description to explicitly cover the broader scope) to keep review, rollback, and release risk manageable.
![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 a wide range of changes, including fixes for macOS compilation, static linking of the zstd library, and a significant number of new features and documentation related to AI, RAG, and anomaly detection. The build system has been substantially refactored to support these new features and improve cross-platform compatibility. My review focuses on the new code and build script modifications. I've identified a few potential issues: a library linking order problem in a new Makefile, a security concern regarding SQL injection in a proof-of-concept tool, and a misconfiguration in the build script for locating OpenSSL on macOS. These should be addressed to ensure build stability and security.
| static void insert_doc(MySQLDB& db, | ||
| int source_id, | ||
| const std::string& source_name, | ||
| const std::string& doc_id, | ||
| const std::string& pk_json, | ||
| const std::string& title, | ||
| const std::string& body, | ||
| const std::string& meta_json) { | ||
| g_logger.debug(std::string("Inserting document: ") + doc_id + | ||
| ", title_length=" + std::to_string(title.size()) + | ||
| ", body_length=" + std::to_string(body.size())); | ||
|
|
||
| std::string e_doc_id = sql_escape_single_quotes(doc_id); | ||
| std::string e_source_name = sql_escape_single_quotes(source_name); | ||
| std::string e_pk_json = sql_escape_single_quotes(pk_json); | ||
| std::string e_title = sql_escape_single_quotes(title); | ||
| std::string e_body = sql_escape_single_quotes(body); | ||
| std::string e_meta = sql_escape_single_quotes(meta_json); | ||
|
|
||
| // Use std::ostringstream to avoid fixed buffer size issues | ||
| std::ostringstream sql; | ||
| sql << "INSERT INTO rag_documents(doc_id, source_id, source_name, pk_json, title, body, metadata_json) " | ||
| << "VALUES('" << e_doc_id << "', " << source_id << ", '" << e_source_name << "', '" | ||
| << e_pk_json << "', '" << e_title << "', '" << e_body << "', '" << e_meta << "')"; | ||
|
|
||
| db.execute(sql.str().c_str()); | ||
| g_logger.trace("Document inserted successfully"); | ||
| } |
There was a problem hiding this comment.
The functions insert_doc, insert_chunk, insert_fts, and insert_vec construct SQL queries by manually escaping strings and concatenating them. This is a common source of SQL injection vulnerabilities and is considered a security risk. Even for a proof-of-concept tool, it's a best practice to use prepared statements with parameter binding. The MySQL C API provides functions like mysql_stmt_init, mysql_stmt_prepare, mysql_stmt_bind_param, and mysql_stmt_execute for this purpose. Using them would eliminate this entire class of vulnerability.
| cd mariadb-client-library/mariadb_client && cmake . -Wno-dev -DCMAKE_BUILD_TYPE=RelWithDebInfo -DOPENSSL_ROOT_DIR=$(SSL_IDIR) -DOPENSSL_LIBRARIES=$(SSL_LDIR) -DICONV_LIBRARIES=$$(brew --prefix libiconv)/lib -DICONV_INCLUDE=$$(brew --prefix libiconv)/include -DCMAKE_POLICY_VERSION_MINIMUM=3.5 . | ||
| else | ||
| cd mariadb-client-library/mariadb_client && cmake . -Wno-dev -DCMAKE_BUILD_TYPE=RelWithDebInfo -DOPENSSL_ROOT_DIR=$(SSL_IDIR) -DOPENSSL_LIBRARIES=$(SSL_LDIR) -DCMAKE_POLICY_VERSION_MINIMUM=3.5 . |
There was a problem hiding this comment.
The OPENSSL_ROOT_DIR variable for CMake is incorrectly being set to $(SSL_IDIR), which is an include directory path (e.g., /opt/homebrew/opt/openssl@3/include). CMake's find_package(OpenSSL) module expects OPENSSL_ROOT_DIR to be the root prefix of the OpenSSL installation (e.g., /opt/homebrew/opt/openssl@3), from which it can locate the include/ and lib/ subdirectories. This misconfiguration can lead to build failures when CMake cannot find the OpenSSL libraries. A variable holding the OpenSSL prefix should be used instead. For example, you could define SSL_PREFIX in common_mk/openssl_flags.mk using pkg-config --variable=prefix openssl and then use -DOPENSSL_ROOT_DIR=$(SSL_PREFIX) here.
| # Use static libcurl | ||
| CURL_STATIC_LIB := $(ROOT_DIR)/deps/curl/curl/lib/.libs/libcurl.a | ||
|
|
||
| LIBS := -lmariadbclient -lssl -lcrypto -lcrypt -ldl -lpthread $(CURL_STATIC_LIB) -lz |
There was a problem hiding this comment.
The order of libraries in this LIBS definition can be problematic. Linkers generally require that libraries are listed in an order where a library is listed after any other libraries that depend on it. Both libmariadbclient and libcurl depend on libssl and libcrypto. A safer ordering would be to list $(CURL_STATIC_LIB) and -lmariadbclient before -lssl and -lcrypto.
LIBS := -lmariadbclient $(CURL_STATIC_LIB) -lssl -lcrypto -lcrypt -ldl -lpthread -lz
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/Makefile (1)
36-44:⚠️ Potential issue | 🟠 MajorVariable ordering issue:
LDIRSis appended before it's defined.The Darwin block on lines 36-39 appends to
LDIRSusing+=, butLDIRSis defined later on lines 41-44. This means the Darwin-specific library paths (-L/usr/local/lib -L/opt/homebrew/lib) will be lost becauseLDIRSis overwritten by the assignment on line 41.🐛 Proposed fix: Move Darwin block after LDIRS definition
-I$(ZSTD_IDIR) \ -ifeq ($(UNAME_S),Linux) - IDIRS += -I$(COREDUMPER_IDIR) -endif -ifeq ($(UNAME_S),Darwin) - IDIRS += -I/usr/local/include -I/opt/homebrew/include - LDIRS += -L/usr/local/lib -L/opt/homebrew/lib -endif - LDIRS := -L$(JEMALLOC_LDIR) \ -L$(RE2_LDIR) \ -L$(LIBINJECTION_LDIR) \ -L$(PROMETHEUS_LDIR) + +ifeq ($(UNAME_S),Linux) + IDIRS += -I$(COREDUMPER_IDIR) +endif +ifeq ($(UNAME_S),Darwin) + IDIRS += -I/usr/local/include -I/opt/homebrew/include + LDIRS += -L/usr/local/lib -L/opt/homebrew/lib +endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Makefile` around lines 36 - 44, The Darwin-specific append to LDIRS (the ifeq block testing UNAME_S == Darwin) is currently before the LDIRS definition so its += additions get overwritten; move the entire ifeq $(UNAME_S),Darwin ... endif block to after the LDIRS := ... assignment (or alternately change the main LDIRS to use += instead of :=) so that the Darwin -L/usr/local/lib and -L/opt/homebrew/lib entries are preserved; reference the LDIRS variable and the ifeq (UNAME_S,Darwin) block when making the change.
🧹 Nitpick comments (2)
deps/Makefile (1)
186-186: Consider declaringzstdas a PHONY target.While the target works correctly because no file named
zstdexists in this directory, declaring it as PHONY is best practice and ensures the rule always runs when invoked. Note: Other similar targets in this file (lz4, ev, curl) also lack PHONY declarations, so this is consistent with existing patterns.♻️ Suggested improvement
zstd: zstd/zstd/lib/libzstd.a +.PHONY: zstd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/Makefile` at line 186, Declare the zstd Makefile target as phony so it always runs: add a .PHONY entry including the target name "zstd" (e.g., .PHONY: zstd) near the other PHONY declarations or at the top of the file; update any grouped PHONY line that already lists lz4, ev, curl to include zstd for consistency with targets like lz4/ev/curl and ensure no behavior changes to the rule "zstd: zstd/zstd/lib/libzstd.a".deps/zstd/README.md (1)
16-19: Minor path clarification in build example.The build example shows
cd zstd/zstd/libafter extracting, but the firstzstdin the path is ambiguous since the reader is already indeps/zstd/. Consider clarifying:📝 Suggested documentation improvement
```bash -cd deps/zstd && tar -zxf zstd-1.5.7.tar.gz -cd zstd/zstd/lib && make libzstd.a +# From repository root: +cd deps/zstd && tar -zxf zstd-1.5.7.tar.gz +cd deps/zstd/zstd-1.5.7/lib && make libzstd.a</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@deps/zstd/README.mdaround lines 16 - 19, Update the README build example to
remove the ambiguous relative path by explicitly showing commands from the
repository root: after extracting zstd-1.5.7, change the second cd to the full
path (e.g., deps/zstd/zstd-1.5.7/lib) so the sequence is unambiguous; include a
short note like "From repository root:" to clarify the context and replace the
currentcd zstd/zstd/libexample in the README.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In@lib/Makefile:
- Around line 36-44: The Darwin-specific append to LDIRS (the ifeq block testing
UNAME_S == Darwin) is currently before the LDIRS definition so its += additions
get overwritten; move the entire ifeq $(UNAME_S),Darwin ... endif block to after
the LDIRS := ... assignment (or alternately change the main LDIRS to use +=
instead of :=) so that the Darwin -L/usr/local/lib and -L/opt/homebrew/lib
entries are preserved; reference the LDIRS variable and the ifeq
(UNAME_S,Darwin) block when making the change.
Nitpick comments:
In@deps/Makefile:
- Line 186: Declare the zstd Makefile target as phony so it always runs: add a
.PHONY entry including the target name "zstd" (e.g., .PHONY: zstd) near the
other PHONY declarations or at the top of the file; update any grouped PHONY
line that already lists lz4, ev, curl to include zstd for consistency with
targets like lz4/ev/curl and ensure no behavior changes to the rule "zstd:
zstd/zstd/lib/libzstd.a".In
@deps/zstd/README.md:
- Around line 16-19: Update the README build example to remove the ambiguous
relative path by explicitly showing commands from the repository root: after
extracting zstd-1.5.7, change the second cd to the full path (e.g.,
deps/zstd/zstd-1.5.7/lib) so the sequence is unambiguous; include a short note
like "From repository root:" to clarify the context and replace the currentcd zstd/zstd/libexample in the README.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `4fcdc7da-54d0-4be1-adf0-001103af52fc` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 540fa174b4160a47b53ae734776b65e663d325e3 and 9fd4bcfe24f1541257ca6e1d77fe79fb3877f7d7. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `deps/zstd/zstd-1.5.7.tar.gz` is excluded by `!**/*.gz` </details> <details> <summary>📒 Files selected for processing (9)</summary> * `.gitignore` * `INSTALL.md` * `deps/Makefile` * `deps/zstd/README.md` * `deps/zstd/zstd` * `include/makefiles_paths.mk` * `lib/Makefile` * `lib/log_utils.cpp` * `src/Makefile` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary
This PR fixes compilation issues on macOS and adds static linking for the zstd compression library, eliminating runtime dependencies on Homebrew/system zstd.
Fixes #5462
Changes
1. Fix macOS Compilation Issues (88be144)
Problem: ProxySQL failed to compile on macOS due to:
pthread_tcast on macOS wherepthread_tis a pointer type (_opaque_pthread_t *), not an integral type like on LinuxSolution:
-I/opt/homebrew/includeto Darwin-specific include paths insrc/Makefileandlib/Makefilepthread_tcast inlib/log_utils.cppusingreinterpret_cast<uintptr_t>2. Add Static Zstd Linking (3f1b0a1)
Problem: zstd was dynamically linked from Homebrew on macOS, creating a runtime dependency and potential compatibility issues.
Solution:
deps/zstd/deps/MakefileZSTD_PATH,ZSTD_IDIR,ZSTD_LDIRtoinclude/makefiles_paths.mklib/Makefileandsrc/MakefileLIBPROXYSQLARfor all platforms-lzstdfrom dynamic linking flags3. Documentation (df97ae9)
deps/zstd/README.mddocumenting source, version, and usagedeps/Makefilefor zstd build ruleinclude/makefiles_paths.mkfor ZSTD pathsINSTALL.mdto note zstd is bundled and statically linked4. Cleanup (9fd4bcf)
cleanalltarget indeps/Makefiledeps/zstd/zstd-*/to.gitignoreFiles Changed
.gitignoreINSTALL.mddeps/Makefiledeps/zstd/README.mddeps/zstd/zstddeps/zstd/zstd-1.5.7.tar.gzinclude/makefiles_paths.mklib/Makefilelib/log_utils.cppsrc/MakefileBenefits
Testing
Tested on:
Before (dynamic linking)
After (static linking)
Build Instructions
macOS
Linux
make build_deps && makeSummary by CodeRabbit
Documentation
Chores