Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to resolve macOS build/compile issues by adjusting the build environment setup and tightening a few C++ conversions that can fail or warn under Clang.
Changes:
- Update
env.shto clear potentially conflicting Homebrew include/lib environment variables and to auto-detect LLVM/libomp locations via Homebrew when using the Clang toolchain. - Add explicit casts around bitwise-negation / masking operations and unary negation to satisfy stricter Clang compilation rules.
- Remove redundant
virtualkeywords from private methods infinaltask classes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| env.sh | Clears env vars that can affect compilation; improves Clang/libomp discovery on macOS via Homebrew. |
| be/src/vec/runtime/ip_address_cidr.h | Adds explicit casts for bitwise negation/masks to avoid Clang conversion issues. |
| be/src/vec/functions/url/find_symbols.h | Casts ~x to uint16_t explicitly to avoid sign/width conversion problems. |
| be/src/vec/functions/math.cpp | Casts unary negation result to the function’s declared column item type. |
| be/src/vec/common/ipv6_to_binary.h | Masks and casts IPv6 prefix mask byte construction more explicitly. |
| be/src/olap/task/engine_clone_task.h | Removes unnecessary virtual on a private helper in a final class. |
| be/src/olap/task/engine_batch_load_task.h | Removes unnecessary virtual on private helpers in a final class. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
env.sh
Outdated
| # unset Homebrew compile env(include/lib path) to avoid conflict | ||
| unset CPATH | ||
| unset C_INCLUDE_PATH | ||
| unset CPLUS_INCLUDE_PATH | ||
| unset CPPFLAGS | ||
| unset LDFLAGS | ||
|
|
There was a problem hiding this comment.
The script unsets CPATH/C_INCLUDE_PATH/CPLUS_INCLUDE_PATH/CPPFLAGS/LDFLAGS unconditionally, even on Linux. This can unexpectedly break developer environments or CI where these variables are intentionally set for non-Homebrew toolchains. Consider scoping these unsets to Darwin/Homebrew-only (e.g., behind the existing uname -s == Darwin check) or preserving/restoring user-provided values via prefixed variables.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
env.sh
Outdated
| BREW_LLVM_PREFIX="$(brew --prefix llvm 2>/dev/null || true)" | ||
| if [[ -n "${BREW_LLVM_PREFIX}" && -x "${BREW_LLVM_PREFIX}/bin/clang++" ]]; then | ||
| DORIS_CLANG_HOME="${BREW_LLVM_PREFIX}" | ||
| else | ||
| DORIS_CLANG_HOME="$(dirname "$(command -v clang)")"/.. | ||
| fi |
There was a problem hiding this comment.
DORIS_CLANG_HOME is now preferred from brew --prefix llvm, but the mac toolchain setup earlier configures llvm@20 in custom_env_mac.sh. If both formulas exist, this can select a different LLVM than the one put on PATH (or fail to find the intended version), leading to mismatched compiler/toolchain behavior. It would be more consistent to query the same formula/version (llvm@20) or derive DORIS_CLANG_HOME purely from the resolved clang in PATH.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
env.sh
Outdated
| if [[ -n "${LIBOMP_PREFIX}" ]]; then | ||
| export OpenMP_ROOT="${LIBOMP_PREFIX}" | ||
| export CPPFLAGS="-I${LIBOMP_PREFIX}/include ${CPPFLAGS}" | ||
| export LDFLAGS="-L${LIBOMP_PREFIX}/lib ${LDFLAGS}" |
There was a problem hiding this comment.
This silently no-ops when libomp is not installed, but parts of the BE build use find_package(OpenMP REQUIRED) and will fail later with a less actionable CMake error on macOS (where OpenMP typically requires Homebrew libomp). Consider making missing libomp a clear early error on Darwin, and/or ensure it is included in the Homebrew tool list generated for macOS so the OpenMP runtime is consistently available.
| export LDFLAGS="-L${LIBOMP_PREFIX}/lib ${LDFLAGS}" | |
| export LDFLAGS="-L${LIBOMP_PREFIX}/lib ${LDFLAGS}" | |
| elif [[ "${TARGET_SYSTEM}" == "Darwin" ]]; then | |
| echo "Error: Homebrew 'libomp' is required for OpenMP when using the clang toolchain on macOS." | |
| echo "Please install it with: brew install libomp" | |
| exit 1 |
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)