[fix](workflow) Fix BE UT macOS job failing with wrong JDK version#65796
Open
morningman wants to merge 16 commits into
Open
[fix](workflow) Fix BE UT macOS job failing with wrong JDK version#65796morningman wants to merge 16 commits into
morningman wants to merge 16 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
The `macos-15` runner (migrated to in apache#58914) is Apple Silicon (arm64), but the workflow still read `JAVA_HOME_17_X64`, which is only set on Intel runners. On arm64 that variable is unset, so `JAVA_HOME` became empty, the build fell back to the system default Java (25), and `check_jdk_version` failed with "ERROR: The JAVA version is 25, it must be JDK-17.". Use `JAVA_HOME_17_arm64` with a fallback to `JAVA_HOME_17_X64` so the job works on Apple Silicon runners while staying compatible with Intel ones. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
After the JDK fix the macOS BE UT job progressed to CMake configure and
failed in contrib/openblas:
CMake Error: Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES)
fatal error: 'omp.h' file not found
AppleClang does not ship an OpenMP runtime, so find_package(OpenMP) cannot
locate omp.h / libomp on its own. Install Homebrew's libomp and export the
include/lib paths via CPPFLAGS/CFLAGS/CXXFLAGS/LDFLAGS so CMake's OpenMP
detection succeeds. be/CMakeLists.txt resets CMAKE_C_FLAGS/CMAKE_CXX_FLAGS,
so EXTRA_CXX_FLAGS is additionally exported to pass the include path to the
BE build itself.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
morningman
force-pushed
the
fix-be-ut-mac-jdk17-arm64
branch
from
July 20, 2026 02:41
74597f9 to
7e12d9a
Compare
With libomp available, find_package(OpenMP) succeeds, but the build then
failed to compile:
clang++: error: unknown argument: '-Xclang -fopenmp'
be/CMakeLists.txt injects OpenMP_CXX_FLAGS globally only when PCH is enabled,
and applies AppleClang's multi-token "-Xclang -fopenmp" as a single argument
that clang rejects. Rather than change the CMake logic, disable PCH for the
macOS UT build. Targets that actually use OpenMP (faiss, openblas and
ann_index) link OpenMP::OpenMP_CXX through their own usage requirements, which
carry the correctly-split flags, include path and libomp, so PCH is not needed
for correctness here.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
morningman
force-pushed
the
fix-be-ut-mac-jdk17-arm64
branch
from
July 20, 2026 03:13
7c9f7f2 to
dffe6de
Compare
env.sh generates custom_env_mac.sh which prepends the Homebrew llvm@20 bin dir to PATH and derives DORIS_CLANG_HOME/CC/CXX from `command -v clang`. The workflow, however, installed llvm@16, so /opt/homebrew/opt/llvm@20/bin did not exist and the build silently fell back to AppleClang (/usr/bin/clang). Using AppleClang is the underlying cause of the macOS-specific failures seen in this job: the multi-token "-Xclang -fopenmp" OpenMP flag and the is_trivially_copyable/bit_cast compile errors in core BE headers. Install llvm@20 so the build uses the intended toolchain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With the intended llvm@20 toolchain, OpenMP_CXX_FLAGS is the single token "-fopenmp", so be/CMakeLists.txt's PCH+OpenMP block applies it correctly and the earlier "unknown argument" failure no longer occurs. The previous ENABLE_PCH=OFF workaround (added for AppleClang) is therefore unnecessary and made the clean build extremely slow on the 3-core runner. Drop it so PCH speeds the build back up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: the JDK and libomp setup now reaches the intended Java 17 and Clang/OpenMP configuration, but the job still cannot complete natively on its arm64 runner.
Critical checkpoint conclusions
- Goal and tests: The arm64 JDK selection is correct, and prior action evidence reaches Java 17.0.19, Clang 20.1.8, and OpenMP 5.1. The workflow is not complete because it feeds x86_64 third-party library objects into a native arm64 build. No final-head run has completed final link and BE UT execution; require a clean successful rerun after the archive fix.
- Scope and focus: The
config.cppedit is semantic no-op source churn used only to satisfybe_changes. The workflow should select its own path instead. - Architecture, compatibility, and parallel paths: The
JAVA_HOME_17_arm64/JAVA_HOME_17_X64fallback is sound, but the third-party archive selection is not architecture-aware. Preserve the existing master/release-branch tag logic while selecting the matching Darwin filename. - Concurrency, lifecycle, configuration, persistence, data, and protocol: Not involved by this patch.
- Error handling, observability, and performance: Shell/CMake failures remain visible, and LLVM 20, libomp discovery, PCH propagation, and runtime loading are coherent. No additional performance or observability defect was substantiated.
- Test coverage: Make workflow-only edits trigger this job, then complete a native configure, compile, link, and BE UT run on the final fixed head.
- User focus: No additional user-provided focus was supplied; the complete two-file patch and its upstream/downstream CI paths were reviewed.
The review converged after three rounds. Every candidate was accepted, deduplicated, or dismissed with evidence, and all final-round reviewers returned NO_NEW_VALUABLE_FINDINGS for the exact comment set below.
| popd | ||
|
|
||
| export JAVA_HOME="${JAVA_HOME_17_X64%\/}" | ||
| # macos-15 runners are Apple Silicon (arm64), so the JDK env var is |
Contributor
There was a problem hiding this comment.
macos-15 is arm64 here, but this job still downloads doris-thirdparty-prebuilt-darwin-x86_64.tar.xz at lines 91-95. The selected bundle contains x86_64 library objects while run-be-ut.sh produces native arm64 objects; Rosetta can run x86 generators but cannot link those objects into arm64 tests. Please derive the Darwin archive filename from normalized uname -m for both URL branches while preserving the current release-tag selection, then rerun through final link and tests.
| // The read size is the size of the reads sent to os. | ||
| // There is a trade off of latency and throughout, trying to keep disks busy but | ||
| // not introduce seeks. The literature seems to agree that with 8 MB reads, random | ||
| // not introduce seeks. The literature seems to agree that with 8 MB reads, random |
Contributor
There was a problem hiding this comment.
Please drop this unrelated source-comment whitespace change and make be_changes include .github/workflows/be-ut-mac.yml instead. Right now this no-op BE edit is the only reason the final-head job runs; once it is removed, future workflow-only fixes will again skip the job they change. Selecting the workflow's own path keeps the patch focused and makes these CI changes self-testing.
Contributor
TPC-H: Total hot run time: 29192 ms |
Contributor
TPC-DS: Total hot run time: 177529 ms |
Contributor
ClickBench: Total hot run time: 25.53 s |
Building the BE unit tests with the macOS (arm64) toolchain failed to compile:
error: conversion from 'long' to 'Decimal64' is ambiguous
Decimal64/Decimal128V2/Decimal128V3/Decimal256 are Decimal<T> with several
non-explicit integer constructors (Int32/Int64/Int128/UInt64/...). A bare
integer literal larger than INT_MAX has type 'long'. On Linux LP64 int64_t is
'long', so the literal matches Decimal(Int64) exactly; on macOS int64_t is
'long long', so 'long' converts equally well to several constructor parameter
types and overload resolution is ambiguous.
Wrap these literals in int64_t(...) so they match the Decimal(Int64)
constructor exactly on both platforms. This mirrors the existing convention in
other tests (e.g. jsonb_document_test.cpp, native_reader_writer_test.cpp).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The macOS job now verifies that the BE compiles on macOS instead of building and running the unit tests. The unit tests are compiled and run locally. Switch from run-be-ut.sh (which forces -DMAKE_TEST=ON and builds/runs the doris_be_test binary, i.e. all of be/test) to build.sh --be, which configures the BE with -DMAKE_TEST=OFF so nothing under be/test is compiled. The Java extensions and cdc client are disabled since they are not relevant to a macOS C++ build check, and -j is passed explicitly because build.sh otherwise defaults to roughly nproc/4 jobs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two more occurrences of the ambiguous 'long'->Decimal64 conversion in column_type_convert_test.cpp (functional-style casts with bare literals larger than INT_MAX). Wrap them in int64_t(...) so they match the Decimal(Int64) constructor exactly, as done for the other tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
RuntimeProfile::Counter has both set(int64_t) and set(double). Calling it with an int or long literal (e.g. set(0L), set(5), set(1000L*1000L*1000L)) is ambiguous on macOS: there int64_t is 'long long', so a 'long'/'int' argument converts equally well to int64_t and double. On Linux int64_t is 'long', so a 'long' literal matches set(int64_t) exactly and there is no ambiguity. Wrap the arguments in int64_t(...) so they match set(int64_t) exactly on both platforms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ol on libc++)
doris_be_test is compiled with -fno-access-control (be/test/CMakeLists.txt),
which is incompatible with libc++'s <ranges> header on macOS: libc++ forward
declares lazy_split_view's __outer_iterator/__inner_iterator as private and
then defines them public, and with access control disabled clang rejects this
as "redeclared with 'public' access". Any test TU that pulls in <ranges> then
fails to compile.
Remove the dependency on <ranges> from the tests:
- drop unused '#include <ranges>' (lru_queue_test, block_file_cache_test_common,
cloud_tablet_test);
- replace the range-view usages that actually need <ranges> with plain loops:
std::ranges::copy_if(std::views::values(...)) in the cloud tablet query tests,
and std::ranges::for_each(std::ranges::iota_view{...}) in block_file_cache_test.
Ranges algorithms that come from <algorithm> (std::ranges::fill/find/sort/...)
are unaffected and left as-is.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ann_index_edge_case_test: RuntimeProfile::Counter::set(1000L) is ambiguous on macOS (int64_t is 'long long' there, so a 'long' matches both set(int64_t) and set(double)); wrap in int64_t(...). - compaction_permit_limiter_test: include <thread> for std::thread, which is not transitively available via libc++ here. - python_env_test: sighandler_t is a glibc-only typedef; use a plain void(*)(int) function pointer so it also builds on macOS (sig_t). - inverted_index_reader_test: for std::vector<bool>, `&value` yields a __bit_iterator proxy, not a real pointer; copy into a local before reinterpret_cast<const void*>. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
morningman
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01,
luwei16 and
yiguolei
as code owners
July 20, 2026 14:59
… failure
The macos-15 runners are Apple Silicon (arm64), but the BE UT job downloaded
the x86_64 prebuilt thirdparty archive. BE sources compiled fine for arm64,
but at the final link the linker ignored every x86_64 thirdparty static
library ("found architecture 'x86_64', required architecture 'arm64'") and
doris_be failed with thousands of undefined symbols (zlib, openssl, curl,
libevent, brpc/bthread, lz4, zstd, ICU, krb5, roaring, minizip, ...).
Download doris-thirdparty-prebuilt-darwin-arm64.tar.xz instead. The arm64
darwin asset is published for the automation (master) and automation-*
(branch) releases, so both download paths are switched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29437 ms |
Contributor
TPC-DS: Total hot run time: 179086 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
The `pull_request` trigger without an explicit `types` list defaults to [opened, synchronize, reopened], so closing and reopening a PR fires the `reopened` event and reruns the whole BE build even when no code changed. Drop `reopened` so only first-open and new commits trigger a run. GitHub check results are keyed by commit SHA, which is unchanged on reopen, so the previous run's status still satisfies required status checks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
skip buildall |
Contributor
There was a problem hiding this comment.
Automated review completed on f6c7fdd9ca96bf3c560fba9828d2a405e8ce2c75 — changes requested.
I found three blocking issues and left inline comments:
- The macOS job now uses
build.sh --be, which configuresMAKE_TEST=OFF, so the check builds production BE but neither builds nor executesdoris_be_test. - Restoring the test target still reaches libc++
<ranges>transitively throughcommon/object_pool.hwhiledoris_be_testapplies-fno-access-controltarget-wide, preserving the compilation failure this PR is intended to fix. - Restricting
pull_requesttoopenedandsynchronizeremoves revalidation when an unchanged head is reopened after the base—and therefore the synthetic merge result—has changed.
Existing review context was deduplicated: I did not repeat the live comments about architecture-dependent third-party archive selection or the workflow path filter/unrelated config.cpp whitespace change. The user supplied no additional review focus.
Checkpoint conclusions:
- Goal and proof: the individual test portability edits are mostly narrow and behavior-preserving, but the PR does not yet accomplish or prove a macOS BE unit-test repair. The required final evidence is a Darwin
doris_be_testcompile, link, and execution after the workflow and transitive-header issues are fixed. - Scope and parallel paths: PR versus schedule, reopen behavior, head checks versus the synthetic merge, master/release assets, arm64/Intel selection,
run-be-ut.shversusbuild.sh, theMAKE_TESTgate, monolithic/ANN targets, and every changed test hunk were checked. - Concurrency and lifecycle: workflow cancellation remains coherent; changed test lock scopes, proxy temporaries, signal-handler save/restore, and thread lifetimes remain sound. No production concurrency or lifecycle path changes.
- Compatibility and correctness: no Doris runtime configuration, persistence, storage/protocol, transaction, data-write, FE/BE variable, nullable-column, or observability boundary changes. The loop rewrites preserve cardinality/work, and the explicit-width conversions preserve values and select the intended overloads.
- Coverage and results: Linux checks cannot prove Darwin/libc++/arm64 behavior, and the current macOS job compiles no changed test source. No result file changes.
Validation was static, per the review contract: authoritative patch/blobs, build scripts/CMake, GitHub Actions logs, and PR/check APIs; no local build was run. Three review rounds converged with every candidate accepted, dismissed with evidence, or deduplicated.
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize] |
Contributor
There was a problem hiding this comment.
[P1] Revalidate the current merge result when reopening
Required checks are attached to the head SHA, but a pull_request workflow checks out refs/pull/<n>/merge, as GitHub documents. If head H passed against base B1, the PR is closed, the base advances to B2, and the same head is reopened, the merge ref is now B2+H while the old green check still belongs to H. Dropping the default reopened event skips validation of that current merge result. Please retain reopened, or only skip after matching a successful check to the exact current merge SHA.
| # macOS C++ build check), and pass -j explicitly because build.sh | ||
| # otherwise defaults to only ~nproc/4 jobs. | ||
| DISABLE_BE_JAVA_EXTENSIONS=ON DISABLE_BE_CDC_CLIENT=ON \ | ||
| ./build.sh --be -j "$(nproc)" |
Contributor
There was a problem hiding this comment.
[P1] Keep compiling and running the macOS unit tests
This replaces run-be-ut.sh --run with build.sh --be, but the latter hard-codes MAKE_TEST=OFF in build.sh. As a result, this check no longer builds doris_be_test or executes any BE unit test; the successful macOS run confirms make test: OFF. That also means none of this PR's Darwin-specific fixes under be/test/** are validated here. Please keep the arm64/JDK/OpenMP setup while restoring run-be-ut.sh --run, or otherwise configure MAKE_TEST=ON and execute the test binary.
| RuntimeProfile::Counter* ticks_counter = profile.add_counter("ticks", TUnit::TIME_NS); | ||
| // set to 1 sec | ||
| ticks_counter->set(1000L * 1000L * 1000L); | ||
| ticks_counter->set(int64_t(1000L * 1000L * 1000L)); |
Contributor
There was a problem hiding this comment.
[P1] Finish removing the libc++ <ranges> conflict
This translation unit still includes common/object_pool.h, which itself includes <ranges> and uses std::ranges::reverse_view. Because doris_be_test applies -fno-access-control target-wide, this still reaches the same libc++ access-redeclaration failure that motivated the direct <ranges> removals in this PR. The production-only workflow does not compile this path. Please remove the transitive conflict (for example, use reverse iterators in ObjectPool or scope the test flag more narrowly), then compile, link, and run doris_be_test on Darwin.
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Gabriel39
approved these changes
Jul 21, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
yiguolei
approved these changes
Jul 21, 2026
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 25th, 2026 6:03 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #58914
Problem Summary:
The
BE UT (macOS)job fails at the JDK version check, e.g. inhttps://github.com/apache/doris/actions/runs/29692786375/job/88208347509:
Root cause: #58914 migrated the runner from the deprecated
macos-13(Intel / x64) to
macos-15, which is Apple Silicon (arm64). GitHub's runnerimages expose the JDK path as
JAVA_HOME_<version>_<arch>, so on arm64 it isJAVA_HOME_17_arm64, and the oldJAVA_HOME_17_X64is unset. The workflowstill read
JAVA_HOME_17_X64, soJAVA_HOMEbecame empty; the build then fellback to the system default Java (25) and
check_jdk_versionfailed.Fix: read
JAVA_HOME_17_arm64with a fallback toJAVA_HOME_17_X64, keepingthe job working on Apple Silicon runners while staying compatible with Intel
ones. The trailing-slash stripping of the original is preserved.
Note: this workflow's paths-filter only triggers on
be/**/gensrc/**changes, so this PR (which only edits the workflow) will not run the macOS
BE UT job itself. The fix will be exercised by the next scheduled run or the
next PR touching BE code. The env-var resolution was verified locally for both
arm64 and x64 cases.
Release note
None
Check List (For Author)
Test
JAVA_HOME_17_arm64/JAVA_HOME_17_X64resolution on both architectures.Behavior changed:
Does this need documentation?