Modernize GUIX and Depends #1630
Conversation
WalkthroughBuild and packaging overhaul: CMake MinGW/static options and OpenSSL link tweaks, Boost/Qt linking changes, widespread Guix modernization and reproducible-build patches, large depends tree rework (toolchains, packages), macOS deploy and signing tooling updates, and minor source fixes (endian, client version). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/CI
participant Guix as Guix scripts
participant CMake as CMake/toolchain
participant Tool as Compiler/Linker
participant Pack as Packaging (macdeploy/NSIS/zip)
Dev->>Guix: start reproducible build
Guix->>Guix: setup env (SOURCE_DATE_EPOCH, patches, toolchain)
Guix->>CMake: cmake configure (-S/-B, toolchain file, flags)
CMake->>Tool: compile & link (platform-specific)
alt MinGW AND STATIC_BUILD
CMake->>CMake: modify OpenSSL::Crypto links (remove "dl"), add static flags
end
Tool->>Pack: produce binaries
Pack->>Dev: produce artifacts (zip/nsis), collect logs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
aleflm
force-pushed
the
dev/aleflm/modernize-guix
branch
from
2b2edfc to
fe5996b
Compare
aleflm
force-pushed
the
dev/aleflm/modernize-guix
branch
from
fe5996b to
6111a06
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
cmake/module/WindowsSystemConfiguration.cmake (1)
78-83:⚠️ Potential issueGuard against the empty list to avoid an invalid
copy_if_differentinvocation
copy_if_different <file>… <dest>requires at least one source file.
If none of the requested DLLs were found, CMake will still executecmake -E copy_if_different <dest-dir>and abort with “requires at least two arguments”.
-add_custom_target(copy_mingw_dlls ALL - COMMAND ${CMAKE_COMMAND} -E make_directory "${PROJECT_BIN_DIR}" - COMMAND ${CMAKE_COMMAND} -E echo "Copying MinGW ${MINGW_ARCH}-bit runtime DLLs to ${PROJECT_BIN_DIR}" - COMMAND ${CMAKE_COMMAND} -E copy_if_different ${FOUND_DLLS} "${PROJECT_BIN_DIR}" - COMMENT "Copying required MinGW ${MINGW_ARCH}-bit DLLs to binary folder." -) +if(FOUND_DLLS) + add_custom_target(copy_mingw_dlls ALL + COMMAND ${CMAKE_COMMAND} -E make_directory "${PROJECT_BIN_DIR}" + COMMAND ${CMAKE_COMMAND} -E echo "Copying MinGW ${MINGW_ARCH}-bit runtime DLLs to ${PROJECT_BIN_DIR}" + COMMAND ${CMAKE_COMMAND} -E copy_if_different ${FOUND_DLLS} "${PROJECT_BIN_DIR}" + COMMENT "Copying required MinGW ${MINGW_ARCH}-bit DLLs to binary folder." + ) +else() + message(WARNING "No MinGW runtime DLLs could be located; skipping copy step.") +endif()
♻️ Duplicate comments (1)
contrib/guix/patches/gcc-remap-guix-store.patch (1)
13-15: 🛠️ Refactor suggestionSame scalability concern as in winpthreads patch
The one-shot inline
find /gnu/store …will replicate the entire store list intoc_flagsfor everymakeinvocation compilinglibgcc. Risks identical to those mentioned in the winpthreads patch (command length, build time).
Refactor to a cached variable or@responsefile to keep the toolchain robust.
🧹 Nitpick comments (17)
depends/packages/fontconfig.mk (1)
11-11:-D_BSD_SOURCEis obsolete – consider dropping it to avoid macro clutter
-D_DEFAULT_SOURCEalready implies the legacy_BSD_SOURCEon glibc ≥ 2.20.
Keeping both is harmless on glibc, but on non-glibc libcs (e.g. musl)_BSD_SOURCEis ignored and occasionally triggers noisy warnings. Since you are explicitly defining_DEFAULT_SOURCE, the extra macro is redundant.- $(package)_cflags+=-D_POSIX_C_SOURCE=200809L -D_DEFAULT_SOURCE -D_BSD_SOURCE + $(package)_cflags+=-D_POSIX_C_SOURCE=200809L -D_DEFAULT_SOURCEIf there is a platform that still needs
_BSD_SOURCE, please gate it behind a conditional to keep the default flags minimal.depends/patches/qt/guix-arm64-darwin-build.patch (1)
1-11: Patch silences-no_warning_for_no_symbols. Scope looks good, but guard its applicationCommenting out this flag avoids an unsupported option with LLVM’s
ranlibshipped on modern macOS.
Ensure the patch is applied only for Darwin/arm64 Guix builds; on Intel macOS the flag is still accepted and suppresses noisy warnings.src/secp256k1/CMakeLists.txt (1)
51-64: Directly rewritingINTERFACE_LINK_LIBRARIESon an imported target is brittleMutating imported targets can collide with other sub-projects that rely on the original property.
A cleaner, non-destructive alternative:if(MINGW) message(STATUS "Removing dl from OpenSSL::Crypto for MinGW") # Do not alter the original target, instead add a wrapper. add_library(OpenSSL::Crypto_NoDL INTERFACE) target_link_libraries(OpenSSL::Crypto_NoDL INTERFACE $<FILTER:$<TARGET_PROPERTY:OpenSSL::Crypto,INTERFACE_LINK_LIBRARIES>,EXCLUDE,dl> ) # Consumers can now use OpenSSL::Crypto_NoDL endif()This preserves the upstream target for any code that still expects it.
depends/builders/default.mk (1)
4-5:TOUCHvalue contains spaces – quote or export as list?
add_build_tool_funcultimately treats the variable as the tool name.
Embedding flags (-h -m -t …) means later recipes will expand:$(build_TOUCH) some/fileand run
touch -h -m -t 200001011200 some/fileThat is probably intended, but:
- If a recipe needs to call plain
touch(without flags) it can no longer rely
on$(TOUCH).- The spaces make
$(build_TOUCH)a multi-word Make variable, which can break
when it is used in places expecting a single token (e.g. inshellor
pattern substitution).Consider introducing a separate variable, e.g.
TOUCH_TIMESTAMP, and keep
TOUCHfor the raw executable path.Also applies to: 9-10, 16-16
depends/packages/zlib.mk (2)
12-19: Darwin-specific block duplicatesARand may override earlier settings
$(package)_build_optsalready containsAR="$($(package)_ar)".
The Darwin branch re-adds the same assignment:$(package)_build_opts_darwin+=AR="$($(package)_ar)"This duplication is harmless but noisy and can confuse future readers.
Also, only Darwin paths tweakARFLAGS; Linux/Windows now default to"r"(previouslylibtool -o). Confirm that static archives on non-Darwin still get ansindex for deterministic builds (rcs) if required.
31-32: Destination of variable assignments: order is fine but keep consistency
$(MAKE) $($(package)_build_opts) DESTDIR=… installworks, yet most other
package files placeDESTDIRfirst. Keeping a uniform order eases grepping and cuts cognitive load.depends/packages/qt.mk (1)
253-254: Consider a more robust approach for conditional patching.Using
[ -z "$GUIX_ENVIRONMENT" ]for conditional patch application could be fragile. Consider using a more explicit flag or build configuration option.Consider using a dedicated build flag:
- [ -z "$GUIX_ENVIRONMENT" ] || patch -p1 < $($(package)_patch_dir)/guix-arm64-darwin-build.patch && \ + $(if $(ENABLE_GUIX_ARM64_DARWIN_PATCH),patch -p1 < $($(package)_patch_dir)/guix-arm64-darwin-build.patch &&) \This would make the build configuration more explicit and controllable.
contrib/guix/patches/glibc-2.27-no-librt.patch (1)
1-14: Potentially obsolete after glibc 2.30Manifest now pulls glibc 2.32, which already carries commit 6e41ef5. Keeping the patch is harmless but wastes CI time and risks merge conflicts. Consider dropping it or guarding by version like:
[ "$GLIBC_VERSION" -lt 230 ] && apply_patch ...depends/hosts/mingw32.mk (2)
1-6: Shell invocation can be simplified & sped up
$(shell $(SHELL) $(.SHELLFLAGS) "command -v …")spawns two shells. A single call is enough:-ifneq ($(shell $(SHELL) $(.SHELLFLAGS) "command -v $(host)-gcc-posix"),) +ifneq ($(shell command -v $(host)-gcc-posix 2>/dev/null),)
25-27: CMake system version hard-codes Windows 10If you still support Windows 7/8, detect the SDK version dynamically or document the new minimum.
depends/hosts/default.mk (1)
42-43: Tool list updatedMacro expansion works, but ensure every new entry has a sane default to avoid empty
host_*variables.depends/packages/boost.mk (2)
32-34: Consider removing the patch content echo.Echoing the entire patch content may clutter build logs unnecessarily. The "Patching with..." message is sufficient.
echo "using $($(package)_toolset_$(host_os)) : : $($(package)_cxx) : <cflags>\"$($(package)_cflags)\" <cxxflags>\"$($(package)_cxxflags)\" <compileflags>\"$($(package)_cppflags)\" <linkflags>\"$($(package)_ldflags)\" <archiver>\"$($(package)_archiver_$(host_os))\" <striper>\"$(host_STRIP)\" <ranlib>\"$(host_RANLIB)\" <rc>\"$(host_WINDRES)\" : ;" > user-config.jam && cat user-config.jam && \ echo "Patching with fix_boost_jam_cross-compilation.patch" && \ - cat $($(package)_patch_dir)/fix_boost_jam_cross-compilation.patch && \ patch -p1 < $($(package)_patch_dir)/fix_boost_jam_cross-compilation.patch
42-42: Use modern command substitution syntax.Replace backticks with
$()for better readability and nesting support.- b2 -d2 -j`nproc` -d1 --prefix=$($(package)_staging_prefix_dir) $($(package)_config_opts) toolset=$($(package)_toolset_$(host_os)) --with-atomic --with-chrono --with-filesystem --with-program_options --with-system --with-thread --with-test --no-cmake-config stage + b2 -d2 -j$$(nproc) -d1 --prefix=$($(package)_staging_prefix_dir) $($(package)_config_opts) toolset=$($(package)_toolset_$(host_os)) --with-atomic --with-chrono --with-filesystem --with-program_options --with-system --with-thread --with-test --no-cmake-config stage- b2 -d2 -j`nproc` --prefix=$($(package)_staging_prefix_dir) $($(package)_config_opts) toolset=$($(package)_toolset_$(host_os)) --with-atomic --with-chrono --with-filesystem --with-program_options --with-system --with-thread --with-test --no-cmake-config install + b2 -d2 -j$$(nproc) --prefix=$($(package)_staging_prefix_dir) $($(package)_config_opts) toolset=$($(package)_toolset_$(host_os)) --with-atomic --with-chrono --with-filesystem --with-program_options --with-system --with-thread --with-test --no-cmake-config installAlso applies to: 46-46
depends/hosts/darwin.mk (1)
16-25: System toolchain usage may affect reproducibility.Using system-installed LLVM tools via
command -vsimplifies the build but could lead to inconsistencies across different build environments. Consider documenting minimum required LLVM versions.Consider adding version checks for the LLVM tools to ensure compatibility, or document the minimum required versions in the build documentation.
contrib/guix/manifest.scm (3)
44-44: Consider adding validation for system string format.The
building-onvariable assumes the system string follows a specific format with a hyphen separator. Consider adding validation or a fallback to handle unexpected system string formats gracefully.-building-on (string-append "--build=" (list-ref (string-split (%current-system) #\-) 0) "-guix-linux-gnu")) +building-on (let ((parts (string-split (%current-system) #\-))) + (if (null? parts) + "--build=unknown-guix-linux-gnu" + (string-append "--build=" (car parts) "-guix-linux-gnu")))
229-231: Remove outdated comment about glibc version.The comment states "Can be removed when we are building 2.32 or later," but this package is already version 2.32. Either remove this workaround if it's no longer needed, or update the comment to reflect the actual requirement.
281-282: Consider including nss-certs for Linux targets.The
nss-certspackage is only included for mingw32 targets, but it might be beneficial for Linux builds as well, especially if any build steps require HTTPS connections.
🛑 Comments failed to post (14)
depends/packages/bls-dash.mk (1)
8-9: 💡 Verification agent
🧩 Analysis chain
CMake invocation now relies on host tool;
CMAKE_POLICY_VERSION_MINIMUMis non-standard
$(package)_dependenciesdropsnative_cmake, yet the build now calls a barecmake. The depends tool-chain containers may not have CMake on$PATH, causing a build break.
• Either addcmaketo the base builder image or keep the explicit$(build_prefix)/bin/cmake.
-DCMAKE_POLICY_VERSION_MINIMUM=3.5is not a recognised CMake cache variable (no effect, may confuse future readers). The canonical way is:cmake_policy(VERSION 3.5)inside the project’s top-level
CMakeLists.txt, or setCMAKE_POLICY_DEFAULT_CMPNNNNvariables.
- If the intention is simply “require ≥3.5”, prefer
cmake -DCMAKE_MINIMUM_REQUIRED_VERSION=3.5which is supported since CMake 3.25.- $(package)_config_opts+= -DCMAKE_POLICY_VERSION_MINIMUM=3.5 + $(package)_config_opts+= -DCMAKE_MINIMUM_REQUIRED_VERSION=3.5Also applies to: 37-38, 68-71
🏁 Script executed:
#!/bin/bash set -e echo "== Dependencies line ==" rg -n "^bls-dash_dependencies" depends/packages/bls-dash.mk || true echo echo "== Config options and policy usage ==" rg -n "_config_opts" depends/packages/bls-dash.mk || true rg -n "CMAKE_POLICY_VERSION_MINIMUM" depends/packages/bls-dash.mk || trueLength of output: 1565
Ensure explicit CMake dependency and use standard policy invocation
Your depends/packages/bls-dash.mk directly calls a bare
cmake(line 70) but does not declare any CMake host‐tool dependency. On minimal host images this will break if CMake isn’t in$PATH. Also, the-DCMAKE_POLICY_VERSION_MINIMUM=3.5flag (line 37) is not a valid CMake cache variable.• Add
native_cmaketo the dependency list:- $(package)_dependencies = gmp + $(package)_dependencies = gmp native_cmake• Replace the non‐standard policy flag by moving the requirement into your top‐level CMakeLists.txt:
cmake_minimum_required(VERSION 3.5)• If you really need to enforce via a CLI option on CMake ≥ 3.25, you can instead do:
- $(package)_config_opts+= -DCMAKE_POLICY_VERSION_MINIMUM=3.5 + $(package)_config_opts+= -DCMAKE_MINIMUM_REQUIRED_VERSION=3.5—but note this only works on CMake 3.25 or newer.
🤖 Prompt for AI Agents
In depends/packages/bls-dash.mk around lines 8 to 9 and also lines 37-38 and 68-71, the build calls a bare cmake but does not declare native_cmake as a host-tool dependency, which can cause build failures on minimal images without cmake in PATH. Add native_cmake to the $(package)_dependencies to ensure cmake is available. Also, remove the invalid -DCMAKE_POLICY_VERSION_MINIMUM=3.5 flag from the cmake command line and instead enforce the minimum required CMake version by adding cmake_minimum_required(VERSION 3.5) in the top-level CMakeLists.txt file. If enforcing via CLI is necessary and CMake ≥ 3.25 is guaranteed, use -DCMAKE_MINIMUM_REQUIRED_VERSION=3.5 instead.depends/packages/zeromq.mk (1)
16-19:
⚠️ Potential issueVariable is overwritten; backup is lost and quoting is off
makeassigns the same variable twice, so only the second line survives.
Additionally, single$expands duringmake, not in the spawned shell, so the value becomes empty.-$(package)_config_env_x86_64_darwin = LIB_LIBRARY_BACKUP=$LIBRARY_PATH -$(package)_config_env_x86_64_darwin = LIBRARY_PATH="" +$(package)_config_env_x86_64_darwin = \ + LIB_LIBRARY_BACKUP=$$LIBRARY_PATH \ + LIBRARY_PATH=This retains the backup and clears
LIBRARY_PATHduring configuration.📝 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.ifdef GUIX_ENVIRONMENT $(package)_config_env_x86_64_darwin = \ LIB_LIBRARY_BACKUP=$$LIBRARY_PATH \ LIBRARY_PATH= endif🤖 Prompt for AI Agents
In depends/packages/zeromq.mk around lines 16 to 19, the variable $(package)_config_env_x86_64_darwin is assigned twice, causing the first assignment to be overwritten and losing the backup of LIBRARY_PATH. Also, single $ signs cause premature expansion in make. To fix this, combine the assignments into one line using proper shell syntax and escape $ by doubling it (use $$) so that LIBRARY_PATH is backed up and then cleared correctly during configuration.CMakeLists.txt (1)
462-477: 🛠️ Refactor suggestion
Avoid acting on a non-existent
INTERFACE_LINK_LIBRARIESproperty
get_target_property()returns the literal stringNOTFOUNDwhen the property
doesn’t exist. The current test treats that as truthy and continues,
resulting in_openssl_libs=NOTFOUNDand finally:list(REMOVE_ITEM _openssl_libs "dl") # no-op, still "NOTFOUND" set_target_properties(OpenSSL::Crypto … INTERFACE_LINK_LIBRARIES "NOTFOUND")which erases all link libraries.
- if(_openssl_libs) + if(_openssl_libs AND NOT _openssl_libs STREQUAL "NOTFOUND")This keeps the original list intact when the property is absent.
📝 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.# Find OpenSSL library find_package(OpenSSL 1.1.1 REQUIRED) if(MINGW) message(STATUS "Patching OpenSSL::Crypto INTERFACE_LINK_LIBRARIES for MinGW") # Retrieve the current INTERFACE_LINK_LIBRARIES property (as a list) get_target_property(_openssl_libs OpenSSL::Crypto INTERFACE_LINK_LIBRARIES) if(_openssl_libs AND NOT _openssl_libs STREQUAL "NOTFOUND") message(STATUS "Before patch: ${_openssl_libs}") # Remove the 'dl' element from the list list(REMOVE_ITEM _openssl_libs "dl") message(STATUS "After patch: ${_openssl_libs}") set_target_properties(OpenSSL::Crypto PROPERTIES INTERFACE_LINK_LIBRARIES "${_openssl_libs}" ) endif() endif()🤖 Prompt for AI Agents
In CMakeLists.txt around lines 462 to 477, the check for the INTERFACE_LINK_LIBRARIES property of OpenSSL::Crypto incorrectly treats the value NOTFOUND as valid, causing the property to be overwritten with NOTFOUND and losing all link libraries. Fix this by explicitly checking that the property value is not NOTFOUND before proceeding with the list removal and set_target_properties call, ensuring the patch only applies if the property actually exists.contrib/guix/patches/winpthreads-remap-guix-store.patch (1)
13-15: 🛠️ Refactor suggestion
Very large
-ffile-prefix-mapexpansion may hit ARG_MAX / slowdown
$(shell find /gnu/store …)injects one-ffile-prefix-mapper every store item.
On typical Guix build hosts this can exceed thousands of tokens, inflating every compile invocation and risking:
- command-line length overflow (ARG_MAX) on some builders,
- noticeably slower
configure/makecycles,- noisy log output that drowns real warnings.
Consider generating the flag list once into a make variable (or an
@file) and re-using it, or explore a leaner mapping strategy (e.g. a wrapper script, GCCspecs, or pruning the list to packages that actually appear in the build path).🤖 Prompt for AI Agents
In contrib/guix/patches/winpthreads-remap-guix-store.patch around lines 13 to 15, the current approach appends a large number of -ffile-prefix-map flags by running a find command inline, which can exceed ARG_MAX and slow down builds. To fix this, generate the list of -ffile-prefix-map flags once and store it in a make variable or an external file, then reference that variable or file in AM_CFLAGS instead of running the find command each time. Alternatively, reduce the list to only relevant store paths or use a wrapper script or GCC specs to apply the mappings more efficiently.contrib/guix/patches/glibc-guix-prefix.patch (1)
13-14: 🛠️ Refactor suggestion
Repeated enumeration of
/gnu/store/*inflates glibc build flagsThe inline back-tick expansion inserts all store directories into every object file’s
CFLAGS-.o. Glibc already pushes the upper limits of command size; this change can break exotic builders or slower shells.A central variable, response file, or
specsoverlay (once per toolchain) would achieve the same reproducibility goal without bloating every compile command.🤖 Prompt for AI Agents
In contrib/guix/patches/glibc-guix-prefix.patch at lines 13-14, the current approach appends a large list of /gnu/store directories to CFLAGS-.o for every object file, causing command line bloat and potential build failures. Instead, refactor to define a central variable or use a response file or specs overlay to apply the -ffile-prefix-map flags once per toolchain invocation, avoiding repeated enumeration in each compile command and reducing command size.contrib/guix/patches/glibc-2.27-guix-prefix.patch (1)
18-18: 🛠️ Refactor suggestion
Use modern command substitution syntax and consider performance impact.
The patch uses legacy backticks for command substitution. Additionally, executing
findduring each compilation could impact build performance.Consider using modern syntax and potentially caching the result:
-+CFLAGS-.o += `find /gnu/store -maxdepth 1 -mindepth 1 -type d -exec echo -n " -ffile-prefix-map={}=/usr" \;` ++CFLAGS-.o += $$(find /gnu/store -maxdepth 1 -mindepth 1 -type d -exec echo -n " -ffile-prefix-map={}=/usr" \; 2>/dev/null)Alternatively, consider computing this value once during configure time to avoid repeated filesystem traversals during the build.
📝 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.+CFLAGS-.o += $$(find /gnu/store -maxdepth 1 -mindepth 1 -type d -exec echo -n " -ffile-prefix-map={}=/usr" \; 2>/dev/null)🤖 Prompt for AI Agents
In contrib/guix/patches/glibc-2.27-guix-prefix.patch at line 18, replace the legacy backtick command substitution with the modern $(...) syntax for better readability and maintainability. Additionally, to avoid performance issues caused by running the find command during each compilation, modify the build process to compute and cache the output of the find command once during the configure phase or before the build starts, then reuse this cached value in the CFLAGS-.o assignment.depends/hosts/linux.mk (2)
43-45: 💡 Verification agent
🧩 Analysis chain
Verify the minimum kernel version requirement.
The CMake system version is set to 3.17.0. Ensure this aligns with the documented minimum kernel requirements.
Run the following script to check if the minimum kernel version is documented:
🏁 Script executed:
#!/bin/bash # Description: Check for documented minimum kernel version requirements # Search for kernel version references in documentation fd -e md -e txt | xargs rg -i "kernel.*3\.17|minimum.*kernel|linux.*3\.17" -A 2 -B 2 # Check if doc/dependencies.md exists and contains kernel information if [ -f "doc/dependencies.md" ]; then echo "=== Checking doc/dependencies.md for kernel requirements ===" cat "doc/dependencies.md" | grep -i -A 5 -B 5 "kernel\|linux.*version" fiLength of output: 119
🏁 Script executed:
#!/bin/bash # List all Markdown files to locate dependencies documentation echo "== Markdown files in repository ==" fd -e md # Specifically look for dependencies.md under common doc directories echo "== Searching for dependencies.md ==" fd -e md | grep -i dependencies.md || echo "No dependencies.md found in markdown files" # If found, show its top contents to verify kernel info if [ -f "doc/dependencies.md" ]; then echo "=== Head of doc/dependencies.md ===" head -n 30 doc/dependencies.md elif [ -f "docs/dependencies.md" ]; then echo "=== Head of docs/dependencies.md ===" head -n 30 docs/dependencies.md else echo "== No dependencies.md under doc/ or docs/ ==" fiLength of output: 4525
Fix documentation reference and verify minimum kernel version
The comment in depends/hosts/linux.mk (lines 43–45) refers to a non-existent doc/dependencies.md. Please:
- Update the path to the correct dependencies doc (for example
depends/README.mdordepends/description.md).- Ensure that the
linux_cmake_system_version=3.17.0matches the minimum kernel version stated in that documentation, or adjust one of them for consistency.🤖 Prompt for AI Agents
In depends/hosts/linux.mk around lines 43 to 45, the comment references a non-existent doc/dependencies.md file. Update this comment to point to the correct dependencies documentation file, such as depends/README.md or depends/description.md. Additionally, verify that the linux_cmake_system_version value of 3.17.0 matches the minimum kernel version specified in that documentation, and if there is a discrepancy, adjust either the version in this file or the documented minimum kernel version to ensure consistency.
16-20: 💡 Verification agent
🧩 Analysis chain
Comprehensive debug mode configuration.
The debug flags enable both GNU libstdc++ and LLVM libc++ debug modes, providing thorough runtime checking. Note that
_LIBCPP_HARDENING_MODE_DEBUGrequires a recent libc++ version.Verify that the libc++ version in use supports the hardening mode:
🌐 Web query:
What is the minimum libc++ version that supports _LIBCPP_HARDENING_MODE_DEBUG?💡 Result:
The
_LIBCPP_HARDENING_MODE_DEBUGmacro is supported starting from libc++ version 18.0.0. This version introduced a new debug mode, replacing the legacy debug mode that was removed in the LLVM 17 release. (reviews.llvm.org)In this context, the
_LIBCPP_HARDENING_MODE_DEBUGmacro enables the debug mode, which includes all available checks in the library, such as heuristic checks with significant performance overhead and internal library assertions. This mode is intended for non-production environments like test suites, continuous integration, or local development. (libcxx.llvm.org)Therefore, to utilize
_LIBCPP_HARDENING_MODE_DEBUG, ensure you are using libc++ version 18.0.0 or later.Citations:
- 1: https://reviews.llvm.org/D159454?utm_source=openai
- 2: https://libcxx.llvm.org/Hardening.html?utm_source=openai
Add libc++ version requirement for hardening mode
Please ensure the
_LIBCPP_HARDENING_MODE_DEBUGflag is only enabled when building against libc++ v18.0.0 or later. Otherwise, builds will break on older libc++ releases.• File: depends/hosts/linux.mk
Lines: 16–20Suggested update:
# https://libcxx.llvm.org/Hardening.html -# enables all checks; requires recent libc++ +# enables all checks; requires libc++ ≥ 18.0.0 for _LIBCPP_HARDENING_MODE_DEBUG linux_debug_CPPFLAGS+=-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUGCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In depends/hosts/linux.mk around lines 16 to 20, the _LIBCPP_HARDENING_MODE_DEBUG flag is set unconditionally, but it requires libc++ version 18.0.0 or later. To fix this, add a conditional check to detect the libc++ version being used and only append the -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG flag if the version is 18.0.0 or newer. This prevents build failures on older libc++ versions that do not support this macro.depends/packages/packages.mk (2)
17-18: 🛠️ Refactor suggestion
Darwin native toolchain wiped
darwin_native_packagesis now empty. If dmg-signing or libtapi are still needed for notarised macOS releases, re-add them conditionally; otherwise confirm CI still produces a valid.dmg.🤖 Prompt for AI Agents
In depends/packages/packages.mk around lines 17 to 18, the darwin_native_packages variable has been cleared, removing tools like dmg-signing and libtapi. Review whether these tools are still required for notarized macOS releases; if so, re-add them conditionally to darwin_native_packages. If not needed, verify that the CI pipeline still successfully produces a valid .dmg file without these tools.
1-1:
⚠️ Potential issue
packagesstill lists removed components
gmp,zlib, andopensslwere dropped elsewhere in the PR, but they are still referenced here.make dependswill fail because their corresponding*.mkfiles are gone.-packages:=boost openssl libevent gmp zlib backtrace tor bls-dash +packages:=boost libevent backtrace tor bls-dash📝 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.packages:=boost libevent backtrace tor bls-dash🤖 Prompt for AI Agents
In depends/packages/packages.mk at line 1, the packages variable includes gmp, zlib, and openssl, which were removed elsewhere in the PR. Remove these three packages from the list to prevent make depends from failing due to missing corresponding *.mk files.depends/hosts/default.mk (1)
31-37:
⚠️ Potential issueFlag-inheritance logic change may discard user flags
When
$1(e.g.CFLAGS) is user-defined, the release-specific variant is cleared:$(host_arch)_$(host_os)_$(release_type)_$1 =This wipes
-O2/-gset later, potentially leading to unoptimised release binaries. Consider appending instead of resetting.🤖 Prompt for AI Agents
In depends/hosts/default.mk around lines 31 to 37, the release-specific variable for user-defined flags is being reset to empty, which discards important flags like -O2 or -g. Instead of clearing the release-specific variable with an equals sign, change the assignment to append the existing value by using += so that user flags are preserved and combined properly.contrib/guix/libexec/build.sh (1)
126-126: 🛠️ Refactor suggestion
Fix shellcheck warning for better error handling.
Declare and assign separately to avoid masking potential errors from the find commands.
- export LD_LIBRARY_PATH="$(find /gnu/store -maxdepth 1 -name "*zlib*" | sort | head -n 1)/lib:$(find /gnu/store -maxdepth 1 -name "*gcc-11*-lib" | sort | head -n 1)/lib" + ZLIB_PATH="$(find /gnu/store -maxdepth 1 -name "*zlib*" | sort | head -n 1)" + GCC_LIB_PATH="$(find /gnu/store -maxdepth 1 -name "*gcc-11*-lib" | sort | head -n 1)" + export LD_LIBRARY_PATH="${ZLIB_PATH}/lib:${GCC_LIB_PATH}/lib"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In contrib/guix/libexec/build.sh at line 126, the export statement combines multiple commands which can mask errors from the find commands. To fix this, separate the declaration and assignment of LD_LIBRARY_PATH into multiple steps: first assign the results of each find command to variables, check for errors if needed, then combine these variables into LD_LIBRARY_PATH and export it. This approach improves error handling and clarity.contrib/guix/manifest.scm (2)
198-198: 💡 Verification agent
🧩 Analysis chain
Consider using a release tag for better version tracking.
Using a commit hash makes it harder to identify the exact glibc release. Consider using a release tag if available for better clarity and maintenance.
🌐 Web query:
What is the official glibc 2.32 release tag or commit hash?💡 Result:
The official release of glibc 2.32 was announced on August 6, 2020[3]. However, neither the official glibc release page nor the Sourceware release branch documentation explicitly provides a single "release tag" or commit hash labeled as glibc-2.32 in the master canonical repository[1][2]. The branch structure for the 2.32 series is documented as
release/2.32/masterwithin the glibc repository[1].A specific commit is referenced in the release documentation for a regression fix (commit
07ed32f920f0bcb1ddb400e4ed606104756dee32), but this is not the release tag or the main commit marking the 2.32 release—rather, it is a later fix cherry-picked into the 2.32 branch[1].Summary Table
Information Details Release Date 2020-08-05 (official, per release branch)[1] Branch Name release/2.32/master[1] Official Release Tag Not explicitly provided in documentation[1] Example Fix Commit 07ed32f920f0bcb1ddb400e4ed606104756dee32 (not release tag)[1] Conclusion
There is no explicit "official release tag" or commit hash for glibc 2.32 listed in the official Sourceware release documentation—just the branch
release/2.32/masteras the rolling release branch[1]. The release itself is marked by the branch's state and official announcement, not a specific tag or commit on the mainline. If you need the exact source code for the release, you should use the branchrelease/2.32/masterfrom the official glibc repository[1].Citations:
- 1: https://sourceware.org/glibc/wiki/Release/2.32
- 2: https://sourceware.org/glibc/wiki/Release
- 3: https://lwn.net/Articles/828210/
- 4: actions/actions-sync#95
- 5: https://github.com/sgerrand/alpine-pkg-glibc/releases
- 6: https://github.com/NenX/glibc-2.32
- 7: GoogleContainerTools/distroless#1342
- 8: https://launchpad.net/ubuntu/+source/glibc/2.32-0ubuntu5
- 9: containers/toolbox#529
- 10: https://code.launchpad.net/glibc
Reference the official
release/2.32/masterbranch for glibc 2.32Glibc 2.32 does not have a standalone “glibc-2.32” tag; upstream tracks it on the
release/2.32/masterbranch. Please update the manifest to fetch from that branch (e.g.(branch "release/2.32/master")) instead of pinning a raw commit hash, for clearer version tracking.🤖 Prompt for AI Agents
In contrib/guix/manifest.scm at line 198, replace the hardcoded commit hash for glibc 2.32 with a reference to the official release branch "release/2.32/master". This means changing the fetch specification to use the branch name instead of a specific commit hash, improving clarity and maintainability by tracking the official release branch rather than a single commit.
129-130:
⚠️ Potential issueAdd bounds checking for target string parsing.
The substring operation assumes the target contains a hyphen. This could throw an error if the target format is unexpected.
- (machine (substring target 0 (string-index target #\-))) + (machine (let ((dash-pos (string-index target #\-))) + (if dash-pos + (substring target 0 dash-pos) + target)))🤖 Prompt for AI Agents
In contrib/guix/manifest.scm around lines 129 to 130, the substring operation extracting the machine part from the target string assumes the target contains a hyphen, which can cause an error if the hyphen is missing. Add bounds checking by verifying that the string-index call returns a valid position before using it in substring. If the hyphen is not found, handle the case gracefully, for example by using the entire target string or providing a default value.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
contrib/guix/manifest.scm (1)
128-151:⚠️ Potential issuePotential typo in
make-mingw-pthreads-cross-toolchainInside the
let*, the variable is defined aspthreads-xlibc, but later in thecross-gcccall the keyword is#:libc pthreads-xlibc(correct) whereas in the propagated inputs list you use back-quoted,pthreads‐xgcc "lib"` but do not propagate the pthreads libc itself. Unless intentional, you probably want:- `(,pthreads-xgcc "lib"))) + pthreads-xlibc ; headers & libs + `(,pthreads-xgcc "lib")))
🧹 Nitpick comments (7)
contrib/guix/libexec/build.sh (4)
77-83: SC2155: avoid masking$?when assigningNATIVE_GCC
NATIVE_GCC="$(store_path gcc-toolchain)"suppresses the previous exit status.
Split declaration and assignment to preserve$?and silence shellcheck.-NATIVE_GCC="$(store_path gcc-toolchain)" +NATIVE_GCC=$(store_path gcc-toolchain)
176-178:/usr/binmutations require root and break non-privileged containersCreating
/usr/binand symlinkingenvwill fail in an un-privileged Guix container and is unnecessary because/usr/bin/envis already available inside build containers created withguix shell --container.
Consider removing the block or guarding it with a check for write permission.
295-304: Darwin SDK discovery is brittle
find "${BASEPREFIX}/SDKs" -type d -name "*extracted*" -o -name "*.sdk" | head -1
returns the first match in lexical order, which is non-deterministic if more than one SDK exists. Prefer a deterministic strategy (e.g. pick the newest version withsort -V, or require$SDK_PATHto be set by the caller).
238-240: Unused variables reported by shellcheck
DEPENDS_PACKAGESandDEPENDS_CACHEare computed but never referenced.
If they are only for debugging, prefix them with_or remove them to avoid confusion.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 238-238: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 239-239: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
contrib/guix/manifest.scm (3)
115-122: Patch helpers: minor naming slip
binutils-mingw-patcheswraps unaligned default; the helper name reads fine, but the parameter shadowing (binutils) is unused – consider renaming the formal argument to_or using it for clarity.
97-104: Forward reference tolinux-base-gccis safe but brittle
make-bitcoin-cross-toolchaindefaults tolinux-base-gccwhich is defined later in the file. Guile’s top-level mutually-recursive bindings handle this, yet future refactors (moving definitions to modules) may break. Consider defining the base toolchains first or passing them explicitly from the call sites.
44-45:building-onembeds host triple but not architecture
building-onis hard-coded to “guix-linux-gnu”. Cross-building on non-x86_64 hosts (e.g. aarch64) will pass an invalid--buildvalue to GCC configure. Use%current-systemunchanged or derive the full triplet.
📜 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 (3)
contrib/guix/libexec/build.sh(9 hunks)contrib/guix/libexec/prelude.bash(1 hunks)contrib/guix/manifest.scm(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/guix/libexec/prelude.bash
🧰 Additional context used
🪛 Shellcheck (0.10.0)
contrib/guix/libexec/build.sh
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 238-238: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 239-239: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-mac-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: build-linux-cmake
🔇 Additional comments (1)
contrib/guix/manifest.scm (1)
224-236:❓ Verification inconclusive
set-etc-rpc-installation-directoryworkaround is now obsolete for glibc 2.32The phase comment says “Can be removed when we are building 2.32 or later.” – but the package version has just been bumped to 2.32. Keeping the patch adds maintenance burden and risks diverging from upstream.
Please verify the build without this phase and drop it if no longer required.
🏁 Script executed:
#!/bin/bash set -e # Search for glibc version bump to 2.32 in the repository rg -n "2\\.32" .Length of output: 522
Drop obsolete RPC installation workaround for glibc 2.32+
We’ve bumped the base‐libc to glibc-2.32 (see manifest.scm lines 105, 197–201), which means upstream now installs the RPC database under
$sysconfdir/rpcby default. The customset-etc-rpc-installation-directoryphase (contrib/guix/manifest.scm, lines 224–236) can therefore be removed:• File: contrib/guix/manifest.scm
• Remove the entire(add-before 'configure 'set-etc-rpc-installation-directory …)block at lines 224–236Please drop this phase and verify that the Guix build succeeds without it.
| # Log the depends build ids | ||
| make -C depends --no-print-directory HOST="$HOST" print-final_build_id_long | tr ':' '\n' > ${LOGDIR}/depends-hashes.txt | ||
| # Build the depends tree, overriding variables that assume multilib gcc | ||
| make -C depends --jobs="$JOBS" HOST="$HOST" \ | ||
| ${V:+V=1} \ | ||
| ${SOURCES_PATH+SOURCES_PATH="$SOURCES_PATH"} \ | ||
| ${BASE_CACHE+BASE_CACHE="$BASE_CACHE"} \ | ||
| ${SDK_PATH+SDK_PATH="$SDK_PATH"} \ | ||
| i686_linux_CC=i686-linux-gnu-gcc \ | ||
| i686_linux_CXX=i686-linux-gnu-g++ \ | ||
| i686_linux_AR=i686-linux-gnu-ar \ | ||
| i686_linux_RANLIB=i686-linux-gnu-ranlib \ | ||
| i686_linux_NM=i686-linux-gnu-nm \ | ||
| i686_linux_STRIP=i686-linux-gnu-strip \ | ||
| OUTDIR="$OUTDIR" \ | ||
| LOGDIR="$LOGDIR" \ | ||
| x86_64_linux_CC=x86_64-linux-gnu-gcc \ | ||
| x86_64_linux_CXX=x86_64-linux-gnu-g++ \ | ||
| x86_64_linux_AR=x86_64-linux-gnu-ar \ | ||
| x86_64_linux_RANLIB=x86_64-linux-gnu-ranlib \ | ||
| x86_64_linux_NM=x86_64-linux-gnu-nm \ | ||
| x86_64_linux_STRIP=x86_64-linux-gnu-strip \ | ||
| qt_config_opts_i686_linux='-platform linux-g++ -xplatform bitcoin-linux-g++' \ | ||
| qt_config_opts_x86_64_linux='-platform linux-g++ -xplatform bitcoin-linux-g++' \ | ||
| FORCE_USE_SYSTEM_CLANG=1 | ||
|
|
||
| x86_64_linux_AR=x86_64-linux-gnu-gcc-ar \ | ||
| x86_64_linux_RANLIB=x86_64-linux-gnu-gcc-ranlib \ | ||
| x86_64_linux_NM=x86_64-linux-gnu-gcc-nm \ | ||
| x86_64_linux_STRIP=x86_64-linux-gnu-strip | ||
|
|
There was a problem hiding this comment.
LOGDIR is used before it is guaranteed to exist / be defined
make -C depends … > ${LOGDIR}/depends-hashes.txt assumes $LOGDIR is non-empty and the directory exists. If the caller forgets to export LOGDIR, the redirection expands to /depends-hashes.txt and the build fails with Permission denied.
Add a sanity check upfront or create the directory:
: "${LOGDIR:?LOGDIR not set}"
mkdir -p "$LOGDIR"🤖 Prompt for AI Agents
In contrib/guix/libexec/build.sh around lines 220 to 236, the script uses LOGDIR
for output redirection before ensuring it is set and the directory exists, which
can cause permission errors if LOGDIR is unset or missing. Add a check at the
start of this section to verify LOGDIR is set using a parameter expansion sanity
check, then create the directory with mkdir -p "$LOGDIR" before any commands
that write to it.
| # Use a fixed timestamp for depends builds so hashes match across commits that | ||
| # don't make changes to the build system. This timestamp is only used for depends | ||
| # packages. Source archive and binary tarballs use the commit date. | ||
| export SOURCE_DATE_EPOCH=1397818193 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
SOURCE_DATE_EPOCH is overwritten later – pick one source of truth
You hard-code SOURCE_DATE_EPOCH=1397818193 for the depends build, but re-export the same variable (lines 252-255) for the source/binary archives without resetting it to the commit timestamp you mention in the comment.
If the intention is “fixed timestamp for depends, commit date for archives”, compute the latter from git and export a different variable or reassign SOURCE_DATE_EPOCH once, not twice.
-# Use a fixed timestamp for depends builds so hashes match across commits …
-export SOURCE_DATE_EPOCH=1397818193
+# Fixed timestamp for depends; will be overwritten with commit date for archives
+DEPENDS_SOURCE_DATE_EPOCH=1397818193
+export SOURCE_DATE_EPOCH="$DEPENDS_SOURCE_DATE_EPOCH"and later:
-# Use COMMIT_TIMESTAMP for the source and release binary archives
-export SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH}
+# Switch to the commit timestamp for the release artefacts
+SOURCE_DATE_EPOCH=$(git log -1 --format=%ct)
+export SOURCE_DATE_EPOCH🏁 Script executed:
#!/bin/bash
# Locate all occurrences of SOURCE_DATE_EPOCH in the build script
grep -n "SOURCE_DATE_EPOCH" contrib/guix/libexec/build.sh
# Show context around the later usage (approx. lines 240–280)
sed -n '240,280p' contrib/guix/libexec/build.shLength of output: 2041
Unify SOURCE_DATE_EPOCH handling
The script currently hard-codes SOURCE_DATE_EPOCH=1397818193 for depends builds (line 48) but never replaces it with the commit timestamp for source/binary archives—despite the comment on line 240 implying it should. As a result, both depends and archives use the same fixed timestamp.
Suggested changes:
- Introduce a separate variable for the depends timestamp.
- Compute and assign the commit timestamp to
SOURCE_DATE_EPOCHbefore generating archives.
Example diff:
--- contrib/guix/libexec/build.sh
@@ -45,7 +45,9 @@
# Use a fixed timestamp for depends builds so hashes match across commits that
# don't make changes to the build system. This timestamp is only used for depends
# packages. Source archive and binary tarballs use the commit date.
-export SOURCE_DATE_EPOCH=1397818193
+DEPENDS_SOURCE_DATE_EPOCH=1397818193
+export SOURCE_DATE_EPOCH="$DEPENDS_SOURCE_DATE_EPOCH"
+
@@ -250,7 +252,11 @@
# Source Tarball Building
###########################
-# Use COMMIT_TIMESTAMP for the source and release binary archives
-export SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH}
+# Switch to the commit date for archives
+SOURCE_DATE_EPOCH=$(git log -1 --format=%ct)
+export SOURCE_DATE_EPOCH
+
+# Update TAR_OPTIONS with the new timestamp
export TAR_OPTIONS="--owner=0 --group=0 --numeric-owner --mtime='@${SOURCE_DATE_EPOCH}' --sort=name"Locations to update:
- contrib/guix/libexec/build.sh: around lines 45–49 and 252–255
🤖 Prompt for AI Agents
In contrib/guix/libexec/build.sh around lines 45 to 49 and 252 to 255, the
script sets SOURCE_DATE_EPOCH to a fixed timestamp for depends builds but later
reassigns SOURCE_DATE_EPOCH for source/binary archives without differentiating
the two, causing both to use the same timestamp. To fix this, define a separate
variable (e.g., DEPENDS_SOURCE_DATE_EPOCH) for the fixed depends timestamp at
lines 45-49, and compute the commit timestamp from git to assign to
SOURCE_DATE_EPOCH only once before generating archives at lines 252-255. This
unifies timestamp handling and respects the intended distinction between depends
and archive builds.
6111a06 to
4a23cfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contrib/guix/libexec/build.sh (2)
45-49:⚠️ Potential issue
SOURCE_DATE_EPOCHstill never switches from the fixed depends value
Past feedback pointed out that the archive artefacts are meant to use the commit timestamp, not the hard-coded1397818193. The second block here only re-exports the same value instead of recomputing it, so both depends and release artefacts remain pinned to 2014.-# Use COMMIT_TIMESTAMP for the source and release binary archives -export SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH} +# Switch to the commit date for the source/binary archives +SOURCE_DATE_EPOCH=$(git log -1 --format=%ct) +export SOURCE_DATE_EPOCHAlso applies to: 252-255
218-222:⚠️ Potential issue
LOGDIRmay be undefined / non-existent – redirection can fail
make -C depends … > ${LOGDIR}/depends-hashes.txtassumes$LOGDIRis set and the directory exists. Failure leads to writes to//depends-hashes.txtor a permission error.+: "${LOGDIR:?LOGDIR not set}" +mkdir -p "${LOGDIR}"Ideally add this right after the required-vars banner.
🧹 Nitpick comments (4)
contrib/guix/libexec/build.sh (2)
126-127: Minor: SC2155 – avoid hidingfind’s exit status
Shellcheck flags the inline assignment because a later command failure would be masked. Harmless, but you can silence it cleanly:ld_lib_android=$(find …) export LD_LIBRARY_PATH="${ld_lib_android}/lib:…"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
238-239:DEPENDS_PACKAGES/DEPENDS_CACHEare computed but never used
Both variables trigger SC2034. If they are only for debugging, prefix with_orreadonly/export; otherwise drop them to keep the script noise-free.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 238-238: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 239-239: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
contrib/guix/manifest.scm (2)
44-44:building-onmay generate an invalid triplet for multi-component systems
(list-ref (string-split (%current-system) #\-) 0)yields just the CPU (x86_64). For e.g.aarch64-linux-gnuorpowerpc64le-linux-gnuyou’ll end up with--build=aarch64-guix-linux-gnu, which is not a recognised triplet and can break configure scripts.Consider deriving the full GNU triplet via
(gnu-build-system-triplet)or by keeping the first three components.
223-236: Obsolete RPC-directory workaround kept for glibc 2.32
Comment says “Can be removed when we are building 2.32 or later.” – but the package is already pinned to 2.32. Please confirm this phase is still necessary; if not, drop it to simplify the recipe.
📜 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 (3)
contrib/guix/libexec/build.sh(9 hunks)contrib/guix/libexec/prelude.bash(1 hunks)contrib/guix/manifest.scm(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/guix/libexec/prelude.bash
🧰 Additional context used
🪛 Shellcheck (0.10.0)
contrib/guix/libexec/build.sh
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 238-238: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 239-239: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-mac-cmake
- GitHub Check: build-linux-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: Create Source Distribution
aleflm
force-pushed
the
dev/aleflm/modernize-guix
branch
from
4a23cfa to
6dcbe97
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
contrib/guix/libexec/build.sh (2)
45-48:SOURCE_DATE_EPOCHstill hard-codes the same value for both depends and archive buildsThe earlier feedback about switching to the commit timestamp for the archive phase has not been addressed. As written, both phases continue to share
1397818193, defeating the reproducibility goal described in the comments around line 252.Also applies to: 252-254
220-222:LOGDIRmay be undefined or non-existent before redirection
make … > ${LOGDIR}/depends-hashes.txtwill fail if the caller did not exportLOGDIRor if the directory has not been created. A simple guard at the top of the script (or immediately before this block) is still required:: "${LOGDIR:?LOGDIR not set}" mkdir -p "$LOGDIR"
🧹 Nitpick comments (1)
contrib/guix/libexec/build.sh (1)
294-306: Darwin SDK discovery is brittle and can silently pick the wrong treeThe
find … -name "*extracted*" -o -name "*.sdk" | head -1heuristic:
- selects the first match, not the newest/expected one,
- happily returns
.sdkdirectories for other architectures,- fails without a diagnostic when multiple candidates exist.
Consider:
-SDK_PATH=$(find "${BASEPREFIX}/SDKs" -type d \( -name '*extracted*' -o -name '*.sdk' \) | head -1) +SDK_PATH=$(fd --type d --max-depth 2 --glob '*.sdk' "${BASEPREFIX}/SDKs" | sort -V | tail -1) +[[ -n $SDK_PATH ]] || { echo "No Darwin SDK found under ${BASEPREFIX}/SDKs" >&2; exit 1; }This is still lightweight but avoids random‐first selection and provides an explicit failure mode.
📜 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 (3)
contrib/guix/libexec/build.sh(9 hunks)contrib/guix/libexec/prelude.bash(1 hunks)contrib/guix/manifest.scm(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/guix/libexec/prelude.bash
🧰 Additional context used
🪛 Shellcheck (0.10.0)
contrib/guix/libexec/build.sh
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 238-238: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 239-239: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-mac-cmake
- GitHub Check: build-linux-cmake
- GitHub Check: build-windows-cmake
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: Create Source Distribution
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-x86_64-w64-mingw32
🔇 Additional comments (2)
contrib/guix/libexec/build.sh (1)
126-127: AndroidLD_LIBRARY_PATHresolution is fragileTwo
findinvocations pick the lexicographically first zlib / gcc-11 candidate in/gnu/store. If multiple versions are present the wrong one may be selected, and the command fails silently when none exist.Prefer a fixed store reference (via
store_path) or fail fast whenfindreturns nothing.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
contrib/guix/manifest.scm (1)
262-264: Verify thatgcc-toolchain-13exists in Guix
gcc-toolchain-13is not a standard package name in upstream Guix (current versions providegcc-toolchainand explicitly versionedgcc-13). Unless a local package module defines it, this will raise an “unknown package” error when instantiating the manifest.
| (define (make-binutils-with-mingw-w64-disable-flags xbinutils) | ||
| (package-with-extra-patches xbinutils | ||
| (search-our-patches "binutils-mingw-w64-disable-flags.patch"))) | ||
| (define building-on (string-append "--build=" (list-ref (string-split (%current-system) #\-) 0) "-guix-linux-gnu")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
building-on flag may generate an invalid triplet
(%current-system) yields e.g. x86_64-linux, producing
"--build=x86_64-guix-linux-gnu". The middle component guix is not part of any canonical GNU triplet and can confuse configure scripts that validate the value.
Recommend composing the flag from (%current-system) unchanged, or from explicit canonical-triplet helpers.
🤖 Prompt for AI Agents
In contrib/guix/manifest.scm at line 44, the building-on flag is constructed by
inserting "guix" into the system triplet, which creates a non-standard GNU
triplet that may confuse configure scripts. To fix this, modify the code to use
the original (%current-system) string directly without inserting "guix", or use
a proper canonical triplet helper function to generate a valid triplet string
for the --build flag.
6dcbe97 to
291fb6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
contrib/guix/libexec/build.sh (2)
218-223:⚠️ Potential issue
LOGDIRstill unchecked & may break the build
make … > ${LOGDIR}/depends-hashes.txtwill fail with “/depends-hashes.txt: Permission denied” when
LOGDIRis unset or the directory doesn’t exist – exactly the issue flagged earlier.+# Sanity-check LOGDIR *before* we write into it +: "${LOGDIR:?LOGDIR not set}" +mkdir -p "${LOGDIR}"
45-48: 🛠️ Refactor suggestionDepends vs source/archive timestamps are still conflated
You continue to export a fixed
SOURCE_DATE_EPOCHfor depends (line 48) and later re-export the same
value for source & binary archives (line 252), contrary to the comment that the latter should use the
commit date. The previous suggestion to introduce a separate variable (e.g.DEPENDS_SOURCE_DATE_EPOCH)
and recomputeSOURCE_DATE_EPOCHright before archive creation has not been implemented.Also applies to: 252-254
contrib/guix/manifest.scm (2)
44-44:⚠️ Potential issueNon-canonical
building-ontriplet still present
"--build=x86_64-guix-linux-gnu"is not a recognised GNU triplet and confuses manyconfigure
scripts. Prior feedback to drop the-guix-infix hasn’t been addressed.
130-136:⚠️ Potential issueSubstring without guard & wrong GCC variant patched
(substring target 0 (string-index target #\-))will throw when the dash is absent; use the guarded
form suggested earlier.gcc-mingw-patchesis still applied to the genericbase-gcc(line 132) instead of
mingw-w64-base-gcc, negating the mingw-specific patches.(let* ((dash (or (string-index target #\-) (string-length target))) (machine (substring target 0 dash)) …)
🧹 Nitpick comments (3)
contrib/guix/libexec/build.sh (3)
238-240: Remove or use the apparently dead variables
DEPENDS_PACKAGESandDEPENDS_CACHEare assigned but never read, triggering SC2034.
If they’re only for debugging, wrap them with anif [[ $V -gt 0 ]]guard; otherwise drop them.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 238-238: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 239-239: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
126-127: Avoid maskingfind’s exit status (SC2155)The inline assignment inside
export LD_LIBRARY_PATH="$(find …)"discardsfind’s return code.
Assign first, then export:- export LD_LIBRARY_PATH="$(find /gnu/store -maxdepth 1 -name "*zlib*" | sort | head -n 1)/lib:…" + _zlib=$(find /gnu/store -maxdepth 1 -name "*zlib*" | sort | head -n 1)/lib + _gcc11=$(find /gnu/store -maxdepth 1 -name "*gcc-11*-lib" | sort | head -n 1)/lib + export LD_LIBRARY_PATH="${_zlib}:${_gcc11}"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
298-307: SDK path discovery is non-deterministic
find … | head -1depends on the filesystem enumeration order and breaks reproducibility.
Sort deterministically:- SDK_PATH=$(find "${BASEPREFIX}/SDKs" -type d -name "*extracted*" -o -name "*.sdk" | head -1) + SDK_PATH=$(find "${BASEPREFIX}/SDKs" -type d \( -name "*extracted*" -o -name "*.sdk" \) | sort | head -1)
📜 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 (3)
contrib/guix/libexec/build.sh(9 hunks)contrib/guix/libexec/prelude.bash(1 hunks)contrib/guix/manifest.scm(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/guix/libexec/prelude.bash
🧰 Additional context used
🪛 Shellcheck (0.10.0)
contrib/guix/libexec/build.sh
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 238-238: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 239-239: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-guix-x86_64-apple-darwin
- 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-arm64-apple-darwin
- GitHub Check: build-linux-cmake
- GitHub Check: Create Source Distribution
- GitHub Check: build-windows-cmake
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
contrib/guix/guix-build (4)
76-83: Comment header and defaultHOSTSlist are out-of-syncThe two comment lines still list the extended host set (incl.
arm-linux-gnueabihf,riscv64-linux-gnu,powerpc64-linux-gnu) but the default assigned toHOSTSbelow only enables the 5-tuple subset.
This inconsistency is confusing for newcomers skimming the script.-# x86_64-linux-gnu arm-linux-gnueabihf aarch64-linux-gnu riscv64-linux-gnu -# x86_64-w64-mingw32 x86_64-apple-darwin arm64-apple-darwin powerpc64-linux-gnu +# Default hosts built by ./guix-build if $HOSTS is unset: +# x86_64-linux-gnu x86_64-w64-mingw32 x86_64-apple-darwin +# aarch64-linux-gnu arm64-apple-darwinEither broaden the default list to match the comment or trim the comment so both lines describe the same behaviour.
130-139:breakshort-circuits further darwin checksIssuing
breakafter the first Darwin match stops the outer loop altogether.
If the user enabled bothx86_64-apple-darwinandarm64-apple-darwinand only the second one has a valid SDK location, the build aborts unnecessarily.- if [ -e "$OSX_SDK" ]; then - echo "Found macOS SDK at '${OSX_SDK}', using..." - break + if [ -e "$OSX_SDK" ]; then + echo "Found macOS SDK for ${host} at '${OSX_SDK}', using..." + continue
continuekeeps searching the remaining hosts while still accepting the first success.
366-370: Flag names still referenceENVIRONMENTafter the switch toguix shellThe script now uses
guix shell, yet printsADDITIONAL_GUIX_ENVIRONMENT_FLAGS.
For clarity (and to avoid future misuse) consider aliasing or renaming toADDITIONAL_GUIX_SHELL_FLAGS, keeping the old name as a deprecated fallback.No functional bug, purely UX.
438-456: Unquoted flag expansion enables accidental word-splitting
${ADDITIONAL_GUIX_COMMON_FLAGS}and friends are expanded unquoted in thetime-machine shellinvocation.
If a flag contains spaces (e.g.--verbose --debug), it will be split into two arguments as intended, but any trailing/leading whitespace or glob characters will also be interpreted by the shell.A safer pattern:
- ${ADDITIONAL_GUIX_COMMON_FLAGS} ${ADDITIONAL_GUIX_ENVIRONMENT_FLAGS} \ + ${ADDITIONAL_GUIX_COMMON_FLAGS:+${ADDITIONAL_GUIX_COMMON_FLAGS}} \ + ${ADDITIONAL_GUIX_ENVIRONMENT_FLAGS:+${ADDITIONAL_GUIX_ENVIRONMENT_FLAGS}} \Or pack them into arrays and use
"${array[@]}".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contrib/guix/guix-build(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-mac-cmake
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-windows-cmake
- GitHub Check: build-linux-cmake
- GitHub Check: Create Source Distribution
🔇 Additional comments (1)
contrib/guix/guix-build (1)
153-156: Sanity-check the updated disk-space tableThe required KiB values were bumped (
darwin440 MiB,mingw≈ 7.3 GiB, default ≈ 6.1 GiB).
Confirm that these numbers reflect the new dependency set; they drive the pre-flight abort message and easily get stale.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
contrib/guix/libexec/build.sh (2)
46-49: SOURCE_DATE_EPOCH handling still needs fixing.This issue was flagged in previous reviews but remains unresolved. The script sets a fixed timestamp for depends builds but never switches to the commit timestamp for archives as intended by the comment.
221-222: LOGDIR usage before validation remains problematic.This issue was flagged in previous reviews. The script uses
${LOGDIR}/depends-hashes.txtwithout ensuringLOGDIRis set and the directory exists, which can cause permission errors.contrib/guix/manifest.scm (2)
109-109: Building-on flag issue remains unresolved.This issue was flagged in previous reviews. The flag construction creates a non-standard GNU triplet with "guix" inserted, which may confuse configure scripts expecting canonical triplets.
194-202: Mingw toolchain definition issues need attention.Several issues flagged in previous reviews appear unresolved:
gcc-mingw-patchesmight be applied to wrong base gcc- Potential crash in
substringifstring-indexreturns#f- Verify
pthreads-xlibcreference is correct
🧹 Nitpick comments (2)
contrib/guix/libexec/build.sh (2)
239-240: Address unused variables flagged by static analysis.ShellCheck indicates
DEPENDS_PACKAGESandDEPENDS_CACHEare unused. If these are intended for future use or external consumption, consider exporting them or adding a comment explaining their purpose.+# Variables for external use or debugging DEPENDS_PACKAGES="$(make -C depends --no-print-directory HOST="$HOST" print-all_packages)" DEPENDS_CACHE="$(make -C depends --no-print-directory ${BASE_CACHE+BASE_CACHE="$BASE_CACHE"} print-BASE_CACHE)" +export DEPENDS_PACKAGES DEPENDS_CACHE
370-370: Quote variable expansion to prevent word splitting.ShellCheck warns about unquoted expansion that could cause word splitting.
- make -C build package -j$(nproc) + make -C build package -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 (4)
cmake/module/Maintenance.cmake(1 hunks)contrib/guix/libexec/build.sh(11 hunks)contrib/guix/manifest.scm(6 hunks)src/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
src/CMakeLists.txt (1)
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.
cmake/module/Maintenance.cmake (1)
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.
contrib/guix/libexec/build.sh (1)
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.
🪛 Shellcheck (0.10.0)
contrib/guix/libexec/build.sh
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 239-239: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 240-240: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 370-370: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (10)
src/CMakeLists.txt (2)
552-552: LGTM: Component-based installation.Adding
COMPONENT GUIproperly categorizes the installable targets for component-based installations.
556-576: Excellent conditional man page installation logic.The implementation correctly:
- Installs man pages only for built components
- Renames from Bitcoin Core to Firo project names
- Excludes Windows where man pages aren't typically used
Well-structured conditional logic that aligns with the project rebrand.
cmake/module/Maintenance.cmake (3)
104-111: Robust strip command detection.The logic properly handles different environments by prioritizing CMAKE_STRIP, falling back to native strip on macOS, and searching for llvm-strip/strip as alternatives.
116-119: Improved strip handling for macOS.Removing the
--stripoption from cmake install and performing manual stripping afterwards with failure tolerance (|| true) addresses potential compatibility issues with different strip implementations.
124-128: Useful OSX volume name generation.The new
osx_volnametarget generates a sanitized volume name for macOS packaging, supporting the deployment process.contrib/guix/libexec/build.sh (2)
351-361: Excellent modernization to CMake build system.The transition from autotools to CMake with proper toolchain files, build flags, and configuration options represents a significant modernization that should improve build reliability and maintainability.
544-558: Improved safety for directory operations.The safer handling of
ACTUAL_OUTDIRandOUTDIRvariables with validation before removal/moving operations prevents potential data loss from invalid paths.contrib/guix/manifest.scm (3)
40-100: Well-structured Python package definitions.The new Python packages (
python-biplist,python-mac-alias,python-ds-store) follow proper Guix package definition patterns with appropriate metadata, build systems, and dependencies.
162-171: Solid toolchain modernization.The updates to use
gcc-13,linux-libre-headers-6.1, andglibc-2.32with keyword arguments represent a well-planned modernization of the build toolchain infrastructure.
304-363: Strategic package list modernization.The shift to minimal packages (
cmake-minimal,python-minimal,git-minimal) and updated toolchain versions (gcc-toolchain-13,clang-toolchain-18) reduces dependencies while maintaining functionality. The conditional target-specific packages are well-organized.
aleflm
force-pushed
the
dev/aleflm/modernize-guix
branch
from
af3e18f to
4b4bee3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
contrib/guix/libexec/build.sh (2)
221-237: LOGDIR usage still needs safety checkThis is the same issue flagged in previous reviews. The script uses
${LOGDIR}/depends-hashes.txtwithout ensuringLOGDIRis set and the directory exists.Add a safety check before using LOGDIR:
+# Ensure LOGDIR is set and create directory +: "${LOGDIR:?LOGDIR not set}" +mkdir -p "$LOGDIR" + # Log the depends build ids make -C depends --no-print-directory HOST="$HOST" print-final_build_id_long | tr ':' '\n' > ${LOGDIR}/depends-hashes.txt
46-50: SOURCE_DATE_EPOCH timestamp handling still unresolvedThe previous review comment about SOURCE_DATE_EPOCH remains unaddressed. The script sets a fixed timestamp for depends but never updates it to the commit timestamp for archives as intended.
The comment on line 253 mentions using commit timestamp for archives, but SOURCE_DATE_EPOCH is never updated. Consider this fix:
-# Use a fixed timestamp for depends builds so hashes match across commits that -# don't make changes to the build system. This timestamp is only used for depends -# packages. Source archive and binary tarballs use the commit date. -export SOURCE_DATE_EPOCH=1397818193 +# Use a fixed timestamp for depends builds so hashes match across commits that +# don't make changes to the build system. +DEPENDS_SOURCE_DATE_EPOCH=1397818193 +export SOURCE_DATE_EPOCH="$DEPENDS_SOURCE_DATE_EPOCH"And later at line 253:
-# Use COMMIT_TIMESTAMP for the source and release binary archives -export SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH} +# Use commit timestamp for the source and release binary archives +SOURCE_DATE_EPOCH=$(git log -1 --format=%ct) +export SOURCE_DATE_EPOCHcontrib/guix/manifest.scm (2)
109-109: Invalid GNU triplet generation persists.This is the same issue from previous reviews. The
building-onflag constructs a non-standard triplet by inserting "guix" between system components, which can confuse configure scripts that validate GNU triplets.Use the original
(%current-system)directly or a canonical triplet helper:-(define building-on (string-append "--build=" (list-ref (string-split (%current-system) #\-) 0) "-guix-linux-gnu")) +(define building-on (string-append "--build=" (%current-system)))
194-202: Critical issues in MinGW toolchain definition remain.Two problems from previous reviews persist:
- Line 195:
string-indexcan return#fwhen no dash is found, causingsubstringto crash- Line 197:
gcc-mingw-patchesis still applied to globalbase-gccinstead of the MinGW-specific variantApply this fix:
-(machine (substring target 0 (string-index target #\-))) +(machine (substring target 0 (or (string-index target #\-) (string-length target)))) (pthreads-xlibc (winpthreads-patches (make-mingw-w64 machine - #:xgcc (cross-gcc target #:xgcc (gcc-mingw-patches base-gcc)) + #:xgcc (cross-gcc target #:xgcc (gcc-mingw-patches mingw-w64-base-gcc)) #:with-winpthreads? #t)))
🧹 Nitpick comments (2)
contrib/guix/libexec/build.sh (2)
370-370: Quote the nproc command to prevent word splittingThe shellcheck warning is valid -
nprocoutput should be quoted to prevent potential word splitting issues.- make -C build package -j$(nproc) + make -C build package -j"$(nproc)"
239-240: Remove or utilize unused variablesDEPENDS_PACKAGES and DEPENDS_CACHE are assigned but never used. Either remove them or add the intended functionality.
If these variables are intended for future use, consider adding a comment explaining their purpose. Otherwise, remove them:
-# Log the depends package hashes -DEPENDS_PACKAGES="$(make -C depends --no-print-directory HOST="$HOST" print-all_packages)" -DEPENDS_CACHE="$(make -C depends --no-print-directory ${BASE_CACHE+BASE_CACHE="$BASE_CACHE"} print-BASE_CACHE)" +# TODO: Add package hash logging if needed in the future
📜 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 (63)
CMakeLists.txt(1 hunks)cmake/module/AddBoostIfNeeded.cmake(1 hunks)cmake/module/FindQt.cmake(1 hunks)cmake/module/Maintenance.cmake(1 hunks)cmake/module/WindowsSystemConfiguration.cmake(1 hunks)cmake/utilities.cmake(1 hunks)contrib/guix/guix-build(11 hunks)contrib/guix/libexec/build.sh(11 hunks)contrib/guix/libexec/prelude.bash(1 hunks)contrib/guix/manifest.scm(6 hunks)contrib/guix/patches/binutils-unaligned-default.patch(1 hunks)contrib/guix/patches/gcc-remap-guix-store.patch(1 hunks)contrib/guix/patches/glibc-2.27-guix-prefix.patch(1 hunks)contrib/guix/patches/glibc-2.27-no-librt.patch(1 hunks)contrib/guix/patches/glibc-2.27-riscv64-Use-__has_include-to-include-asm-syscalls.h.patch(1 hunks)contrib/guix/patches/glibc-2.27-riscv64-fix-incorrect-jal-with-HIDDEN_JUMPTARGET.patch(1 hunks)contrib/guix/patches/glibc-guix-prefix.patch(1 hunks)contrib/guix/patches/winpthreads-remap-guix-store.patch(1 hunks)depends/Makefile(8 hunks)depends/builders/darwin.mk(1 hunks)depends/builders/default.mk(1 hunks)depends/config.guess(27 hunks)depends/config.sub(31 hunks)depends/funcs.mk(9 hunks)depends/hosts/darwin.mk(3 hunks)depends/hosts/default.mk(2 hunks)depends/hosts/linux.mk(2 hunks)depends/hosts/mingw32.mk(1 hunks)depends/packages/bls-dash.mk(3 hunks)depends/packages/boost.mk(1 hunks)depends/packages/dbus.mk(0 hunks)depends/packages/fontconfig.mk(1 hunks)depends/packages/gmp.mk(2 hunks)depends/packages/libICE.mk(0 hunks)depends/packages/libSM.mk(0 hunks)depends/packages/libX11.mk(0 hunks)depends/packages/libXext.mk(0 hunks)depends/packages/miniupnpc.mk(0 hunks)depends/packages/native_b2.mk(1 hunks)depends/packages/native_ccache.mk(0 hunks)depends/packages/native_cctools.mk(0 hunks)depends/packages/native_clang.mk(0 hunks)depends/packages/native_cmake.mk(0 hunks)depends/packages/native_ds_store.mk(0 hunks)depends/packages/native_libdmg-hfsplus.mk(0 hunks)depends/packages/native_libtapi.mk(0 hunks)depends/packages/native_mac_alias.mk(0 hunks)depends/packages/native_protobuf.mk(0 hunks)depends/packages/packages.mk(2 hunks)depends/packages/qt.mk(5 hunks)depends/packages/qt46.mk(0 hunks)depends/packages/tor.mk(1 hunks)depends/packages/xextproto.mk(0 hunks)depends/packages/xproto.mk(1 hunks)depends/packages/xtrans.mk(0 hunks)depends/packages/zeromq.mk(1 hunks)depends/packages/zlib.mk(2 hunks)depends/patches/boost/fix_boost_jam_cross-compilation.patch(1 hunks)depends/patches/qt/guix-arm64-darwin-build.patch(1 hunks)depends/patches/qt/mac-qmake.conf(0 hunks)src/CMakeLists.txt(1 hunks)src/compat/endian.h(2 hunks)src/secp256k1/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (19)
- depends/packages/native_mac_alias.mk
- depends/packages/xextproto.mk
- depends/packages/native_libtapi.mk
- depends/patches/qt/mac-qmake.conf
- depends/packages/native_ccache.mk
- depends/packages/miniupnpc.mk
- depends/packages/native_ds_store.mk
- depends/packages/native_libdmg-hfsplus.mk
- depends/packages/native_cmake.mk
- depends/packages/libICE.mk
- depends/packages/libX11.mk
- depends/packages/libXext.mk
- depends/packages/xtrans.mk
- depends/packages/native_clang.mk
- depends/packages/native_cctools.mk
- depends/packages/dbus.mk
- depends/packages/libSM.mk
- depends/packages/native_protobuf.mk
- depends/packages/qt46.mk
✅ Files skipped from review due to trivial changes (4)
- depends/patches/qt/guix-arm64-darwin-build.patch
- contrib/guix/patches/glibc-2.27-guix-prefix.patch
- contrib/guix/patches/winpthreads-remap-guix-store.patch
- depends/patches/boost/fix_boost_jam_cross-compilation.patch
🚧 Files skipped from review as they are similar to previous changes (31)
- depends/packages/tor.mk
- cmake/module/AddBoostIfNeeded.cmake
- depends/packages/xproto.mk
- contrib/guix/patches/glibc-2.27-riscv64-Use-__has_include-to-include-asm-syscalls.h.patch
- cmake/module/FindQt.cmake
- depends/packages/native_b2.mk
- contrib/guix/patches/glibc-guix-prefix.patch
- depends/packages/bls-dash.mk
- contrib/guix/patches/gcc-remap-guix-store.patch
- src/secp256k1/CMakeLists.txt
- contrib/guix/patches/binutils-unaligned-default.patch
- CMakeLists.txt
- depends/packages/fontconfig.mk
- cmake/utilities.cmake
- contrib/guix/libexec/prelude.bash
- cmake/module/Maintenance.cmake
- depends/builders/default.mk
- depends/packages/zlib.mk
- src/compat/endian.h
- depends/packages/qt.mk
- contrib/guix/patches/glibc-2.27-riscv64-fix-incorrect-jal-with-HIDDEN_JUMPTARGET.patch
- depends/packages/packages.mk
- src/CMakeLists.txt
- contrib/guix/patches/glibc-2.27-no-librt.patch
- depends/packages/zeromq.mk
- depends/hosts/default.mk
- depends/hosts/linux.mk
- depends/hosts/mingw32.mk
- depends/packages/gmp.mk
- depends/packages/boost.mk
- depends/builders/darwin.mk
🧰 Additional context used
🧠 Learnings (4)
cmake/module/WindowsSystemConfiguration.cmake (1)
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.
contrib/guix/guix-build (1)
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.
depends/Makefile (1)
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.
contrib/guix/libexec/build.sh (1)
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.
🪛 checkmake (0.2.2)
depends/Makefile
[warning] 72-72: Target "host" should be declared PHONY.
(phonydeclared)
[warning] 74-74: Target "host" should be declared PHONY.
(phonydeclared)
🪛 Shellcheck (0.10.0)
contrib/guix/libexec/build.sh
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 239-239: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 240-240: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 370-370: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-linux-cmake
- 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-windows-cmake
- GitHub Check: Create Source Distribution
- GitHub Check: build-mac-cmake
🔇 Additional comments (31)
cmake/module/WindowsSystemConfiguration.cmake (1)
47-71: Enhanced MinGW DLL detection logic looks good.The two-step approach (PATH lookup followed by compiler-based fallback) is more robust and should handle different MinGW installation scenarios better. The change from
FATAL_ERRORtoWARNINGwhen DLLs cannot be located is also appropriate, allowing builds to continue in development scenarios.contrib/guix/guix-build (6)
6-6: LGTM: Updated repository reference.Correctly updated the comment to reference "Firo repository" instead of Bitcoin.
76-82: Good expansion of supported architectures.The addition of
arm-linux-gnueabihf,riscv64-linux-gnu, andpowerpc64-linux-gnuto the default HOSTS list expands platform support appropriately.
136-140: Improved macOS SDK detection logic.The early
breakon finding a valid SDK is a good optimization, and the updated error message with theSDK_PATHenvironment variable suggestion is more helpful.
153-156: Updated disk space requirements.The recalibrated disk space requirements per host type reflect current build needs appropriately.
366-370: Good addition of environment variable visibility.Printing the additional Guix flags in the informational output improves build transparency and debugging capabilities.
438-455: Confirm Guix version support fortime-machine shell
Please ensure that the target Guix installation on your build host is new enough to recognizetime-machine shell(which replaces the oldertime-machine environmentsyntax). If you need to maintain compatibility with older Guix releases, you may have to fall back to--environmentor detect the supported subcommand at runtime.• Verify on the build machines:
guix --versionguix time-machine --help | grep shellguix time-machine --help | grep environmentdepends/config.guess (6)
3-7: Updated script metadata.The timestamp and copyright year have been correctly updated to 2024, and the shellcheck disable comment provides good rationale for using classic shell syntax.
98-99: Good introduction of deferred output pattern.The
GUESSvariable initialization supports the new pattern of accumulating results instead of immediate output, which is cleaner and more maintainable.
126-126: Enhanced compiler detection.Adding
c17to the compiler detection loop improves compatibility with modern C compilers.
158-178: Extended libc detection capabilities.The addition of
__ANDROID__and__LLVM_LIBC__macro detection expands platform support for Android and LLVM-based systems appropriately.
940-982: Good addition of modern OS support.The support for SerenityOS and Managarm with proper architecture detection enhances the script's coverage of contemporary operating systems.
1605-1610: Improved output control flow.The deferred output pattern with the final
GUESScheck and output is cleaner than the previous immediate echo/exit approach scattered throughout the case statement.depends/config.sub (1)
1-2355: LGTM - Standard GNU autotools config.sub updateThis is a standard update of the GNU autotools config.sub script from 2019 to 2024, bringing improved platform support and enhanced validation. The changes include new architecture support (e.g., arm64e, arm64ec, javascript), new OS support (managarm, zephyr), and better machine code format handling through the new
objvariable.depends/Makefile (3)
35-50: Excellent build system modernizationThe addition of new configuration variables (
NO_BOOST,NO_LIBEVENT,NO_USDT,LTO,C_STANDARD,CXX_STANDARD) provides much better build configurability and aligns with modern C++ build practices.
84-86: Good BSD support additionAdding support for FreeBSD, NetBSD, and OpenBSD improves platform coverage.
144-153: Enhanced build ID computation improves reproducibilityThe expanded build ID generation now includes compiler flags, linker flags, and additional environment variables, which will significantly improve build reproducibility and dependency tracking.
depends/funcs.mk (3)
15-16: Good addition of archiver flags supportAdding
arflagssupport provides better control over archiver behavior, which is important for cross-compilation scenarios.
174-176: Proper ARFLAGS integration in autoconfThe conditional inclusion of ARFLAGS in the autoconf command line ensures that custom archiver flags are properly passed through to the build system.
209-214: Simplified command execution improves maintainabilityThe removal of complex logging redirection and the move to inline environment variable settings makes the build commands cleaner and easier to debug.
contrib/guix/libexec/build.sh (1)
544-558: Improved safety in directory operationsThe enhanced safety checks for ACTUAL_OUTDIR and OUTDIR operations are a good improvement to prevent accidental directory deletion.
depends/hosts/darwin.mk (4)
1-6: Excellent modernization of macOS build targetsUpdating the minimum macOS version to 13.0, SDK to 14.0, and Xcode to 15.0 brings the build system up to current standards while the LLD version update ensures compatibility with modern toolchains.
19-25: Good standardization on LLVM toolchainSwitching to LLVM tools (
llvm-ar,llvm-nm,llvm-ranlib, etc.) provides better consistency and removes dependencies on cctools. This aligns well with the removal of native cctools packages mentioned in the PR objectives.
54-71: Well-documented and modern compiler configurationThe explicit use of
--target,-isysroot, and-nostdlibincwith detailed flag explanations shows good engineering practices. The conditional LLD flags for non-Darwin builds ensure proper cross-compilation support.
79-82: Proper CMake system configurationThe addition of
darwin_cmake_system_versionand the rename of the system name variable improve CMake integration and provide better version specificity.contrib/guix/manifest.scm (6)
1-37: Modernized imports look good.The selective imports using
#:selecthelp reduce the build environment footprint and make dependencies explicit. The transition from full packages to minimal variants (e.g.,cmake-minimal,python-minimal,git-minimal) aligns well with the modernization goals.
40-100: Python package definitions are well-structured.The new minimal Python packages (
python-biplist,python-mac-alias,python-ds-store) are properly defined with disabled tests to minimize build dependencies. The propagated inputs inpython-ds-storecorrectly establish the dependency chain.
180-190: New patching functions are well-implemented.The
gcc-mingw-patches,binutils-mingw-patches, andwinpthreads-patchesfunctions provide clean abstractions for applying necessary patches to MinGW components. The use ofpackage-with-extra-patchesis the correct approach.
222-260: Enhanced GCC configurations are comprehensive.The updated base GCC packages (
mingw-w64-base-gccandlinux-base-gcc) include appropriate security and compatibility flags. The rpath-to-rpath-link substitution in the Linux variant is a clever solution for Guix-specific linking issues.
262-301: glibc-2.32 definition is well-structured.The updated glibc package correctly uses a git source, applies necessary patches, and includes security-focused configure flags. The RPC installation directory fix addresses a common build issue.
327-362: Modernized package manifest aligns with build system updates.The updated toolchain versions (gcc-13, gcc-toolchain-13, linux-libre-headers-6.1) and platform-specific package selections properly support the modernized build infrastructure. The addition of minimal packages and removal of legacy components is consistent with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
contrib/guix/libexec/build.sh (3)
221-223:LOGDIRstill used before it is guaranteed to exist
Same issue as previously flagged: if$LOGDIRis unset or the directory is missing, the redirection expands to/depends-hashes.txtand the build aborts with a permission error.+# Sanity-check LOGDIR +: "${LOGDIR:?LOGDIR not set}" +mkdir -p "$LOGDIR" + # Log the depends build ids -make -C depends --no-print-directory HOST="$HOST" print-final_build_id_long | tr ':' '\n' > ${LOGDIR}/depends-hashes.txt +make -C depends --no-print-directory HOST="$HOST" print-final_build_id_long \ + | tr ':' '\n' \ + > "${LOGDIR}/depends-hashes.txt"
46-49:SOURCE_DATE_EPOCHhandling still mixes depends vs. archive timestamps
The script exports a fixed epoch (1397818193) and later re-exports the same value for source/binary archives despite the comment that it should switch to the commit timestamp. This was raised earlier but is still unresolved.-# packages. Source archive and binary tarballs use the commit date. -export SOURCE_DATE_EPOCH=1397818193 +# packages. +DEPENDS_SOURCE_DATE_EPOCH=1397818193 +export SOURCE_DATE_EPOCH="$DEPENDS_SOURCE_DATE_EPOCH"-# Use COMMIT_TIMESTAMP for the source and release binary archives -export SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH} +# Switch to commit timestamp for source/binary archives +SOURCE_DATE_EPOCH="$(git log -1 --format=%ct)" +export SOURCE_DATE_EPOCHAlso applies to: 253-255
126-129: Combine-and-export pattern masks errors (SC2155)
Theexport LD_LIBRARY_PATH="$(find …)"construction still hidesfindfailures and makes debugging harder.- export LD_LIBRARY_PATH="$(find /gnu/store -maxdepth 1 -name "*zlib*" | sort | head -n 1)/lib:$(find /gnu/store -maxdepth 1 -name "*gcc-11*-lib" | sort | head -n 1)/lib" + LD_LIBRARY_PATH="$(find /gnu/store -maxdepth 1 -name "*zlib*" | sort | head -n 1)/lib:$(find /gnu/store -maxdepth 1 -name "*gcc-11*-lib" | sort | head -n 1)/lib" + export LD_LIBRARY_PATH
🧹 Nitpick comments (1)
contrib/guix/libexec/build.sh (1)
370-375: Quote$(nproc)in themake packageinvocation
Without quotes the-jflag can be split ifnprocever returns empty or contains whitespace (rare, but defensive quoting is cheap).- make -C build package -j$(nproc) + make -C build package "-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 (1)
contrib/guix/libexec/build.sh(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
contrib/guix/libexec/build.sh (1)
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.
🪛 Shellcheck (0.10.0)
contrib/guix/libexec/build.sh
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 239-239: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 240-240: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 370-370: Quote this to prevent word splitting.
(SC2046)
[warning] 517-517: Use "${var:?}" to ensure this never expands to / .
(SC2115)
⏰ Context from checks skipped due to timeout of 90000ms (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-linux-gnu
- GitHub Check: build-windows-cmake
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: Create Source Distribution
- GitHub Check: build-linux-cmake
…n CI and adjust build flags in build.sh for Guix
aleflm
force-pushed
the
dev/aleflm/modernize-guix
branch
from
4ec9a88 to
fd50ef5
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
contrib/guix/libexec/build.sh (3)
46-49: SOURCE_DATE_EPOCH timestamp handling needs clarificationThe comment indicates this timestamp is "only used for depends packages" and that "source archive and binary tarballs use the commit date," but the script never actually switches to the commit timestamp later (lines 254-255 just re-export the same value).
126-128: Separate variable assignment from export to avoid masking errorsCombining
exportwith command substitution masks potential failures in thefindcommands.- export LD_LIBRARY_PATH="$(find /gnu/store -maxdepth 1 -name "*zlib*" | sort | head -n 1)/lib:$(find /gnu/store -maxdepth 1 -name "*gcc-11*-lib" | sort | head -n 1)/lib" + LD_LIBRARY_PATH="$(find /gnu/store -maxdepth 1 -name "*zlib*" | sort | head -n 1)/lib:$(find /gnu/store -maxdepth 1 -name "*gcc-11*-lib" | sort | head -n 1)/lib" + export LD_LIBRARY_PATH
221-237: LOGDIR usage needs validationThe script uses
${LOGDIR}/depends-hashes.txtwithout ensuring LOGDIR is set or the directory exists, which could cause permission errors.Add validation before using LOGDIR:
+# Validate LOGDIR is set and create directory +: "${LOGDIR:?LOGDIR not set}" +mkdir -p "$LOGDIR" + # Log the depends build ids
🧹 Nitpick comments (5)
contrib/guix/libexec/build.sh (1)
401-401: Quote nproc to prevent word splittingThe
nproccommand should be quoted to prevent potential word splitting issues.- make -C build deploy -j$(nproc) + make -C build deploy -j"$(nproc)"contrib/macdeploy/install-dmg.md (1)
11-11: Minor: Remove trailing punctuation from headingMarkdown style guidelines recommend avoiding trailing punctuation in headings.
-### On Debian/Ubuntu, install prerequisites: +### On Debian/Ubuntu, install prerequisitescontrib/macdeploy/detached-sig.sh (3)
17-18: Quote path to guard against word-splitting
${UNSIGNED_BINARY}is unquoted inside command substitution. If the bundle name ever contains spaces (common on macOS), word splitting breaks the call.-ARCH=$(${SIGNAPPLE} info ${UNSIGNED_BINARY} | head -n 1 | cut -d " " -f 1) +ARCH=$("${SIGNAPPLE}" info "${UNSIGNED_BINARY}" | head -n 1 | cut -d " " -f 1)
24-38: Argument validation duplicates but misses mutually-exclusive caseThe two branches effectively re-implement the same usage logic and don’t forbid passing extra arguments in
WITHOUT_NOTARIZEmode. Consider canonicalising validation to a single block.
84-86: Clean-up path is unguarded
rm -rf "${TEMPDIR}"runs after packaging; if$TEMPDIRever becomes empty due to an earlier unset, this turns into a project-root wipe. Addingset -u(see above) plus a sanity guard prevents foot-guns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/ci-master.ymlis excluded by!**/*.ymlcontrib/gitian-descriptors/gitian-osx.ymlis excluded by!**/*.yml
📒 Files selected for processing (15)
CMakeLists.txt(3 hunks)Makefile.am(1 hunks)cmake/module/GenerateSetupNsi.cmake(1 hunks)cmake/module/Maintenance.cmake(2 hunks)cmake/module/WindowsSystemConfiguration.cmake(3 hunks)configure.ac(1 hunks)contrib/guix/libexec/build.sh(9 hunks)contrib/macdeploy/create-dmg.sh(1 hunks)contrib/macdeploy/detached-sig-apply.sh(0 hunks)contrib/macdeploy/detached-sig-create.sh(0 hunks)contrib/macdeploy/detached-sig.sh(1 hunks)contrib/macdeploy/install-dmg.md(1 hunks)share/setup.nsi.in(2 hunks)src/CMakeLists.txt(3 hunks)src/clientversion.cpp(2 hunks)
💤 Files with no reviewable changes (2)
- contrib/macdeploy/detached-sig-apply.sh
- contrib/macdeploy/detached-sig-create.sh
✅ Files skipped from review due to trivial changes (2)
- configure.ac
- share/setup.nsi.in
🚧 Files skipped from review as they are similar to previous changes (4)
- cmake/module/WindowsSystemConfiguration.cmake
- src/CMakeLists.txt
- cmake/module/Maintenance.cmake
- CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: pkg-config should not be used in the firo cmake build system as it's not available by default on win...
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:
cmake/module/GenerateSetupNsi.cmakecontrib/guix/libexec/build.sh
🪛 Shellcheck (0.10.0)
contrib/guix/libexec/build.sh
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 239-239: DEPENDS_PACKAGES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 240-240: DEPENDS_CACHE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 401-401: Quote this to prevent word splitting.
(SC2046)
[warning] 549-549: Use "${var:?}" to ensure this never expands to / .
(SC2115)
🪛 markdownlint-cli2 (0.17.2)
contrib/macdeploy/install-dmg.md
11-11: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ 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-w64-mingw32
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-windows-cmake
- GitHub Check: build-mac-cmake
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: Create Source Distribution
- GitHub Check: build-linux-cmake
🔇 Additional comments (24)
cmake/module/GenerateSetupNsi.cmake (4)
9-13: LGTM! Consider making WINDOWS_BITS configurable.The new package metadata variables provide good consistency for the NSIS installer generation. The hardcoded
WINDOWS_BITS = "64"is appropriate for modern builds, though consider making it configurable if 32-bit support is needed in the future.
10-10: Good branding consistency improvement.Capitalizing
CLIENT_TARNAMEto "Firo" aligns with proper branding conventions and maintains consistency with other package name variables.
14-17: LGTM! Explicit variable setting ensures proper template substitution.While these assignments appear redundant, explicitly setting the CLIENT_VERSION_* variables ensures they're available in the current scope for the
configure_file()operation. This is a good defensive practice in CMake.
25-27: Excellent abstraction for build system compatibility.The
EXECUTABLE_DIRvariable properly abstracts the difference between CMake/Guix and autotools executable placement. The detailed comment clearly explains the rationale, which will be valuable for future maintainers.src/clientversion.cpp (3)
42-46: LGTM: Clear documentation of build.h contentsThe comment effectively explains what the generated
build.hfile may contain, making the conditional logic below more understandable.
57-58: LGTM: New macro for commit ID formattingThe
BUILD_DESC_FROM_COMMIT_IDmacro correctly formats version strings without the "g" prefix, providing flexibility in version string construction.
67-74: LGTM: Enhanced build description logicThe expanded conditional logic properly handles the new
BUILD_GIT_TAGandBUILD_GIT_COMMITmacros with appropriate fallbacks. The priority order (tag → commit → suffix → commit ID → unknown) is logical.contrib/guix/libexec/build.sh (13)
10-11: LGTM: Updated shell utilities sourcingGood practice to source git utilities and use proper shellcheck directive.
78-92: LGTM: Improved toolchain path setupThe native GCC toolchain setup with platform-specific library path handling is well-structured and handles the different host types appropriately.
103-107: LGTM: Refined mingw cross-toolchain selectionThe architecture-specific winpthreads package selection properly handles both i686 and x86_64 mingw targets.
177-178: LGTM: Safer symlink creationUsing
--no-dereferenceflag prevents issues if the target already exists as a symlink.
192-192: LGTM: Added i686 Linux supportGood addition of i686-linux-gnu dynamic linker path support.
258-281: LGTM: Improved source tarball creation with submodulesThe new two-step archive process properly includes submodules in the source tarball, which is a significant improvement over the previous single
git archiveapproach.
290-320: LGTM: Enhanced platform-specific compiler flagsThe refined CFLAGS, CXXFLAGS, and LDFLAGS handling with platform-specific optimizations and SDK detection for Darwin is well-implemented.
323-331: LGTM: Improved build environment setupSetting
GIT_DISCOVERY_ACROSS_FILESYSTEM, mandatory Trezor support, and disabling Git build info for extracted archives are appropriate configurations.
346-366: LGTM: CMake transition with proper static linkingThe transition from autotools to CMake with proper static linking flags for MinGW builds is well-structured.
450-498: LGTM: Robust DMG binary detection and packagingThe comprehensive logic for finding and using DMG binaries from multiple sources (depends, Guix, local) with appropriate fallbacks is well-implemented.
529-567: LGTM: Safer MinGW packaging with staging areasThe improved zip creation using temporary staging areas and proper file exclusion is much safer than the previous approach.
616-630: LGTM: Safer directory operationsThe improved validation before directory removal and moves prevents accidental data loss.
633-642: LGTM: Improved checksum generationStoring checksums in a variable before writing prevents partial file creation on errors.
Makefile.am (1)
53-55: LGTM: Updated macOS packaging filesThe replacement of separate detached signature scripts with the consolidated
detached-sig.sh, addition ofcreate-dmg.sh, and inclusion ofinstall-dmg.mddocumentation properly reflects the modernized macOS packaging workflow.contrib/macdeploy/install-dmg.md (1)
1-49: LGTM: Comprehensive DMG tool installation guideThe documentation provides clear, step-by-step instructions for building and installing the
dmgtool with proper prerequisites, build steps, and uninstall instructions. This complements the newcreate-dmg.shscript well.contrib/macdeploy/create-dmg.sh (2)
7-15: LGTM: Thorough prerequisite validationThe script properly validates all required tools and files before proceeding, with clear error messages for missing dependencies.
17-20: LGTM: Clean DMG creation workflowThe two-step process of creating an uncompressed ISO first, then converting to compressed DMG, followed by cleanup is the standard approach and well-implemented.
| export LC_ALL=C | ||
| set -e | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden script safety flags (set -u -o pipefail)
Only set -e is enabled. Missed unset-variable (-u) and pipe-failure (-o pipefail) detection can silently mask logic errors or partial failures inside pipelines (e.g. ARCH=$(…) | cut …).
-set -e
+set -euo pipefail📝 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.
| export LC_ALL=C | |
| set -e | |
| export LC_ALL=C | |
| set -euo pipefail |
🤖 Prompt for AI Agents
In contrib/macdeploy/detached-sig.sh at lines 6 to 8, the script currently only
uses 'set -e' which stops execution on errors but does not detect unset
variables or failures in pipelines. To improve script safety, add 'set -u' to
treat unset variables as errors and 'set -o pipefail' to ensure pipeline
failures are detected. Update the set command to 'set -euo pipefail' to enable
all three flags together.
| SIGNAPPLE=signapple | ||
| TEMPDIR=sign.temp | ||
|
|
There was a problem hiding this comment.
Validate signapple availability before use
SIGNAPPLE=signapple is assumed to be on $PATH and executable; if missing, the script aborts later with a cryptic error.
SIGNAPPLE=signapple
+# Ensure the helper is available early
+command -v "${SIGNAPPLE}" >/dev/null 2>&1 || {
+ echo "error: '${SIGNAPPLE}' not found in PATH" >&2
+ exit 127
+}📝 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.
| SIGNAPPLE=signapple | |
| TEMPDIR=sign.temp | |
| SIGNAPPLE=signapple | |
| # Ensure the helper is available early | |
| command -v "${SIGNAPPLE}" >/dev/null 2>&1 || { | |
| echo "error: '${SIGNAPPLE}' not found in PATH" >&2 | |
| exit 127 | |
| } | |
| TEMPDIR=sign.temp |
🤖 Prompt for AI Agents
In contrib/macdeploy/detached-sig.sh around lines 9 to 11, the script assigns
SIGNAPPLE=signapple assuming it is available in the PATH, but does not check its
presence. Add a validation step after this assignment to check if the signapple
command is available and executable. If not found, print a clear error message
and exit the script immediately to avoid cryptic errors later.
| # Prompt for signing key passphrase | ||
| stty -echo | ||
| printf "Enter the passphrase for %s: " "$1" | ||
| read cs_key_pass | ||
| printf "\n" | ||
| stty echo | ||
|
|
There was a problem hiding this comment.
Interactive stty prompts break CI automation
stty -echo assumes an attached TTY; in headless GUIX/CI runs the call errors and the script hangs waiting for input. Provide a non-interactive alternative (env var or --passphrase-file) and skip stty when stdin is not a TTY.
🤖 Prompt for AI Agents
In contrib/macdeploy/detached-sig.sh around lines 43 to 49, the use of `stty
-echo` and interactive read breaks CI automation because it assumes an attached
TTY. Modify the script to check if stdin is a TTY before using `stty`. Add
support for a non-interactive alternative by allowing the passphrase to be
provided via an environment variable or a `--passphrase-file` argument, and skip
the interactive prompt and `stty` commands when running in non-TTY environments.
| find . -maxdepth 3 -wholename "*/bin/*" -type f -exec realpath --relative-to=. {} \; | while read -r bin | ||
| do | ||
| bin_dir=$(dirname "${bin}") | ||
| bin_name=$(basename "${bin}") | ||
| ${SIGNAPPLE} sign -f --hardened-runtime --detach "${OUTROOT}/${bin_dir}" --passphrase "${cs_key_pass}" "$1" "${bin}" | ||
| ${SIGNAPPLE} apply "${bin}" "${OUTROOT}/${bin_dir}/${bin_name}.${ARCH}sign" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
GNU–specific realpath --relative-to reduces macOS portability
realpath --relative-to exists only in GNU coreutils; default macOS does not ship it. Either replace with a POSIX-portable construct or vendor GNU coreutils explicitly.
-find . -maxdepth 3 -wholename "*/bin/*" -type f -exec realpath --relative-to=. {} \; |
+find . -maxdepth 3 -path "*/bin/*" -type f -print0 |
+ sed -e 's|^\./||' -z |
while IFS= read -r -d '' binCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contrib/macdeploy/detached-sig.sh around lines 68 to 74, the use of 'realpath
--relative-to' is not portable to macOS as it relies on GNU coreutils. Replace
this with a POSIX-compliant method to compute relative paths, such as using
'perl', 'python', or a shell function that calculates relative paths without
GNU-specific options, or alternatively document and ensure GNU coreutils are
installed and used explicitly.
| binaries_dir=$(dirname "$(find . -maxdepth 2 -wholename '*/bin' -type d -exec realpath --relative-to=. {} \;)") | ||
| ${SIGNAPPLE} notarize --passphrase "${api_key_pass}" "$2" "$3" "${binaries_dir}" | ||
| else | ||
| echo "Skipping binaries notarization due to WITHOUT_NOTARIZE=1" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
dirname $(find …) only handles the first match
When multiple bin directories exist, only the first one is notarised. Iterate over all or build an array.
🤖 Prompt for AI Agents
In contrib/macdeploy/detached-sig.sh around lines 78 to 82, the current code
uses dirname with find which only processes the first matched 'bin' directory,
causing only one directory to be notarized. Modify the script to iterate over
all found 'bin' directories by capturing all matches into an array or loop, then
run the notarize command on each directory to ensure all binaries are notarized.
|
FYI, the failure was not from build fails. CI machine ran out of storage ( GUIX uses a lot storage ) |
|
When building on WSL2 2.2.4.0 and only for x86_64-w64-mingw32, the following behaviour was observed: Guix LogsAdditional informationThis was fixed by removing |
aleflm
force-pushed
the
dev/aleflm/modernize-guix
branch
2 times, most recently
from
a5f72dd to
f081a83
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
Jenkinsfile (11)
8-11: Set a deterministic locale to avoid Guix/ctest encoding issues.Add LANG and avoid LC_ALL leaks from the host.
environment { CCACHE_DIR = '/tmp/.ccache' - HOST = 'x86_64-linux-gnu' + HOST = "${params.HOST ?: 'x86_64-linux-gnu'}" + LANG = 'C.UTF-8' }Optionally add in test/run shells:
unset LC_ALL || true.
10-10: Parameterize HOST (or matrix) instead of hard-coding.Enables linux/mingw/aarch64 jobs without editing the file.
Additional snippet (outside this hunk):
parameters { choice(name: 'HOST', choices: ['x86_64-linux-gnu','x86_64-w64-mingw32','aarch64-linux-gnu'], description: 'Target triplet') }
17-17: Minor: use $(nproc) and a reusable JOBS var.Cleaner and reusable across stages.
- sh 'make -j`nproc` HOST=${HOST}' + sh 'JOBS=${JOBS:-$(nproc)}; make -j"$JOBS" HOST="${HOST}"'
23-30: Harden CMake configure: ccache, hermetic pkg-config, generator agnostic paths.Improves build speed and determinism.
sh ''' - export HOST_TRIPLET=${HOST} - env PKG_CONFIG_PATH="$(pwd)/depends/$HOST_TRIPLET/lib/pkgconfig:$PKG_CONFIG_PATH" \\ - cmake -DCMAKE_TOOLCHAIN_FILE=$(pwd)/depends/$HOST_TRIPLET/toolchain.cmake \\ - -DBUILD_CLI=ON -DBUILD_TESTS=ON -DBUILD_GUI=ON -DCMAKE_BUILD_TYPE=Release \\ - -DCLIENT_VERSION_IS_RELEASE=true -DENABLE_CRASH_HOOKS=ON \\ - -S$(pwd) -B$(pwd)/build + set -euo pipefail + export HOST_TRIPLET="${HOST}" + export BUILD_DIR="$(pwd)/build" + export DEP_PREFIX="$(pwd)/depends/$HOST_TRIPLET" + export JOBS="${JOBS:-$(nproc)}" + export PKG_CONFIG_LIBDIR="$DEP_PREFIX/lib/pkgconfig" + export PKG_CONFIG_PATH="$PKG_CONFIG_LIBDIR${PKG_CONFIG_PATH:+:$PKG_CONFIG_PATH}" + cmake -DCMAKE_TOOLCHAIN_FILE="$DEP_PREFIX/toolchain.cmake" \\ + -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \\ + -DBUILD_CLI=ON -DBUILD_TESTS=ON -DBUILD_GUI=ON -DCMAKE_BUILD_TYPE=Release \\ + -DCLIENT_VERSION_IS_RELEASE=true -DENABLE_CRASH_HOOKS=ON \\ + -S . -B "$BUILD_DIR" '''
31-33: Use generator-agnostic build invocation.Works with Ninja/Unix Makefiles alike.
- sh 'make -j`nproc`' + sh 'cmake --build . -j"${JOBS:-$(nproc)}"'
47-54: Capture more diagnostics (CMakeError/Output) with robust find.Helps triage configure/build failures.
mkdir -p test-logs find build -name "LastTest.log" -exec cp {} test-logs/ctest-last.log \\; || true find build -name "LastTestsFailed.log" -exec cp {} test-logs/ctest-failed.log \\; || true - find build -path "*/Testing/Temporary/*" -name "*.log" -exec cp {} test-logs/ \\; || true + find build -path "*/Testing/Temporary/*" -name "*.log" -exec cp {} test-logs/ \\; || true + find build \\( -name "CMakeError.log" -o -name "CMakeOutput.log" \\) -exec cp {} test-logs/ \\; || true find build -name "*test*.log" -exec cp {} test-logs/ \\; || true
56-58: Publish JUnit and fingerprint logs.Improves Jenkins test reporting and artifact traceability.
- archiveArtifacts artifacts: 'test-logs/**', - allowEmptyArchive: true + archiveArtifacts artifacts: 'test-logs/**', fingerprint: true, allowEmptyArchive: true + junit testResults: 'test-logs/*.xml', allowEmptyResults: true
62-67: RPC tests: harden shell, set locale, add timeout.Prevents silent hangs and locale-induced failures.
sh ''' - export FIROD="$(pwd)/build/bin/firod" - export FIROCLI="$(pwd)/build/bin/firo-cli" - qa/pull-tester/rpc-tests.py -extended + set -euo pipefail + export LANG=C.UTF-8 + unset LC_ALL || true + export FIROD="$(pwd)/build/bin/firod" + export FIROCLI="$(pwd)/build/bin/firo-cli" + timeout 90m qa/pull-tester/rpc-tests.py -extended '''
70-79: Archive checksums and fingerprint binaries.Helps consumers verify artifacts.
mkdir -p artifacts cp build/bin/firo* artifacts/ || true + (cd artifacts && sha256sum * > SHA256SUMS || true) ''' } - archiveArtifacts artifacts: 'artifacts/**', - allowEmptyArchive: true + archiveArtifacts artifacts: 'artifacts/**', fingerprint: true, allowEmptyArchive: true
2-6: Pin the builder image instead of :latest.Improves reproducibility; use a version tag or digest.
Example:
agent { docker { image 'firoorg/firo-builder-depends:2025-08-01' // or @sha256:<digest> alwaysPull true } }
1-7: Consider a matrix for HOST triplets.Build linux, mingw, and aarch64 in parallel; add per-axis env and artifacts suffixes.
Example:
stages { stage('Matrix') { matrix { axes { axis { name 'HOST'; values 'x86_64-linux-gnu','x86_64-w64-mingw32' } } stages { /* reuse existing stages */ } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
.github/workflows/ci-master.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
Jenkinsfile(1 hunks)
⏰ 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-linux-gnu
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-windows-cmake
- GitHub Check: Create Source Distribution
- GitHub Check: build-linux-cmake
| dir('build') { | ||
| sh 'make test' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer ctest with failure output and emit JUnit for Jenkins.
Also set a safe UTF-8 locale and clear LC_ALL to avoid WSL/Guix issues noted in the PR.
- sh 'make test'
+ sh '''
+ export LANG=C.UTF-8
+ unset LC_ALL || true
+ mkdir -p ../test-logs
+ ctest --output-on-failure -j"${JOBS:-$(nproc)}" --output-junit ../test-logs/ctest.xml
+ '''📝 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.
| dir('build') { | |
| sh 'make test' | |
| } | |
| dir('build') { | |
| sh ''' | |
| export LANG=C.UTF-8 | |
| unset LC_ALL || true | |
| mkdir -p ../test-logs | |
| ctest --output-on-failure -j"${JOBS:-$(nproc)}" --output-junit ../test-logs/ctest.xml | |
| ''' | |
| } |
🤖 Prompt for AI Agents
Jenkinsfile lines 39-41: replace the 'make test' step with a shell that clears
LC_ALL, sets a safe UTF-8 locale (e.g. export LANG=C.UTF-8), then runs ctest
with failure output and JUnit output (e.g. ctest --output-on-failure
--output-junit test-results.xml), and ensure the produced test-results.xml is
available to Jenkins for JUnit reporting (archive/record the file in the
pipeline).
f081a83 to
e354bbb
Compare
aleflm
force-pushed
the
dev/aleflm/modernize-guix
branch
from
e354bbb to
9db3a47
Compare
PR intention
Remove outdated/unnecessary packages. Modernize our GUIX build and add GUIX CI.
Firo Refactor Progress
Status Legend
Dependencies Packages (
depends/packages)|
Other Components