-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cmake): Add Homebrew path detection for mariadb-connector-c
to fix macOS build failure.
#582
Conversation
WalkthroughThe changes introduce enhancements to the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
endif() | ||
|
||
if(NOT mariadbclient_PKGCONF_FOUND) | ||
message(FATAL_ERROR "pkg_config cannot find mariadb") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a placeholder error message. May need a more meaningful fatal log
execute_process(COMMAND brew --prefix mariadb-connector-c OUTPUT_VARIABLE mariadb_MACOS_PREFIX) | ||
string(STRIP "${mariadb_MACOS_PREFIX}" mariadb_MACOS_PREFIX) | ||
set(CMAKE_PREFIX_PATH "${mariadb_MACOS_PREFIX};${CMAKE_PREFIX_PATH}") | ||
pkg_check_modules(mariadbclient_PKGCONF QUIET "lib${mariadbclient_LIBNAME}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might not need QUIET
here
pkg_check_modules(mariadbclient_PKGCONF QUIET "lib${mariadbclient_LIBNAME}") | |
pkg_check_modules(mariadbclient_PKGCONF "lib${mariadbclient_LIBNAME}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/core/cmake/Modules/FindMariaDBClient.cmake (1)
30-32
: Enhance error message with platform-specific guidanceThe error message should provide platform-specific installation instructions to help users resolve the issue.
Consider this improved implementation:
if(NOT mariadbclient_PKGCONF_FOUND) - message(FATAL_ERROR "pkg_config cannot find mariadb") + if(APPLE) + message(FATAL_ERROR "pkg_config cannot find mariadb. Please ensure mariadb-connector-c is installed via Homebrew: brew install mariadb-connector-c") + else() + message(FATAL_ERROR "pkg_config cannot find mariadb. Please ensure mariadb-connector-c development package is installed on your system") + endif() endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/core/cmake/Modules/FindMariaDBClient.cmake
(1 hunks)
🔇 Additional comments (1)
components/core/cmake/Modules/FindMariaDBClient.cmake (1)
23-32
: Verify the fix resolves the macOS workflow failure
The implementation looks correct for handling keg-only Homebrew formulae. Let's verify the changes resolve the specific workflow failure.
✅ Verification successful
Implementation correctly handles keg-only MariaDB Connector/C on macOS
The changes are well-implemented and follow established patterns in the codebase. The solution:
- Properly handles keg-only Homebrew formulae by using
brew --prefix
- Follows the same approach used for other dependencies (e.g., libarchive) in the codebase
- Maintains the pkg-config workflow while adding macOS-specific handling
- Preserves compatibility with other platforms through conditional execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the CMake configuration works with the Homebrew-installed mariadb-connector-c
# Test: Check if the error message appears in recent workflow runs
gh run list --workflow ".github/workflows/macos.yml" --limit 5 --json conclusion,databaseId | \
jq -r '.[] | select(.conclusion=="failure") | .databaseId' | \
while read -r run_id; do
gh run view "$run_id" --log | grep -A 5 "pkg_config cannot find mariadb"
done
Length of output: 282
Script:
#!/bin/bash
# Let's verify the CMake configuration and MariaDB client handling
# Check for any existing MariaDB client configurations
fd -e cmake -e txt "MariaDB" -X cat {}
# Look for pkg-config related configurations
rg -A 5 "pkg_check_modules"
# Check for any other Homebrew-related configurations
rg -A 5 "brew.*prefix"
Length of output: 10699
Co-authored-by: kirkrodrigues <[email protected]>
* ffi: Add support for serializing/deserializing auto-generated and user-generated schema tree node IDs. (y-scope#557) Co-authored-by: kirkrodrigues <[email protected]> * clp: Add missing C++ standard library includes in IR parsing files. (y-scope#561) Co-authored-by: kirkrodrigues <[email protected]> * log-viewer-webui: Update `yscope-log-viewer` to the latest version (which uses `clp-ffi-js`). (y-scope#562) * package: Upgrade dependencies to resolve security issues. (y-scope#536) * clp-s: Implement table packing (y-scope#466) Co-authored-by: wraymo <[email protected]> Co-authored-by: Kirk Rodrigues <[email protected]> Co-authored-by: wraymo <[email protected]> * log-viewer-webui: Update `yscope-log-viewer` to the latest version. (y-scope#565) * ci: Switch GitHub macOS build workflow to use macos-13 (x86) and macos-14 (ARM) runners. (y-scope#566) * core: Add support for user-defined HTTP headers in `NetworkReader`. (y-scope#568) Co-authored-by: Lin Zhihao <[email protected]> Co-authored-by: Xiaochong Wei <[email protected]> * chore: Update to the latest version of yscope-dev-utils. (y-scope#574) * build(core): Upgrade msgpack to v7.0.0. (y-scope#575) * feat(ffi): Update IR stream protocol version handling in preparation for releasing the kv-pair IR stream format: (y-scope#573) - Bump the IR stream protocol version to 0.1.0 for the kv-pair IR stream format. - Treat the previous IR stream format's versions as backwards compatible. - Differentiate between backwards-compatible and supported versions during validation. Co-authored-by: kirkrodrigues <[email protected]> * fix(taskfiles): Trim trailing slash from URL prefix in `download-and-extract-tar` (fixes y-scope#577). (y-scope#578) * fix(ffi): Correct `clp::ffi::ir_stream::Deserializer::deserialize_next_ir_unit`'s return value when failing to read the next IR unit's type tag. (y-scope#579) * fix(taskfiles): Update `yscope-log-viewer` sources in `log-viewer-webui-clients` sources list (fixes y-scope#576). (y-scope#580) * fix(cmake): Add Homebrew path detection for `mariadb-connector-c` to fix macOS build failure. (y-scope#582) Co-authored-by: kirkrodrigues <[email protected]> * refactor(ffi): Make `get_schema_subtree_bitmap` a public method of `KeyValuePairLogEvent`. (y-scope#581) * ci: Schedule GitHub workflows to daily run to detect failures due to upgraded dependencies or environments. (y-scope#583) * docs: Update the required version of task. (y-scope#567) * Add pr check workflow --------- Co-authored-by: kirkrodrigues <[email protected]> Co-authored-by: Junhao Liao <[email protected]> Co-authored-by: Henry8192 <[email protected]> Co-authored-by: Devin Gibson <[email protected]> Co-authored-by: wraymo <[email protected]> Co-authored-by: wraymo <[email protected]> Co-authored-by: Xiaochong(Eddy) Wei <[email protected]> Co-authored-by: Xiaochong Wei <[email protected]> Co-authored-by: haiqi96 <[email protected]>
…fix macOS build failure. (y-scope#582) Co-authored-by: kirkrodrigues <[email protected]>
Description
Recent macOS build workflow failed due to the version bump of
mariadb-connector-c
: https://github.com/y-scope/clp/actions/runs/11760063129The detailed log message:
This PR fixes the issue by changing the cmake file to detect
mariadb-connector-c
's homebrew path on macOS ifPkgConfig
failed to locate mariadb client.Validation performed
mariadb-connector-c
Summary by CodeRabbit
New Features
Bug Fixes