fix: macOS build - sqlite3 link and missing libtool#5664
Conversation
- deps/Makefile: Use -dynamiclib -undefined dynamic_lookup for sqlite3 shared lib on Darwin (Linux -shared allows unresolved symbols, macOS does not). Also link OpenSSL for SHA1 symbol resolution. - workflows: Add libtool to brew install (provides glibtoolize needed by curl's autoreconf).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 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 |
![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 updates the deps/Makefile to correctly build libsqlite3.so on macOS by introducing conditional logic for the Darwin platform and applying specific linker flags for dynamic libraries. The review feedback suggests that the macOS build documentation should be updated to include the new libtool dependency mentioned in the PR description. Additionally, it is recommended to remove the explicit linking of SSL and Crypto libraries on macOS, as they are redundant when using -undefined dynamic_lookup and could lead to symbol conflicts.
| cd sqlite3/sqlite3 && patch -p0 < ../sqlite3_pass_exts.patch | ||
| cd sqlite3/sqlite3 && patch -p0 < ../throw.patch | ||
| cd sqlite3/sqlite3 && ${CC} ${MYCFLAGS} -fPIC -c -o sqlite3.o sqlite3.c -DSQLITE_ENABLE_MEMORY_MANAGEMENT -DSQLITE_ENABLE_JSON1 -DSQLITE_ENABLE_FTS5 -DSQLITE_DLL=1 -DSQLITE_ENABLE_MATH_FUNCTIONS | ||
| ifeq ($(UNAME_S),Darwin) |
There was a problem hiding this comment.
The pull request description mentions that libtool was added to the Homebrew install list (required for autoreconf in the curl build), but this change is missing from doc/BUILD-MACOS.md. Please include the update to the documentation to ensure that the build environment setup is complete for other users.
| cd sqlite3/sqlite3 && patch -p0 < ../throw.patch | ||
| cd sqlite3/sqlite3 && ${CC} ${MYCFLAGS} -fPIC -c -o sqlite3.o sqlite3.c -DSQLITE_ENABLE_MEMORY_MANAGEMENT -DSQLITE_ENABLE_JSON1 -DSQLITE_ENABLE_FTS5 -DSQLITE_DLL=1 -DSQLITE_ENABLE_MATH_FUNCTIONS | ||
| ifeq ($(UNAME_S),Darwin) | ||
| cd sqlite3/sqlite3 && ${CC} -dynamiclib -undefined dynamic_lookup -install_name @rpath/libsqlite3.so -o libsqlite3.so sqlite3.o $(LIB_SSL_PATH) $(LIB_CRYPTO_PATH) |
There was a problem hiding this comment.
Linking against $(LIB_SSL_PATH) and $(LIB_CRYPTO_PATH) on macOS is likely redundant when using -undefined dynamic_lookup. On Linux (line 277), libsqlite3.so is linked without these libraries, and ProxySQL (the host process) already provides the OpenSSL symbols at runtime. By using -undefined dynamic_lookup, the macOS linker will similarly allow these symbols to be resolved at runtime. Linking them statically here increases binary size and can lead to duplicate symbol issues if the host process also contains them. Consider removing these flags to maintain consistency with the Linux build.
cd sqlite3/sqlite3 && ${CC} -dynamiclib -undefined dynamic_lookup -install_name @rpath/libsqlite3.so -o libsqlite3.so sqlite3.o
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deps/Makefile (1)
274-278: Darwin sqlite3 link flags look correct; minor nit on.soextension.Using
-dynamiclib -undefined dynamic_lookupplus linking OpenSSL to resolve SHA1 is the right fix for Darwin. Two small notes:
- The output is still named
libsqlite3.soon macOS. Since this library is consumed viadlopen(plugin_name, RTLD_NOW)inlib/sqlite3db.cppwith an explicit path, this works, but the conventional macOS extension is.dylib. Keeping.sois fine for consistency with the Linux path/plugin_name and to avoid touching the loader logic — just flagging.-install_name@rpath/libsqlite3.so`` has no practical effect here because the library isdlopen'd by path rather than linked as a dependency (so `LC_RPATH`/install_name resolution isn't exercised). Harmless, but you could drop it to keep the rule minimal.No change required; the fix resolves the reported build failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/Makefile` around lines 274 - 278, The Darwin linking uses -dynamiclib -undefined dynamic_lookup and explicitly links OpenSSL which fixes the SHA1 symbol issue; leave the current rule that produces libsqlite3.so unchanged (it works with the dlopen(plugin_name, RTLD_NOW) call in lib/sqlite3db.cpp), but optionally you may replace the macOS extension with .dylib or remove the -install_name `@rpath/libsqlite3.so` for minimalism—no functional change required to the Makefile targets or flags in the sqlite3/sqlite3 rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deps/Makefile`:
- Around line 274-278: The Darwin linking uses -dynamiclib -undefined
dynamic_lookup and explicitly links OpenSSL which fixes the SHA1 symbol issue;
leave the current rule that produces libsqlite3.so unchanged (it works with the
dlopen(plugin_name, RTLD_NOW) call in lib/sqlite3db.cpp), but optionally you may
replace the macOS extension with .dylib or remove the -install_name
`@rpath/libsqlite3.so` for minimalism—no functional change required to the
Makefile targets or flags in the sqlite3/sqlite3 rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc4e2a27-dadf-4b0e-a2b6-57a58dcebf6f
📒 Files selected for processing (7)
.github/workflows/CI-build-macos-macos-13-genai.yml.github/workflows/CI-build-macos-macos-13-v31.yml.github/workflows/CI-build-macos-macos-13.yml.github/workflows/CI-build-macos-macos-14-genai.yml.github/workflows/CI-build-macos-macos-14-v31.yml.github/workflows/CI-build-macos-macos-14.ymldeps/Makefile
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🔇 Additional comments (6)
.github/workflows/CI-build-macos-macos-13-v31.yml (1)
39-39: LGTM —libtooladded forglibtoolize.Needed by
autoreconf -fiduring the curl build step. Change is consistent across all six macOS workflows..github/workflows/CI-build-macos-macos-14-genai.yml (1)
39-39: LGTM.Same
libtooladdition; consistent with sibling workflows..github/workflows/CI-build-macos-macos-13-genai.yml (1)
39-39: LGTM.
libtooladded to provideglibtoolizefor curl'sautoreconf..github/workflows/CI-build-macos-macos-14-v31.yml (1)
39-39: LGTM.Consistent
libtooladdition across macOS workflows..github/workflows/CI-build-macos-macos-13.yml (1)
39-39: LGTM.
libtoolprovidesglibtoolizerequired byautoreconf -fiduring the curl build..github/workflows/CI-build-macos-macos-14.yml (1)
39-39: LGTM.Matches the other five workflows; resolves the missing
glibtoolizeon clean macOS runners.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/proxysql_gtid.cpp`:
- Around line 161-171: The operators TrxId_Interval::operator<, operator== and
operator!= are const and call cmp(), so update cmp's signature in both its
declaration and definition to be a const member (e.g., int cmp(const
TrxId_Interval& other) const) so it can be called from const methods; also drop
the redundant const qualifier on the int return type if present. Modify the cmp
declaration in the class and the cmp definition in lib/proxysql_gtid.cpp
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d4a85fb-f7eb-4f1f-8301-75654cd36cbd
📒 Files selected for processing (2)
include/proxysql_gtid.hlib/proxysql_gtid.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (3)
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
#ifndef __CLASS_*_Hinclude guards in header files
Files:
include/proxysql_gtid.h
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
include/proxysql_gtid.hlib/proxysql_gtid.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file typically in
lib/directory
Files:
lib/proxysql_gtid.cpp
🧠 Learnings (1)
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code
Applied to files:
include/proxysql_gtid.hlib/proxysql_gtid.cpp
🔇 Additional comments (1)
include/proxysql_gtid.h (1)
31-33: LGTM — declarations are consistent with the.cpp.Making the comparison operators
const-qualified is the right fix so instances can be compared throughconstreferences (required bystd::list::sorton the containedTrxId_Intervals). Dropping the meaninglessconston theboolreturn type is also an improvement.Optional: for consistency, consider applying the same treatment to
contains,to_string,append,merge, andcmpin a follow-up (those are unchanged here, andcmpin particular is relevant — see the review comment onlib/proxysql_gtid.cpp).
|
1 participant
Summary
Fixes two issues preventing ProxySQL from building on macOS (GitHub Actions runners):
sqlite3 shared lib link —
cc -sharedon macOS requires all symbols resolved at link time, unlike Linux. Added-dynamiclib -undefined dynamic_lookupwith OpenSSL link flags for Darwin.Missing libtool —
autoreconf(called by curl build) requiresglibtoolize, provided by Homebrew'slibtoolpackage. Added to brew install list.These are real bugs that also affect local macOS builds in clean environments — the sqlite3 issue was masked on machines where OpenSSL was already in the linker search path.
Summary by CodeRabbit