backport: merge bitcoin#28900, #29249, #29895, #30387, #31461, #31484, #31498, #31552, #31627, #30774, #31125, #31661, #31800, #31500, #33494, partial bitcoin#30940 (build backports: part 3) #6918
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
kwvg
changed the title
kwvg
force-pushed
the
build_bps_p3
branch
2 times, most recently
from
d00e4b0 to
8e6677d
Compare
kwvg
changed the title
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6918.da547409. A new comment will be made when the image is pushed. |
Checksums for da54740 |
kwvg
changed the title
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6918.da547409. The image should be on dockerhub soon. |
The current fix applies only to one package and assumes a fixed HOST, neither is ideal. Propagate the fix instead.
As of this commit, Qt <6.5 are considered historical versions, which are located at a different path. Adjust accordingly.
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6918.c8fcd68f. 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/v23.0.0-devpr6918.c8fcd68f. The image should be on dockerhub soon. |
Checksums for c8fcd68 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR backports multiple Bitcoin Core build system improvements focused on dependency management, cross-compilation, and build reproducibility.
Key Changes:
- Windres fix generalization: Replaces hardcoded
x86_64-w64-mingw32-windrespath with$(host_toolchain)windresvariable to support future Windows ARM builds. Usescommand -vinstead oftest -ffor proper PATH traversal (fixes detection issue from dash#6294). - Qt 5.15.16 update: Updates from 5.15.14 to 5.15.16 and changes download URL from
/official_releases/to/archive/since older Qt versions are now archived. Removes three patches (fix-macos-linker.patch,memory_resource.patch,zlib-timebits64.patch) that are now included upstream. - qrencode source change: Switches from fukuchi.org to GitHub releases for more reliable downloads.
- Build ID improvements: Adds
CPPFLAGS,CFLAGS,CXXFLAGS,LDFLAGS,NM,lld, andmoldtogen_idfor better cache invalidation when toolchain flags change. - Reproducibility improvements: Adds
-fdebug-prefix-mapand-fmacro-prefix-mapflags to multiple packages, changes libevent toCMAKE_BUILD_TYPE=None. - BSD improvements: Adds
-gflag to debug builds for FreeBSD/NetBSD/OpenBSD, adds NetBSD fixup patch for libevent. - Boost format change: Switches from
.tar.bz2to.tar.gzformat.
All changes are consistent with upstream Bitcoin Core improvements and properly integrated into Dash's build system.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk - it contains well-tested Bitcoin Core backports
- All changes are from established Bitcoin Core commits that have been tested upstream. The windres fix properly addresses the original issue from dash#6294 with a more flexible solution. URL changes are necessary for archived Qt versions and improve download reliability. Build reproducibility improvements follow best practices. No logic changes to core functionality.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/Makefile.am | 5/5 | changed test -f to command -v for windres detection, which properly traverses PATH |
| depends/hosts/mingw32.mk | 5/5 | added mingw32_WINDRES variable to generalize windres fix for different target triples |
| depends/config.site.in | 5/5 | added WINDRES configuration for mingw32 builds to pass to configure.ac |
| depends/Makefile | 5/5 | added WINDRES substitution and updated gen_id calls with new parameters for cache busting |
| depends/packages/qt.mk | 5/5 | updated to Qt 5.15.16 and changed URL to archives location, removed patches now included upstream |
| depends/packages/qrencode.mk | 5/5 | changed download source from fukuchi.org to GitHub releases with updated hash |
| depends/gen_id | 5/5 | added FLAGS, lld, mold, and NM to build ID calculation for better cache invalidation |
| depends/packages/zeromq.mk | 5/5 | removed hardcoded windres path and changed to fdebug-prefix-map and fmacro-prefix-map |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant Make as depends/Makefile
participant GenID as gen_id
participant Host as hosts/*.mk
participant Config as config.site.in
participant Pkg as packages/*.mk
participant Configure as configure.ac
participant SrcMake as src/Makefile.am
Dev->>Make: make -C depends
Make->>GenID: Calculate build_id with CC/CXX/AR/NM/FLAGS
GenID-->>Make: Returns build hash
Make->>GenID: Calculate host_id with FLAGS/WINDRES
GenID-->>Make: Returns host hash
Make->>Host: Load mingw32.mk
Host->>Host: Set mingw32_WINDRES=$(host_toolchain)windres
Host-->>Make: Return host tools (CC/CXX/WINDRES/etc)
Make->>Pkg: Build packages (qt/qrencode/libevent)
Pkg->>Pkg: Download from archive URLs
Pkg->>Pkg: Apply patches & build with prefix-map flags
Pkg-->>Make: Packages built
Make->>Config: Generate config.site
Config->>Config: Set WINDRES for mingw32
Config-->>Make: config.site created
Dev->>Configure: ./configure --prefix=depends/triple
Configure->>Config: Source config.site
Config-->>Configure: Set WINDRES variable
Configure->>Configure: AC_PATH_TOOL finds windres via PATH
Configure-->>Dev: Configuration complete
Dev->>SrcMake: make
SrcMake->>SrcMake: Check windres with command -v $(WINDRES)
SrcMake->>SrcMake: Compile .rc files using $(WINDRES)
SrcMake-->>Dev: Build complete
38 files reviewed, no comments
WalkthroughThis PR updates build and test tooling and dependency manifests across the tree. Tests invoking Python security/symbol checks now forward the C++ compiler variables (CXX/CXXFLAGS) instead of C compiler variables (CC/CFLAGS) and test source files were converted from .c to .cpp. Many dependency packages switched archives from .tar.bz2 to .tar.gz with updated SHA256s. Host toolchain handling was extended to include WINDRES and related config.site/make changes. Fetch/stamp behavior and gen_id diagnostics were expanded, several package versions/patches changed, and BSD debug CFLAGS now include -g. Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (34)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (18)
🧰 Additional context used📓 Path-based instructions (3)doc/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
depends/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
contrib/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-01-06T09:51:03.167ZApplied to files:
📚 Learning: 2025-07-20T18:42:49.794ZApplied to files:
🧬 Code graph analysis (2)contrib/devtools/test-security-check.py (1)
contrib/devtools/test-symbol-check.py (1)
🪛 Flake8 (7.3.0)contrib/devtools/test-security-check.py[error] 64-64: continuation line under-indented for visual indent (E128) [error] 66-66: continuation line under-indented for visual indent (E128) [error] 68-68: continuation line under-indented for visual indent (E128) [error] 70-70: continuation line under-indented for visual indent (E128) [error] 72-72: continuation line under-indented for visual indent (E128) [error] 74-74: continuation line under-indented for visual indent (E128) [error] 76-76: continuation line under-indented for visual indent (E128) [error] 79-79: continuation line under-indented for visual indent (E128) [error] 81-81: continuation line under-indented for visual indent (E128) [error] 83-83: continuation line under-indented for visual indent (E128) [error] 85-85: continuation line under-indented for visual indent (E128) [error] 87-87: continuation line under-indented for visual indent (E128) [error] 89-89: continuation line under-indented for visual indent (E128) [error] 100-100: continuation line under-indented for visual indent (E128) [error] 102-102: continuation line under-indented for visual indent (E128) [error] 104-104: continuation line under-indented for visual indent (E128) [error] 106-106: continuation line under-indented for visual indent (E128) [error] 108-108: continuation line under-indented for visual indent (E128) [error] 110-110: continuation line under-indented for visual indent (E128) [error] 112-112: continuation line under-indented for visual indent (E128) [error] 125-125: continuation line under-indented for visual indent (E128) [error] 127-127: continuation line under-indented for visual indent (E128) [error] 129-129: continuation line under-indented for visual indent (E128) [error] 131-131: continuation line under-indented for visual indent (E128) [error] 133-133: continuation line under-indented for visual indent (E128) [error] 137-137: continuation line under-indented for visual indent (E128) [error] 139-139: continuation line under-indented for visual indent (E128) [error] 141-141: continuation line under-indented for visual indent (E128) contrib/devtools/test-symbol-check.py[error] 53-53: continuation line under-indented for visual indent (E128) [error] 71-71: continuation line under-indented for visual indent (E128) [error] 91-91: continuation line under-indented for visual indent (E128) [error] 108-108: continuation line under-indented for visual indent (E128) [error] 121-121: continuation line under-indented for visual indent (E128) [error] 140-140: continuation line under-indented for visual indent (E128) [error] 141-141: continuation line over-indented for visual indent (E127) [error] 155-155: continuation line under-indented for visual indent (E128) 🪛 markdownlint-cli2 (0.18.1)doc/dependencies.md36-36: Link text should be descriptive (MD059, descriptive-link-text) 🪛 Ruff (0.14.3)contrib/devtools/test-security-check.py43-43: (S603) 43-43: Consider iterable unpacking instead of concatenation Replace with iterable unpacking (RUF005) 44-44: (S603) 48-48: (S603) 48-48: Consider Replace with (RUF005) contrib/devtools/test-symbol-check.py25-25: (S603) 25-25: Consider iterable unpacking instead of concatenation Replace with iterable unpacking (RUF005) 26-26: (S603) ⏰ 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). (1)
🔇 Additional comments (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| moreutils | ||
| ;; Compression and archiving | ||
| tar | ||
| bzip2 |
There was a problem hiding this comment.
29895: should drop it from Dockerfile-s too I guess
There was a problem hiding this comment.
Resolved in latest push
There was a problem hiding this comment.
We have two Guix containers, one introduced by dash#5285 and the other introduced by dash#5449, I'm not aware of usage of the former (based on fanquake/core-review's container, source, hasn't been synced in a while) and I use the latter. @knst, can we drop the former container?
There was a problem hiding this comment.
contrib/guix/Dockerfile is mentioned here https://github.com/dashpay/dash/blob/develop/contrib/guix/INSTALL.md?plain=1#L63
There was a problem hiding this comment.
I'm not using contrib/guix/Dockerfile either. I guess we could point INSTALL.md to contrib/containers/guix/Dockerfile and drop contrib/guix/Dockerfile then. @knst @PastaPastaPasta ?
…nment includes: - 06b4c33
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6918.8e07d338. 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/v23.0.0-devpr6918.8e07d338. The image should be on dockerhub soon. |
4 participants
Additional Information
Dependency for backport: merge bitcoin#31100, #31626, #31840, #30584, #31982, #32086, #31998, #32505, #32568, #32690, #32731, #32716, #32837, #32266, #30095, #30137, #33580, partial bitcoin#30454 (build backports: part 4) #6919
The current windres fix was introduced by dash#6294 to fix mingw32 builds, the problem with the fix is that it assumes the target triple is fixed (i.e.
x86_64-w64-mingw32), this may not hold true in the long run as Windows for ARM support is currently being tracked upstream (see bitcoin#31388).To mitigate this, the fix has been generalised by setting the
WINDRESvariable, which is checked byconfigure.ac(source).This fix had the effect of breaking detection (see error below) as
test -fcannot traverse throughPATH(source), this has been resolved by usingcommand -v, which is a better fit (source)Versions below Qt 6.5 are considered (as of this writing), archived (source), this results in fetch failures that result in more usage of the cache fallback when trying to fetch Qt 5.15, which is now located in the archives (source). The URL has been updated to reflect the same.
While upstream reverted bitcoin#33494 with bitcoin#33577, the reasoning was to do with their cache and its interaction with the release process. As the underlying rationale for the revert doesn't match our case, we can retain the backport.
Breaking Changes
None expected.
Checklist