[codex] build: add data substrate third-party workspace#487
Open
liunyl wants to merge 3 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a third-party workspace, manifest, and Ubuntu 24.04 install flow. Introduces a prefix-aware CMake helper module, updates build scripts to fetch and build dependencies, and switches several CMake targets to external package discovery with prefix validation. ChangesThird-party workspace and metadata
Build and CMake integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
third_party/manifest.yml (1)
10-10: 💤 Low valueConsider quoting the date value for consistency.
The
generated_ondate value is unquoted. While many YAML parsers handle this correctly, explicit quoting ensures consistent interpretation across all tools:- generated_on: 2026-06-04 + generated_on: "2026-06-04"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/manifest.yml` at line 10, The unquoted YAML value for the key generated_on can be misinterpreted by some parsers; update the manifest so the generated_on value is explicitly quoted (e.g., use "2026-06-04" or '2026-06-04') to ensure consistent parsing; locate the generated_on entry in the manifest and replace the unquoted date with a quoted string.cmake/EloqThirdParty.cmake (2)
45-47: 💤 Low valueConsider whether forcing
CMAKE_FIND_PACKAGE_PREFER_CONFIGis necessary.The
FORCEflag on line 47 will override any user-set value ofCMAKE_FIND_PACKAGE_PREFER_CONFIG. While this ensures consistent behavior for third-party package discovery, it prevents users from customizing this setting.If the build system depends on config-mode discovery, this is appropriate. Otherwise, consider using a default value without
FORCE:set(CMAKE_FIND_PACKAGE_PREFER_CONFIG TRUE CACHE BOOL "Prefer package config files from the third-party prefix" - FORCE) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/EloqThirdParty.cmake` around lines 45 - 47, The current CMake snippet forcefully sets the cache variable CMAKE_FIND_PACKAGE_PREFER_CONFIG with FORCE which overrides user choices; either remove the FORCE flag so the cache default can be overridden, or change the logic to only set the cache when it's not already defined (e.g. guard with an if(NOT DEFINED CMAKE_FIND_PACKAGE_PREFER_CONFIG) and then set(... CACHE BOOL "Prefer package config files from the third-party prefix") ), keeping the same variable name CMAKE_FIND_PACKAGE_PREFER_CONFIG so downstream code still reads it.
64-67: 💤 Low valueFunction only validates the first element of list variables.
Line 64 uses
list(GET ${path_var} 0 ...)to extract the first element. If${path_var}contains multiple paths, only the first is validated. Based on the usage patterns from callers (likeeloq_assert_under_prefix(ROCKSDB_LIB "RocksDB library")), this appears intentional since most checks apply to single-value variables.If validation of all elements in a list is needed in the future, consider:
foreach(_ELOQ_ASSERT_PATH IN LISTS ${path_var}) # validation logic endforeach()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/EloqThirdParty.cmake` around lines 64 - 67, The current code only validates the first element of the list variable using list(GET ${path_var} 0 _ELOQ_ASSERT_PATH); replace that with a foreach loop that iterates all members (foreach(_ELOQ_ASSERT_PATH IN LISTS ${path_var})) and perform the same IS_ABSOLUTE check inside the loop, returning if any element is not absolute; update the block around the existing IS_ABSOLUTE check so every path in the variable is validated rather than only the first.scripts/third_party/build-ubuntu2404.sh (2)
68-70: ⚡ Quick winVerify brpc build output directory exists before rsync.
Lines 68-70 use
rsyncto copy brpc build outputs. If the cmake build fails to create the output directory structure,rsyncmight succeed but copy nothing, leading to an incomplete installation. Consider checking that the source directories exist:🛡️ Suggested defensive check
run cmake --build "${THIRD_PARTY_BUILD}/brpc" -- -j"${JOBS}" mkdir -p "${THIRD_PARTY_PREFIX}/include" "${THIRD_PARTY_PREFIX}/lib" +[ -d "${THIRD_PARTY_BUILD}/brpc/output/include" ] || { echo "brpc output not found" >&2; exit 1; } run rsync -a "${THIRD_PARTY_BUILD}/brpc/output/include/" "${THIRD_PARTY_PREFIX}/include/" run rsync -a "${THIRD_PARTY_BUILD}/brpc/output/lib/" "${THIRD_PARTY_PREFIX}/lib/"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/third_party/build-ubuntu2404.sh` around lines 68 - 70, Before running rsync, verify that the brpc build output directories exist (check "${THIRD_PARTY_BUILD}/brpc/output/include" and "${THIRD_PARTY_BUILD}/brpc/output/lib"); if either is missing or empty, print a clear error via stderr and exit non‑zero to fail the script instead of silently copying nothing. Update the block that uses THIRD_PARTY_BUILD and THIRD_PARTY_PREFIX to perform these existence checks and conditional rsyncing, referencing the variables "${THIRD_PARTY_BUILD}/brpc/output/include", "${THIRD_PARTY_BUILD}/brpc/output/lib", and the rsync commands so the script fails fast on build problems.
74-74: 💤 Low valueThe sed command removing
libbrpc.ais fragile.Line 74 uses a global substitution (
s/libbrpc.a//g) to remove all occurrences oflibbrpc.afrom braft'sCMakeLists.txt. This is necessary because the build uses shared libraries, but the pattern might inadvertently affect comments or strings containing that text.If braft's CMakeLists.txt structure is stable, consider a more targeted patch that modifies specific lines or uses a more precise regex.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/third_party/build-ubuntu2404.sh` at line 74, The sed command in build-ubuntu2404.sh currently does a global blind replace of "libbrpc.a" in CMakeLists.txt; instead target only link/library lines to avoid touching comments/strings — update the sed invocation (the command at line using sed -i 's/libbrpc.a//g') to a more precise regex that only edits lines like target_link_libraries(...) or set(...LIBS...) (for example use sed -E -i '/target_link_libraries|set\([A-Za-z0-9_]*LIBS/ s/\blibbrpc\.a\b//g' CMakeLists.txt) so only actual linkage entries are modified while leaving comments and other text intact.scripts/third_party/fetch.sh (1)
8-31: Submodule list infetch.shmatches.gitmodules(current state)The 23
third_party/src/*entries hardcoded inscripts/third_party/fetch.shalign with the paths listed in.gitmodulestoday. A small future-proofing check (or deriving the list from.gitmodules) would still help prevent drift if.gitmoduleschanges later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/third_party/fetch.sh` around lines 8 - 31, The hardcoded TOP_LEVEL_THIRD_PARTY_SUBMODULES array in scripts/third_party/fetch.sh can drift from .gitmodules; update fetch.sh so it reads and derives the submodule paths from .gitmodules (or adds a validation step) instead of using the static TOP_LEVEL_THIRD_PARTY_SUBMODULES list: implement logic that parses .gitmodules for the "path =" entries (or compares the parsed list) and either constructs TOP_LEVEL_THIRD_PARTY_SUBMODULES dynamically or fails with a clear message when the hardcoded array and .gitmodules differ; target the symbol TOP_LEVEL_THIRD_PARTY_SUBMODULES and the fetch.sh entrypoint that consumes it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/README.md`:
- Around line 16-17: Update the README commands so the paths match this file's
location under top-level third_party: reference the local install script
(scripts/third_party/install-ubuntu2404.sh) rather than
data_substrate/scripts/..., and update the cmake invocation to set
DELOQ_THIRD_PARTY_PREFIX to the third_party install directory under the
repository root (replace the data_substrate/... prefix). Edit the lines that
mention install-ubuntu2404.sh and the cmake -S . -B build
-DELOQ_THIRD_PARTY_PREFIX=... to use the correct relative paths from
third_party/.
In `@third_party/src/aws-sdk-cpp`:
- Line 1: The AWS SDK C++ submodule is pinned to a vulnerable commit
(b5153b8309526738d498ee86a73ccc386fee0979); update the submodule reference in
third_party/src/aws-sdk-cpp to the fixed commit
4aca5630ba6e49869a0f173f9439ae73722d4558 (or any tag/release >= v1.11.712), run
git submodule sync/update and commit the new submodule SHA, and ensure any
build/CI scripts or dependency manifests that pin the AWS SDK for C++ are
updated accordingly and tests rebuilt to verify the change.
In `@third_party/system-packages.ubuntu2404.txt`:
- Line 44: The package entry "redis" is currently listed in
third_party/system-packages.ubuntu2404.txt but appears not required during
third-party build/CI; either remove it from that file and add it to runtime/test
dependency lists (e.g., CI/test setup docs or a runtime prerequisites file) or
add a short justification comment in third_party/system-packages.ubuntu2404.txt
explaining why a Redis server package is needed at build time; update any CI
scripts or packaging docs that reference runtime services so Redis is installed
only where actually required.
---
Nitpick comments:
In `@cmake/EloqThirdParty.cmake`:
- Around line 45-47: The current CMake snippet forcefully sets the cache
variable CMAKE_FIND_PACKAGE_PREFER_CONFIG with FORCE which overrides user
choices; either remove the FORCE flag so the cache default can be overridden, or
change the logic to only set the cache when it's not already defined (e.g. guard
with an if(NOT DEFINED CMAKE_FIND_PACKAGE_PREFER_CONFIG) and then set(... CACHE
BOOL "Prefer package config files from the third-party prefix") ), keeping the
same variable name CMAKE_FIND_PACKAGE_PREFER_CONFIG so downstream code still
reads it.
- Around line 64-67: The current code only validates the first element of the
list variable using list(GET ${path_var} 0 _ELOQ_ASSERT_PATH); replace that with
a foreach loop that iterates all members (foreach(_ELOQ_ASSERT_PATH IN LISTS
${path_var})) and perform the same IS_ABSOLUTE check inside the loop, returning
if any element is not absolute; update the block around the existing IS_ABSOLUTE
check so every path in the variable is validated rather than only the first.
In `@scripts/third_party/build-ubuntu2404.sh`:
- Around line 68-70: Before running rsync, verify that the brpc build output
directories exist (check "${THIRD_PARTY_BUILD}/brpc/output/include" and
"${THIRD_PARTY_BUILD}/brpc/output/lib"); if either is missing or empty, print a
clear error via stderr and exit non‑zero to fail the script instead of silently
copying nothing. Update the block that uses THIRD_PARTY_BUILD and
THIRD_PARTY_PREFIX to perform these existence checks and conditional rsyncing,
referencing the variables "${THIRD_PARTY_BUILD}/brpc/output/include",
"${THIRD_PARTY_BUILD}/brpc/output/lib", and the rsync commands so the script
fails fast on build problems.
- Line 74: The sed command in build-ubuntu2404.sh currently does a global blind
replace of "libbrpc.a" in CMakeLists.txt; instead target only link/library lines
to avoid touching comments/strings — update the sed invocation (the command at
line using sed -i 's/libbrpc.a//g') to a more precise regex that only edits
lines like target_link_libraries(...) or set(...LIBS...) (for example use sed -E
-i '/target_link_libraries|set\([A-Za-z0-9_]*LIBS/ s/\blibbrpc\.a\b//g'
CMakeLists.txt) so only actual linkage entries are modified while leaving
comments and other text intact.
In `@scripts/third_party/fetch.sh`:
- Around line 8-31: The hardcoded TOP_LEVEL_THIRD_PARTY_SUBMODULES array in
scripts/third_party/fetch.sh can drift from .gitmodules; update fetch.sh so it
reads and derives the submodule paths from .gitmodules (or adds a validation
step) instead of using the static TOP_LEVEL_THIRD_PARTY_SUBMODULES list:
implement logic that parses .gitmodules for the "path =" entries (or compares
the parsed list) and either constructs TOP_LEVEL_THIRD_PARTY_SUBMODULES
dynamically or fails with a clear message when the hardcoded array and
.gitmodules differ; target the symbol TOP_LEVEL_THIRD_PARTY_SUBMODULES and the
fetch.sh entrypoint that consumes it.
In `@third_party/manifest.yml`:
- Line 10: The unquoted YAML value for the key generated_on can be
misinterpreted by some parsers; update the manifest so the generated_on value is
explicitly quoted (e.g., use "2026-06-04" or '2026-06-04') to ensure consistent
parsing; locate the generated_on entry in the manifest and replace the unquoted
date with a quoted string.
🪄 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: 36aa1d5c-ced7-46d7-bf2a-2729f7005238
📒 Files selected for processing (39)
.gitmodulesCMakeLists.txtbuild_tx_service.cmakecmake/EloqThirdParty.cmakescripts/third_party/build-ubuntu2404.shscripts/third_party/common.shscripts/third_party/fetch.shscripts/third_party/install-ubuntu2404.shstore_handler/eloq_data_store_service/CMakeLists.txtstore_handler/eloq_data_store_service/eloqstorethird_party/.gitignorethird_party/README.mdthird_party/manifest.ymlthird_party/src/Catch2third_party/src/FakeItthird_party/src/abseil-cppthird_party/src/aws-sdk-cppthird_party/src/braftthird_party/src/brpcthird_party/src/crc32cthird_party/src/cuckoofilterthird_party/src/glogthird_party/src/google-cloud-cppthird_party/src/grpcthird_party/src/liburingthird_party/src/luathird_party/src/mimallocthird_party/src/nlohmann-jsonthird_party/src/prometheus-cppthird_party/src/protobufthird_party/src/re2third_party/src/rocksdbthird_party/src/rocksdb-cloudthird_party/src/usearchthird_party/src/yaml-cppthird_party/system-packages.ubuntu2404.txttx_service/CMakeLists.txttx_service/abseil-cpptx_service/include/cc/catalog_cc_map.h
💤 Files with no reviewable changes (1)
- tx_service/abseil-cpp
Comment on lines
+16
to
+17
| data_substrate/scripts/third_party/install-ubuntu2404.sh | ||
| cmake -S . -B build -DELOQ_THIRD_PARTY_PREFIX="$PWD/data_substrate/third_party/install" |
There was a problem hiding this comment.
Fix local-build command paths to match the repository layout.
Line 16 and Line 17 use data_substrate/... paths, but this README lives under top-level third_party/, so these commands look inconsistent with the current tree and are likely to fail when copied verbatim.
Suggested doc fix
-data_substrate/scripts/third_party/install-ubuntu2404.sh
-cmake -S . -B build -DELOQ_THIRD_PARTY_PREFIX="$PWD/data_substrate/third_party/install"
+scripts/third_party/install-ubuntu2404.sh
+cmake -S . -B build -DELOQ_THIRD_PARTY_PREFIX="$PWD/third_party/install"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| data_substrate/scripts/third_party/install-ubuntu2404.sh | |
| cmake -S . -B build -DELOQ_THIRD_PARTY_PREFIX="$PWD/data_substrate/third_party/install" | |
| scripts/third_party/install-ubuntu2404.sh | |
| cmake -S . -B build -DELOQ_THIRD_PARTY_PREFIX="$PWD/third_party/install" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@third_party/README.md` around lines 16 - 17, Update the README commands so
the paths match this file's location under top-level third_party: reference the
local install script (scripts/third_party/install-ubuntu2404.sh) rather than
data_substrate/scripts/..., and update the cmake invocation to set
DELOQ_THIRD_PARTY_PREFIX to the third_party install directory under the
repository root (replace the data_substrate/... prefix). Edit the lines that
mention install-ubuntu2404.sh and the cmake -S . -B build
-DELOQ_THIRD_PARTY_PREFIX=... to use the correct relative paths from
third_party/.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/third_party/common.sh`:
- Around line 126-131: The existing block that creates or initializes the git
repo must also ensure the origin remote matches the requested repo URL; in the
fetch_git_source logic, after the repo exists (i.e., when ${source_dir} already
contains a git repo) run a git remote set-url origin "${repo}" (or add origin if
missing) using the same git -C "${source_dir}" invocation so that origin is
always kept in sync with the requested repo; update the code paths referencing
source_dir and repo to perform git -C "${source_dir}" remote get-url origin &&
git -C "${source_dir}" remote set-url origin "${repo}" (or add if absent) before
any fetch/checkout operations.
In `@scripts/third_party/fetch.sh`:
- Around line 47-48: The emptiness check uses a piped find | head which can fail
under set -o pipefail; replace the pipeline with a single find that stops after
the first entry (use find ... -mindepth 1 -maxdepth 1 -print -quit) or use a
shell glob/ls check so the if condition that references DATA_SUBSTRATE_ROOT and
submodule_path does not rely on piping to head; update the conditional that
currently calls find "${DATA_SUBSTRATE_ROOT}/${submodule_path}" | head -n 1 to
instead use find with -print -quit (or an equivalent non-piped existence check)
so it won’t produce a SIGPIPE-induced non-zero exit.
🪄 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: 241f56b1-85c9-4cff-bf16-1c6649d6a14e
📒 Files selected for processing (9)
.gitmodulesscripts/third_party/build-ubuntu2404.shscripts/third_party/common.shscripts/third_party/fetch.shscripts/third_party/install-ubuntu2404.shthird_party/.gitignorethird_party/README.mdthird_party/manifest.ymlthird_party/patches/cuckoofilter/0001-eloq-build-fixes.patch
💤 Files with no reviewable changes (1)
- .gitmodules
✅ Files skipped from review due to trivial changes (3)
- third_party/README.md
- third_party/.gitignore
- third_party/manifest.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/third_party/build-ubuntu2404.sh
Comment on lines
+126
to
+131
| if [ ! -d "${source_dir}/.git" ]; then | ||
| rm -rf "${source_dir}" | ||
| mkdir -p "${source_dir}" | ||
| run git -C "${source_dir}" init | ||
| run git -C "${source_dir}" remote add origin "${repo}" | ||
| fi |
There was a problem hiding this comment.
Ensure origin is kept in sync with the requested repo URL.
When ${source_dir} already exists, fetch_git_source() never updates origin. Reused workspaces can silently fetch from the wrong remote while still checking out ${ref}, breaking the pinned-source contract.
Suggested fix
if [ ! -d "${source_dir}/.git" ]; then
rm -rf "${source_dir}"
mkdir -p "${source_dir}"
run git -C "${source_dir}" init
run git -C "${source_dir}" remote add origin "${repo}"
+else
+ if git -C "${source_dir}" remote get-url origin >/dev/null 2>&1; then
+ run git -C "${source_dir}" remote set-url origin "${repo}"
+ else
+ run git -C "${source_dir}" remote add origin "${repo}"
+ fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ ! -d "${source_dir}/.git" ]; then | |
| rm -rf "${source_dir}" | |
| mkdir -p "${source_dir}" | |
| run git -C "${source_dir}" init | |
| run git -C "${source_dir}" remote add origin "${repo}" | |
| fi | |
| if [ ! -d "${source_dir}/.git" ]; then | |
| rm -rf "${source_dir}" | |
| mkdir -p "${source_dir}" | |
| run git -C "${source_dir}" init | |
| run git -C "${source_dir}" remote add origin "${repo}" | |
| else | |
| if git -C "${source_dir}" remote get-url origin >/dev/null 2>&1; then | |
| run git -C "${source_dir}" remote set-url origin "${repo}" | |
| else | |
| run git -C "${source_dir}" remote add origin "${repo}" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/third_party/common.sh` around lines 126 - 131, The existing block
that creates or initializes the git repo must also ensure the origin remote
matches the requested repo URL; in the fetch_git_source logic, after the repo
exists (i.e., when ${source_dir} already contains a git repo) run a git remote
set-url origin "${repo}" (or add origin if missing) using the same git -C
"${source_dir}" invocation so that origin is always kept in sync with the
requested repo; update the code paths referencing source_dir and repo to perform
git -C "${source_dir}" remote get-url origin && git -C "${source_dir}" remote
set-url origin "${repo}" (or add if absent) before any fetch/checkout
operations.
Comment on lines
+47
to
+48
| if [ ! -d "${DATA_SUBSTRATE_ROOT}/${submodule_path}" ] || \ | ||
| [ -z "$(find "${DATA_SUBSTRATE_ROOT}/${submodule_path}" -mindepth 1 -maxdepth 1 | head -n 1)" ]; then |
There was a problem hiding this comment.
Avoid find | head in strict+pipefail mode for emptiness checks.
This fallback path can exit unexpectedly because find may get SIGPIPE when head exits, and set -o pipefail propagates that non-zero status.
Suggested fix
- if [ ! -d "${DATA_SUBSTRATE_ROOT}/${submodule_path}" ] || \
- [ -z "$(find "${DATA_SUBSTRATE_ROOT}/${submodule_path}" -mindepth 1 -maxdepth 1 | head -n 1)" ]; then
+ if [ ! -d "${DATA_SUBSTRATE_ROOT}/${submodule_path}" ] || \
+ [ -z "$(find "${DATA_SUBSTRATE_ROOT}/${submodule_path}" -mindepth 1 -maxdepth 1 -print -quit)" ]; then
echo "Missing retained third-party source: ${submodule_path}" >&2
echo "Initialize retained data_substrate submodules before copying this tree." >&2
exit 1
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/third_party/fetch.sh` around lines 47 - 48, The emptiness check uses
a piped find | head which can fail under set -o pipefail; replace the pipeline
with a single find that stops after the first entry (use find ... -mindepth 1
-maxdepth 1 -print -quit) or use a shell glob/ls check so the if condition that
references DATA_SUBSTRATE_ROOT and submodule_path does not rely on piping to
head; update the conditional that currently calls find
"${DATA_SUBSTRATE_ROOT}/${submodule_path}" | head -n 1 to instead use find with
-print -quit (or an equivalent non-piped existence check) so it won’t produce a
SIGPIPE-induced non-zero exit.
liunyl
force-pushed
the
chore/data-substrate-third-party-workspace
branch
from
5eb5759 to
2c965ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build_tx_service.cmake`:
- Around line 234-244: The braft discovery in build_tx_service.cmake is not
constrained to the shared third-party prefix, so it can resolve a system braft
while other dependencies come from the workspace. Update the braft lookup logic
around find_path(BRAFT_INCLUDE_PATH) and find_library(BRAFT_LIB) to apply the
same prefix guard used for the other third-party libraries, ensuring BRAFT_* is
only accepted from the shared third-party location before populating
HOST_MANAGER_INCLUDE_DIR.
- Around line 273-285: Replace the FetchContent-based yaml-cpp setup with
consumption from the third-party prefix, since build_tx_service.cmake should
align with the existing yaml-cpp resolution used by tx_service/CMakeLists.txt.
Remove the FetchContent_Declare/FetchContent_MakeAvailable block and instead
rely on the already-installed package target found via the centralized
dependency workspace, keeping HOST_MANAGER_LINK_LIB wired to the imported
yaml-cpp target name.
🪄 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: 097cb254-2a69-4cb6-9143-05e7db3a9fd0
📒 Files selected for processing (20)
.gitmodulesCMakeLists.txtbuild_tx_service.cmakecmake/EloqThirdParty.cmakescripts/third_party/build-ubuntu2404.shscripts/third_party/common.shscripts/third_party/fetch.shscripts/third_party/install-ubuntu2404.shstore_handler/eloq_data_store_service/CMakeLists.txtthird_party/.gitignorethird_party/README.mdthird_party/manifest.ymlthird_party/patches/cuckoofilter/0001-eloq-build-fixes.patchthird_party/src/braftthird_party/src/brpcthird_party/src/mimallocthird_party/src/rocksdb-cloudthird_party/system-packages.ubuntu2404.txttx_service/CMakeLists.txttx_service/abseil-cpp
💤 Files with no reviewable changes (16)
- third_party/.gitignore
- scripts/third_party/install-ubuntu2404.sh
- third_party/src/mimalloc
- third_party/README.md
- third_party/src/brpc
- tx_service/abseil-cpp
- third_party/src/braft
- store_handler/eloq_data_store_service/CMakeLists.txt
- third_party/src/rocksdb-cloud
- third_party/system-packages.ubuntu2404.txt
- third_party/patches/cuckoofilter/0001-eloq-build-fixes.patch
- third_party/manifest.yml
- scripts/third_party/fetch.sh
- scripts/third_party/common.sh
- tx_service/CMakeLists.txt
- scripts/third_party/build-ubuntu2404.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- CMakeLists.txt
- .gitmodules
- cmake/EloqThirdParty.cmake
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build_tx_service.cmake`:
- Around line 234-244: The braft discovery in build_tx_service.cmake is not
constrained to the shared third-party prefix, so it can resolve a system braft
while other dependencies come from the workspace. Update the braft lookup logic
around find_path(BRAFT_INCLUDE_PATH) and find_library(BRAFT_LIB) to apply the
same prefix guard used for the other third-party libraries, ensuring BRAFT_* is
only accepted from the shared third-party location before populating
HOST_MANAGER_INCLUDE_DIR.
- Around line 273-285: Replace the FetchContent-based yaml-cpp setup with
consumption from the third-party prefix, since build_tx_service.cmake should
align with the existing yaml-cpp resolution used by tx_service/CMakeLists.txt.
Remove the FetchContent_Declare/FetchContent_MakeAvailable block and instead
rely on the already-installed package target found via the centralized
dependency workspace, keeping HOST_MANAGER_LINK_LIB wired to the imported
yaml-cpp target name.
🪄 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: 097cb254-2a69-4cb6-9143-05e7db3a9fd0
📒 Files selected for processing (20)
.gitmodulesCMakeLists.txtbuild_tx_service.cmakecmake/EloqThirdParty.cmakescripts/third_party/build-ubuntu2404.shscripts/third_party/common.shscripts/third_party/fetch.shscripts/third_party/install-ubuntu2404.shstore_handler/eloq_data_store_service/CMakeLists.txtthird_party/.gitignorethird_party/README.mdthird_party/manifest.ymlthird_party/patches/cuckoofilter/0001-eloq-build-fixes.patchthird_party/src/braftthird_party/src/brpcthird_party/src/mimallocthird_party/src/rocksdb-cloudthird_party/system-packages.ubuntu2404.txttx_service/CMakeLists.txttx_service/abseil-cpp
💤 Files with no reviewable changes (16)
- third_party/.gitignore
- scripts/third_party/install-ubuntu2404.sh
- third_party/src/mimalloc
- third_party/README.md
- third_party/src/brpc
- tx_service/abseil-cpp
- third_party/src/braft
- store_handler/eloq_data_store_service/CMakeLists.txt
- third_party/src/rocksdb-cloud
- third_party/system-packages.ubuntu2404.txt
- third_party/patches/cuckoofilter/0001-eloq-build-fixes.patch
- third_party/manifest.yml
- scripts/third_party/fetch.sh
- scripts/third_party/common.sh
- tx_service/CMakeLists.txt
- scripts/third_party/build-ubuntu2404.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- CMakeLists.txt
- .gitmodules
- cmake/EloqThirdParty.cmake
🛑 Comments failed to post (2)
build_tx_service.cmake (2)
234-244: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## build_tx_service.cmake around reported lines\n' nl -ba build_tx_service.cmake | sed -n '210,270p' printf '\n## search for prefix assertions and braft-related checks\n' rg -n "eloq_assert_under_prefix|braft|BRPC_INCLUDE_PATH|BRAFT_INCLUDE_PATH|BRAFT_LIB" -S . --glob '!**/build/**' --glob '!**/dist/**' printf '\n## locate the prefix assertion macro definition/usages\n' rg -n "function\\s*\\(.*eloq_assert_under_prefix|macro\\s*\\(.*eloq_assert_under_prefix|eloq_assert_under_prefix\\(" -S . --glob '!**/build/**' --glob '!**/dist/**'Repository: eloqdata/tx_service
Length of output: 246
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## build_tx_service.cmake around reported lines\n' sed -n '210,270p' build_tx_service.cmake | cat -n printf '\n## search for prefix assertions and braft-related checks\n' rg -n "eloq_assert_under_prefix|braft|BRPC_INCLUDE_PATH|BRAFT_INCLUDE_PATH|BRAFT_LIB" -S . --glob '!**/build/**' --glob '!**/dist/**' printf '\n## locate the prefix assertion macro definition/usages\n' rg -n "function\\s*\\(.*eloq_assert_under_prefix|macro\\s*\\(.*eloq_assert_under_prefix|eloq_assert_under_prefix\\(" -S . --glob '!**/build/**' --glob '!**/dist/**'Repository: eloqdata/tx_service
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## prefix assertion helper\n' sed -n '1,120p' cmake/EloqThirdParty.cmake | cat -n printf '\n## tx_service CMake setup around braft/brpc\n' sed -n '100,140p' tx_service/CMakeLists.txt | cat -n printf '\n' sed -n '270,305p' tx_service/CMakeLists.txt | cat -n printf '\n## where build_tx_service.cmake is referenced\n' rg -n "build_tx_service\\.cmake" -S . --glob '!**/build/**' --glob '!**/dist/**'Repository: eloqdata/tx_service
Length of output: 9062
Assert
BRAFT_*under the shared third-party prefix here too.build_tx_service.cmakestill resolvesbraftwithout the prefix guard, so this target can mix a workspacebrpcwith a different systembraft.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build_tx_service.cmake` around lines 234 - 244, The braft discovery in build_tx_service.cmake is not constrained to the shared third-party prefix, so it can resolve a system braft while other dependencies come from the workspace. Update the braft lookup logic around find_path(BRAFT_INCLUDE_PATH) and find_library(BRAFT_LIB) to apply the same prefix guard used for the other third-party libraries, ensuring BRAFT_* is only accepted from the shared third-party location before populating HOST_MANAGER_INCLUDE_DIR.
273-285: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the cited section and nearby context wc -l build_tx_service.cmake sed -n '240,310p' build_tx_service.cmake # Find yaml-cpp usage and any dependency-resolution helpers rg -n --hidden --glob '!**/.git/**' 'yaml-cpp|FetchContent|shared prefix|third-party|eloq_assert_target_under_prefix|find_package\(yaml-cpp' . # Locate manifest/workspace files that might already provide yaml-cpp git ls-files | rg '(^|/)(CMakeLists\.txt|.*\.cmake|.*manifest.*|.*workspace.*|.*lock.*|.*bazel.*|.*conan.*|.*vcpkg.*|.*spack.*)$'Repository: eloqdata/tx_service
Length of output: 7610
🏁 Script executed:
#!/bin/bash set -euo pipefail # See whether build_tx_service.cmake is still wired into the build rg -n --hidden --glob '!**/.git/**' 'build_tx_service\.cmake|include\(.+build_tx_service|include\(.+tx_service' CMakeLists.txt . # Inspect the dependency resolution helper and tx_service yaml-cpp handling sed -n '1,140p' cmake/EloqThirdParty.cmake printf '\n--- tx_service/CMakeLists.txt excerpt ---\n' sed -n '240,275p' tx_service/CMakeLists.txtRepository: eloqdata/tx_service
Length of output: 6597
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether build_tx_service.cmake is included from a top-level or wrapper CMake file rg -n --hidden --glob '!**/.git/**' 'build_tx_service\.cmake|include\(.+build_tx_service|include\(.+tx_service|add_subdirectory\(.+tx_service' CMakeLists.txt */CMakeLists.txt */*.cmake # Show the top-level CMake context around any references sed -n '1,120p' CMakeLists.txtRepository: eloqdata/tx_service
Length of output: 6532
Consume
yaml-cppfrom the third-party prefix here.FetchContentstill pulls from GitHub during configure, whiletx_service/CMakeLists.txtalready resolvesyaml-cppthroughfind_package(... CONFIG REQUIRED)pluseloq_assert_target_under_prefix(...). Keeping this block bypasses the centralized dependency workspace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build_tx_service.cmake` around lines 273 - 285, Replace the FetchContent-based yaml-cpp setup with consumption from the third-party prefix, since build_tx_service.cmake should align with the existing yaml-cpp resolution used by tx_service/CMakeLists.txt. Remove the FetchContent_Declare/FetchContent_MakeAvailable block and instead rely on the already-installed package target found via the centralized dependency workspace, keeping HOST_MANAGER_LINK_LIB wired to the imported yaml-cpp target name.
liunyl
force-pushed
the
chore/data-substrate-third-party-workspace
branch
from
2c965ec to
07b4755
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/third_party/build-ubuntu2404.sh`:
- Around line 11-18: The build scripts currently patch the shared braft checkout
in place via restore_patched_sources and the EXIT trap, which can leave
third_party/src/braft/CMakeLists.txt dirty for other builds. Update the braft
setup in the affected build script(s) to operate on a build-local copy of the
sources instead of modifying THIRD_PARTY_SRC/braft directly, using the existing
braft/CMakeLists.txt flow as the entry point. Once the local-copy approach is in
place, remove the restore_patched_sources cleanup path, the trap, and any git
diff-based restoration logic.
🪄 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: f4aee206-bd68-486b-b5b0-19aa9fc5262e
📒 Files selected for processing (20)
.gitmodulesCMakeLists.txtbuild_tx_service.cmakecmake/EloqThirdParty.cmakescripts/third_party/build-ubuntu2404.shscripts/third_party/common.shscripts/third_party/fetch.shscripts/third_party/install-ubuntu2404.shstore_handler/eloq_data_store_service/CMakeLists.txtthird_party/.gitignorethird_party/README.mdthird_party/manifest.ymlthird_party/patches/cuckoofilter/0001-eloq-build-fixes.patchthird_party/src/braftthird_party/src/brpcthird_party/src/mimallocthird_party/src/rocksdb-cloudthird_party/system-packages.ubuntu2404.txttx_service/CMakeLists.txttx_service/abseil-cpp
💤 Files with no reviewable changes (1)
- tx_service/abseil-cpp
✅ Files skipped from review due to trivial changes (5)
- third_party/src/rocksdb-cloud
- third_party/.gitignore
- third_party/README.md
- third_party/manifest.yml
- .gitmodules
🚧 Files skipped from review as they are similar to previous changes (11)
- third_party/src/mimalloc
- third_party/src/braft
- third_party/src/brpc
- CMakeLists.txt
- third_party/system-packages.ubuntu2404.txt
- build_tx_service.cmake
- third_party/patches/cuckoofilter/0001-eloq-build-fixes.patch
- store_handler/eloq_data_store_service/CMakeLists.txt
- cmake/EloqThirdParty.cmake
- scripts/third_party/common.sh
- tx_service/CMakeLists.txt
Comment on lines
+11
to
+18
| restore_patched_sources() { | ||
| if [ -f "${BRAFT_CMAKE_BACKUP}" ]; then | ||
| mv "${BRAFT_CMAKE_BACKUP}" \ | ||
| "${THIRD_PARTY_SRC}/braft/CMakeLists.txt" | ||
| fi | ||
| } | ||
|
|
||
| trap restore_patched_sources EXIT |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Avoid patching the shared braft checkout in place.
This block rewrites third_party/src/braft/CMakeLists.txt inside the shared workspace. The EXIT trap helps on normal failures, but it does not protect against concurrent builds or abrupt termination, so later builds can start from a dirty submodule. Patch a build-local copy instead.
Proposed direction
- cd "${THIRD_PARTY_SRC}/braft"
- cp CMakeLists.txt "${BRAFT_CMAKE_BACKUP}"
- sed -i 's/libbrpc.a//g' CMakeLists.txt
- run cmake -S . -B "${THIRD_PARTY_BUILD}/braft" \
+ BRAFT_SRC_COPY="${THIRD_PARTY_BUILD}/braft-src"
+ rm -rf "${BRAFT_SRC_COPY}"
+ run rsync -a "${THIRD_PARTY_SRC}/braft/" "${BRAFT_SRC_COPY}/"
+ sed -i 's/libbrpc.a//g' "${BRAFT_SRC_COPY}/CMakeLists.txt"
+ run cmake -S "${BRAFT_SRC_COPY}" -B "${THIRD_PARTY_BUILD}/braft" \
-DCMAKE_INSTALL_PREFIX="${THIRD_PARTY_PREFIX}" \
-DCMAKE_PREFIX_PATH="${THIRD_PARTY_PREFIX}" \
-DBRPC_WITH_GLOG=ONThen the restore_patched_sources/trap/git diff cleanup can go away.
Also applies to: 69-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/third_party/build-ubuntu2404.sh` around lines 11 - 18, The build
scripts currently patch the shared braft checkout in place via
restore_patched_sources and the EXIT trap, which can leave
third_party/src/braft/CMakeLists.txt dirty for other builds. Update the braft
setup in the affected build script(s) to operate on a build-local copy of the
sources instead of modifying THIRD_PARTY_SRC/braft directly, using the existing
braft/CMakeLists.txt flow as the entry point. Once the local-copy approach is in
place, remove the restore_patched_sources cleanup path, the trap, and any git
diff-based restoration logic.
353c472 to
2b47b83
Compare
Squash of the 8 chore/data-substrate-third-party-workspace commits, rebased onto current main and reconciled with main's in-tree restructuring (#509): - Build third-party deps from pinned sources: third_party/manifest.yml, scripts/third_party/{fetch,build,install,common}.sh, cmake/EloqThirdParty.cmake. brpc/braft/mimalloc/rocksdb-cloud remain submodules; others fetched from pinned upstream sources. - tx-log-protos, log_service, and host_manager follow main's open-sourced in-tree layout (not re-introduced as submodules). - Drop the top-level abseil-cpp submodule (workspace provides third_party/src/abseil-cpp). - Build the workspace abseil with its default options.h (ABSL_OPTION_USE_*=2 => std:: string_view/any/optional/variant under C++17+), matching how main consumes the abseil submodule via add_subdirectory(tx_service/abseil-cpp). This avoids any source changes for flat_hash_map<std::string> lookups. Build verification pending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
liunyl
force-pushed
the
chore/data-substrate-third-party-workspace
branch
from
2b47b83 to
ce0ca6f
Compare
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
2 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.
Summary
Related PRs
Validation
Notes
Summary by CodeRabbit