Remove all the files related to auto tools #1676
Conversation
WalkthroughReplaces Autotools with a CMake-centric build: removes Autotools/autogen scripts and many m4 macros (root and vendored), updates Dockerfile and docs for CMake flows, changes build-info generation and CMake target ordering, and updates source includes to the new generated header path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/CI
participant CMake as CMake
participant Gen as generate_build_info
participant Build as Build System
participant Test as Test Runner
Dev->>CMake: cmake -B build (toolchain, -DBUILD_TESTS, -DBUILD_GUI=OFF)
CMake->>Gen: request generation (add_custom_command -> OUTPUT config/build.h)
Gen-->>CMake: config/build.h created
CMake->>Build: cmake --build build (generate_build_info is dependency)
Build->>Test: run tests (ctest / make test)
Test-->>Dev: report results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CMakeLists.txt (1)
39-71: Duplicate compilation of clientversion.cpp leads to duplicate symbolsclientversion.cpp is compiled into bitcoin_clientversion (OBJECT lib) and also listed in bitcoin_util sources. Since bitcoin_util links bitcoin_clientversion, this risks duplicate symbol definitions at final link.
Apply:
add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_batchverifier.h ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_ies.cpp ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_ies.h ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_worker.cpp ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_worker.h ${CMAKE_CURRENT_SOURCE_DIR}/chainparamsbase.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/clientversion.cpp ${CMAKE_CURRENT_SOURCE_DIR}/compat/glibc_sanity.cpp ${CMAKE_CURRENT_SOURCE_DIR}/compat/glibcxx_sanity.cpp ${CMAKE_CURRENT_SOURCE_DIR}/compat/strnlen.cpp ${CMAKE_CURRENT_SOURCE_DIR}/crypto/scrypt.cpp ${CMAKE_CURRENT_SOURCE_DIR}/fs.cpp ${CMAKE_CURRENT_SOURCE_DIR}/mbstring.cpp
🧹 Nitpick comments (9)
src/clientversion.cpp (1)
24-38: Update outdated comment to reflect new mechanismComments still mention HAVE_BUILD_INFO/BUILD_DESC from legacy flow. Suggest tightening to the new BUILD_GIT_TAG/BUILD_GIT_COMMIT logic.
Apply:
- * The following part of the code determines the CLIENT_BUILD variable. - * Several mechanisms are used for this: - * * first, if HAVE_BUILD_INFO is defined, include build.h, a file that is - * generated by the build environment, possibly containing the output - * of git-describe in a macro called BUILD_DESC - * * secondly, if this is an exported version of the code, GIT_ARCHIVE will - * be defined (automatically using the export-subst git attribute), and - * GIT_COMMIT will contain the commit id. - * * then, three options exist for determining CLIENT_BUILD: - * * if BUILD_DESC is defined, use that literally (output of git-describe) - * * if not, but GIT_COMMIT is defined, use v[maj].[min].[rev].[build]-g[commit] - * * otherwise, use v[maj].[min].[rev].[build]-unk - * finally CLIENT_VERSION_SUFFIX is added + * Determine CLIENT_BUILD using generated config/build.h: + * - If BUILD_GIT_TAG is defined, use that tag literally. + * - Else if BUILD_GIT_COMMIT is defined, use v[maj].[min].[rev].[build]-[commit]. + * - Else if building from a git archive, use GIT_COMMIT_ID provided by archive substitution. + * - Otherwise fallback to v[maj].[min].[rev].[build]-unk. + * Finally append CLIENT_VERSION_SUFFIX.doc/build-unix.md (4)
55-56: Package list: prefer python3 over pythonMost current distros no longer ship python as Python 2; recommend python3 explicitly.
- sudo apt-get install git curl python build-essential cmake pkg-config + sudo apt-get install git curl python3 build-essential cmake pkg-config
180-196: Remove or rewrite Autotools-only sectionsThese sections (disable-wallet via ./configure, additional configure flags) are no longer applicable after removing Autotools. Replace with CMake equivalents or delete to avoid confusion.
I can propose CMake equivalents for wallet toggles, hardening flags, and cross-compilation examples if helpful.
198-209: Arch example: switch to -DCMAKE_TOOLCHAIN_FILE and add ctestAlign with new flags and test invocation.
- cd depends && make -j$(nproc) && cd .. - cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF - cmake --build build -j$(nproc) - cd build && make test + cd depends && make -j$(nproc) && cd .. + cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF + cmake --build build -j$(nproc) + ctest --test-dir build --output-on-failure
224-236: ARM cross-compilation still shows ./configure; update to CMakeModernize to the depends+CMake flow (toolchain file) to avoid mixed instructions.
I can draft an ARM section using: depends HOST=arm-linux-gnueabihf, then cmake -DCMAKE_TOOLCHAIN_FILE=depends/arm-linux-gnueabihf/toolchain.cmake and -DBUILD_GUI=OFF.
README.md (4)
111-113: Use python3 in apt dependenciesKeep consistent with modern distros.
-sudo apt-get install python git curl build-essential cmake pkg-config +sudo apt-get install python3 git curl build-essential cmake pkg-config
176-181: Prefer ctest to run tests and avoid re-configuring repeatedlyYou can avoid repeating cmake -B lines and use ctest.
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ - -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON -cd build && make -j$(nproc) -make test +cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ + -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON +cmake --build build -j$(nproc) +ctest --test-dir build --output-on-failure
193-203: Fix table formatting and clarify BUILD_CLI vs BUILD_TXAdd blank lines around the table (MD058) and ensure descriptions are accurate.
-### CMake Options Reference - -| Flag | Description | Default | -|------|-------------|---------| -| `BUILD_DAEMON` | Build `firod` executable | `ON` | -| `BUILD_GUI` | Build `firo-qt` GUI client | `ON` | -| `BUILD_CLI` | Build `firo-tx` and other command-line tools | `ON` | -| `ENABLE_WALLET` | Enable wallet functionality | `ON` | -| `BUILD_TESTS` | Build test suite | `OFF` | -| `BUILD_TX` | Build `firo-tx` transaction tool | Subset of `BUILD_CLI` | -| `ENABLE_CRASH_HOOKS` | Enable crash reporting/stack traces | `OFF` | -| `WITH_ZMQ` | Enable ZeroMQ notifications | `ON` | +### CMake Options Reference + +| Flag | Description | Default | +|------|-------------|---------| +| `BUILD_DAEMON` | Build `firod` executable | `ON` | +| `BUILD_GUI` | Build `firo-qt` GUI client | `ON` | +| `BUILD_CLI` | Build CLI components (e.g., `firo-cli`) | `ON` | +| `ENABLE_WALLET` | Enable wallet functionality | `ON` | +| `BUILD_TESTS` | Build test suite | `OFF` | +| `BUILD_TX` | Build `firo-tx` transaction tool | `ON` when `BUILD_CLI=ON` | +| `ENABLE_CRASH_HOOKS` | Enable crash reporting/stack traces | `OFF` | +| `WITH_ZMQ` | Enable ZeroMQ notifications | `ON` | +
206-229: Cross-compilation: standardize toolchain flagUse -DCMAKE_TOOLCHAIN_FILE consistently.
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake \ - -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake \ + -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON cd build && make -j$(nproc)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (41)
Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_check_preproc_flag.m4
- build-aux/m4/bitcoin_find_bdb48.m4
- build-aux/m4/ax_gcc_func_attribute.m4
- build-aux/m4/ax_boost_system.m4
- build-aux/m4/l_atomic.m4
- build-aux/m4/ax_boost_unit_test_framework.m4
- libbitcoinconsensus.pc.in
- Makefile.am
- build-aux/m4/ax_subdirs_configure.m4
- build-aux/m4/ax_check_compile_flag.m4
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- build-aux/m4/ax_boost_program_options.m4
- src/Makefile.am
- build-aux/m4/ax_boost_base.m4
- src/univalue/Makefile.am
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_check_link_flag.m4
- src/secp256k1/Makefile.am
- src/univalue/autogen.sh
- build-aux/m4/ax_boost_chrono.m4
- build-aux/m4/ax_boost_filesystem.m4
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- src/univalue/configure.ac
- src/univalue/build-aux/m4/.gitignore
- autogen.sh
- build-aux/m4/ax_boost_thread.m4
- configure.ac
- build-aux/m4/bitcoin_subdir_to_include.m4
- src/secp256k1/configure.ac
- build-aux/m4/bitcoin_qt.m4
- build-aux/m4/ax_pthread.m4
- doc/man/Makefile.am
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-31T04:27:35.223Z
Learnt from: aleflm
PR: firoorg/firo#1504
File: CMakeLists.txt:457-484
Timestamp: 2025-03-31T04:27:35.223Z
Learning: Pkg-config should not be used in the Firo CMake build system as it's not available by default on Windows, and the project aims to support Windows natively in the future.
Applied to files:
doc/build-unix.mdREADME.md
🪛 markdownlint-cli2 (0.18.1)
README.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
232-232: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-mac-cmake
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-linux-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: build
🔇 Additional comments (6)
src/clientversion.h (1)
10-10: Include new build-info header only when config is present — LGTMThis aligns with the CMake-generated config/build.h. No functional concerns here.
Please confirm the build defines HAVE_CONFIG_H for targets that need build metadata (especially Windows RC resources). Otherwise, those translation units won’t see macros from config/build.h via this header (clientversion.cpp includes it unconditionally, but RC preprocessing uses this header path).
src/clientversion.cpp (2)
41-41: Unconditional include of config/build.h — OKMatches the new generation flow; combined with the CMake dependency this should be reliable.
57-59: Macro uses raw commit literal (not stringized) — correct with new headerBUILD_GIT_COMMIT/GIT_COMMIT_ID are string literals, so concatenation is valid. No issue.
cmake/script/GenerateBuildInfo.cmake (2)
13-16: Guarded read — OKReading existing header to avoid unnecessary writes is good for incremental builds.
102-114: Emit header guard and status messages — nice improvementsGood usability and avoids redundant rebuilds when content is unchanged.
README.md (1)
145-152: depends build: ensure parallel make and keep consistentSmall style nit; current lines are fine. No action required.
| # Only update the header if necessary. | ||
| if(NOT "${INFO}" STREQUAL "${NEWINFO}") | ||
| file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}\n") | ||
| file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}") | ||
| endif() |
There was a problem hiding this comment.
Create parent directory and quote path before writing
Currently writing to ${...}/config/build.h will fail if config/ doesn’t exist. Also quote the path to be safe.
Apply:
-# Only update the header if necessary.
-if(NOT "${INFO}" STREQUAL "${NEWINFO}")
- file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}")
-endif()
+# Ensure parent directory exists and write only if content changed.
+get_filename_component(_BUILD_INFO_DIR "${BUILD_INFO_HEADER_PATH}" DIRECTORY)
+file(MAKE_DIRECTORY "${_BUILD_INFO_DIR}")
+if(NOT "${INFO}" STREQUAL "${NEWINFO}")
+ file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}")
+endif()📝 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.
| # Only update the header if necessary. | |
| if(NOT "${INFO}" STREQUAL "${NEWINFO}") | |
| file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}\n") | |
| file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}") | |
| endif() | |
| # Ensure parent directory exists and write only if content changed. | |
| get_filename_component(_BUILD_INFO_DIR "${BUILD_INFO_HEADER_PATH}" DIRECTORY) | |
| file(MAKE_DIRECTORY "${_BUILD_INFO_DIR}") | |
| if(NOT "${INFO}" STREQUAL "${NEWINFO}") | |
| file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}") | |
| endif() |
🤖 Prompt for AI Agents
In cmake/script/GenerateBuildInfo.cmake around lines 118-121, the script writes
to ${BUILD_INFO_HEADER_PATH} which will fail if the parent directory doesn't
exist and also uses an unquoted path; update the logic to (1) compute the parent
directory with get_filename_component(PARENT_DIR "${BUILD_INFO_HEADER_PATH}"
DIRECTORY), (2) call file(MAKE_DIRECTORY "${PARENT_DIR}") before attempting to
write, and (3) quote the path and contents in the file(WRITE) call (e.g.
file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}")) so the write succeeds even
when directories are missing or paths contain spaces.
Dockerfile
Outdated
| RUN cmake -B build --toolchain depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ | ||
| cmake --build build -j$(nproc) && \ | ||
| cd build && make test && \ | ||
| cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu |
There was a problem hiding this comment.
Fix two build blockers: unsupported --toolchain flag and wrong install path after cd
- CMake on Debian bullseye typically doesn’t support the --toolchain CLI flag. Use -DCMAKE_TOOLCHAIN_FILE=… instead.
- After cd build, running cmake --install build points to a non-existent nested build/build directory. Use cmake --install . or call install before cd.
Apply this diff:
-RUN cmake -B build --toolchain depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \
- cmake --build build -j$(nproc) && \
- cd build && make test && \
- cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu
+RUN cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \
+ cmake --build build -j$(nproc) && \
+ ctest --test-dir build --output-on-failure && \
+ cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu📝 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.
| RUN cmake -B build --toolchain depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ | |
| cmake --build build -j$(nproc) && \ | |
| cd build && make test && \ | |
| cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu | |
| RUN cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ | |
| cmake --build build -j$(nproc) && \ | |
| ctest --test-dir build --output-on-failure && \ | |
| cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu |
🤖 Prompt for AI Agents
In Dockerfile around lines 22 to 25, the cmake invocation uses the unsupported
--toolchain flag and the subsequent cmake --install build after cd build targets
a non-existent nested build/build; replace the --toolchain usage with
-DCMAKE_TOOLCHAIN_FILE=depends/$(uname -m)-linux-gnu/toolchain.cmake (keeping
other -D flags), and change the install step to run cmake --install . after cd
build (or alternatively run cmake --install build before cd build) so the
install path is correct (/tmp/firo/depends/$(uname -m)-linux-gnu).
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
from
4ac9239 to
703d1c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CMakeLists.txt (1)
39-47: Duplicate compilation of clientversion.cpp and misuse of OBJECT library cause ODR/duplicate symbol risks.clientversion.cpp is compiled into:
OBJECT library bitcoin_clientversion, and
bitcoin_util sources,
and bitcoin_util also “links” bitcoin_clientversion. This can yield duplicate symbol definitions in consumers.Remove clientversion.cpp from bitcoin_util sources.
Consume OBJECTs from bitcoin_clientversion explicitly.
Proposed snippet to apply after bitcoin_clientversion is defined:
# Remove clientversion.cpp from bitcoin_util sources list. # Instead, reuse the object(s) from bitcoin_clientversion. target_sources(bitcoin_util PRIVATE $<TARGET_OBJECTS:bitcoin_clientversion>) # Since bitcoin_clientversion is an OBJECT library, keeping it in target_link_libraries # is unnecessary unless you rely on its usage requirements. You may remove it: # target_link_libraries(bitcoin_util PRIVATE bitcoin_clientversion)Also applies to: 67-71
🧹 Nitpick comments (7)
src/clientversion.cpp (2)
34-36: Align comments and formatting with actual version string output (drop "-g").
- The comment still documents "-g[commit]" but BUILD_DESC_FROM_COMMIT produces "...-".
- Also switched to BUILD_DESC_FROM_COMMIT for GIT_COMMIT_ID path; reflect that in the comment.
Apply:
- * * if not, but GIT_COMMIT is defined, use v[maj].[min].[rev].[build]-g[commit] + * * if not, but GIT_COMMIT is defined, use v[maj].[min].[rev].[build]-[commit]And (optional) clarify the GIT_COMMIT_ID path:
-#elif defined(GIT_COMMIT_ID) -#define BUILD_DESC BUILD_DESC_FROM_COMMIT(CLIENT_VERSION_MAJOR, CLIENT_VERSION_MINOR, CLIENT_VERSION_REVISION, CLIENT_VERSION_BUILD, GIT_COMMIT_ID) +#elif defined(GIT_COMMIT_ID) +#define BUILD_DESC BUILD_DESC_FROM_COMMIT(CLIENT_VERSION_MAJOR, CLIENT_VERSION_MINOR, CLIENT_VERSION_REVISION, CLIENT_VERSION_BUILD, GIT_COMMIT_ID) // "-g" intentionally omittedAlso applies to: 57-59, 71-72
41-46: Redundant include vs. documented behavior.clientversion.h is now responsible for including config/build.h (behind HAVE_CONFIG_H). Including config/build.h here unconditionally conflicts with the comment above (“if HAVE_BUILD_INFO... include build.h”) and may be redundant.
- Option A (preferred): rely on clientversion.h to include config/build.h and remove this include.
- Option B: keep this include but update the comment block accordingly.
Proposed change:
-#include "config/build.h" +// build info comes from clientversion.h -> config/build.h when availablesrc/CMakeLists.txt (1)
67-79: Minor: univalue appears twice in link libraries.Deduplicate to keep link interfaces clean.
target_link_libraries(bitcoin_util PRIVATE core_interface - bitcoin_clientversion + bitcoin_clientversion bitcoin_crypto Boost::thread Boost::chrono - univalue secp256k1 leveldb univalue $<$<BOOL:${WIN32}>:windows_system> )README.md (1)
108-113: Drop pkg-config from Linux prerequisites (not required for CMake path).Per project direction to avoid pkg-config reliance (Windows parity), omit it from Debian/Ubuntu deps unless strictly needed.
-sudo apt-get install python git curl build-essential cmake pkg-config +sudo apt-get install python git curl build-essential cmakeIf any component still requires pkg-config at runtime, please point to that and we can scope a targeted note instead.
doc/build-unix.md (3)
55-56: Trim pkg-config from dependency lists unless strictly needed.Keep Linux instructions aligned with the CMake-first flow.
- sudo apt-get install git curl python build-essential cmake pkg-config + sudo apt-get install git curl python build-essential cmake- sudo dnf install bzip2 perl-lib perl-FindBin gcc-c++ make cmake patch which + sudo dnf install bzip2 perl-lib perl-FindBin gcc-c++ make cmake patch which(Confirm no hidden pkg-config usage remains.)
Also applies to: 87-88
171-172: Correct example binary name for scanelf.The project installs firod/firo-qt/firo-cli, not “bitcoin”.
- `scanelf -e ./bitcoin` + `scanelf -e ./firod`
180-186: Replace Autotools “configure” wallet toggles with CMake equivalents.These sections still reference ./configure flags; switch to CMake options for consistency.
Examples:
- ./configure --disable-wallet + cmake -B build -DENABLE_WALLET=OFF [...]- ./configure --enable-hardening - ./configure --disable-hardening + cmake -B build -DENABLE_HARDENING=ON [...] + cmake -B build -DENABLE_HARDENING=OFF [...]If ENABLE_HARDENING differs in this tree, adjust to the actual CMake option names.
Also applies to: 192-196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (42)
Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)depends/Makefile(1 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_gcc_func_attribute.m4
- build-aux/m4/ax_subdirs_configure.m4
- build-aux/m4/ax_check_compile_flag.m4
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- build-aux/m4/ax_boost_filesystem.m4
- src/univalue/autogen.sh
- src/univalue/Makefile.am
- build-aux/m4/ax_boost_program_options.m4
- build-aux/m4/ax_pthread.m4
- build-aux/m4/bitcoin_qt.m4
- build-aux/m4/ax_boost_base.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_check_preproc_flag.m4
- configure.ac
- build-aux/m4/bitcoin_subdir_to_include.m4
- build-aux/m4/l_atomic.m4
- src/secp256k1/Makefile.am
- Makefile.am
- build-aux/m4/ax_boost_thread.m4
- autogen.sh
- doc/man/Makefile.am
- src/univalue/configure.ac
- build-aux/m4/bitcoin_find_bdb48.m4
- libbitcoinconsensus.pc.in
- build-aux/m4/ax_boost_system.m4
- src/secp256k1/configure.ac
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- build-aux/m4/ax_boost_unit_test_framework.m4
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/ax_boost_chrono.m4
- build-aux/m4/ax_check_link_flag.m4
- src/Makefile.am
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
✅ Files skipped from review due to trivial changes (1)
- depends/Makefile
🚧 Files skipped from review as they are similar to previous changes (3)
- Dockerfile
- cmake/script/GenerateBuildInfo.cmake
- src/clientversion.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-31T04:27:35.223Z
Learnt from: aleflm
PR: firoorg/firo#1504
File: CMakeLists.txt:457-484
Timestamp: 2025-03-31T04:27:35.223Z
Learning: Pkg-config should not be used in the Firo CMake build system as it's not available by default on Windows, and the project aims to support Windows natively in the future.
Applied to files:
doc/build-unix.mdREADME.md
🪛 markdownlint-cli2 (0.18.1)
README.md
174-174: Hard tabs
Column: 1
(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-mac-cmake
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-linux-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: build
🔇 Additional comments (5)
doc/build-unix.md (3)
32-35: Use -DCMAKE_TOOLCHAIN_FILE (portable) instead of --toolchain.--toolchain isn’t widely supported in distro CMake. Switch to the standard variable.
-cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -cmake --build build -j$(nproc) -cmake --install build # optional +cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmake +cmake --build build -j$(nproc) +cmake --install build # optional
206-209: Repeat: replace --toolchain with -DCMAKE_TOOLCHAIN_FILE in all instances.Ensure consistency across the doc.
- cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF + cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF- cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake + cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmakeAlso applies to: 267-269
203-209: Fix “Setup and Build Example” repo/commands (Bitcoin → Firo; toolchain flag).Update to the correct repository and CMake toolchain flag.
- git clone https://github.com/bitcoin/bitcoin.git - cd bitcoin/ - cd depends && make -j$(nproc) && cd .. - cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF - cmake --build build -j$(nproc) - cd build && make test + git clone https://github.com/firoorg/firo.git + cd firo/ + cd depends && make -j$(nproc) && cd .. + cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF + cmake --build build -j$(nproc) + cd build && ctest --output-on-failuresrc/clientversion.cpp (1)
64-68: BUILD_GIT_COMMIT/BUILD_GIT_TAG are emitted as quoted string literals — no change required.cmake/script/GenerateBuildInfo.cmake writes
#define BUILD_GIT_COMMIT \"${GIT_COMMIT}\"and#define BUILD_GIT_TAG \"${GIT_TAG}\", so the macros are already quoted.README.md (1)
169-172: No action needed — README.md code block (lines 169–172) has no hard tabsVerified: those lines contain zero tab characters; MD010 (hard-tabs) does not apply here.
Likely an incorrect or invalid review comment.
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
from
703d1c0 to
fd0ed0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (2)
156-164: Remove redundant depends rebuild in headless instructions.
Step 2 already builds depends. Rebuilding with NO_QT immediately after is confusing and redundant.-```sh -cd depends -NO_QT=true make -j`nproc` -cd .. -cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ --DBUILD_GUI=OFF -DBUILD_CLI=ON -cd build && make -j$(nproc) -``` +```sh +cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ + -DBUILD_GUI=OFF -DBUILD_CLI=ON +cd build && make -j$(nproc) +```Optionally, instruct users who want headless-only to run
NO_QT=true make -j$(nproc)in Step 2 instead of the generic build.
176-181: Preferctestovermake test.
Using ctest is the CMake-native test driver and provides better reporting.-cd build && make -j$(nproc) -make test +cd build && make -j$(nproc) +ctest --output-on-failure
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (42)
Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)depends/Makefile(1 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_boost_base.m4
- autogen.sh
- build-aux/m4/ax_check_preproc_flag.m4
- build-aux/m4/ax_subdirs_configure.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_boost_thread.m4
- build-aux/m4/bitcoin_subdir_to_include.m4
- build-aux/m4/ax_boost_system.m4
- build-aux/m4/bitcoin_qt.m4
- configure.ac
- build-aux/m4/ax_boost_program_options.m4
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- doc/man/Makefile.am
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- build-aux/m4/l_atomic.m4
- build-aux/m4/ax_gcc_func_attribute.m4
- build-aux/m4/bitcoin_find_bdb48.m4
- src/Makefile.am
- src/secp256k1/Makefile.am
- build-aux/m4/ax_boost_unit_test_framework.m4
- build-aux/m4/ax_check_link_flag.m4
- src/univalue/Makefile.am
- Makefile.am
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_pthread.m4
- build-aux/m4/ax_check_compile_flag.m4
- libbitcoinconsensus.pc.in
- src/univalue/configure.ac
- src/univalue/autogen.sh
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- build-aux/m4/ax_boost_filesystem.m4
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/ax_boost_chrono.m4
- src/secp256k1/configure.ac
✅ Files skipped from review due to trivial changes (1)
- src/clientversion.h
🚧 Files skipped from review as they are similar to previous changes (4)
- depends/Makefile
- cmake/script/GenerateBuildInfo.cmake
- doc/build-unix.md
- Dockerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-31T04:27:35.223Z
Learnt from: aleflm
PR: firoorg/firo#1504
File: CMakeLists.txt:457-484
Timestamp: 2025-03-31T04:27:35.223Z
Learning: Pkg-config should not be used in the Firo CMake build system as it's not available by default on Windows, and the project aims to support Windows natively in the future.
Applied to files:
README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-windows-cmake
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-linux-cmake
- GitHub Check: build-mac-cmake
- GitHub Check: build
🔇 Additional comments (3)
src/clientversion.cpp (2)
71-71: Switch to BUILD_DESC_FROM_COMMIT for GIT_COMMIT_ID path LGTM; verify downstream assumptions.
If keeping the no “-g” format, make sure RPC/version string consumers and any packaging scripts aren’t expecting “-g”.
41-41: Include path change LGTM — no stray #include "build.h" found.
Repository search returned zero matches for#include "build.h"or<build.h>.src/CMakeLists.txt (1)
12-23: Make build-info generation directory-safe and resilient to HEAD changes.
Without creating${CMAKE_CURRENT_BINARY_DIR}/config, header generation may fail on clean builds. Also consider rerunning when HEAD changes and making the target part of ALL.add_custom_command( OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/config/build.h - COMMAND ${CMAKE_COMMAND} + COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/config + COMMAND ${CMAKE_COMMAND} -DBUILD_INFO_HEADER_PATH=${CMAKE_CURRENT_BINARY_DIR}/config/build.h -DSOURCE_DIR=${PROJECT_SOURCE_DIR} -P ${PROJECT_SOURCE_DIR}/cmake/script/GenerateBuildInfo.cmake COMMENT "Generating build.h" VERBATIM ) -add_custom_target(generate_build_info +add_custom_target(generate_build_info ALL DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/config/build.h )Additionally (outside this hunk), near the top:
# Re-run build info generation when HEAD changes (best-effort). if(EXISTS "${PROJECT_SOURCE_DIR}/.git/HEAD") set(_GIT_DEPENDS "${PROJECT_SOURCE_DIR}/.git/HEAD") endif()…and extend the custom command with:
DEPENDS ${_GIT_DEPENDS}#!/bin/bash # Ensure the generator script creates its output dir if the above change is skipped. rg -n "file\\s*\\(MAKE_DIRECTORY" cmake/script/GenerateBuildInfo.cmake || true
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
from
fd0ed0d to
b5f9191
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
106-113: Update README: CMake 3.22 required — Ubuntu 18.04 note is misleadingCMakeLists.txt sets cmake_minimum_required(VERSION 3.22) (CMakeLists.txt:10) but README.md still lists "minimum Ubuntu 18.04" (README.md lines 106-113) — Ubuntu 18.04 ships CMake 3.10. Either raise the documented minimum to Ubuntu 22.04+ or add instructions to install a newer CMake (e.g., Kitware APT repo) for older distros.
🧹 Nitpick comments (3)
README.md (3)
187-189: Clarify path to GUI binaryBinaries land in build/bin; from repo root, ./bin/firo-qt won’t exist.
-./bin/firo-qt +build/bin/firo-qt +# or: +# cd build && ./bin/firo-qt
206-210: Markdown: add blank line before the table (MD058)Improves rendering and satisfies markdownlint.
-To build for other platforms, specify the `HOST` variable when building dependencies: +To build for other platforms, specify the `HOST` variable when building dependencies: + | Host Target | Platform |
232-234: Fix bullet formatting and spacing around code spanAddresses markdownlint indentation and a missing space after backtick.
- * The toolchain path in `CMAKE_TOOLCHAIN_FILE`must match your target architecture. - * `BUILD_TX` is automatically enabled if `BUILD_CLI=ON` is enabled. +* The toolchain path in `CMAKE_TOOLCHAIN_FILE` must match your target architecture. +* `BUILD_TX` is automatically enabled if `BUILD_CLI=ON` is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (43)
CMakeLists.txt(1 hunks)Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)depends/Makefile(1 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_boost_system.m4
- build-aux/m4/ax_boost_filesystem.m4
- libbitcoinconsensus.pc.in
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- src/univalue/Makefile.am
- build-aux/m4/ax_boost_thread.m4
- Makefile.am
- build-aux/m4/bitcoin_subdir_to_include.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- configure.ac
- build-aux/m4/ax_subdirs_configure.m4
- build-aux/m4/ax_check_preproc_flag.m4
- src/secp256k1/Makefile.am
- build-aux/m4/ax_boost_chrono.m4
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- autogen.sh
- build-aux/m4/ax_gcc_func_attribute.m4
- build-aux/m4/ax_boost_base.m4
- src/secp256k1/configure.ac
- build-aux/m4/ax_check_link_flag.m4
- build-aux/m4/bitcoin_find_bdb48.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- src/univalue/configure.ac
- build-aux/m4/l_atomic.m4
- build-aux/m4/ax_pthread.m4
- src/univalue/autogen.sh
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- src/Makefile.am
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/bitcoin_qt.m4
- build-aux/m4/ax_boost_program_options.m4
- build-aux/m4/ax_check_compile_flag.m4
- doc/man/Makefile.am
- build-aux/m4/ax_boost_unit_test_framework.m4
🚧 Files skipped from review as they are similar to previous changes (7)
- src/clientversion.h
- src/CMakeLists.txt
- Dockerfile
- cmake/script/GenerateBuildInfo.cmake
- doc/build-unix.md
- depends/Makefile
- src/clientversion.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-31T04:27:35.223Z
Learnt from: aleflm
PR: firoorg/firo#1504
File: CMakeLists.txt:457-484
Timestamp: 2025-03-31T04:27:35.223Z
Learning: Pkg-config should not be used in the Firo CMake build system as it's not available by default on Windows, and the project aims to support Windows natively in the future.
Applied to files:
README.md
🪛 markdownlint-cli2 (0.18.1)
README.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
232-232: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build
- GitHub Check: build-mac-cmake
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-linux-cmake
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-windows-cmake
🔇 Additional comments (1)
README.md (1)
174-181: Same fix in tests snippet; also minor consistency for nproc-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON cd build && make -j$(nproc) make test
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
2 times, most recently
from
08880e3 to
5665bce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
depends/Makefile (1)
207-207: Makefile syntax error: stray text breaks the ruleThe line contains “Add commentMore actions”, which will make make(1) choke.
- $(AT)@mkdir -p $(@D)Add commentMore actions + $(AT)@mkdir -p $(@D)
🧹 Nitpick comments (8)
depends/Makefile (1)
197-197: Fix message: recommend CMake’s standard toolchain variableUpdate the hint to match docs and portable usage.
- echo To build Firo with these packages, pass \'--toolchain $(@D)/toolchain.cmake\' to the first CMake invocation. + echo To build Firo with these packages, pass \'-DCMAKE_TOOLCHAIN_FILE=$(@D)/toolchain.cmake\' to the first CMake invocation.Dockerfile (1)
5-16: Reduce image size: clean apt lists in the same layerTighten the layer by removing package lists.
-RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y \ bsdmainutils \ cmake \ curl \ g++ \ make \ pkg-config \ patch \ m4 \ autoconf \ automake \ - libtool + libtool && \ + rm -rf /var/lib/apt/lists/*doc/build-unix.md (4)
30-35: Use -DCMAKE_TOOLCHAIN_FILE and correct toolchain path; use ctest for testsFix the toolchain invocation and test call.
-make -j$(nproc) +make -j$(nproc) cd .. -cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake +cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(./depends/config.guess)/toolchain.cmake cmake --build build -j$(nproc) -cmake --install build # optional +cmake --install build # optional
55-55: Prefer python3 package name on modern distros- sudo apt-get install git curl python build-essential cmake pkg-config + sudo apt-get install git curl python3 build-essential cmake pkg-config
205-209: Arch example: fix toolchain path and use ctest- cd depends && make -j$(nproc) && cd .. - cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF - cmake --build build -j$(nproc) - cd build && make test + cd depends && make -j$(nproc) && cd .. + cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(./depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF + cmake --build build -j$(nproc) + ctest --test-dir build --output-on-failure
266-269: FreeBSD snippet: use -DCMAKE_TOOLCHAIN_FILE and drop duplicate -jCMake already passes -j.
- cd depends && gmake -j$(nproc) && cd .. - cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake - cmake --build build -j$(nproc) -- -j$(nproc) + cd depends && gmake -j$(nproc) && cd .. + cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(./depends/config.guess)/toolchain.cmake + cmake --build build -j$(nproc)README.md (2)
110-113: Use python3 in Debian/Ubuntu deps-sudo apt-get install python git curl build-essential cmake pkg-config +sudo apt-get install python3 git curl build-essential cmake pkg-config
206-214: Surround the table with blank lines (markdownlint MD058) and minor wordingAdd blank lines before/after the table and keep the note concise. No behavior change; improves rendering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (43)
CMakeLists.txt(1 hunks)Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)depends/Makefile(1 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- doc/man/Makefile.am
- build-aux/m4/bitcoin_subdir_to_include.m4
- autogen.sh
- build-aux/m4/l_atomic.m4
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_pthread.m4
- build-aux/m4/ax_boost_chrono.m4
- build-aux/m4/ax_subdirs_configure.m4
- build-aux/m4/ax_boost_base.m4
- build-aux/m4/ax_boost_thread.m4
- build-aux/m4/ax_check_preproc_flag.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_boost_program_options.m4
- build-aux/m4/ax_gcc_func_attribute.m4
- build-aux/m4/bitcoin_find_bdb48.m4
- Makefile.am
- src/secp256k1/configure.ac
- build-aux/m4/ax_check_link_flag.m4
- src/univalue/Makefile.am
- build-aux/m4/ax_boost_system.m4
- src/univalue/autogen.sh
- build-aux/m4/ax_boost_unit_test_framework.m4
- configure.ac
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- src/univalue/build-aux/m4/.gitignore
- src/secp256k1/Makefile.am
- src/Makefile.am
- build-aux/m4/ax_boost_filesystem.m4
- build-aux/m4/bitcoin_qt.m4
- libbitcoinconsensus.pc.in
- build-aux/m4/ax_check_compile_flag.m4
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- src/univalue/configure.ac
🚧 Files skipped from review as they are similar to previous changes (3)
- CMakeLists.txt
- src/clientversion.h
- src/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-31T04:27:35.223Z
Learnt from: aleflm
PR: firoorg/firo#1504
File: CMakeLists.txt:457-484
Timestamp: 2025-03-31T04:27:35.223Z
Learning: Pkg-config should not be used in the Firo CMake build system as it's not available by default on Windows, and the project aims to support Windows natively in the future.
Applied to files:
doc/build-unix.mdREADME.mdDockerfile
🪛 markdownlint-cli2 (0.18.1)
doc/build-unix.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
266-266: Images should have alternate text (alt text)
(MD045, no-alt-text)
README.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
232-232: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-mac-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: build-linux-cmake
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build
🔇 Additional comments (11)
depends/Makefile (1)
46-47: Confirm new primary mirror availability and artifact paritySwapping the primary/alternative mirrors changes your blast radius during outages. Please confirm firo.org hosts a complete, stable mirror with identical checksums and that CI has network access to it. If not guaranteed, consider keeping bitcoincore.org as primary.
src/clientversion.cpp (3)
41-41: LGTM: include path matches generated headerIncluding config/build.h aligns with the CMake-generated build info.
71-71: Version format consistency checkIf you keep the updated macro above, this usage remains correct and yields the expected v[...]-g form. If you prefer the new format, update the comment block to avoid confusion.
57-59: Restore legacy version format (“-g”) or update the commentHeader comment says v[...]-g[commit], but macro emits v[...]-commit. Keep the “-g” for compatibility unless intentionally changed.
-#define BUILD_DESC_FROM_COMMIT(maj, min, rev, build, commit) \ - "v" DO_STRINGIZE(maj) "." DO_STRINGIZE(min) "." DO_STRINGIZE(rev) "." DO_STRINGIZE(build) "-" commit +#define BUILD_DESC_FROM_COMMIT(maj, min, rev, build, commit) \ + "v" DO_STRINGIZE(maj) "." DO_STRINGIZE(min) "." DO_STRINGIZE(rev) "." DO_STRINGIZE(build) "-g" commitcmake/script/GenerateBuildInfo.cmake (3)
15-16: LGTM: quoted read avoids path parsing issuesQuoting BUILD_INFO_HEADER_PATH in file(READ) is correct.
102-113: LGTM: explicit status and minimal header contentClear status messages and a single-define guarded header are appropriate.
118-121: Create parent directory and quote path before writingEnsure the directory exists and quote the target on write.
-# Only update the header if necessary. -if(NOT "${INFO}" STREQUAL "${NEWINFO}") - file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}") -endif() +# Only update the header if necessary. +get_filename_component(_BUILD_INFO_DIR "${BUILD_INFO_HEADER_PATH}" DIRECTORY) +file(MAKE_DIRECTORY "${_BUILD_INFO_DIR}") +if(NOT "${INFO}" STREQUAL "${NEWINFO}") + file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}") +endif()README.md (3)
161-164: Fix invalid toolchain path substitution and keep out-of-source build-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=OFF -DBUILD_CLI=ON -cd build && make -j$(nproc) +cd build && make -j$(nproc)
169-172: Same fix for GUI snippet-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=ON -DBUILD_CLI=ON cd build && make -j$(nproc)
177-181: Tests: use ctest and correct toolchain path-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON cd build && make -j$(nproc) -make test +ctest --output-on-failureDockerfile (1)
26-29: Fix two build blockers: use -DCMAKE_TOOLCHAIN_FILE and correct install/test invocations
- --toolchain isn’t portable; use -DCMAKE_TOOLCHAIN_FILE=…
- After cd build, cmake --install build points at build/build. Either don’t cd or install “.”.
- Prefer ctest.
-RUN cmake -B build --toolchain depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ - cmake --build build -j$(nproc) && \ - cd build && make test && \ - cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu +RUN cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ + cmake --build build -j$(nproc) && \ + ctest --test-dir build --output-on-failure && \ + cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
2 times, most recently
from
36a48a1 to
15bd6ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CMakeLists.txt (1)
24-33: Avoid compiling clientversion.cpp twice; consume OBJECTs correctly.clientversion.cpp is compiled in bitcoin_clientversion (OBJECT) and again in bitcoin_util, which risks duplicate symbols and wastes cycles. Prefer building it once and reusing the objects.
Option A (recommended):
- Remove clientversion.cpp from bitcoin_util sources.
- Add bitcoin_clientversion objects to bitcoin_util.
Example (outside this hunk):
# Remove clientversion.cpp from the bitcoin_util source list target_sources(bitcoin_util PRIVATE $<TARGET_OBJECTS:bitcoin_clientversion>) add_dependencies(bitcoin_util generate_build_info)Option B:
- Drop the OBJECT library and keep clientversion.cpp only in bitcoin_util; add:
add_dependencies(bitcoin_util generate_build_info)
🧹 Nitpick comments (5)
README.md (4)
109-116: Use python3 and document minimum CMake version (>= 3.22).Replace python with python3 and add a note about the required CMake version and how to install it on older Ubuntu. This aligns with prior guidance/preferences.
sudo apt-get update -sudo apt-get install python git curl build-essential cmake pkg-config +sudo apt-get install python3 git curl build-essential cmake pkg-config # Also needed for GUI wallet only: sudo apt-get install qttools5-dev qttools5-dev-tools libxcb-xkb-dev bison-If you use a later version of Ubuntu, you may need to replace `python` with `python3`. +Requirements: Python 3 and CMake >= 3.22. +If your Ubuntu release ships an older CMake, install the latest CMake from Kitware’s APT repository.Note: Keep pkg-config in the OS deps, but do not rely on pkg-config inside the CMake build itself (per earlier project direction).
This suggestion follows your previously stated preference to avoid pkg-config in the CMake build system on Windows (long-term learnings applied).
145-151: Avoid re-building depends twice; streamline headless vs GUI paths.Step 2 builds depends, then headless build re-builds depends (NO_QT=true). Either:
- Build depends once with NO_QT=true for headless, or
- Keep Step 2 and drop the extra depends build in Step 3 (headless).
Minimal change (remove the duplicate in Step 3):
Headless (command-line only for servers etc.): ```sh -cd depends -NO_QT=true make -j`nproc` -cd .. cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ -DBUILD_GUI=OFF -DBUILD_CLI=ON cd build && make -j$(nproc)Also applies to: 155-164 --- `191-204`: **Fix markdown lint: surround table with blank lines.** Add a blank line after the table to satisfy MD058. ```diff | `WITH_ZMQ` | Enable ZeroMQ notifications | `ON` | + ### Supported Cross-Compilation Targets
231-235: Fix markdown list indentation and spacing (MD007).Remove leading spaces before “*” and fix missing space after backtick.
- * The toolchain path in `CMAKE_TOOLCHAIN_FILE`must match your target architecture. - * `BUILD_TX` is automatically enabled if `BUILD_CLI=ON` is enabled. +* The toolchain path in `CMAKE_TOOLCHAIN_FILE` must match your target architecture. +* `BUILD_TX` is automatically enabled if `BUILD_CLI=ON`.Dockerfile (1)
5-16: Reduce image size: clean apt lists in same layer.Remove apt caches to slim the build stage.
RUN apt-get update && apt-get install -y \ bsdmainutils \ cmake \ curl \ g++ \ make \ pkg-config \ patch \ m4 \ autoconf \ automake \ - libtool + libtool \ + && rm -rf /var/lib/apt/lists/*
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (43)
CMakeLists.txt(1 hunks)Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)depends/Makefile(1 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_boost_thread.m4
- src/secp256k1/configure.ac
- build-aux/m4/ax_gcc_func_attribute.m4
- build-aux/m4/ax_pthread.m4
- build-aux/m4/ax_boost_system.m4
- build-aux/m4/ax_check_compile_flag.m4
- build-aux/m4/l_atomic.m4
- build-aux/m4/ax_check_preproc_flag.m4
- build-aux/m4/bitcoin_find_bdb48.m4
- build-aux/m4/ax_boost_base.m4
- autogen.sh
- src/univalue/configure.ac
- configure.ac
- doc/man/Makefile.am
- src/secp256k1/Makefile.am
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/ax_boost_chrono.m4
- src/univalue/Makefile.am
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- build-aux/m4/ax_boost_filesystem.m4
- build-aux/m4/ax_boost_program_options.m4
- build-aux/m4/ax_boost_unit_test_framework.m4
- build-aux/m4/bitcoin_qt.m4
- src/Makefile.am
- libbitcoinconsensus.pc.in
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_check_link_flag.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- Makefile.am
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- src/univalue/autogen.sh
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- build-aux/m4/bitcoin_subdir_to_include.m4
- build-aux/m4/ax_subdirs_configure.m4
🚧 Files skipped from review as they are similar to previous changes (5)
- CMakeLists.txt
- src/clientversion.cpp
- src/clientversion.h
- doc/build-unix.md
- depends/Makefile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-31T04:27:35.223Z
Learnt from: aleflm
PR: firoorg/firo#1504
File: CMakeLists.txt:457-484
Timestamp: 2025-03-31T04:27:35.223Z
Learning: Pkg-config should not be used in the Firo CMake build system as it's not available by default on Windows, and the project aims to support Windows natively in the future.
Applied to files:
DockerfileREADME.md
🪛 markdownlint-cli2 (0.18.1)
README.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
232-232: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-windows-cmake
- GitHub Check: build-mac-cmake
- GitHub Check: build-linux-cmake
- GitHub Check: build
🔇 Additional comments (5)
README.md (2)
183-184: Output location note LGTM.
Binaries in build/bin are correctly called out.
161-164: Fix invalid toolchain path expansion: use $(./depends/config.guess) and quote path.Current examples try to expand a non-existent shell variable and will fail. Execute the script instead and quote the toolchain path.
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=OFF -DBUILD_CLI=ON-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=ON -DBUILD_CLI=ON-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON cd build && make -j$(nproc) make test#!/bin/bash # Find and review any remaining bad expansions of depends/config.guess rg -nC2 -P '\$\((?:depends\/config\.guess)\)' README.md doc --ignore-caseAlso applies to: 169-172, 177-181
Dockerfile (1)
26-29: Fix build blockers: unsupported --toolchain flag, wrong install path, and use ctest.Use -DCMAKE_TOOLCHAIN_FILE, run ctest, and install from the correct dir.
-RUN cmake -B build --toolchain depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ - cmake --build build -j$(nproc) && \ - cd build && make test && \ - cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu +RUN cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ + cmake --build build -j$(nproc) && \ + ctest --test-dir build --output-on-failure && \ + cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnusrc/CMakeLists.txt (1)
12-23: build.h generation: ensure directory, always build, and refresh on HEAD changes.Without mkdir, the command fails if config/ doesn’t exist. Also, the target won’t re-run on new commits.
add_custom_command( OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/config/build.h - COMMAND ${CMAKE_COMMAND} + COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/config + COMMAND ${CMAKE_COMMAND} -DBUILD_INFO_HEADER_PATH=${CMAKE_CURRENT_BINARY_DIR}/config/build.h -DSOURCE_DIR=${PROJECT_SOURCE_DIR} -P ${PROJECT_SOURCE_DIR}/cmake/script/GenerateBuildInfo.cmake COMMENT "Generating build.h" VERBATIM ) -add_custom_target(generate_build_info +add_custom_target(generate_build_info ALL DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/config/build.h )Additionally, consider:
# Near the top of the file: if(EXISTS "${PROJECT_SOURCE_DIR}/.git/HEAD") set(_GIT_DEPENDS "${PROJECT_SOURCE_DIR}/.git/HEAD") endif() # Extend the custom command: add_custom_command( OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/config/build.h DEPENDS ${_GIT_DEPENDS} ... )cmake/script/GenerateBuildInfo.cmake (1)
118-121: Ensure parent directory exists and quote path before writing build.h.Without mkdir and quoting, writing can fail or mis-handle spaces.
-# Only update the header if necessary. -if(NOT "${INFO}" STREQUAL "${NEWINFO}") - file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}") -endif() +# Only update the header if necessary. +get_filename_component(_BUILD_INFO_DIR "${BUILD_INFO_HEADER_PATH}" DIRECTORY) +file(MAKE_DIRECTORY "${_BUILD_INFO_DIR}") +if(NOT "${INFO}" STREQUAL "${NEWINFO}") + file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}") +endif()
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
2 times, most recently
from
efb9126 to
7c77a1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/clientversion.h (1)
10-10: Avoid including generated build header from a public headerIncluding config/build.h from a header can cause widespread rebuilds and macro redefinition warnings. Prefer including it only in the .cpp that needs it (clientversion.cpp already does).
#if defined(HAVE_CONFIG_H) #include "config/bitcoin-config.h" -#include "config/build.h" #elsePlease confirm no other TU relies on macros from build.h via this header.
src/clientversion.cpp (1)
64-74: Minor: terminology mismatch (GIT_COMMIT vs GIT_COMMIT_ID) in commentThe prose mentions GIT_COMMIT, while code uses GIT_COMMIT_ID. Consider aligning the comment to reduce confusion.
README.md (3)
226-229: Cross-compile snippet: use cmake --build and avoid cdMinor consistency fix.
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake \ - -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON \ -cd build && make -j$(nproc) +cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake" \ + -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON +cmake --build build -j"$(nproc)"
191-203: Markdown lint nits: surround tables with blank linesAdd a blank line before and after each table to satisfy MD058.
95-103: Docs still reference Autotools flowsSeveral sections still mention ./configure flags. Consider updating to CMake equivalents (e.g., -DENABLE_WALLET=OFF, -DWITH_ZMQ=ON) or moving Autotools notes to a legacy appendix.
I can draft the CMake equivalents for the referenced configuration flags.
doc/build-unix.md (1)
1-10: Docs still contain substantial Autotools contentConsider pruning or moving Autotools-only sections (configure flags, hardening via configure, etc.) to a legacy appendix and adding CMake equivalents.
Happy to draft a concrete restructuring PR section-by-section.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (43)
CMakeLists.txt(1 hunks)Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)depends/Makefile(1 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_check_preproc_flag.m4
- libbitcoinconsensus.pc.in
- build-aux/m4/l_atomic.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- build-aux/m4/ax_subdirs_configure.m4
- Makefile.am
- build-aux/m4/ax_gcc_func_attribute.m4
- configure.ac
- build-aux/m4/ax_boost_chrono.m4
- build-aux/m4/ax_boost_base.m4
- build-aux/m4/bitcoin_qt.m4
- build-aux/m4/bitcoin_subdir_to_include.m4
- doc/man/Makefile.am
- build-aux/m4/ax_boost_thread.m4
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- build-aux/m4/ax_pthread.m4
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_boost_filesystem.m4
- autogen.sh
- build-aux/m4/bitcoin_find_bdb48.m4
- build-aux/m4/ax_boost_system.m4
- src/secp256k1/Makefile.am
- src/univalue/configure.ac
- src/univalue/Makefile.am
- src/Makefile.am
- build-aux/m4/ax_check_link_flag.m4
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/ax_boost_program_options.m4
- build-aux/m4/ax_check_compile_flag.m4
- src/secp256k1/configure.ac
- build-aux/m4/ax_boost_unit_test_framework.m4
- src/univalue/autogen.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- CMakeLists.txt
- src/CMakeLists.txt
- depends/Makefile
- cmake/script/GenerateBuildInfo.cmake
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-31T04:27:35.223Z
Learnt from: aleflm
PR: firoorg/firo#1504
File: CMakeLists.txt:457-484
Timestamp: 2025-03-31T04:27:35.223Z
Learning: Pkg-config should not be used in the Firo CMake build system as it's not available by default on Windows, and the project aims to support Windows natively in the future.
Applied to files:
Dockerfiledoc/build-unix.mdREADME.md
🪛 markdownlint-cli2 (0.18.1)
doc/build-unix.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
266-266: Images should have alternate text (alt text)
(MD045, no-alt-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-mac-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: build-linux-cmake
- GitHub Check: build
🔇 Additional comments (12)
src/clientversion.cpp (2)
41-41: LGTM: include path switched to config/build.hConsistent with the new CMake-generated build info.
57-58: Version string format regressed: restore “-g” before commit hash or update docs/testsComment (Lines 34–37) still says v[maj].[min].[rev].[build]-g[commit], but the macro dropped “-g”. Either restore it or update the comment and any consumers that expect the legacy format.
-#define BUILD_DESC_FROM_COMMIT(maj, min, rev, build, commit) \ - "v" DO_STRINGIZE(maj) "." DO_STRINGIZE(min) "." DO_STRINGIZE(rev) "." DO_STRINGIZE(build) "-" commit +#define BUILD_DESC_FROM_COMMIT(maj, min, rev, build, commit) \ + "v" DO_STRINGIZE(maj) "." DO_STRINGIZE(min) "." DO_STRINGIZE(rev) "." DO_STRINGIZE(build) "-g" commit#!/bin/bash # Verify there are no other definitions and detect downstream assumptions. set -euo pipefail rg -nP -C2 '\bBUILD_DESC_FROM_COMMIT\b' src || true rg -nP -C2 'CLIENT_BUILD|FormatFullVersion|FormatSubVersion' src || true rg -nP -C2 --type=md --type=txt '\-g[0-9a-f]{6,}' || trueREADME.md (5)
110-113: LGTM: updated Linux deps include cmakeMatches the CMake migration.
133-136: LGTM: macOS prerequisites noteGood pointer to platform doc.
161-164: Fix toolchain path and use cmake/ctest idioms$(depends/config.guess) is a shell variable, not a command. Also prefer cmake --build and ctest.
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ --DBUILD_GUI=OFF -DBUILD_CLI=ON -cd build && make -j$(nproc) +cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ + -DBUILD_GUI=OFF -DBUILD_CLI=ON +cmake --build build -j"$(nproc)"
169-172: Apply same fixes in GUI build snippetUse executable config.guess and cmake --build.
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ - -DBUILD_GUI=ON -DBUILD_CLI=ON -cd build && make -j$(nproc) +cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ + -DBUILD_GUI=ON -DBUILD_CLI=ON +cmake --build build -j"$(nproc)"
176-181: Tests: switch to ctest and fix toolchain invocationKeep the toolchain path fix and use ctest.
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ - -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON -cd build && make -j$(nproc) -make test +cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ + -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON +cmake --build build -j"$(nproc)" +ctest --test-dir build --output-on-failuredoc/build-unix.md (4)
55-55: LGTM: updated Ubuntu/Debian deps include cmakeMatches migration.
87-87: LGTM: Fedora deps include cmake and patchConsistent with build steps.
30-35: Use -DCMAKE_TOOLCHAIN_FILE and modern cmake/ctest invocations--toolchain is non-portable; also prefer cmake --build and ctest.
-cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -cmake --build build -j$(nproc) -cmake --install build # optional +cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" +cmake --build build -j"$(nproc)" +ctest --test-dir build --output-on-failure +# optional +cmake --install build
1-1: No MD045/MD058 issues in doc/build-unix.md The file contains neither images nor tables, so those rules do not apply.Likely an incorrect or invalid review comment.
Dockerfile (1)
26-29: Fix build blockers: replace --toolchain, correct install path, and run tests via ctest--toolchain isn’t portable; and cmake --install build after cd build targets build/build. Prefer -DCMAKE_TOOLCHAIN_FILE and ctest.
-RUN cmake -B build --toolchain depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ - cmake --build build -j$(nproc) && \ - cd build && make test && \ - cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu +RUN cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ + cmake --build build -j"$(nproc)" && \ + ctest --test-dir build --output-on-failure && \ + cmake --install build --prefix "/tmp/firo/depends/$(./depends/config.guess)"
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
5 times, most recently
from
4f3e6eb to
8170607
Compare
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
from
8170607 to
8769f25
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
183-188: Fix inconsistent binary path reference in GUI run example.Line 183 correctly states binaries are generated in
build/bin/, but the GUI run example on line 188 references./bin/firo-qt, which is inconsistent. This will cause the command to fail if users follow the documented output location.Apply this diff to fix the path:
- ./bin/firo-qt + ./build/bin/firo-qt
♻️ Duplicate comments (7)
doc/build-unix.md (3)
30-35: Use -DCMAKE_TOOLCHAIN_FILE and ctestReplace non-portable flag and modernize testing.
[ suggest_recommended_refactor ]-make -j$(nproc) +make -j"$(nproc)" cd .. -cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -cmake --build build -j$(nproc) -cmake --install build # optional +cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" +cmake --build build -j"$(nproc)" +ctest --test-dir build --output-on-failure +# cmake --install build # optional
205-208: Arch example: same toolchain/test fixesAlign with the new flow.
[ suggest_recommended_refactor ]- cd depends && make -j$(nproc) && cd .. - cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF - cmake --build build -j$(nproc) - cd build && make test + cd depends && make -j"$(nproc)" && cd .. + cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" -DENABLE_WALLET=OFF -DBUILD_GUI=OFF + cmake --build build -j"$(nproc)" + ctest --test-dir build --output-on-failure
266-268: FreeBSD snippet: drop --toolchain and duplicate -jAvoid passing -j twice; prefer a single parallel spec.
[ suggest_recommended_refactor ]- cd depends && gmake -j$(nproc) && cd .. - cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake - cmake --build build -j$(nproc) -- -j$(nproc) + cd depends && gmake -j"$(nproc)" && cd .. + cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" + cmake --build build -j"$(nproc)"cmake/script/GenerateBuildInfo.cmake (1)
118-121: Create parent dir and quote path before writing build.hPrevents write failures when config/ doesn’t exist and handles spaces.
[ raise_major_issue ]# Only update the header if necessary. if(NOT "${INFO}" STREQUAL "${NEWINFO}") - file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}") + get_filename_component(_BUILD_INFO_DIR "${BUILD_INFO_HEADER_PATH}" DIRECTORY) + file(MAKE_DIRECTORY "${_BUILD_INFO_DIR}") + file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}") endif()src/clientversion.cpp (1)
57-59: Restore “-g” before short commit in version string or update comment/consumersKeep legacy format vX.Y.Z.W-g.
[ raise_minor_issue ]-#define BUILD_DESC_FROM_COMMIT(maj, min, rev, build, commit) \ - "v" DO_STRINGIZE(maj) "." DO_STRINGIZE(min) "." DO_STRINGIZE(rev) "." DO_STRINGIZE(build) "-" commit +#define BUILD_DESC_FROM_COMMIT(maj, min, rev, build, commit) \ + "v" DO_STRINGIZE(maj) "." DO_STRINGIZE(min) "." DO_STRINGIZE(rev) "." DO_STRINGIZE(build) "-g" commitIf the new format is intentional, update the descriptive comment and any tests/docs accordingly.
Dockerfile (1)
26-29: Fix build blockers: replace --toolchain, correct test/install pathsUse -DCMAKE_TOOLCHAIN_FILE and avoid install-from-nested build/build. Also prefer ctest.
-RUN cmake -B build --toolchain depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ - cmake --build build -j$(nproc) && \ - cd build && make test && \ - cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu +RUN cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(uname -m)-linux-gnu/toolchain.cmake" -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ + cmake --build build -j"$(nproc)" && \ + ctest --test-dir build --output-on-failure && \ + cmake --install build --prefix "/tmp/firo/depends/$(uname -m)-linux-gnu"README.md (1)
161-161: Fix invalid command substitution for toolchain file path (still unresolved from prior review).Lines 161, 169, and 177 all use
$(depends/config.guess)which attempts to expand a non-existent shell variable. This should execute the script via command substitution:$(./depends/config.guess). This was flagged in prior review cycles but remains unaddressed.Apply this diff to fix all three occurrences:
- cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ + cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \Repeat this fix for lines 169 and 177.
Also applies to: 169-169, 177-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (42)
CMakeLists.txt(1 hunks)Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_check_compile_flag.m4
- src/univalue/autogen.sh
- build-aux/m4/ax_boost_system.m4
- src/secp256k1/Makefile.am
- build-aux/m4/bitcoin_qt.m4
- build-aux/m4/ax_boost_filesystem.m4
- build-aux/m4/ax_boost_thread.m4
- Makefile.am
- build-aux/m4/ax_gcc_func_attribute.m4
- autogen.sh
- src/univalue/configure.ac
- doc/man/Makefile.am
- build-aux/m4/l_atomic.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- build-aux/m4/ax_check_preproc_flag.m4
- src/secp256k1/configure.ac
- configure.ac
- src/univalue/Makefile.am
- build-aux/m4/bitcoin_subdir_to_include.m4
- build-aux/m4/ax_boost_program_options.m4
- build-aux/m4/ax_subdirs_configure.m4
- build-aux/m4/bitcoin_find_bdb48.m4
- build-aux/m4/ax_boost_chrono.m4
- build-aux/m4/ax_pthread.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/ax_boost_base.m4
- build-aux/m4/ax_boost_unit_test_framework.m4
- libbitcoinconsensus.pc.in
- build-aux/m4/ax_check_link_flag.m4
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- src/Makefile.am
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- src/clientversion.h
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
174-174: Hard tabs
Column: 1
(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-linux-cmake
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-mac-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: build
- GitHub Check: Create Source Distribution
🔇 Additional comments (5)
src/CMakeLists.txt (2)
12-23: Quote -D args and type-hint paths for robustnessPrevent breakage with spaces and ensure correct value types.
[ suggest_recommended_refactor ]add_custom_command( OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/config/build.h - COMMAND ${CMAKE_COMMAND} - -DBUILD_INFO_HEADER_PATH=${CMAKE_CURRENT_BINARY_DIR}/config/build.h - -DSOURCE_DIR=${PROJECT_SOURCE_DIR} + COMMAND ${CMAKE_COMMAND} + -D BUILD_INFO_HEADER_PATH:FILEPATH="${CMAKE_CURRENT_BINARY_DIR}/config/build.h" + -D SOURCE_DIR:PATH="${PROJECT_SOURCE_DIR}" -P ${PROJECT_SOURCE_DIR}/cmake/script/GenerateBuildInfo.cmake COMMENT "Generating build.h" VERBATIM ) add_custom_target(generate_build_info DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/config/build.h )
39-66: Avoid recompiling clientversion.cpp; consume OBJECTs insteadCompile clientversion.cpp once in bitcoin_clientversion and reuse it in bitcoin_util.
[ suggest_recommended_refactor ]add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_batchverifier.h ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_ies.cpp ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_ies.h ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_worker.cpp ${CMAKE_CURRENT_SOURCE_DIR}/bls/bls_worker.h ${CMAKE_CURRENT_SOURCE_DIR}/chainparamsbase.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/clientversion.cpp ${CMAKE_CURRENT_SOURCE_DIR}/compat/glibc_sanity.cpp ${CMAKE_CURRENT_SOURCE_DIR}/compat/glibcxx_sanity.cpp ${CMAKE_CURRENT_SOURCE_DIR}/compat/strnlen.cpp ${CMAKE_CURRENT_SOURCE_DIR}/crypto/scrypt.cpp ${CMAKE_CURRENT_SOURCE_DIR}/fs.cpp ${CMAKE_CURRENT_SOURCE_DIR}/mbstring.cpp ${CMAKE_CURRENT_SOURCE_DIR}/policy/rbf.cpp ${CMAKE_CURRENT_SOURCE_DIR}/primitives/block.cpp ${CMAKE_CURRENT_SOURCE_DIR}/random.cpp ${CMAKE_CURRENT_SOURCE_DIR}/rpc/protocol.cpp ${CMAKE_CURRENT_SOURCE_DIR}/support/cleanse.cpp ${CMAKE_CURRENT_SOURCE_DIR}/support/lockedpool.cpp ${CMAKE_CURRENT_SOURCE_DIR}/sync.cpp ${CMAKE_CURRENT_SOURCE_DIR}/threadinterrupt.cpp ${CMAKE_CURRENT_SOURCE_DIR}/util.cpp ${CMAKE_CURRENT_SOURCE_DIR}/utilmoneystr.cpp ${CMAKE_CURRENT_SOURCE_DIR}/utilstrencodings.cpp ${CMAKE_CURRENT_SOURCE_DIR}/utiltime.cpp + $<TARGET_OBJECTS:bitcoin_clientversion> )src/clientversion.cpp (1)
41-41: Good change: include generated header via config/Include path matches CMake’s output location.
README.md (2)
110-110: LGTM!The cmake dependency has been correctly added to both Debian/Ubuntu and Redhat/Fedora package lists, aligning with the shift from Autotools to CMake.
Also applies to: 121-121
204-230: LGTM!The cross-compilation section is well-structured, with clear host target examples and a practical usage workflow. The explicit host targets in the CMake invocation (line 226) correctly avoid the shell variable expansion issue seen elsewhere.
| 4. *(optional)* It is recommended to build and run the unit tests: | ||
|
|
There was a problem hiding this comment.
Replace hard tabs with spaces to comply with markdown linting rules.
Lines 174–175 contain hard tab characters (flagged by markdownlint MD010). Replace these with spaces to maintain consistent markdown formatting.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
174-174: Hard tabs
Column: 1
(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🤖 Prompt for AI Agents
In README.md around lines 174–175, there are hard tab characters causing
markdownlint MD010; replace the hard tabs with spaces (use consistent
indentation, e.g., 4 spaces per tab) on those lines, scan the file for any other
hard tabs and replace them as well, and re-run the markdown linter to verify the
issue is resolved.
| ``` | ||
|
|
||
| ### Notes | ||
| * The toolchain path in `CMAKE_TOOLCHAIN_FILE`must match your target architecture. |
There was a problem hiding this comment.
Minor: Add missing space in prose.
Line 232 reads: "toolchain path in CMAKE_TOOLCHAIN_FILEmust match". Add a space between the code block and the word "must".
- * The toolchain path in `CMAKE_TOOLCHAIN_FILE`must match your target architecture.
+ * The toolchain path in `CMAKE_TOOLCHAIN_FILE` must match your target architecture.📝 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.
| * The toolchain path in `CMAKE_TOOLCHAIN_FILE`must match your target architecture. | |
| * The toolchain path in `CMAKE_TOOLCHAIN_FILE` must match your target architecture. |
🤖 Prompt for AI Agents
In README.md around line 232, there is a missing space in the sentence
"toolchain path in `CMAKE_TOOLCHAIN_FILE`must match"; insert a single space
between the closing backtick and the word "must" so it reads
"`CMAKE_TOOLCHAIN_FILE` must match" to correct the prose.
3b40b93 to
2df7319
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
cmake/script/GenerateBuildInfo.cmake (1)
118-121: Quote the path and ensure parent directory exists before writing.The
file(WRITE)command will fail if the parent directoryconfig/doesn't exist in the binary directory. Additionally, the path should be quoted to handle spaces or special characters safely.Apply this diff:
# Only update the header if necessary. +get_filename_component(_BUILD_INFO_DIR "${BUILD_INFO_HEADER_PATH}" DIRECTORY) +file(MAKE_DIRECTORY "${_BUILD_INFO_DIR}") if(NOT "${INFO}" STREQUAL "${NEWINFO}") - file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}") + file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}") endif()Based on past review comments.
doc/build-unix.md (3)
32-32: Use-DCMAKE_TOOLCHAIN_FILEinstead of the non-portable--toolchainflag.The
--toolchainoption is not widely supported across CMake versions. Use the standard-DCMAKE_TOOLCHAIN_FILEvariable instead.Apply this diff:
-cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake +cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmakeBased on past review comments.
206-208: Fix toolchain flag and use CMake-native test invocation.Two issues: (1)
--toolchainshould be-DCMAKE_TOOLCHAIN_FILE, and (2)cd build && make testshould usectestfor consistency with CMake workflow.Apply this diff:
- cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF + cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF cmake --build build -j$(nproc) - cd build && make test + ctest --test-dir build --output-on-failureBased on past review comments.
266-268: Fix toolchain flag and remove duplicate parallelism specification.Two issues: (1)
--toolchainshould be-DCMAKE_TOOLCHAIN_FILE, and (2)-j$(nproc)appears twice, which is redundant.Apply this diff:
- cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake - cmake --build build -j$(nproc) -- -j$(nproc) + cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmake + cmake --build build -j$(nproc)Based on past review comments.
README.md (4)
161-162: Fix invalid command substitution syntax.
$(depends/config.guess)attempts to expand a shell variable that doesn't exist. It should execute the script using$(./depends/config.guess).Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=OFF -DBUILD_CLI=ONBased on past review comments.
169-170: Same fix: use correct command substitution.Same issue as above - the script must be executed with
$(./depends/config.guess).Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=ON -DBUILD_CLI=ONBased on past review comments.
177-178: Apply same command substitution fix.Same shell syntax issue - use
$(./depends/config.guess)to execute the script.Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ONBased on past review comments.
232-232: Add missing space in documentation.Missing space between the closing backtick and "must".
Apply this diff:
- * The toolchain path in `CMAKE_TOOLCHAIN_FILE`must match your target architecture. + * The toolchain path in `CMAKE_TOOLCHAIN_FILE` must match your target architecture.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (42)
CMakeLists.txt(1 hunks)Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_check_preproc_flag.m4
- configure.ac
- build-aux/m4/ax_gcc_func_attribute.m4
- src/secp256k1/configure.ac
- Makefile.am
- src/univalue/Makefile.am
- build-aux/m4/ax_subdirs_configure.m4
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/ax_boost_base.m4
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- build-aux/m4/ax_boost_filesystem.m4
- build-aux/m4/ax_boost_thread.m4
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- build-aux/m4/bitcoin_subdir_to_include.m4
- build-aux/m4/ax_boost_unit_test_framework.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- build-aux/m4/ax_boost_system.m4
- build-aux/m4/l_atomic.m4
- build-aux/m4/ax_pthread.m4
- doc/man/Makefile.am
- build-aux/m4/bitcoin_qt.m4
- build-aux/m4/ax_check_compile_flag.m4
- build-aux/m4/ax_check_link_flag.m4
- libbitcoinconsensus.pc.in
- src/univalue/configure.ac
- autogen.sh
- build-aux/m4/ax_boost_program_options.m4
- src/univalue/autogen.sh
- build-aux/m4/ax_boost_chrono.m4
- build-aux/m4/bitcoin_find_bdb48.m4
- src/Makefile.am
- src/secp256k1/Makefile.am
🚧 Files skipped from review as they are similar to previous changes (4)
- CMakeLists.txt
- src/clientversion.h
- Dockerfile
- src/clientversion.cpp
🔇 Additional comments (4)
cmake/script/GenerateBuildInfo.cmake (2)
103-107: LGTM: Well-formed header guards.The multi-line header block with include guards is correctly structured for the GIT_TAG case.
109-113: LGTM: Consistent with GIT_TAG formatting.The BUILD_GIT_COMMIT block follows the same well-formed header guard pattern.
doc/build-unix.md (1)
55-55: LGTM: Build dependencies correctly updated for CMake.The package lists appropriately reflect the migration from Autotools to CMake.
Also applies to: 87-87
src/CMakeLists.txt (1)
12-23: ****CMake's
file(WRITE ...)command automatically creates parent directories for output files. TheGenerateBuildInfo.cmakescript usesfile(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}")which will create theconfig/directory as needed without explicitfile(MAKE_DIRECTORY)calls. The code as-is is correct and will succeed on clean builds.Likely an incorrect or invalid review comment.
| cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake \ | ||
| -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON \ | ||
| cd build && make -j$(nproc) |
There was a problem hiding this comment.
Add missing line continuation and quote the path.
Line 226 appears to be missing a line continuation backslash, and the toolchain path should be quoted.
Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake \
- -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON \
+cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake" \
+ -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON
cd build && make -j$(nproc)📝 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.
| cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake \ | |
| -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON \ | |
| cd build && make -j$(nproc) | |
| cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake" \ | |
| -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON | |
| cd build && make -j$(nproc) |
🤖 Prompt for AI Agents
In README.md around lines 226 to 228, the cmake command is missing a trailing
backslash for line continuation and the toolchain path should be quoted; add a
backslash at the end of the line that sets -DBUILD_GUI=ON so the cmake arguments
span multiple lines correctly, wrap the
$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake path in quotes to handle
spaces, and ensure the subsequent cd build && make -j$(nproc) remains on a
separate line after the cmake invocation.
2df7319 to
c4a4c2e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
Dockerfile (1)
32-35: Fix install path after directory change.Line 34 changes to the
builddirectory, then line 35 runscmake --install build, which attempts to install from a non-existent nestedbuild/builddirectory. This will cause the install to fail.Apply this diff to fix the install path:
RUN cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ cmake --build build -j$(nproc) && \ cd build && make test && \ - cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu + cmake --install . --prefix /tmp/firo/depends/$(uname -m)-linux-gnuAlternatively, run the install before
cd build:RUN cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ cmake --build build -j$(nproc) && \ + cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu && \ cd build && make test && \ - cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnucmake/script/GenerateBuildInfo.cmake (1)
118-121: Create parent directory and quote paths before writing.Writing to
${BUILD_INFO_HEADER_PATH}will fail if the parent directory (e.g.,config/) doesn't exist. Additionally, the path should be quoted to handle spaces safely.Apply this diff:
+# Ensure parent directory exists and write only if content changed. +get_filename_component(_BUILD_INFO_DIR "${BUILD_INFO_HEADER_PATH}" DIRECTORY) +file(MAKE_DIRECTORY "${_BUILD_INFO_DIR}") + # Only update the header if necessary. if(NOT "${INFO}" STREQUAL "${NEWINFO}") - file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}") + file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}") endif()README.md (4)
161-164: Fix command substitution: script must be executed, not expanded as variable.Line 161 uses
$(depends/config.guess)which attempts to expand a non-existent shell variable. The script must be executed with$(./depends/config.guess). This issue was marked as addressed in a previous review but remains unfixed.Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=OFF -DBUILD_CLI=ON
169-172: Same fix needed: use executable command substitution.Same issue as line 161—use
$(./depends/config.guess)instead of$(depends/config.guess).Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=ON -DBUILD_CLI=ON cd build && make -j$(nproc)
226-228: Fix line continuation and quote the toolchain path.Line 227 is missing proper line continuation syntax, and the toolchain path should be quoted for safety. Previous review flagged this.
Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake \ - -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake" \ + -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON cd build && make -j$(nproc)
232-233: Fix markdown formatting: add missing space and adjust list indentation.Line 232 is missing a space between the inline code and "must". Lines 232-233 have incorrect list indentation (MD007).
Apply this diff:
-### Notes - * The toolchain path in `CMAKE_TOOLCHAIN_FILE`must match your target architecture. - * `BUILD_TX` is automatically enabled if `BUILD_CLI=ON` is enabled. +### Notes + +* The toolchain path in `CMAKE_TOOLCHAIN_FILE` must match your target architecture. +* `BUILD_TX` is automatically enabled if `BUILD_CLI=ON` is enabled.doc/build-unix.md (3)
30-35: Replace deprecated --toolchain flag with standard CMake variable.The
--toolchainflag isn't widely supported in distro CMake versions. Use-DCMAKE_TOOLCHAIN_FILEinstead. This issue was marked as addressed in a previous review but remains unfixed.Apply this diff:
cd depends make -j$(nproc) cd .. -cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake +cmake -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" cmake --build build -j$(nproc) cmake --install build # optional
205-209: Fix Arch Linux example: use standard CMake flag and fix command substitution.Same issues: deprecated
--toolchainflag and incorrect command substitution$(depends/config.guess).Apply this diff:
cd depends && make -j$(nproc) && cd .. -cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF +cmake -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" -DENABLE_WALLET=OFF -DBUILD_GUI=OFF cmake --build build -j$(nproc) cd build && make test
266-269: Fix FreeBSD example: use standard CMake flag and fix command substitution.Same issues in the FreeBSD build example.
Apply this diff:
cd depends && gmake -j$(nproc) && cd .. -cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake +cmake -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" cmake --build build -j$(nproc) -- -j$(nproc)Note: The duplicate
-j$(nproc)on line 268 should also be removed (one fromcmake --buildis sufficient), but that's a separate optional improvement.
🧹 Nitpick comments (2)
Dockerfile (1)
18-22: Remove duplicate package declarations.Lines 18-22 duplicate packages already declared earlier:
patchis new, butm4,autoconf,automake, andlibtoolare already listed on lines 6-7, 14-15. Remove the duplicates to clean up the Dockerfile.Apply this diff:
pkg-config \ - patch \ - m4 \ - autoconf \ - automake \ - libtool + patchREADME.md (1)
207-214: Add blank line around table for markdown compliance.The table starting at line 207 should be surrounded by blank lines per MD058.
Apply this diff:
#### Usage Example: 1. Build dependencies: + ```bash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (42)
CMakeLists.txt(1 hunks)Dockerfile(2 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- src/secp256k1/configure.ac
- build-aux/m4/ax_gcc_func_attribute.m4
- build-aux/m4/l_atomic.m4
- build-aux/m4/ax_boost_program_options.m4
- build-aux/m4/ax_boost_chrono.m4
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_boost_filesystem.m4
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_boost_unit_test_framework.m4
- build-aux/m4/bitcoin_subdir_to_include.m4
- src/secp256k1/Makefile.am
- build-aux/m4/ax_pthread.m4
- build-aux/m4/ax_check_compile_flag.m4
- build-aux/m4/ax_boost_system.m4
- doc/man/Makefile.am
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- build-aux/m4/ax_boost_thread.m4
- autogen.sh
- src/univalue/autogen.sh
- Makefile.am
- build-aux/m4/bitcoin_find_bdb48.m4
- src/univalue/Makefile.am
- libbitcoinconsensus.pc.in
- src/univalue/configure.ac
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/ax_subdirs_configure.m4
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- configure.ac
- src/Makefile.am
- build-aux/m4/bitcoin_qt.m4
- build-aux/m4/ax_boost_base.m4
- build-aux/m4/ax_check_link_flag.m4
- build-aux/m4/ax_check_preproc_flag.m4
🚧 Files skipped from review as they are similar to previous changes (3)
- src/CMakeLists.txt
- src/clientversion.h
- CMakeLists.txt
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
232-232: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
doc/build-unix.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
266-266: Images should have alternate text (alt text)
(MD045, no-alt-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-linux-cmake
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-mac-cmake
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-windows-cmake
- GitHub Check: build
🔇 Additional comments (3)
src/clientversion.cpp (2)
41-41: LGTM: Include path updated to match new build system.The include path correctly references
config/build.h, aligning with the CMake-based build-info generation flow introduced in this PR.
57-58: Verify version format: macro doesn't match header comment.The header comment on line 35 states the format should be
v[maj].[min].[rev].[build]-g[commit], but this macro producesv[maj].[min].[rev].[build]-[commit](missing the "g" prefix). A previous review flagged this as addressed, but the code still shows the discrepancy.Confirm whether:
- The format change (removing "-g") is intentional, in which case update the comment on line 35, or
- The "-g" should be restored to maintain the documented format
If the format change is intentional, apply this diff to update the comment:
- * * if not, but GIT_COMMIT is defined, use v[maj].[min].[rev].[build]-g[commit] + * * if not, but GIT_COMMIT is defined, use v[maj].[min].[rev].[build]-[commit]If "-g" should be restored, apply this diff to the macro:
#define BUILD_DESC_FROM_COMMIT(maj, min, rev, build, commit) \ - "v" DO_STRINGIZE(maj) "." DO_STRINGIZE(min) "." DO_STRINGIZE(rev) "." DO_STRINGIZE(build) "-" commit + "v" DO_STRINGIZE(maj) "." DO_STRINGIZE(min) "." DO_STRINGIZE(rev) "." DO_STRINGIZE(build) "-g" commitREADME.md (1)
110-110: LGTM: Dependencies updated for CMake build.Package list correctly includes
cmakeand removes Autotools-specific dependencies, aligning with the PR's migration to CMake.
Update docs Update dockerfile to use cmake instead of autotools Remove autotools targets Fix Github CI hash, and fix build.h generation process. Fix README.md
aleflm
force-pushed
the
dev/aleflm/remove-autotools
branch
from
c4a4c2e to
37c8dd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
cmake/script/GenerateBuildInfo.cmake (1)
118-121: Ensure parent directory exists before writing the header.The script writes to
${BUILD_INFO_HEADER_PATH}(config/build.h) without ensuring its parent directory exists, which will fail during the first build. Additionally, the path should be quoted for safety.Apply this diff to fix the issue:
+# Ensure parent directory exists and write only if content changed. +get_filename_component(_BUILD_INFO_DIR "${BUILD_INFO_HEADER_PATH}" DIRECTORY) +file(MAKE_DIRECTORY "${_BUILD_INFO_DIR}") # Only update the header if necessary. if(NOT "${INFO}" STREQUAL "${NEWINFO}") - file(WRITE ${BUILD_INFO_HEADER_PATH} "${NEWINFO}") + file(WRITE "${BUILD_INFO_HEADER_PATH}" "${NEWINFO}") endif()README.md (4)
169-172: Same fix needed: use $(./depends/config.guess) for command execution.Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=ON -DBUILD_CLI=ON
161-164: Fix invalid command substitution syntax.The command uses
$(depends/config.guess)which attempts to expand a non-existent shell variable. It should execute the script with$(./depends/config.guess).Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ -DBUILD_GUI=OFF -DBUILD_CLI=ON
232-233: Fix prose spacing and list indentation.Two minor formatting issues:
- Missing space between backtick and "must" on line 232
- List items should not be indented (MD007)
Apply this diff:
### Notes - * The toolchain path in `CMAKE_TOOLCHAIN_FILE`must match your target architecture. - * `BUILD_TX` is automatically enabled if `BUILD_CLI=ON` is enabled. + * The toolchain path in `CMAKE_TOOLCHAIN_FILE` must match your target architecture. + * `BUILD_TX` is automatically enabled if `BUILD_CLI=ON` is enabled.
226-228: Fix line continuation and quote the toolchain path.The cmake command is missing a line continuation backslash after the -DBUILD_GUI=ON line, and the toolchain path should be quoted.
Apply this diff:
-cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake \ - -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON \ +cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/x86_64-w64-mingw32/toolchain.cmake" \ + -DBUILD_TESTS=ON -DBUILD_CLI=ON -DBUILD_GUI=ON cd build && make -j$(nproc)doc/build-unix.md (3)
266-269: Fix FreeBSD example: use standard CMake toolchain flag.The
--toolchainflag is non-standard and should be replaced with-DCMAKE_TOOLCHAIN_FILE. Also, the double-jspecification is redundant.Apply this diff:
cd depends && gmake -j$(nproc) && cd .. - cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake - cmake --build build -j$(nproc) -- -j$(nproc) + cmake -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" + cmake --build build -j$(nproc)
32-35: Use standard -DCMAKE_TOOLCHAIN_FILE flag instead of --toolchain.The
--toolchainflag is not supported by many CMake versions. Use the standard-DCMAKE_TOOLCHAIN_FILEvariable instead. Additionally, the command substitution is invalid.Apply this diff:
-cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake +cmake -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" cmake --build build -j$(nproc) cmake --install build # optional
205-209: Fix Arch Linux example: use standard CMake flags and command substitution.Two critical issues:
- The
--toolchainflag should be-DCMAKE_TOOLCHAIN_FILE$(depends/config.guess)should be$(./depends/config.guess)Apply this diff:
cd depends && make -j$(nproc) && cd .. - cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake -DENABLE_WALLET=OFF -DBUILD_GUI=OFF + cmake -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake" -DENABLE_WALLET=OFF -DBUILD_GUI=OFF cmake --build build -j$(nproc) cd build && make testDockerfile (1)
32-35: Fix incorrect install path after directory change.After
cd buildon line 34, the commandcmake --install buildon line 35 attempts to install frombuild/build, which doesn't exist. This will cause the build to fail.Apply this diff:
RUN cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(uname -m)-linux-gnu/toolchain.cmake -DBUILD_GUI=OFF -DBUILD_TESTS=ON && \ cmake --build build -j$(nproc) && \ - cd build && make test && \ - cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu + ctest --test-dir build --output-on-failure && \ + cmake --install build --prefix /tmp/firo/depends/$(uname -m)-linux-gnu
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (42)
CMakeLists.txt(1 hunks)Dockerfile(3 hunks)Makefile.am(0 hunks)README.md(5 hunks)autogen.sh(0 hunks)build-aux/m4/ax_boost_base.m4(0 hunks)build-aux/m4/ax_boost_chrono.m4(0 hunks)build-aux/m4/ax_boost_filesystem.m4(0 hunks)build-aux/m4/ax_boost_program_options.m4(0 hunks)build-aux/m4/ax_boost_system.m4(0 hunks)build-aux/m4/ax_boost_thread.m4(0 hunks)build-aux/m4/ax_boost_unit_test_framework.m4(0 hunks)build-aux/m4/ax_check_compile_flag.m4(0 hunks)build-aux/m4/ax_check_link_flag.m4(0 hunks)build-aux/m4/ax_check_preproc_flag.m4(0 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(0 hunks)build-aux/m4/ax_gcc_func_attribute.m4(0 hunks)build-aux/m4/ax_pthread.m4(0 hunks)build-aux/m4/ax_subdirs_configure.m4(0 hunks)build-aux/m4/bitcoin_find_bdb48.m4(0 hunks)build-aux/m4/bitcoin_qt.m4(0 hunks)build-aux/m4/bitcoin_subdir_to_include.m4(0 hunks)build-aux/m4/l_atomic.m4(0 hunks)cmake/script/GenerateBuildInfo.cmake(2 hunks)configure.ac(0 hunks)doc/build-unix.md(5 hunks)doc/man/Makefile.am(0 hunks)libbitcoinconsensus.pc.in(0 hunks)src/CMakeLists.txt(1 hunks)src/Makefile.am(0 hunks)src/clientversion.cpp(3 hunks)src/clientversion.h(1 hunks)src/secp256k1/Makefile.am(0 hunks)src/secp256k1/build-aux/m4/ax_jni_include_dir.m4(0 hunks)src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4(0 hunks)src/secp256k1/build-aux/m4/bitcoin_secp.m4(0 hunks)src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4(0 hunks)src/secp256k1/configure.ac(0 hunks)src/univalue/Makefile.am(0 hunks)src/univalue/autogen.sh(0 hunks)src/univalue/build-aux/m4/.gitignore(0 hunks)src/univalue/configure.ac(0 hunks)
💤 Files with no reviewable changes (34)
- build-aux/m4/ax_boost_base.m4
- Makefile.am
- src/Makefile.am
- doc/man/Makefile.am
- build-aux/m4/ax_cxx_compile_stdcxx.m4
- build-aux/m4/ax_subdirs_configure.m4
- build-aux/m4/bitcoin_subdir_to_include.m4
- build-aux/m4/ax_boost_unit_test_framework.m4
- build-aux/m4/ax_boost_system.m4
- build-aux/m4/ax_gcc_func_attribute.m4
- src/secp256k1/build-aux/m4/m4_ax_cxx_compile_stdcxx.m4
- src/univalue/autogen.sh
- libbitcoinconsensus.pc.in
- src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
- build-aux/m4/ax_check_compile_flag.m4
- build-aux/m4/ax_boost_chrono.m4
- build-aux/m4/ax_pthread.m4
- src/univalue/build-aux/m4/.gitignore
- build-aux/m4/bitcoin_qt.m4
- build-aux/m4/ax_boost_thread.m4
- src/secp256k1/build-aux/m4/bitcoin_secp.m4
- build-aux/m4/ax_check_link_flag.m4
- build-aux/m4/l_atomic.m4
- src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
- build-aux/m4/ax_check_preproc_flag.m4
- configure.ac
- build-aux/m4/ax_boost_program_options.m4
- src/secp256k1/Makefile.am
- build-aux/m4/bitcoin_find_bdb48.m4
- src/secp256k1/configure.ac
- autogen.sh
- src/univalue/configure.ac
- src/univalue/Makefile.am
- build-aux/m4/ax_boost_filesystem.m4
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- src/clientversion.cpp
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
232-232: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-mac-cmake
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-linux-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: build
🔇 Additional comments (2)
src/CMakeLists.txt (1)
12-23: Well-structured two-step build info generation.The new custom command and target correctly implement the build information generation flow, ensuring config/build.h is produced before downstream targets consume it.
src/clientversion.h (1)
10-10: Correct integration with new build header path.The include directive properly references the generated config/build.h header, aligning with the CMake-based build info generation changes.
| patch \ | ||
| m4 \ | ||
| autoconf \ | ||
| automake \ | ||
| libtool |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate dependency declarations.
Lines 18-22 duplicate dependencies already declared on lines 6-7 and 14 (m4, autoconf, automake, libtool). This is redundant and makes maintenance harder.
Apply this diff to remove duplicates:
pkg-config \
- patch \
- m4 \
- autoconf \
- automake \
- libtool
+ patch📝 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.
| patch \ | |
| m4 \ | |
| autoconf \ | |
| automake \ | |
| libtool | |
| patch |
🤖 Prompt for AI Agents
In Dockerfile around lines 18 to 22, the dependency list contains duplicate
entries (m4, autoconf, automake, libtool) that are already declared on lines 6-7
and 14; remove the redundant block at lines 18-22 so each dependency is declared
only once, keeping one canonical list (prefer the existing earlier declarations)
and ensure package ordering/line breaks remain valid after removal.
| 4. *(optional)* It is recommended to build and run the unit tests: | ||
|
|
||
| --- | ||
|
|
||
| ### Build Instructions | ||
|
|
||
| #### 1. Build Dependencies | ||
| ```bash | ||
| cd depends | ||
| make -j$(nproc) | ||
| cd .. | ||
| ```sh | ||
| cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ | ||
| -DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON | ||
| cd build && make -j$(nproc) | ||
| make test | ||
| ``` |
There was a problem hiding this comment.
Multiple issues in test build example.
This code segment has two issues:
- Hard tab characters on lines 174-175 (flagged by markdownlint MD010)
- Invalid command substitution
$(depends/config.guess)on line 177
Apply this diff to fix both issues:
- ```sh
- cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \
+```sh
+cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \
-DBUILD_TESTS=ON -DBUILD_GUI=OFF -DBUILD_CLI=ON
cd build && make -j$(nproc)
make test
<details>
<summary>🤖 Prompt for AI Agents</summary>
In README.md around lines 174 to 181, fix the test build example by removing the
hard tab characters on lines 174–175 and correcting the invalid command
substitution on line 177; replace the tab-indented fenced code block with a
proper ```sh fenced block, and change the toolchain path to use
"$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" (quote the whole
path) so shell substitution is valid and spaces are handled safely.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| To build for other platforms, specify the `HOST` variable when building dependencies: | ||
| | Host Target | Platform | | ||
| |--------------------------|---------------------------| | ||
| | `x86_64-pc-linux-gnu` | Linux 64-bit (default) | | ||
| | `x86_64-w64-mingw32` | Windows 64-bit | | ||
| | `aarch64-apple-darwin` | macOS | | ||
| | `arm-linux-gnueabihf` | Linux ARM 32-bit | | ||
| | `aarch64-linux-gnu` | Linux ARM 64-bit | | ||
|
|
There was a problem hiding this comment.
Add blank line before table to comply with markdown standards.
Markdownlint MD058 requires tables to be surrounded by blank lines.
Apply this diff:
To build for other platforms, specify the `HOST` variable when building dependencies:
+
| Host Target | Platform |
|--------------------------|---------------------------|📝 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.
| To build for other platforms, specify the `HOST` variable when building dependencies: | |
| | Host Target | Platform | | |
| |--------------------------|---------------------------| | |
| | `x86_64-pc-linux-gnu` | Linux 64-bit (default) | | |
| | `x86_64-w64-mingw32` | Windows 64-bit | | |
| | `aarch64-apple-darwin` | macOS | | |
| | `arm-linux-gnueabihf` | Linux ARM 32-bit | | |
| | `aarch64-linux-gnu` | Linux ARM 64-bit | | |
| To build for other platforms, specify the `HOST` variable when building dependencies: | |
| | Host Target | Platform | | |
| |--------------------------|---------------------------| | |
| | `x86_64-pc-linux-gnu` | Linux 64-bit (default) | | |
| | `x86_64-w64-mingw32` | Windows 64-bit | | |
| | `aarch64-apple-darwin` | macOS | | |
| | `arm-linux-gnueabihf` | Linux ARM 32-bit | | |
| | `aarch64-linux-gnu` | Linux ARM 64-bit | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
In README.md around lines 206 to 214 the markdown table is not surrounded by
blank lines which violates MD058; add a single blank line immediately before the
table (and one after the table) so the table is preceded and followed by empty
lines to comply with markdownlint.
3 participants
PR intention
Update docs
Update Dockerfile to use CMake instead of autotools
Remove autotools targets
Fix GitHub CI hash, and fix the build.h generation process.