backport: merge bitcoin#24866, #25612, #28432, #28757, #28845, #28932, #28973, #21778, #30204, #29765, #30201, #30287, #30511 (toolchain backports: part 3) #6516
Conversation
…NG=1` `dsymutil` is defined as only available in `cctools` but Guix builds with `FORCE_USE_SYSTEM_CLANG`, which omits `cctools` (and therefore, `dsymutil`) but we use `dsymutil` to generate debug symbols, so we need to make sure that we get `dsymutil` one way or another.
kwvg
changed the title
Expected hashes for 472945d |
Expected hashes for ee0a9f2 |
kwvg
changed the title
WalkthroughThis pull request introduces a comprehensive set of changes focused on transitioning the macOS build and deployment process from using DMG files to ZIP files. The modifications span multiple configuration files, build scripts, and documentation across the project's build system. Key changes include updating the macOS deployment toolchain to use LLVM 18 and LLD linker, removing several native package dependencies, and modifying build configurations to support ZIP-based packaging. The changes affect build scripts, Makefiles, CI configurations, and deployment instructions. The modifications streamline the build process for macOS, update compiler and linker toolchains, and simplify the packaging mechanism by replacing DMG files with ZIP archives. These updates reflect an evolution in the project's build infrastructure, focusing on more modern and flexible deployment methods for macOS applications. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
depends/patches/gmp/include_ldflags_in_configure.patch (1)
1-7: Consider adding clarifying comments or commit message notes.It might be helpful to state explicitly in the patch file or commit message how this impacts macOS cross-builds and why
$LDFLAGSmust be appended during compilation. This provides future maintainers with clearer context.depends/hosts/darwin.mk (1)
19-21: Validate LLVM tool discovery logic.Using
command -vforAR,DSYMUTIL,NM,RANLIB, andSTRIPensures the correct binary is used if multiple versions exist. Please double-check to confirm these discovered paths match the intended version (particularly if there are multiple LLVM installations).Also applies to: 23-24
depends/config.site.in (1)
127-129: Solid addition of DSYMUTIL in config.Defining
ac_cv_path_DSYMUTILensures thatdsymutilis recognized. Consider logging or a warning ifDSYMUTILis missing or points to an unexpected path, to help diagnose Davin or macOS-specific build issues.Makefile.am (2)
123-123: Revisit naming to emphasize directory-level target.
Usingdeploydir: $(OSX_ZIP)is clear, but consider whether a more descriptive target name (e.g.deploy_zip_dir) might help new contributors understand the difference between directory preparation and the final artifact.
137-137: Consistent naming for the deploy target.
The deploy target now produces a.zipfile instead of a.dmg. Make sure documentation references (like in README or wiki) are also updated to avoid confusion.ci/test/00_setup_env_native_fuzz_with_valgrind.sh (1)
17-17: Update PACKAGES to specify Clang 18.While BITCOIN_CONFIG has been updated to use Clang 18, the PACKAGES variable still uses generic "clang llvm". Consider updating PACKAGES to explicitly specify the version for consistency.
-export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-dev valgrind" +export PACKAGES="clang-18 llvm-18 python3 libevent-dev bsdmainutils libboost-dev valgrind"ci/test/00_setup_env_native_valgrind.sh (1)
14-14: Consider tracking GUI enablement separately.The TODO comment about enabling GUI is unrelated to the current toolchain updates. Consider creating a separate issue to track this enhancement.
Would you like me to create a GitHub issue to track the GUI enablement task?
ci/test/00_setup_env_native_ubsan.sh (1)
Line range hint
7-7: Consider tracking sanitizer configuration update.The TODO comment suggests unifying undefined behavior sanitizer configuration with Bitcoin. This might be worth addressing in a future toolchain update.
Would you like me to create a GitHub issue to track the sanitizer configuration unification task?
contrib/macdeploy/README.md (3)
69-69: Improve precision in deterministic build descriptionThe phrase "somewhat deterministic" introduces uncertainty about the build process's determinism.
Consider rephrasing to be more precise about which aspects of the build process are deterministic:
-for the build process to remain somewhat deterministic. Here's how it works: +for the build process to maintain determinism where possible. Here's how it works:🧰 Tools
🪛 LanguageTool
[style] ~69-~69: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...n order for the build process to remain somewhat deterministic. Here's how it works: - ...(SOMEWHAT)
71-73: Document the verification process for unsigned buildsThe documentation describes the build process but doesn't mention how users can verify the unsigned artifacts.
Consider adding a section about verification steps:
users may choose to bless, self-codesign, and run. It also outputs an unsigned app structure in the form of a tarball. + + To verify the unsigned artifacts: + 1. Check the SHA256 hash against the release manifest + 2. Verify the Guix attestation for the build + 3. For self-codesigning, follow Apple's documented process at <link>Also applies to: 77-78
61-62: Add SDK version compatibility informationThe documentation should specify which SDK versions are compatible with the build process.
Consider adding version compatibility information:
All builds must target an Apple SDK. These SDKs are free to download, but not redistributable. See the SDK Extraction notes above for how to obtain it. + +Supported SDK versions: +- Minimum: Xcode 15.0 (macOS 14.0) +- Recommended: Xcode 15.0 (macOS 14.0) +- Maximum: Xcode 15.x (macOS 14.x)depends/README.md (1)
54-58: Grammar and clarity improvements needed.
- Fix grammar in the dependency statement
- Consider adding specific SDK version requirements
Apply these changes:
-Clang 18 or later is required. You must also obtain the macOS SDK before -proceeding with a cross-compile. Under the depends directory, create a +Clang 18 or later is required. You must also obtain the macOS SDK before +proceeding with a cross-compile. Under the depends directory, create a +subdirectory named `SDKs`. Then, place the extracted SDK under this new directory. -subdirectory named `SDKs`. Then, place the extracted SDK under this new directory.🧰 Tools
🪛 LanguageTool
[grammar] ~57-~57: The verb ‘depend’ requires the preposition ‘on’ (or ‘upon’).
Context: ...ceeding with a cross-compile. Under the depends directory, create a subdirectory named ...(DEPEND_ON)
depends/Makefile (1)
210-211: Fix comment line wrapping.The comment lines are not properly aligned with the surrounding text. Consider adjusting the indentation to match the rest of the comment block.
-# it needs to be referred to by its absolute path, such as would be output -# by the AC_PATH_{PROG,TOOL} macros. +# it needs to be referred to by its absolute path, such as would be output by +# the AC_PATH_{PROG,TOOL} macros.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/macdeploy/background.tiffis excluded by!**/*.tiff
📒 Files selected for processing (41)
.gitignore(1 hunks)Makefile.am(3 hunks)ci/test/00_setup_env_mac.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_fuzz_with_valgrind.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_tsan.sh(1 hunks)ci/test/00_setup_env_native_ubsan.sh(1 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)configure.ac(2 hunks)contrib/containers/ci/Dockerfile(2 hunks)contrib/devtools/symbol-check.py(2 hunks)contrib/devtools/test-security-check.py(1 hunks)contrib/guix/libexec/build.sh(3 hunks)contrib/guix/libexec/codesign.sh(1 hunks)contrib/guix/manifest.scm(2 hunks)contrib/macdeploy/README.md(2 hunks)contrib/macdeploy/macdeployqtplus(4 hunks)depends/Makefile(2 hunks)depends/README.md(2 hunks)depends/builders/darwin.mk(2 hunks)depends/builders/default.mk(1 hunks)depends/config.site.in(2 hunks)depends/funcs.mk(1 hunks)depends/gen_id(0 hunks)depends/hosts/darwin.mk(3 hunks)depends/hosts/default.mk(1 hunks)depends/packages/gmp.mk(2 hunks)depends/packages/native_cctools.mk(0 hunks)depends/packages/native_ds_store.mk(0 hunks)depends/packages/native_libtapi.mk(0 hunks)depends/packages/native_llvm.mk(0 hunks)depends/packages/native_mac_alias.mk(0 hunks)depends/packages/packages.mk(0 hunks)depends/packages/qt.mk(3 hunks)depends/patches/gmp/include_ldflags_in_configure.patch(1 hunks)depends/patches/native_libtapi/disable_zlib.patch(0 hunks)depends/patches/qt/mac-qmake.conf(1 hunks)depends/patches/qt/no_warnings_for_symbols.patch(1 hunks)doc/build-osx.md(2 hunks)doc/release-process.md(1 hunks)
💤 Files with no reviewable changes (8)
- depends/packages/packages.mk
- depends/patches/native_libtapi/disable_zlib.patch
- depends/packages/native_libtapi.mk
- depends/gen_id
- depends/packages/native_ds_store.mk
- depends/packages/native_cctools.mk
- depends/packages/native_mac_alias.mk
- depends/packages/native_llvm.mk
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- doc/release-process.md
- depends/patches/qt/no_warnings_for_symbols.patch
🧰 Additional context used
🪛 LanguageTool
depends/README.md
[grammar] ~57-~57: The verb ‘depend’ requires the preposition ‘on’ (or ‘upon’).
Context: ...ceeding with a cross-compile. Under the depends directory, create a subdirectory named ...
(DEPEND_ON)
contrib/macdeploy/README.md
[style] ~69-~69: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...n order for the build process to remain somewhat deterministic. Here's how it works: - ...
(SOMEWHAT)
🔇 Additional comments (48)
depends/patches/gmp/include_ldflags_in_configure.patch (1)
16-17: Excellent job incorporating $LDFLAGS into the compilation commands.All these additions ensure that cross-compilation scenarios (particularly when using
lld) handle linker flags properly. This patch substantially improves compatibility and reduces the risk of missing linker arguments.Also applies to: 26-27, 35-36, 44-45, 53-54, 61-62, 75-76, 84-85, 93-94, 102-103, 111-112, 120-121, 129-130, 138-139, 147-148, 156-157, 165-166, 174-175, 183-184, 192-193, 201-202, 210-211, 219-220, 228-229, 237-238, 246-247, 255-256, 264-265, 273-274, 282-283, 291-292, 300-301, 309-310, 318-319, 327-328, 336-337, 345-346, 354-355, 363-364, 372-373, 381-382, 390-391, 399-400, 408-409, 417-418, 426-427, 435-436, 444-445, 453-454, 462-463, 471-472, 480-481, 489-490, 498-499, 507-508, 516-517, 525-526, 534-535, 543-544, 552-553, 565-566, 573-574, 583-584, 593-594, 604-605, 610-611, 618-619
depends/builders/default.mk (1)
16-16: Adding DSYMUTIL to the list of build tools is a welcome enhancement.By defining DSYMUTIL in this list, you ensure that symbol management is consistently integrated into the build process across various configurations.
depends/packages/gmp.mk (2)
6-6: Explicitly listing the patch for GMP is clear and maintainable.Including
include_ldflags_in_configure.patchin$(package)_patchesclarifies that it must be applied to GMP before configuration, preventing any confusion about where the patch should be used.
15-17: Applying the patch during the preprocess step is well-sequenced.Applying the patch right before the configure commands ensures changes to the build script are in effect during the entire GMP build process. This avoids partial or missed merges.
depends/patches/qt/mac-qmake.conf (1)
16-16: Utilizing lld for C++ linking.Appending
-fuse-ld=lldcan reduce compilation time and provide consistency if you require the LLVM toolchain. However, confirm that the environment always haslldavailable, especially in CI or user setups.depends/builders/darwin.mk (1)
8-8: Introducing dsymutil for Darwin is a sensible step.The commands
xcrun -f dsymutilensure a correct path todsymutil, enabling improved debugging symbol processing. For older or unconventional Xcode installations, consider verifying whetherxcrunreferences the tool properly.Also applies to: 20-20
depends/hosts/default.mk (1)
41-41: Ensure DSYMUTIL is consistently handled in all host tool contexts.By adding
DSYMUTILto the loop, you are making it part of the host toolchain configuration. Make sure that other build scripts or environment paths are aware of this addition (e.g., verifying it is installed under the intended name, ensuring the correct references in config files, etc.).depends/hosts/darwin.mk (3)
5-5: Confirm backward compatibility of LLD version.You have
LLD_VERSION=711; verify that older or explicitly supported build environments using different LLD versions won’t break. This is especially relevant when cross-compiling or building on systems with older package versions.
68-69: macOS minimum version flags look correct.The shift to
-mmacos-version-min=$(OSX_MIN_VERSION)aligns with modern conventions. This is appropriate as long as build machines use macOS ≥ 11.x. Otherwise, consider a fallback or user override for older machines.
73-75: Check linking flags for consistency across builds.Adding
-mlinker-version=$(LLD_VERSION)and-Wl,-no_adhoc_codesign -fuse-ld=lldis good for reproducible builds. Ensure these flags only apply on non-darwin hosts, as intended, and thatlldis always present or gracefully handled if not found.depends/config.site.in (2)
103-103: Renamed or newly introduced AR variable detection is correct.The assignment to
ac_cv_path_ARensures the correct AR tool is picked up. No further changes appear necessary here.
124-125: Refine OBJDUMP usage check on Darwin.You renamed the variable to
ac_cv_path_OBJDUMP. Confirm that references within the build system (like scripts or other config tests) also use this new variable.contrib/devtools/test-security-check.py (1)
124-132: Maintain coverage for Mach-O security checks.These test calls comprehensively validate
NOUNDEFS,Canary,FIXUP_CHAINS, and more. The expanded check for “-fcf-protection=full” ensures robust coverage. Keep an eye on future linker or Clang changes that may require new flags.Makefile.am (4)
37-37: Use consistent naming for ZIP artifact.
Great job introducingOSX_ZIP. Ensure that all references to the file name, including documentation, comments, and scripts, are updated to reduce confusion in the future.
120-121: Validate environment for Python usage.
You've introduced a Python command to build the ZIP. Confirm that Python is available in all build environments—especially for cross-compiling setups—and consider adding a check in the configure script if it’s critical.
127-129: Confirm deterministic timestamps in deployment.
You're touching timestamps withSOURCE_DATE_EPOCH. Great for reproducible builds! Ensure it also covers newly created directories and any non-file artifacts to guarantee fully deterministic outputs.
296-296: Include the new macOS ZIP artifact in CLEANFILES.
Including$(OSX_ZIP)inCLEANFILESis correct. No further issues here.depends/funcs.mk (1)
49-49: Clarify removed toolchain dependencies.
Removing extended dependencies from$(1)_all_dependencieshelps simplify the build flow, but ensure you have verified that no downstream packages rely on the removed references.depends/packages/qt.mk (3)
18-18: Watch for potential merge conflicts in patch lists.
Addingno_warnings_for_symbols.patchis fine, but keep an eye on the order of patch application if future patches touch the same code regions.
142-142: Check clang's CROSS_COMPILE usage.
Switching tollvm-is usually stable, but verify thatllvm-objcopy,llvm-strip, etc., are installed on all cross-compiling hosts you target. Missing tools could break certain build scenarios.
252-252: Consolidate patch application steps.
The new patch application step is correct, but watch out for patch dependencies. If “no_warnings_for_symbols.patch” modifies build scripts or flags, confirm they are used early enough so subsequent patches can benefit.contrib/macdeploy/macdeployqtplus (4)
386-386: Documentation updated to ZIP approach.
The comment accurately reflects the switch from DMG to ZIP. Make sure any upstream references in docs or scripts are also updated to avoid confusion.
397-397: New “-zip” option introduced.
The new argument ensures backward compatibility as it’s an optional flag. Looks good.
418-420: Prevent leftover artifacts from interfering.
Removing existingappname.zipif present is a good housekeeping measure. Confirm you handle potential concurrency issues if multiple builds run in parallel.
500-501: Use ofshutil.make_archiveis concise and effective.
Great choice to create ZIP archives in a single step. Confirm that thezipformat is widely supported by environment constraints.contrib/guix/manifest.scm (3)
596-602: Double-check the interplay of zip packaging with Windows cross-compilation.Using
zipfor Windows packaging is consistent with cross-platform distribution. Ensure that this transition doesn't conflict with scripts or build stages still referencing.exeinstallers or alternative packaging formats like.msi.
580-580: Consider verifying compatibility with older GCC versions.When introducing
gcc-toolchain-12, ensure that no build consumers still expectgcc-toolchain-9orgcc-toolchain-11. Confirm downstream scripts or environments are prepared for GCC 12.✅ Verification successful
No compatibility issues found with GCC 12 upgrade
Based on the search results, the codebase appears to be compatible with GCC 12. The findings show:
The project is already using GCC 12 as the base compiler in the Guix build system:
contrib/guix/manifest.scmdefinesbase-gccas inheriting fromgcc-12- No references to GCC 9 or 11 were found in the codebase
The build system is designed to handle GCC version changes:
- Cross-compilation toolchains are properly configured
- Build scripts use version-agnostic GCC environment variables and paths
- No hard dependencies on specific GCC versions were found
The only GCC-specific code found relates to:
- Generic GCC configuration for cross-compilation
- Standard GCC toolchain utilities (ar, nm, ranlib)
- Common GCC build flags and paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if references to older GCC toolchains exist in the repository. rg -A 3 "gcc-toolchain-(9|11)"Length of output: 32
Script:
#!/bin/bash # Let's check for any gcc toolchain references in a broader context rg -i "gcc.?toolchain" # Also check specifically in build-related files fd -e mk -e cmake -e scm -e sh | xargs rg -i "gcc" # Check the git history for recent changes related to gcc toolchain git log --oneline -n 10 | grep -i "gcc"Length of output: 20796
605-609: Validate clang-toolchain-18 and lld-18 references on macOS.Confirm that both
clang-toolchain-18andlld-18are available in the targeted Guix channels. Also confirm compatibility with the new Darwin-based environment to avoid potential breakages in older macOS versions.configure.ac (1)
2014-2014: Ensure AR and ARFLAGS are honored correctly in all build scenarios.Printing the archiver and its flags is helpful, but confirm that your updated toolchain environment actually references the correct archiver (e.g.,
llvm-arvs.ar). If you expect to usellvm-ar, consider validating.✅ Verification successful
Let me gather more information about AR usage in the build system.
AR variable is properly configured across different build environments
The build system correctly handles AR configuration across different platforms and build scenarios:
- For Android builds: Uses
llvm-arfrom the Android toolchain- For Darwin/macOS: Dynamically selects
llvm-arif available- For Linux: Uses
gcc-arwith fallback to standardar- For BSD systems: Uses platform-specific archiver configurations
- The Rust bindings explicitly use
llvm-arwhen needed- Dependencies properly inherit and respect the AR variable settings
The system appropriately determines and propagates the archiver tool based on the host platform and toolchain, with proper fallbacks in place. The printing of AR in configure.ac serves as a useful diagnostic output.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for references or usage of llvm-ar in the build scripts rg "llvm-ar"Length of output: 434
Script:
#!/bin/bash # Check for AR variable references and assignments in build files rg -g '*.{ac,am,mk,mak}' 'AR\s*[:=]' # Check for ar command usage patterns rg -g '*.{ac,am,mk,mak}' '\bar\b'Length of output: 2659
ci/test/00_setup_env_native_multiprocess.sh (1)
14-14: Confirm Clang 18 availability in CI environment.Switching from Clang 16 to Clang 18 is significant. Ensure your CI images or runners include Clang 18 by default to avoid build failures.
✅ Verification successful
Clang 18 is properly configured in the CI environment
The CI environment is set up correctly for Clang 18. The Dockerfile in
contrib/containers/ci/Dockerfileexplicitly installs Clang 18 and all necessary components through the LLVM repository. The installation includes:
- Base Clang 18 compiler
- Additional tools like clang-format, clang-tidy
- Required libraries (libc++, libc++abi, libclang-rt)
- Sets up proper alternatives and paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Attempt to confirm clang-18 presence which clang-18 || { echo "clang-18 not found" >&2; exit 1; }Length of output: 168
Script:
#!/bin/bash # Check for clang-18 using alternative methods and inspect CI configuration command -v clang-18 || echo "clang-18 not found" command -v clang++-18 || echo "clang++-18 not found" # Look for CI configuration files that might specify the Clang version fd -e yml -e yaml . .github/workflows/ || true cat .github/workflows/*.yml .github/workflows/*.yaml 2>/dev/null || true # Check if there are any Dockerfiles or container configurations fd Dockerfile || true cat Dockerfile* 2>/dev/null || trueLength of output: 20390
Script:
#!/bin/bash # Check CI container Dockerfile for clang-18 installation cat contrib/containers/ci/Dockerfile # Check if there are any scripts that install or configure clang fd -t f -e sh . ci/ | xargs grep -l "clang" || trueLength of output: 6974
ci/test/00_setup_env_native_fuzz_with_valgrind.sh (1)
17-17: Verify Valgrind compatibility with Clang 18.Let's ensure the current valgrind version is compatible with Clang 18 instrumentation.
✅ Verification successful
Let me try a different approach to verify this.
Let me try one more search to find specific valgrind configuration details.
Let me try one final search to check for any specific valgrind configurations in the test setup.
Clang 18 and Valgrind compatibility is properly configured
Based on the analysis of the configuration files and test setup:
- The Valgrind test environment is properly set up with necessary suppressions for known issues
- The test wrapper script (
wrap-valgrind.sh) correctly configures Valgrind with the required suppressions- The fuzz testing configuration combines Clang 18 with Valgrind using appropriate flags:
- Uses
--valgrindfor fuzz tests- Configures with
--enable-fuzz --with-sanitizers=fuzzer- Explicitly sets
CC=clang-18 CXX=clang++-18The setup ensures compatibility between Clang 18 and Valgrind through proper configuration and suppression of known issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check valgrind version in current package repositories apt-cache show valgrind | grep -E "^Version:" # Search for any known compatibility issues between Clang 18 and Valgrind rg -A 3 "clang.*18|LLVM.*18" --glob "CHANGELOG*" rg -A 3 "valgrind" --glob "CHANGELOG*"Length of output: 237
Script:
#!/bin/bash # Check if there are any specific valgrind configurations or version requirements fd -t f "configure.ac|README.md|doc/dependencies.md" --exec grep -l "valgrind" # Look for valgrind-related build configurations fd -t f "*.sh|*.ac|*.am" --exec grep -i "valgrind" # Check for any specific clang version requirements fd "README.md|doc/dependencies.md" --exec grep -i "clang"Length of output: 1376
Script:
#!/bin/bash # Check contents of the files that mentioned valgrind cat ./src/test/README.md cat ./src/secp256k1/configure.ac # Look for valgrind test configurations fd -g "*.sh" -E "*.git*" --exec grep -l "valgrind" # Check configure options related to valgrind fd "configure" --type f --exec grep -i "valgrind"Length of output: 25664
Script:
#!/bin/bash # Check valgrind test configuration cat ./ci/test/00_setup_env_native_fuzz_with_valgrind.sh cat ./ci/test/wrap-valgrind.sh # Look for any valgrind suppressions fd -g "*.supp" --exec cat {}Length of output: 5166
ci/test/00_setup_env_native_ubsan.sh (1)
11-11: LGTM! Consistent version updates.The changes correctly update both PACKAGES and BITCOIN_CONFIG to use Clang 18, maintaining consistency across the configuration.
Also applies to: 14-14
ci/test/00_setup_env_native_fuzz.sh (1)
19-19: Verify Clang 18 availability in CI environmentThe update to Clang 18 aligns with the PR objectives. However, we should ensure:
- Clang 18 packages are available in the CI environment
- The fuzzer configuration remains compatible with the new compiler version
✅ Verification successful
Let me try a different approach to verify the Clang 18 configuration in the CI environment.
Let me check the container configuration to verify Clang 18 availability.
Clang 18 is properly configured in the CI environment
The verification shows that:
- Clang 18 is explicitly installed in the CI container through LLVM's official repository (see
contrib/containers/ci/Dockerfile)- The container setup includes all necessary Clang 18 components, including those needed for fuzzing
- Multiple test environments are consistently configured to use Clang 18, including:
- Native fuzzing
- Fuzzing with Valgrind
- Native Valgrind tests
The update from Clang 16 to 18 is properly supported by the CI infrastructure and the fuzzer configuration remains compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if clang-18 packages are available in the CI environment apt-cache policy clang-18 clang++-18 # Verify if there are any known issues with Clang 18 fuzzer integration rg -A 3 "clang.*18.*fuzz" .github/ ci/Length of output: 179
Script:
#!/bin/bash # Check CI configuration files for Clang 18 setup rg -l "clang.*18" .github/workflows/ ci/ # Check for any fuzzing-related configurations rg "fuzz.*clang|clang.*fuzz" .github/workflows/ ci/ # Look for Dockerfile or container configurations that might set up Clang fd Dockerfile fd "container" --type f .github ciLength of output: 1215
Script:
#!/bin/bash # Check CI container configuration for Clang setup cat contrib/containers/ci/Dockerfile # Check other test environment files that use Clang 18 cat ci/test/00_setup_env_native_fuzz_with_valgrind.sh cat ci/test/00_setup_env_native_valgrind.shLength of output: 8118
ci/test/00_setup_env_native_tsan.sh (2)
10-11: Verify completeness of package dependenciesThe package list has been updated for Clang 18, but we should ensure all required LLVM components are included for ThreadSanitizer functionality.
✅ Verification successful
Let me try another verification approach to check the package dependencies.
Package dependencies for TSAN build are complete and correct
Based on the verification results, the package list in
00_setup_env_native_tsan.shis complete and correctly specified for ThreadSanitizer functionality with Clang 18:
- The current packages include all essential components:
clang-18andllvm-18for the base toolchainlibclang-rt-18-devwhich provides the runtime libraries including TSANlibc++abi-18-devandlibc++-18-devfor C++ standard library supportpython3-zmqfor testing infrastructureComparing with other sanitizer configurations in the codebase and the developer documentation, this setup provides all necessary dependencies for ThreadSanitizer functionality. The TSAN-specific runtime is included via
libclang-rt-18-devpackage, which is correctly versioned to match Clang 18.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if all required TSAN dependencies are available apt-cache depends clang-18 llvm-18 libclang-rt-18-dev | grep "Depends:" # Verify if any additional LLVM components are typically required for TSAN rg -l "ThreadSanitizer" $(fd -t f . ./doc)Length of output: 4111
Script:
#!/bin/bash # Search for other TSAN setup files in the codebase rg -l "TSAN|ThreadSanitizer" $(fd -t f "*.sh" ./ci) # Look for any other package dependencies in similar setup files rg "PACKAGES.*clang" $(fd -t f "*.sh" ./ci) # Check the developer notes for TSAN requirements rg -A 5 -B 5 "ThreadSanitizer|TSAN" ./doc/developer-notes.mdLength of output: 3527
15-15: Ensure TSAN compatibility with Clang 18The configuration updates the compiler but maintains existing sanitizer settings. Let's verify ThreadSanitizer compatibility with Clang 18.
✅ Verification successful
ThreadSanitizer configuration is properly set up for Clang 18
The verification shows that the TSAN configuration is properly maintained:
- The CI environment correctly sets up TSAN with Clang 18 including required packages (
libclang-rt-18-dev,libc++abi-18-dev,libc++-18-dev)- ThreadSanitizer suppressions are properly configured via
TSAN_OPTIONS- The build configuration correctly specifies thread sanitizer with
--with-sanitizers=thread- The compiler flags and settings are consistent with other sanitizer configurations in the CI
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any known TSAN issues with Clang 18 rg -A 3 "thread.*sanitizer.*clang-18|tsan.*clang-18" ./doc ./ci # Check if there are any specific TSAN flags needed for Clang 18 ast-grep --pattern 'CXXFLAGS.*-fsanitize=thread'Length of output: 282
Script:
#!/bin/bash # Let's check for any TSAN-related configurations and flags in CI files rg -i "tsan|thread.*sanitizer" ./ci # Check the current compiler configurations in CI rg "clang-18" ./ci # Look for any sanitizer-related build configurations rg "with-sanitizers" ./ciLength of output: 3497
contrib/guix/libexec/codesign.sh (1)
81-82: LGTM! Verify ZIP file contents match DMG contents.The change from DMG to ZIP packaging looks correct. The implementation properly handles paths and maintains error handling.
Let's verify that all required files are included in the ZIP:
✅ Verification successful
Let me gather more information about the DMG packaging to ensure we're not missing any files.
Let me check one more thing to understand what files are being signed before packaging.
Based on the gathered information, I can now provide the final response:
ZIP packaging correctly includes all required files from dist/
The change from DMG to ZIP is correctly implemented and maintains all necessary files:
- The
dist/Dash-Qt.appis properly code-signed before packaging- The
zipcommand captures all contents from thedist/directory, which matches the previous DMG contents- The packaging process maintains the same workflow: signing -> packaging -> output
The implementation follows the same pattern used in the existing macdeployqtplus script, which also creates ZIP archives from the same
dist/Dash-Qt.appcontent, confirming that the ZIP format is a suitable replacement for DMG.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare files included in ZIP vs previously included in DMG # Look for any references to DMG contents in the codebase to ensure we're not missing files rg -A 5 "\.dmg" --glob '!*.{md,txt}'Length of output: 6208
Script:
#!/bin/bash # Check for DMG creation and packaging logic in the codebase rg -A 10 "create.*dmg|make.*dmg" --glob '!*.{md,txt}' # Check the directory where dist/* files are created fd -t f "^codesign.sh$" --exec grep -A 10 "dist/"Length of output: 542
Script:
#!/bin/bash # Check what files are being signed and included in the package rg -B 5 -A 5 "Dash-Qt.app" --glob '!*.{md,txt}' # Also check if there are any other files that should be included fd -t f "^codesign.sh$" --exec grep -B 5 -A 5 "mkdir.*dist"Length of output: 3440
depends/README.md (2)
48-51: LGTM! Common dependencies are well organized.The new common dependencies section improves clarity by separating shared requirements from platform-specific ones.
69-69: LGTM! Dependencies simplified.The linux dependencies have been appropriately streamlined to essential packages.
contrib/containers/ci/Dockerfile (2)
58-58: LGTM! LLVM version update and installation method.The update to LLVM 18 and the installation method using the official script is correct.
Also applies to: 61-61
166-166: LGTM! Added zip package for macOS packaging.The addition of the zip package aligns with the transition from DMG to ZIP packaging.
contrib/devtools/symbol-check.py (2)
254-256: LGTM: Function updated to check for LLD version.The function has been correctly renamed from
check_MACHO_ld64tocheck_MACHO_lldand updated to verify LLD version 18.1.6, which aligns with the PR's objective of updating to Clang 18.
298-298: LGTM: CHECKS dictionary updated consistently.The CHECKS dictionary entry has been updated to use the new
check_MACHO_lldfunction name, maintaining consistency with the function rename.doc/build-osx.md (2)
216-217: LGTM: Updated deployment format documentation.Documentation correctly reflects the change from DMG to ZIP packaging format and clearly states the Python requirement.
286-286: LGTM: Added ZIP deployment command example.Clear documentation of the
make deploycommand for creating ZIP packages.depends/Makefile (1)
226-226: LGTM: Added DSYMUTIL variable substitution.The addition of DSYMUTIL variable substitution in config.site generation is consistent with other tool variables and correctly implements the PR's objective of ensuring proper DSYMUTIL definition.
contrib/guix/libexec/build.sh (3)
138-138: LGTM! Clean consolidation of RPATH configuration.The consolidation of
GUIX_LD_WRAPPER_DISABLE_RPATHexport improves maintainability while preserving the same functionality.
307-307: LGTM! Transition from DMG to ZIP for macOS builds.The change from DMG to ZIP format for macOS builds aligns with the PR objectives and simplifies the build process.
Let's ensure the documentation is updated to reflect this change:
183-183: Verify toolchain compatibility after FORCE_USE_SYSTEM_CLANG removal.The removal of
FORCE_USE_SYSTEM_CLANGaligns with the toolchain updates mentioned in the PR objectives. However, let's verify there are no unintended consequences.Run this script to check for any remaining references to system Clang:
✅ Verification successful
Let me check the remaining reference in the lint script to understand the context better.
Let me check for any other Clang-related configurations or dependencies in the build system.
Toolchain compatibility verified after FORCE_USE_SYSTEM_CLANG removal
The removal of
FORCE_USE_SYSTEM_CLANGis safe and compatible with the current toolchain setup. Here's why:
- The only remaining Clang reference is in
ci/lint/04_install.shwhich is specifically for code formatting tools (clang-format-9) and doesn't affect the build toolchain- The build system in
contrib/guix/correctly handles toolchain selection:
- Uses GCC toolchain for Linux builds (x86_64-linux-gnu-gcc/g++)
- Uses Clang toolchain 18 specifically for Darwin/macOS builds through proper Guix package management
- All compiler flags and environment variables are properly configured for each target platform
The removal aligns well with the structured toolchain management in the Guix build system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to system Clang that might need updating # Search for system Clang references in build scripts rg -l "FORCE_USE_SYSTEM_CLANG|/usr/bin/clang|/usr/local/bin/clang" # Search for Clang-related configuration ast-grep --pattern 'FORCE_USE_SYSTEM_CLANG'Length of output: 300
Script:
#!/bin/bash # Check the context of the remaining reference in the lint script rg -A 5 -B 5 "FORCE_USE_SYSTEM_CLANG|/usr/bin/clang|/usr/local/bin/clang" ci/lint/04_install.shLength of output: 619
Script:
#!/bin/bash # Check for Clang-related configurations in build scripts rg -A 3 -B 3 "clang|CLANG" contrib/guix/ # Also check for any compiler-related configurations that might be affected rg -A 3 -B 3 "CC=|CXX=|CFLAGS|CXXFLAGS" contrib/guix/Length of output: 5044
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6516.ee0a9f2b. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6516.ee0a9f2b. The image should be on dockerhub soon. |
…OURCE_DATE_EPOCH
Needed to ensure macOS cross-compilation doesn't break when moving to an LLD based toolchain, which will happen in the next commit.
…or `mmacos-version-min`
Expected hashes for ee04e1f |
kwvg
changed the title
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6516.ee04e1f5. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
depends/patches/gmp/include_ldflags_in_configure.patch (1)
1-621: Consider adding test cases for macOS cross-compilation.While the changes look correct, it would be beneficial to have specific test cases that verify the patch works as intended for macOS cross-compilation scenarios.
Consider adding:
- Test cases that specifically exercise the macOS cross-compilation path
- Verification steps in the build process to ensure lld is being used correctly
- Documentation for cross-compilation setup and requirements
depends/funcs.mk (1)
Line range hint
214-234: Review CMAKE cross-compilation configurationThe CMAKE configuration block correctly handles cross-compilation settings, which is crucial for the Clang 18 update mentioned in the PR. However, consider adding a comment explaining the CMAKE cross-compilation flags for better maintainability.
Add documentation above the CMAKE cross-compilation block:
ifeq ($($(1)_type),build) $(1)_cmake += -DCMAKE_INSTALL_RPATH:PATH="$$($($(1)_type)_prefix)/lib" else +# Configure CMAKE for cross-compilation when host differs from build ifneq ($(host),$(build)) $(1)_cmake += -DCMAKE_SYSTEM_NAME=$($(host_os)_cmake_system) $(1)_cmake += -DCMAKE_C_COMPILER_TARGET=$(host) $(1)_cmake += -DCMAKE_CXX_COMPILER_TARGET=$(host) endif endifcontrib/macdeploy/README.md (1)
57-69: Improve clarity in deterministic build descriptionConsider removing the word "somewhat" as it introduces uncertainty. The build process is either deterministic or not.
-for the build process to remain somewhat deterministic. Here's how it works: +for the build process to remain deterministic. Here's how it works:🧰 Tools
🪛 LanguageTool
[style] ~69-~69: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...n order for the build process to remain somewhat deterministic. Here's how it works: - ...(SOMEWHAT)
depends/README.md (1)
56-58: Fix grammar in SDK instructionsThe preposition is missing in the directory reference.
-proceeding with a cross-compile. Under the depends directory, create a +proceeding with a cross-compile. Under the depends/ directory, create a🧰 Tools
🪛 LanguageTool
[grammar] ~57-~57: The verb ‘depend’ requires the preposition ‘on’ (or ‘upon’).
Context: ...ceeding with a cross-compile. Under the depends directory, create a subdirectory named ...(DEPEND_ON)
contrib/macdeploy/macdeployqtplus (1)
500-501: Consider improving the ZIP creation logic.
- Add error handling around the
make_archivecall to gracefully handle potential failures.- Consider making the output app name configurable instead of hardcoding 'Dash-Qt.app'.
if config.zip is not None: - shutil.make_archive('{}'.format(appname), format='zip', root_dir='dist', base_dir='Dash-Qt.app') + try: + output_app = os.getenv('OUTPUT_APP_NAME', 'Dash-Qt.app') + shutil.make_archive('{}'.format(appname), format='zip', root_dir='dist', base_dir=output_app) + except Exception as e: + sys.stderr.write(f"Error creating ZIP archive: {str(e)}\n") + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/macdeploy/background.tiffis excluded by!**/*.tiff
📒 Files selected for processing (40)
.gitignore(1 hunks)Makefile.am(3 hunks)ci/test/00_setup_env_mac.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_fuzz_with_valgrind.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_tsan.sh(1 hunks)ci/test/00_setup_env_native_ubsan.sh(1 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)configure.ac(2 hunks)contrib/containers/ci/Dockerfile(2 hunks)contrib/devtools/symbol-check.py(2 hunks)contrib/devtools/test-security-check.py(1 hunks)contrib/guix/libexec/build.sh(3 hunks)contrib/guix/libexec/codesign.sh(1 hunks)contrib/guix/manifest.scm(2 hunks)contrib/macdeploy/README.md(2 hunks)contrib/macdeploy/macdeployqtplus(4 hunks)depends/Makefile(2 hunks)depends/README.md(2 hunks)depends/builders/darwin.mk(2 hunks)depends/config.site.in(2 hunks)depends/funcs.mk(1 hunks)depends/gen_id(0 hunks)depends/hosts/darwin.mk(3 hunks)depends/packages/gmp.mk(2 hunks)depends/packages/native_cctools.mk(0 hunks)depends/packages/native_ds_store.mk(0 hunks)depends/packages/native_libtapi.mk(0 hunks)depends/packages/native_llvm.mk(0 hunks)depends/packages/native_mac_alias.mk(0 hunks)depends/packages/packages.mk(0 hunks)depends/packages/qt.mk(3 hunks)depends/patches/gmp/include_ldflags_in_configure.patch(1 hunks)depends/patches/native_libtapi/disable_zlib.patch(0 hunks)depends/patches/qt/mac-qmake.conf(1 hunks)depends/patches/qt/no_warnings_for_symbols.patch(1 hunks)doc/build-osx.md(2 hunks)doc/release-notes-6516.md(1 hunks)doc/release-process.md(1 hunks)
💤 Files with no reviewable changes (8)
- depends/patches/native_libtapi/disable_zlib.patch
- depends/gen_id
- depends/packages/native_libtapi.mk
- depends/packages/native_cctools.mk
- depends/packages/packages.mk
- depends/packages/native_ds_store.mk
- depends/packages/native_mac_alias.mk
- depends/packages/native_llvm.mk
✅ Files skipped from review due to trivial changes (1)
- doc/release-notes-6516.md
🚧 Files skipped from review as they are similar to previous changes (22)
- doc/release-process.md
- ci/test/00_setup_env_mac.sh
- ci/test/00_setup_env_native_fuzz_with_valgrind.sh
- ci/test/00_setup_env_native_multiprocess.sh
- depends/patches/qt/no_warnings_for_symbols.patch
- .gitignore
- ci/test/00_setup_env_native_valgrind.sh
- depends/packages/gmp.mk
- contrib/devtools/test-security-check.py
- contrib/devtools/symbol-check.py
- ci/test/00_setup_env_native_fuzz.sh
- ci/test/00_setup_env_native_ubsan.sh
- depends/builders/darwin.mk
- depends/config.site.in
- depends/patches/qt/mac-qmake.conf
- depends/Makefile
- ci/test/00_setup_env_native_tsan.sh
- contrib/guix/manifest.scm
- Makefile.am
- contrib/guix/libexec/build.sh
- depends/hosts/darwin.mk
- configure.ac
🧰 Additional context used
🪛 LanguageTool
contrib/macdeploy/README.md
[style] ~69-~69: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...n order for the build process to remain somewhat deterministic. Here's how it works: - ...
(SOMEWHAT)
depends/README.md
[grammar] ~57-~57: The verb ‘depend’ requires the preposition ‘on’ (or ‘upon’).
Context: ...ceeding with a cross-compile. Under the depends directory, create a subdirectory named ...
(DEPEND_ON)
🔇 Additional comments (19)
contrib/containers/ci/Dockerfile (5)
68-69: LGTM: Required LLVM runtime packagesThe addition of
libclang-rtandlldpackages is necessary for the complete toolchain setup.
73-78: Great: Addressed previous feedback about llvm-covThe update-alternatives configuration now includes
llvm-covas suggested in the previous review. The additional LLD symlinks are also properly configured.
166-166: LGTM: Package updates align with build system changesThe addition of the
zippackage aligns with the PR objectives of transitioning from DMG to ZIP files for macOS packaging.
58-61: LGTM: LLVM upgrade and installation methodThe upgrade to LLVM 18 and the use of the official llvm.sh installation script is a good improvement.
Let's verify the LLVM installation script's integrity:
✅ Verification successful
LLVM 18 installation script verified and supported
The LLVM installation script has been verified:
- LLVM version 18 is officially supported by the script
- The script includes proper version pattern matching for LLVM 18 (
LLVM_VERSION_PATTERNS[18]="-18")- The installation method follows the recommended approach using the official apt.llvm.org script
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the LLVM installation script's integrity and contents curl -sL https://apt.llvm.org/llvm.sh | sha256sum curl -sL https://apt.llvm.org/llvm.sh | grep -A 5 "LLVM_VERSION_PATTERN"Length of output: 996
Line range hint
58-166: Verify the complete toolchain setupThe changes create a comprehensive build environment. Let's verify the complete toolchain setup:
✅ Verification successful
Toolchain setup is properly configured and integrated
The verification results show that the LLVM toolchain (clang, clang++, lld, and other LLVM utilities) is well-integrated into the build system:
- The tools are referenced in key build configuration files (configure.ac, Makefile.am)
- Multiple components across the codebase use these tools, including:
- Core build system (configure.ac, depends/*)
- Test infrastructure (ci/test/*)
- Development tools (contrib/devtools/*)
- Various source components (src/*)
The specific version pinning in the Dockerfile (LLVM 18) aligns with the toolchain requirements found in the codebase, and the environment setup (LD_LIBRARY_PATH, alternatives) ensures proper tool resolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the complete toolchain setup # Check if all required tools are available with correct versions # Check LLVM tools versions echo "Checking LLVM tools versions..." for tool in clang clang++ lld llvm-ar llvm-nm llvm-objdump llvm-ranlib llvm-strip llvm-cov; do rg -l "${tool}(-[0-9]+)?" | grep -v "contrib/containers/ci/Dockerfile" done # Check build system dependencies on these tools echo "Checking build system dependencies..." rg -g 'configure.ac' -g 'Makefile.am' "zip|lld|llvm"Length of output: 11068
depends/patches/gmp/include_ldflags_in_configure.patch (2)
1-6: LGTM! Clear and informative patch description.The patch description effectively explains:
- The purpose: Including
LDFLAGSin test program compilation- The use case: macOS cross-compilation with lld
- The rationale: Avoiding unused argument warnings
8-556: LGTM! Systematic and consistent changes to acinclude.m4.The changes consistently add
$LDFLAGSto all compilation commands in various test scenarios. This ensures that linker flags are properly propagated throughout the build process.Let's verify the consistency of the changes:
✅ Verification successful
All compilation commands now properly include LDFLAGS
The patch systematically adds
$LDFLAGSto all compilation commands in bothacinclude.m4andconfigurefiles. The verification shows:
- All
gmp_compilecommands now include$LDFLAGS- All
gmp_cxxcompilecommands now include$LDFLAGS- All compiler test commands (CC/CXX) now include
$LDFLAGS- The changes are consistent across both test compilations and actual build commands
The patch correctly implements the needed changes to support macOS cross-compilation with lld linker by ensuring linker flags are properly propagated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all compilation commands include LDFLAGS # Check if any compilation commands were missed rg -U 'gmp_compile="[^"]*"' acinclude.m4 | rg -v '\$LDFLAGS'Length of output: 115
Script:
#!/bin/bash # Let's check both acinclude.m4 and configure files for any missed LDFLAGS # First, find the files fd -t f "acinclude.m4|configure$" # Then check their content for compilation commands without LDFLAGS echo "=== Checking compilation commands without LDFLAGS ===" fd -t f "acinclude.m4|configure$" -x rg -U 'gmp_compile="[^"]*"' {} | rg -v '\$LDFLAGS' # Also check for any other compilation-related variables echo -e "\n=== Checking other compilation variables ===" fd -t f "acinclude.m4|configure$" -x rg -U '(gmp_cxxcompile|cc_for_build|gmp_compile)="[^"]*"' {} | rg -v '\$LDFLAGS'Length of output: 471
Script:
#!/bin/bash # Let's check the patch file directly since we're not finding the source files echo "=== Checking the patch file ===" fd -t f "include_ldflags_in_configure.patch" -x cat {} # Also let's check if we can find any compilation commands that might have been missed echo -e "\n=== Checking for potentially missed compilation commands ===" fd -t f "include_ldflags_in_configure.patch" -x rg -U '([^L]CC|[^X]CXX)[= ].*(-c|-o)' {}Length of output: 26989
depends/packages/qt.mk (4)
Line range hint
1-300: LGTM: Overall Qt package configurationThe Qt package configuration is well-structured with appropriate build options, dependencies, and platform-specific configurations. The changes align with the PR objectives of updating the toolchain and improving the build process.
18-18: Verify the purpose and impact of symbol warning suppressionThe addition of
no_warnings_for_symbols.patchsuggests there are symbol-related warnings being suppressed. While this might clean up the build output, we should verify that no critical warnings are being hidden.
142-142: Verify LLVM toolchain availability for Darwin buildsThe change from
$(host)-tollvm-for CROSS_COMPILE aligns with the PR's objective of supporting an LLD-based toolchain, but requires verification of LLVM tools availability.✅ Verification successful
LLVM toolchain configuration is correctly handled for Darwin builds
The change from
$(host)-tollvm-for CROSS_COMPILE in Qt's configuration is correct and properly supported by the build system. Thedepends/hosts/darwin.mkfile explicitly configures all necessary LLVM tools through variables likedarwin_AR,darwin_NM,darwin_RANLIB, etc., which are set usingcommand -vto locate the LLVM tools at build time. While our sandbox environment doesn't have these tools, the actual build system:
- Properly detects LLVM tools availability at build time
- Falls back appropriately for native Darwin builds
- Configures cross-compilation correctly with explicit LLVM toolchain for non-Darwin builds
- Aligns with the project's move towards an LLD-based toolchain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify LLVM tools availability in the toolchain # Expected: All required LLVM tools should be present # Check for essential LLVM tools for tool in llvm-ar llvm-ranlib llvm-nm llvm-objcopy llvm-strip; do if ! command -v $tool &> /dev/null; then echo "Missing required LLVM tool: $tool" fi doneLength of output: 875
Script:
#!/bin/bash # Let's check the build system configuration to understand how LLVM tools are handled # First, check if there are any variables or configurations related to LLVM tools rg -l "llvm-" depends/ # Then check specifically for Darwin/macOS related configurations rg "darwin|DARWIN|macos|MACOS" depends/packages/qt.mk # Also check for any host toolchain related configurations rg "CROSS_COMPILE|HOST" depends/packages/qt.mkLength of output: 1704
Script:
#!/bin/bash # Let's check how the Darwin host is configured and if it handles LLVM tools differently cat depends/hosts/darwin.mk # Also check if there are any relevant variables set in the Android host config # since it also showed up in the LLVM search cat depends/hosts/android.mkLength of output: 3947
Line range hint
252-259: Verify patch application order and dependenciesThe new patches are properly added to the preprocessing commands. However, we should verify that there are no conflicts between patches and that they apply cleanly, especially with the Clang 18 update.
✅ Verification successful
Let me try a different approach to locate and examine the patches.
Let me check the content of these patches to verify their dependencies and potential conflicts.
Patches are compatible and correctly ordered
After analyzing the patches and their modifications, I can confirm that:
- The patches modify different files or different sections of code, avoiding any direct conflicts
- The order is appropriate:
no_warnings_for_symbols.patchmodifies build configurationclang_18_libpng.patchfixes Clang 18 compatibility- Other patches handle specific optimizations and features
The patches are independent of each other and can be safely applied in the specified order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check patch dependencies and application order # Expected: Patches should apply without conflicts # List all patches in application order echo "=== Patch application order ===" for patch in no_warnings_for_symbols.patch clang_18_libpng.patch rcc_hardcode_timestamp.patch duplicate_lcqpafonts.patch utc_from_string_no_optimize.patch fast_fixed_dtoa_no_optimize.patch guix_cross_lib_path.patch windows_lto.patch; do if [ -f "$(package)_patch_dir/$patch" ]; then echo "Found: $patch" else echo "Missing: $patch" fi doneLength of output: 3892
Script:
#!/bin/bash # Find all patch files in the repository echo "=== Finding patch files ===" fd -e patch # Check the content of depends/patches directory if it exists echo -e "\n=== Checking depends/patches directory ===" if [ -d "depends/patches" ]; then ls -la depends/patches fi # Look for patch files specifically in Qt-related directories echo -e "\n=== Looking for Qt patches ===" fd -e patch -p depends/patches/qtLength of output: 4026
Script:
#!/bin/bash # Check the content of the relevant patches echo "=== Checking clang_18_libpng.patch ===" cat depends/patches/qt/clang_18_libpng.patch echo -e "\n=== Checking no_warnings_for_symbols.patch ===" cat depends/patches/qt/no_warnings_for_symbols.patch echo -e "\n=== Checking utc_from_string_no_optimize.patch ===" cat depends/patches/qt/utc_from_string_no_optimize.patch echo -e "\n=== Checking fast_fixed_dtoa_no_optimize.patch ===" cat depends/patches/qt/fast_fixed_dtoa_no_optimize.patch # List modified files in each patch to check for overlaps echo -e "\n=== Files modified by each patch ===" for patch in depends/patches/qt/*.patch; do echo -e "\nPatch: $(basename $patch)" grep "^+++ " "$patch" | cut -f2- -d' ' | sort -u doneLength of output: 13948
depends/funcs.mk (2)
Line range hint
166-169: LGTM: Comprehensive LDFLAGS handlingThe LDFLAGS configuration is thorough and correct:
- Properly accumulates flags from different sources (release type, host arch, host OS)
- Correctly passed to both autoconf and CMAKE builds
- Handles the GMP build requirements mentioned in the PR objectives
Also applies to: 191-194
49-49: Verify removal of native toolchain dependenciesThe removal of native toolchain and binutils dependencies from
$(1)_all_dependenciescould impact packages that implicitly relied on these tools. While this change aligns with the toolchain backports, we should verify that all packages explicitly declare their toolchain requirements.Run the following script to identify packages that might be affected:
✅ Verification successful
Removal of native toolchain dependencies is safe
Based on the analysis of the codebase:
- The change to
$(1)_all_dependenciesonly affects explicit dependency resolution, not the actual toolchain usage.- Each package properly defines its own toolchain variables through
int_varsfunction, which sets CC, CXX, AR, etc., from the package's type-specific values.- Build tools are properly configured in
builders/default.mkand passed to packages through the build system.- Packages that need compilation tools (like qt, libnatpmp, miniupnpc) explicitly set their build environment with the correct toolchain variables.
- No package directly depends on
native_toolchainornative_binutilsin their dependency declarations.The removal of implicit native toolchain dependencies is safe as the build system correctly handles toolchain configuration through the type-specific variables and builder configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for packages that might need explicit toolchain dependencies # Look for packages using compiler/linker tools without declaring dependencies # Find all package makefiles echo "Checking package definitions..." fd -e mk . packages/ -x sh -c ' # Extract package name from filename pkg=$(basename {} .mk) # Look for compiler/linker tool usage without corresponding dependencies if grep -q "CC\|CXX\|LD\|AR\|RANLIB" {} && ! grep -q "dependencies.*native_toolchain\|dependencies.*native_binutils" {}; then echo "Package $pkg uses build tools but may need explicit toolchain dependencies" echo "Referenced tools:" grep -h "CC=\|CXX=\|LD=\|AR=\|RANLIB=" {} fi 'Length of output: 641
Script:
#!/bin/bash # First, let's check if the packages directory exists with a different name or location echo "Finding package definitions..." fd -t f -e mk . depends/ # Now let's examine the funcs.mk to understand the dependency resolution echo -e "\nChecking funcs.mk for dependency handling..." cat depends/funcs.mk # Let's also check if there are any other makefiles that might define toolchain dependencies echo -e "\nChecking other makefiles for toolchain references..." rg -l "native_toolchain|native_binutils" depends/Length of output: 15877
Script:
#!/bin/bash # Let's check how dependencies are defined in package makefiles echo "Checking package dependency definitions..." rg "dependencies.*=.*" depends/packages/ # Let's also check how toolchain-related variables are used echo -e "\nChecking toolchain variable usage..." rg "(CC|CXX|AR|RANLIB|OBJC|OBJCXX).*=.*" depends/packages/ # Finally, let's check the builders directory for toolchain configuration echo -e "\nChecking builder configurations..." cat depends/builders/default.mkLength of output: 4871
contrib/macdeploy/README.md (2)
9-9: LGTM: Clear output format specificationThe change from DMG to ZIP format is clearly documented.
71-78: Verify build process stepsThe documented build process steps look correct, but let's verify the repository links and build artifacts.
✅ Verification successful
Let me gather more information about the build process and scripts.
Build process steps and repository links are accurate
The verification confirms that:
- The detached signatures repository (dashpay/dash-detached-sigs) exists and is actively maintained
- The build process steps align with the documented release process in
doc/release-process.md- The codesigning workflow is properly implemented with:
contrib/guix/libexec/codesign.shfor handling signatures- Support for detached signatures integration via
DETACHED_SIGS_REPOin the Guix build- The process correctly describes the flow: Guix build → detached signature creation → final deterministic build
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and accessibility of the detached signatures repository gh repo view dashpay/dash-detached-sigs --json url,description,isArchived # Check for any references to DMG files that might have been missed rg -i "\.dmg" --type mdLength of output: 746
Script:
#!/bin/bash # Check for Guix build related files and scripts fd -e scm -e sh guix # Look for codesigning related scripts fd -g "*sign*.sh" # Check for any references to detached signatures in the codebase rg "detached.*sig" --type mdLength of output: 1823
contrib/guix/libexec/codesign.sh (1)
81-86: LGTM: Robust deterministic ZIP creationThe implementation ensures deterministic ZIP creation through:
- Safe filename handling with null-terminated strings
- Consistent timestamps using SOURCE_DATE_EPOCH
- Sorted file order for reproducible archives
depends/README.md (1)
54-57: Verify Clang version requirementLet's verify the Clang version requirement and its availability.
🧰 Tools
🪛 LanguageTool
[grammar] ~57-~57: The verb ‘depend’ requires the preposition ‘on’ (or ‘upon’).
Context: ...ceeding with a cross-compile. Under the depends directory, create a subdirectory named ...(DEPEND_ON)
doc/build-osx.md (1)
216-217: LGTM! Documentation accurately reflects the new deployment format.The changes correctly document the transition from
.dmgto.zipformat for macOS deployment.Also applies to: 286-286
contrib/macdeploy/macdeployqtplus (1)
386-386: LGTM! Clean transition from DMG to ZIP format.The argument parser and file checks are correctly updated to handle ZIP files instead of DMG files.
Also applies to: 397-397, 418-420
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6516.ee04e1f5. The image should be on dockerhub soon. |
Additional Information
Depends on backport: merge bitcoin#24241, #24534, #24948, #28622, #28880, #29185, #29170, #29233, #29298, #29598, #29732, #29890, #29739, #30074, #30198, #29072 (toolchain backports: part 2) #6385
Omissions related to explicitly defining the location of
DSYMUTIL(see dash#6384 for more information) have been remedied and are now included (completing bitcoin#24866 and bitcoin#25612 in this pull request), courtesy of a fix that definesDSYMUTILeven whenFORCE_USE_SYSTEM_CLANGis defined (commit).DSYMUTILis defined in theFORCE_USE_SYSTEM_CLANGcase andcctoolsis no longer used.GMP runs its test programs during build configuration sans
LDFLAGS(source), which are quite important when using an LLD-based toolchain as we rely on it to take onld64's duties (source) using-fuse-ld=lld(source). This has been remedied by patchingacinclude.m4andconfigure.acto includeLDFLAGS.configure failure:
config.log excerpt:
Note:
llditself will never respond like this, as it will exit with a catch-all message requesting you to invoke it with the correct nameNor is it calling the Unix-specific variant mistakenly
The error displayed above is consistent with GNU
ldA similar problem is observed for Qt, which was resolved upstream by adding it to
CXXFLAGS(source) (but this comes at the downside of an unused argument warnings whenever the compiler doesn't intend to do any linking and is why we didn't just modifydarwin_{CC,CXX}instead)Warning messages when building Qt:
Clang has been bumped to 18 as bitcoin#30201 drops
native_llvm(formerly known asnative_clang) and mandates the presence of Clang 18 or higher for cross-compilation (source).Breaking Changes
None expected
Checklist