Remove all the files related to auto tools #1676
Conversation
WalkthroughReplaces the Autotools-based build system with a CMake-based workflow: removes many Autotools/Automake/m4 files, updates Dockerfile and docs to use cmake, changes GenerateBuildInfo to emit guarded Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / CI
participant CMake as CMake
participant Gen as GenerateBuildInfo
participant Build as Build Engine (ninja/make)
participant Test as Test Runner (ctest)
participant Install as Installer
Dev->>CMake: cmake -B build --toolchain <toolchain> -D... -DBUILD_GUI=OFF -DBUILD_TESTS=ON
CMake->>Gen: invoke add_custom_command -> produce `config/build.h`
Gen-->>CMake: write guarded header only if changed (no trailing newline)
Dev->>Build: cmake --build build -j$(nproc)
Build->>Build: compile targets (generate_build_info runs before core targets)
Dev->>Test: ctest --test-dir build or cd build && make test
Test-->>Dev: results
Dev->>Install: cmake --install build --prefix <depends>
Install-->>Dev: installed artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (43)
💤 Files with no reviewable changes (34)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-03-31T04:27:35.223ZApplied to files:
🪛 markdownlint-cli2 (0.18.1)README.md174-174: Hard tabs (MD010, no-hard-tabs) 175-175: Hard tabs (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)
🔇 Additional comments (9)
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.
| 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
| cmake -B build -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$(depends/config.guess)/toolchain.cmake \ | ||
| -DBUILD_GUI=ON -DBUILD_CLI=ON | ||
| cd build && make -j$(nproc) | ||
| ``` |
There was a problem hiding this comment.
Same fix: use $(./depends/config.guess) in GUI build 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)📝 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/$(depends/config.guess)/toolchain.cmake \ | |
| -DBUILD_GUI=ON -DBUILD_CLI=ON | |
| cd build && make -j$(nproc) | |
| ``` | |
| cmake -B build -DCMAKE_TOOLCHAIN_FILE="$(pwd)/depends/$(./depends/config.guess)/toolchain.cmake" \ | |
| -DBUILD_GUI=ON -DBUILD_CLI=ON | |
| cd build && make -j$(nproc) |
🤖 Prompt for AI Agents
In README.md around lines 169 to 172 the cmake GUI build snippet uses
$(depends/config.guess) which is incorrect; change it to use
$(./depends/config.guess) so the toolchain path is resolved relative to the
repository root (i.e., update the -DCMAKE_TOOLCHAIN_FILE assignment to reference
$(./depends/config.guess)/toolchain.cmake), leaving the rest of the command and
subsequent cd build && make -j$(nproc) unchanged.
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)"
| 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 | ||
|
|
There was a problem hiding this comment.
Arch example: fix toolchain flag and test invocation
Align with the new flow.
- 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-failureCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
207-207: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
In doc/build-unix.md around lines 206 to 209, the Arch example uses an incorrect
cmake flag and an outdated test invocation; replace the --toolchain option with
-DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmake and
replace the final "cd build && make test" with a CMake-native test invocation
such as "cmake --build build --target test" or "ctest --test-dir build" so it
follows the new flow.
| cd depends && gmake -j$(nproc) && cd .. | ||
| cmake -B build --toolchain depends/$(depends/config.guess)/toolchain.cmake | ||
| cmake --build build -j$(nproc) -- -j$(nproc) | ||
|
|
There was a problem hiding this comment.
FreeBSD snippet: same toolchain fix
Replace --toolchain and avoid duplicate -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 -S . -B build -DCMAKE_TOOLCHAIN_FILE="depends/$(./depends/config.guess)/toolchain.cmake"
+ cmake --build build -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.
| 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)" |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
266-266: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
In doc/build-unix.md around lines 266 to 269, the cmake invocations use the
non-standard --toolchain flag and pass -j twice; replace the --toolchain usage
with the standard
-DCMAKE_TOOLCHAIN_FILE=depends/$(depends/config.guess)/toolchain.cmake, and
change the build invocation to avoid duplicate -j by using either cmake --build
build --parallel $(nproc) or cmake --build build -- -j$(nproc) (pick one) so the
parallelism is specified only once.
6ba6ed0 to
4f3e6eb
Compare
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
4f3e6eb to
8170607
Compare
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.