Add toolchains to infinity#3369
Open
euvre wants to merge 10 commits into
Open
Conversation
Signed-off-by: noob <[email protected]>
|
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:
📝 WalkthroughWalkthroughThis PR updates CMake build configuration and toolchains (adds arch-specific toolchains and libstdc++ module manifests), extends jemalloc/vcpkg discovery, pins python3 in vcpkg, converts RocksDB handles to std::unique_ptr in admin handlers, and adds a Flex lexer header plus diagnostic guards around Python.h. ChangesCMake Build System & Toolchain Infrastructure
Source Code Infrastructure Updates
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels:
🚥 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. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/admin/admin_executor_impl.cpp (1)
647-654:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd status validation after
OpenForReadOnlyto prevent null-pointer dereference.All 14 updated handlers call
rocksdb::TransactionDB::OpenForReadOnly(...)without capturing or checking the returned status, then immediately dereferencedbviadb->NewIterator(...)within 13–17 lines. IfOpenForReadOnlyfails,dbremains uninitialized, causing a crash.Suggested fix pattern
- rocksdb::TransactionDB::OpenForReadOnly(options, catalog_dir, &db); + auto s = rocksdb::TransactionDB::OpenForReadOnly(options, catalog_dir, &db); + if (!s.ok() || db == nullptr) { + query_result.result_table_ = nullptr; + query_result.status_ = Status::InternalError(fmt::format("Failed to open catalog db: {}", s.ToString())); + return query_result; + }Also applies to: 801–816, 954–969, 1112–1127, 1262–1277, 1416–1432, 1563–1579, 1715–1732, 1861–1876, 2026–2042, 2203–2218, 2371–2387, 2538–2554, 2708–2725
🤖 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 `@src/admin/admin_executor_impl.cpp` around lines 647 - 654, The call to rocksdb::TransactionDB::OpenForReadOnly(options, catalog_dir, &db) is not checking the returned rocksdb::Status, so db may be null and subsequent db->NewIterator(...) dereferences a nullptr; update each handler (the blocks that call OpenForReadOnly and then db->NewIterator) to capture the returned rocksdb::Status, check status.ok(), and on failure log the error (include status.ToString()), clean up/return an error status from the handler instead of continuing, only calling db->NewIterator(...) when status.ok(); use the same pattern for OpenForReadOnly, db, and NewIterator in functions referenced by these blocks to avoid crashes.
🤖 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 `@cmake/aarch64-libstdc`++.modules.json:
- Around line 7-15: The JSON hardcodes toolchain include roots under the
"local-arguments" -> "system-include-directories" array (entries referencing
"c++/15" and "clang/20") which reduces cross-compilation portability; annotate
that block with an inline comment explaining the exact GCC/Clang versions
required for this module file and add guidance for developers on how to generate
or override these paths (e.g., use CMake variables or environment-provided
include roots and regenerate this JSON), or replace the hardcoded entries with
placeholders that your build scripts substitute at generation time, updating the
"source-path" and directory entries accordingly.
In `@cmake/aarch64.cmake`:
- Line 3: The find_program call for AARCH64_GCC should be removed or made
conditional because AARCH64_GCC (from find_program) is never used and the
project uses clang-style cross-compilation via --target rather than setting
CMAKE_C_COMPILER/CMAKE_CXX_COMPILER; replace or change the line using
find_program(AARCH64_GCC ...) to either delete it entirely or change it to an
optional probe (no REQUIRED) and only run it when
CMAKE_C_COMPILER/CMAKE_CXX_COMPILER are unset and you intend to use a gcc
toolchain; reference symbols: AARCH64_GCC, find_program, CMAKE_C_COMPILER,
CMAKE_CXX_COMPILER, and the --target cross-compilation flag.
In `@cmake/x86_64-v4.cmake`:
- Line 3: The toolchain wrongly relies on passing --target= to GCC found by
find_program(x86_64_GCC x86_64-linux-gnu-gcc REQUIRED); instead set
CMAKE_C_COMPILER and CMAKE_CXX_COMPILER to the appropriate cross-compiler
executables (e.g., the x86_64-linux-gnu gcc/ g++ binaries found) and remove any
--target= flags from compiler flags, using only target-appropriate options like
-march= when needed; apply the same change to the aarch64 toolchain file that
follows the same pattern.
In `@CMakeLists.txt`:
- Around line 261-266: Move the CI cross-compilation toolchain selection so
CMAKE_TOOLCHAIN_FILE is set before the project() call: detect
IS_CI_TEST_PIPELINE and the X86_64/ARM64 flags earlier and set
CMAKE_TOOLCHAIN_FILE to "${CMAKE_CURRENT_SOURCE_DIR}/cmake/x86_64-v2.cmake" or
"${CMAKE_CURRENT_SOURCE_DIR}/cmake/aarch64.cmake" accordingly (do not rely on
VCPKG_CHAINLOAD_TOOLCHAIN_FILE for this); keep the conditional logic but
relocate it above the project() invocation and remove/replace the current
VCPKG_CHAINLOAD_TOOLCHAIN_FILE assignments.
- Around line 161-164: The jemalloc Debug-mode check currently evaluates
CMAKE_BUILD_TYPE before CMAKE_BUILD_TYPE is initialized, so move the block that
checks if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug") ... set(ENABLE_JEMALLOC OFF)
to appear after the CMAKE_BUILD_TYPE initialization logic; ensure the check
references the same CMAKE_BUILD_TYPE variable and sets ENABLE_JEMALLOC OFF when
Debug is detected so the Debug behavior is applied whether CMAKE_BUILD_TYPE
comes from the cache or was previously set.
- Line 295: The find_package call for Python3 should be made explicit and
required: update the find_package invocation for the Python3 component
(currently "find_package(Python3 COMPONENTS Development)") to include the
REQUIRED keyword so CMake fails fast if Python3 (and its Development component)
is missing; ensure any downstream consumption of Python3_INCLUDE_DIRS in
src/CMakeLists.txt remains valid after this change (refer to the find_package
and Python3 symbols).
---
Outside diff comments:
In `@src/admin/admin_executor_impl.cpp`:
- Around line 647-654: The call to
rocksdb::TransactionDB::OpenForReadOnly(options, catalog_dir, &db) is not
checking the returned rocksdb::Status, so db may be null and subsequent
db->NewIterator(...) dereferences a nullptr; update each handler (the blocks
that call OpenForReadOnly and then db->NewIterator) to capture the returned
rocksdb::Status, check status.ok(), and on failure log the error (include
status.ToString()), clean up/return an error status from the handler instead of
continuing, only calling db->NewIterator(...) when status.ok(); use the same
pattern for OpenForReadOnly, db, and NewIterator in functions referenced by
these blocks to avoid crashes.
🪄 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: 0405e0d5-22a2-4820-9b6a-562535cad19a
📒 Files selected for processing (13)
CMakeLists.txtcmake/Findjemalloc.cmakecmake/aarch64-libstdc++.modules.jsoncmake/aarch64.cmakecmake/x86_64-v1.cmakecmake/x86_64-v2.cmakecmake/x86_64-v3.cmakecmake/x86_64-v4.cmakesrc/CMakeLists.txtsrc/admin/admin_executor_impl.cppsrc/common/third_party.cppmsrc/parser/FlexLexer.hvcpkg.json
Signed-off-by: noob <[email protected]>
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 `@CMakeLists.txt`:
- Line 285: Replace the incorrect use of add_definitions for a compiler flag:
find the add_definitions(-march=native) entry and change it to use
add_compile_options(-march=native) so the -march flag is passed as a compiler
option rather than a preprocessor definition.
- Around line 250-257: The sanitizer-related flags are being applied
unconditionally when jemalloc is disabled; update the CMake logic so the
message("Enable Sanitizer in target: infinity") plus the set(CMAKE_CXX_FLAGS
"... -fno-stack-protector -fno-var-tracking "),
add_compile_options(-fsanitize-recover=all),
add_link_options(-fsanitize-recover=all),
add_compile_options("-fno-omit-frame-pointer"), and
add_link_options("-fno-omit-frame-pointer") are only executed when a sanitizer
is actually enabled (e.g., inside the existing sanitizer check or behind a
boolean like ENABLE_ASAN/ENABLE_TSAN/ENABLE_UBSAN or a combined ANY_SANITIZER
variable); move these lines into that conditional or add a guard that tests for
any sanitizer flag before modifying CMAKE_CXX_FLAGS or calling
add_compile_options/add_link_options and ensure the misleading message is
emitted only when the sanitizer condition is true.
🪄 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: 3503ec08-47f3-4f28-ae7e-a45d1ae770bc
📒 Files selected for processing (4)
CMakeLists.txtcmake/x86_64-v1.cmakecmake/x86_64-v2.cmakecmake/x86_64-v3.cmake
🚧 Files skipped from review as they are similar to previous changes (3)
- cmake/x86_64-v1.cmake
- cmake/x86_64-v3.cmake
- cmake/x86_64-v2.cmake
Signed-off-by: noob <[email protected]>
Signed-off-by: noob <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmake/aarch64-libstdc++.modules.json (1)
7-13:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake
source-pathversion/path derivation configurable instead of hardcoding GCC 15 paths.Lines 7 and 12 hardcode
/usr/aarch64-linux-gnu/include/c++/15/...; this can fail on toolchains/sysroots that ship a different libstdc++ major. Please derive this from toolchain/CMake variables (or generate this JSON during configure) so module paths track the active compiler/sysroot.#!/bin/bash # Read-only verification: check whether module JSON GCC major is synchronized # with toolchain/compiler configuration in repo files. set -euo pipefail echo "== module map paths ==" rg -n '"source-path":\s*".*include/c\+\+/[0-9]+/bits/std(\.compat)?\.cc"' cmake echo echo "== toolchain/compiler version hints ==" rg -n 'aarch64|gcc|g\+\+|clang|CMAKE_(C|CXX)_COMPILER|CMAKE_SYSROOT|include/c\+\+' cmake echo echo "== other module map files for consistency ==" fd --glob '*.module*.json' cmake -x sh -c 'echo "--- {} ---"; cat "{}"'🤖 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/aarch64-libstdc`++.modules.json around lines 7 - 13, The JSON currently hardcodes libstdc++ major version in the "source-path" entries (e.g., lines referencing "/usr/aarch64-linux-gnu/include/c++/15/..."), which breaks when toolchains ship a different GCC major; update the build so the path is derived from CMake toolchain/compiler variables or generated at configure time: change generation of cmake/aarch64-libstdc++.modules.json to substitute the include/c++/<major> segment using a CMake variable computed from CMAKE_CXX_COMPILER_VERSION or CMAKE_SYSROOT (or generate the JSON in a CMake configure_file step) so logical-name "std.compat" and the other "source-path" entries point at the active compiler/sysroot rather than a hardcoded "15".
🤖 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.
Duplicate comments:
In `@cmake/aarch64-libstdc`++.modules.json:
- Around line 7-13: The JSON currently hardcodes libstdc++ major version in the
"source-path" entries (e.g., lines referencing
"/usr/aarch64-linux-gnu/include/c++/15/..."), which breaks when toolchains ship
a different GCC major; update the build so the path is derived from CMake
toolchain/compiler variables or generated at configure time: change generation
of cmake/aarch64-libstdc++.modules.json to substitute the include/c++/<major>
segment using a CMake variable computed from CMAKE_CXX_COMPILER_VERSION or
CMAKE_SYSROOT (or generate the JSON in a CMake configure_file step) so
logical-name "std.compat" and the other "source-path" entries point at the
active compiler/sysroot rather than a hardcoded "15".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24578f70-10a0-4ead-b9e9-493be1bd3369
📒 Files selected for processing (6)
cmake/aarch64-libstdc++.modules.jsoncmake/x86_64-libstdc++.module.jsoncmake/x86_64-v1.cmakecmake/x86_64-v2.cmakecmake/x86_64-v3.cmakecmake/x86_64-v4.cmake
✅ Files skipped from review due to trivial changes (1)
- cmake/x86_64-libstdc++.module.json
🚧 Files skipped from review as they are similar to previous changes (4)
- cmake/x86_64-v2.cmake
- cmake/x86_64-v3.cmake
- cmake/x86_64-v4.cmake
- cmake/x86_64-v1.cmake
Signed-off-by: noob <[email protected]>
Signed-off-by: noob <[email protected]>
Signed-off-by: noob <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
CMakeLists.txt (2)
276-283:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCI toolchain selection is configured too late to affect the current configure pass.
At Line 278-280,
VCPKG_CHAINLOAD_TOOLCHAIN_FILEis set afterproject()(Line 26), so it won’t reliably drive toolchain selection for this configure run. This can silently bypass intended x86_64/aarch64 CI cross-build behavior.Suggested fix
+option(IS_CI_TEST_PIPELINE "is ci test pipeline: " OFF) +if (IS_CI_TEST_PIPELINE) + if (CMAKE_SYSTEM_PROCESSOR MATCHES "^(x86_64|amd64)$") + set(CMAKE_TOOLCHAIN_FILE "${CMAKE_CURRENT_SOURCE_DIR}/cmake/x86_64-v2.cmake" CACHE FILEPATH "" FORCE) + elseif (CMAKE_SYSTEM_PROCESSOR MATCHES "^(arm64|aarch64).*$") + set(CMAKE_TOOLCHAIN_FILE "${CMAKE_CURRENT_SOURCE_DIR}/cmake/aarch64.cmake" CACHE FILEPATH "" FORCE) + else () + message(FATAL_ERROR "Not support your arch yet.") + endif () +endif () project(infinity VERSION 0.7.0) @@ -option(IS_CI_TEST_PIPELINE "is ci test pipeline: " OFF) if (IS_CI_TEST_PIPELINE) - if (X86_64) - set(VCPKG_CHAINLOAD_TOOLCHAIN_FILE "${CMAKE_CURRENT_SOURCE_DIR}/cmake/x86_64-v2.cmake") - elseif (ARM64) - set(VCPKG_CHAINLOAD_TOOLCHAIN_FILE "${CMAKE_CURRENT_SOURCE_DIR}/cmake/aarch64.cmake") - else () - message(FATAL_ERROR "Not support your arch yet.") - endif () -else () + # toolchain is selected before project() +else () add_definitions(-march=native) endif ()#!/bin/bash set -euo pipefail echo "Verify ordering of project/toolchain assignments:" rg -nP '^\s*project\s*\(|^\s*set\s*\(\s*CMAKE_TOOLCHAIN_FILE|^\s*set\s*\(\s*VCPKG_CHAINLOAD_TOOLCHAIN_FILE' CMakeLists.txt echo echo "CI toolchain block context:" sed -n '268,290p' CMakeLists.txt🤖 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 `@CMakeLists.txt` around lines 276 - 283, The VCPKG_CHAINLOAD_TOOLCHAIN_FILE assignment happens after project(), so it cannot influence the current CMake configure pass; move the conditional block that sets VCPKG_CHAINLOAD_TOOLCHAIN_FILE (the IS_CI_TEST_PIPELINE / X86_64 / ARM64 branch) to a location before the project() invocation (or otherwise set CMAKE_TOOLCHAIN_FILE early) so the toolchain selection takes effect during configure; update references to VCPKG_CHAINLOAD_TOOLCHAIN_FILE (and/or CMAKE_TOOLCHAIN_FILE) accordingly and ensure the fallback message(FATAL_ERROR) remains if neither X86_64 nor ARM64 is detected.
310-310:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake Python3 discovery fail fast for required build inputs.
Line 310 omits
REQUIRED. If Python3 Development is missing, configuration may proceed and fail later where includes/libs are consumed.Suggested fix
-find_package(Python3 COMPONENTS Development) +find_package(Python3 REQUIRED COMPONENTS Development)#!/bin/bash set -euo pipefail echo "Inspect Python3 discovery and downstream usage:" rg -nP 'find_package\s*\(\s*Python3|Python3_(FOUND|INCLUDE_DIRS|LIBRARIES)|Python3::' CMakeLists.txt src/CMakeLists.txt🤖 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 `@CMakeLists.txt` at line 310, The CMakeLists currently calls find_package(Python3 COMPONENTS Development) without REQUIRED, so missing Python dev files won't fail early; update the call to find_package(Python3 COMPONENTS Development REQUIRED) (locate the find_package(Python3 COMPONENTS Development) invocation) so configuration stops immediately if Python3 Development is absent and downstream uses of Python3::.../Python3_INCLUDE_DIRS/Python3_LIBRARIES won't proceed silently.
🤖 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.
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 276-283: The VCPKG_CHAINLOAD_TOOLCHAIN_FILE assignment happens
after project(), so it cannot influence the current CMake configure pass; move
the conditional block that sets VCPKG_CHAINLOAD_TOOLCHAIN_FILE (the
IS_CI_TEST_PIPELINE / X86_64 / ARM64 branch) to a location before the project()
invocation (or otherwise set CMAKE_TOOLCHAIN_FILE early) so the toolchain
selection takes effect during configure; update references to
VCPKG_CHAINLOAD_TOOLCHAIN_FILE (and/or CMAKE_TOOLCHAIN_FILE) accordingly and
ensure the fallback message(FATAL_ERROR) remains if neither X86_64 nor ARM64 is
detected.
- Line 310: The CMakeLists currently calls find_package(Python3 COMPONENTS
Development) without REQUIRED, so missing Python dev files won't fail early;
update the call to find_package(Python3 COMPONENTS Development REQUIRED) (locate
the find_package(Python3 COMPONENTS Development) invocation) so configuration
stops immediately if Python3 Development is absent and downstream uses of
Python3::.../Python3_INCLUDE_DIRS/Python3_LIBRARIES won't proceed silently.
Signed-off-by: noob <[email protected]>
Signed-off-by: noob <[email protected]>
Signed-off-by: noob <[email protected]>
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
1 participant
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?
Add x84-64 and aarch64 toolchain to infinity.
Type of change