-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: cmake prereq 2 #6294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backport: cmake prereq 2 #6294
Conversation
|
A few of the depends builds are running into this issue: cannot reproduce locally, or really find anything about this in bitcoin. Any thoughts @UdjinM6 |
cab2085 to
03d6c1b
Compare
|
This pull request has conflicts, please rebase. |
BACKPORT NOTE: excludes documentation changes in doc/build-android.md 4ba4920 doc: Add minimum supported Android NDK version (Hennadii Stepanov) 6393bdc doc: Move Android dependencies guide into `build-android.md` (Hennadii Stepanov) ac323a7 build: Switch to llvm buinutils for Android builds (Hennadii Stepanov) Pull request description: The new Long Term Support release of the Android NDK is [available](https://groups.google.com/g/android-ndk-announce/c/MS6Qoub0DKE/m/Zfp5Ys8eAAAJ) since 2021-08-11: > As r23 is the new LTS, the support windows for r21 and r22 have now ended. On master (8ae4ba4), dependency build fails because it expects GNU Binutils are present in the Android NDK. In [fact](https://android.googlesource.com/platform/ndk/+/master/docs/BuildSystemMaintainers.md#binutils): > GNU Binutils remains available up to and including r22. All binutils tools with the exception of the assembler (GAS) were removed in r23. GAS was removed in r24. This PR switches our depends build system to llvm binutils. The usage of `llvm-ar` and `llvm-ranlib` tools effectively makes r21 the minimum supported version of NDK. With this PR: - building depends against NDK r23 LTS now is possible with `NO_QT=1` - building the `qt` package in depends against NDK r23 LTS still fails: ``` Creating qmake... ... ERROR: Cannot detect Android NDK toolchain. Please use -android-toolchain-version to specify it. ``` The issue with the `qt` package is going to be addressed in another PR. ACKs for top commit: fanquake: ACK 4ba4920 Tree-SHA512: cdc8f95ff9a3ad7f12eb55b9ea18b6b6b800d4cceff7e0321985be6e39d15a2b2ea5b1592972307d76d111292a0ed58fd287e5ca285e2f6868b42a286536d310
… packages 86c2889 ci: Make log verbose in error case only (Hennadii Stepanov) 7f65088 depends: Add file-based logging for individual packages (Hennadii Stepanov) Pull request description: This PR adds file-based logging for individual packages in depends. To use this feature one should provide `LOG=1`. A log file is printed out automatically in case of a build error. After successful build log files are being moved along with package archives: ``` $ make -C depends HOST=x86_64-w64-mingw32 LOG=1 $ find ./depends/built/x86_64-w64-mingw32 -name '*.log' | sort ./depends/built/x86_64-w64-mingw32/bdb/bdb-4.8.30-5100a099801.log ./depends/built/x86_64-w64-mingw32/boost/boost-1_71_0-313f82dc7de.log ./depends/built/x86_64-w64-mingw32/libevent/libevent-2.1.12-stable-3fa27048d5e.log ./depends/built/x86_64-w64-mingw32/libnatpmp/libnatpmp-4536032ae32268a45c073a4d5e91bbab4534773a-9db4850dd32.log ./depends/built/x86_64-w64-mingw32/miniupnpc/miniupnpc-2.2.2-75d9a1807e0.log ./depends/built/x86_64-w64-mingw32/native_b2/native_b2-1_71_0-3bf253c19bf.log ./depends/built/x86_64-w64-mingw32/qrencode/qrencode-3.4.4-dfac87af599.log ./depends/built/x86_64-w64-mingw32/qt/qt-5.15.2-9304e03d3ac.log ./depends/built/x86_64-w64-mingw32/sqlite/sqlite-3320100-455acafa7be.log ./depends/built/x86_64-w64-mingw32/zeromq/zeromq-4.3.1-5ff627ec84a.log ``` An example of CI tasks with package build errors -- https://cirrus-ci.com/task/5275741788045312 Closes bitcoin#16368. ACKs for top commit: laanwj: Tested ACK 86c2889 Tree-SHA512: 497f2146fd2e38c952124aecfd80ebb42be22bbc5dc59521491545f4465fc38f23da7787a0caea5686b7c30aa862f2b0c02092ae3fe863e80a5ddd14b3d324b9
0947726 build: support LTO in depends (fanquake) Pull request description: This adds an `LTO` option to depends, i.e `make -C depends LTO=1`, which passes `-flto` when building packages (not currently qt), and automatically configures with `--enable-lto` when doing a build using a `CONFIG_SITE`. The following tables comapres the size (in bytes) of the stripped `x86_64` Linux binaries produced with master and this PR (full depends build): | Binary | stripped master | stripped LTO=1 | saving | | -------- | ----------------: | -------------: | --------: | | bitcoin-cli | 1178632 | 469872 | 60% | | bitcoin-tx | 2710584 | 1866504 | 31% | | bitcoin-util | 952880 | 240104 | 74% | | bitcoin-wallet | 7992888 | 5365984 | 32% | | bitcoind | 13421336 | 11868592 | 12% | | bitcoin-qt | 37680496 | 31640976 | 16% | ACKs for top commit: laanwj: Tested ACK 0947726 Tree-SHA512: 6b8483ea490e57a153105ad8c38b25fb1af5d55b1af22db398c7c2573612aaf71b4d2b4cf09c18fd6331b1358dba01641eeaa03e5018a925392e1937118d984a
0388ad0 depends: switch zmq to CMake (Cory Fields) fefb3bb depends: add zeromq no librt patch (fanquake) a522ef1 depends: add zeromq cmake minimum patch (fanquake) cbbc229 depends: add zeromq windows usage patch (fanquake) 2de68d6 depends: add zeromq builtin sha1 patch (fanquake) 0c86052 depends: add zeromq mktemp macos patch (fanquake) Pull request description: This picks up a change, which is a switch to building zeromq with CMake. It includes a number of patches, some which have already been upstreamed (see each patch for details). ACKs for top commit: hebasto: ACK 0388ad0. Tree-SHA512: 5567e432b4e4e0446c41d502bd61810a80b329dea2399b5d9d9f6e79acc450d1c6ba861c8238ba895de98338cfc5dc44ad2bf86ee8c222ecb3fbf47d6eb60da4
371910a depends: Fix CMake-generated `libzmq.pc` file (Hennadii Stepanov) Pull request description: This is a backport of: zeromq/libzmq#4706. Similar to bitcoin#30488. Addresses bitcoin#29723 (comment): > Looking at the mingw .pc generated by this PR: > > ``` > Libs: -L${libdir} -lzmq > Libs.private: > Requires.private: > ``` > > It looks like we'll need to take [zeromq/libzmq#4706](zeromq/libzmq#4706) as well for CMake. That can be done as a follow-up though, as it's not yet merged upstream. ACKs for top commit: fanquake: ACK 371910a Tree-SHA512: 6f9c2e32f83c0e629e32fd3e4c86712af00ffeaf0906bf85e5c2df889302707b9df102e8031249d1bae036eb4fc019c2a5124655682fbc5652d9337cb21c5f2c
…nced local variable" 44f0878 test: Fix MSVC warning C4101 "unreferenced local variable" (Hennadii Stepanov) 5d25a82 univalue, refactor: Convert indentation tabs to spaces (Hennadii Stepanov) Pull request description: This PR is split from bitcoin#30454 and addresses MSVC warning [C4101](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4101) "unreferenced local variable". The current MSVC build system in the master branch skips building univalue tests, so it is not affected. No behaviour changes. ACKs for top commit: kevkevinpal: utACK [44f0878](bitcoin@44f0878) maflcko: ACK 44f0878 theuni: trivial ACK 44f0878. Tree-SHA512: 661d3b40ddb4f7915de7a65ccb27a24da88ae499ce03c036099007260b0597e83738f1a3a420985b51f798ee309ade32988c6d78f4ffed401099b175a0b2025b
f59e905 depends: switch libevent to CMake (Cory Fields) Pull request description: Switches libevent in depends to be built with CMake. ACKs for top commit: TheCharlatan: ACK f59e905 willcl-ark: ACK f59e905 Tree-SHA512: 875bf9bc57653c78775a1f8192a2c964fea8f4490d733ff796d9efb00e786f0ca9a7c1a3fd610cda032273c4f2ae06394585b03567d5f241ab073c83a47cf927
8c935e6 depends: Fix CMake-generated `libevent*.pc` files (Hennadii Stepanov) Pull request description: Broken out of bitcoin#30454. This is a backport of the merged upstream PR: libevent/libevent#1622. Note that after bitcoin#29835 we might end up dropping pkg-config and using the installed CMake files directly, but that depends on whether or not enough distros actually ship those files. Either way, having fixed up .pc files won't hurt. ACKs for top commit: hebasto: ACK 8c935e6. fanquake: ACK 8c935e6 Tree-SHA512: 259c2ad78fb9e90370a7205dc71c40acda1a872f6509435133bc1c4c2c3de57366e80679aa083e13ed85e7966883dc470c0147ee171a2ed0171a18cd5ffc99b3
03d6c1b to
24973ee
Compare
|
need bitcoin#27496 to fix libevent build |
63c0c4f depends: reuse _config_opts for CMake options (Cory Fields) Pull request description: Context: I'm [currently experimenting with building our depends with CMake when possible](https://github.com/theuni/bitcoin/commits/depends-cmake) as part of our future transition to CMake. Specifically zmq, libevent, libnatpmp, and miniupnpc all have existing CMake buildsystems. Building them with CMake will allow us to drop several deps that we currently have (autoconf, automake, pkg-config, etc) which would be unfortunate to carry over after the switch-over. But that's not relevant for this PR. This is just a very simple change that makes the above work easier to experiment with as it [adds a needed feature for CMake packages](theuni@5733dc2#diff-e6ed342a25092e0a6d0308e0bfd826044578847132cc6726ac4afa2ca767b61aR20). It's a no-op for the current builds. --- From commit description: This will allow us to use the existing machinery for filtering by arch, os, debug/release, etc. For example, the following becomes possible for libevent when building with CMake: `$(package)_config_opts_release=-DEVENT__DISABLE_DEBUG_MODE` Now the define is only set when not building depends with DEBUG=1 ACKs for top commit: hebasto: ACK 63c0c4f Tree-SHA512: 17d123219d2f98c017880196966ffebd5d09d3e2db8e443e227eb7e49da46e9d971fbe7fd2b99a324fc367e1bbe0ac3cd15b399bce98dd87fbb144d767e15fe7
…g more version numbers f12fbad windres: use PACKAGE_VERSION rather than building more version numbers (fanquake) Pull request description: Rather than defining more strings, reuse PACKAGE_VERSION, which is already available. We also already use PACKAGE_VERSION for `ProductVersion` and `FileVersion` in setup.nsi. ACKs for top commit: MarcoFalke: cr ACK f12fbad laanwj: Code review ACK f12fbad Tree-SHA512: b74a37cbba105d208d4da9264d295d7e052009fdd6b0ed54a0d9968bbe2deeba1766d6d310438b2939a81555faa0cbd67d5e53f0c8a2de669ce56353c1c67d22
|
@UdjinM6 Any ideas on following in windows depends build? |
53ea437 to
0d8ff07
Compare
0d8ff07 to
1522896
Compare
|
CI was happy but Guix build failed on my machine with "can't find cmake" or smth like that. Guix on develop looks good. Re-enabled Guix action for now and force-pushed with no changes to see if the issue can be reproduced here too. EDIT: Disabled Guix action again. |
|
try bitcoin#29725 it seems relevant
|
007ea32 depends: switch to building libqrencode with CMake (fanquake) 884330c guix: make cmake-minimal a global requirement (fanquake) Pull request description: Switch to building libqrencode with CMake. Note that upstream (https://github.com/fukuchi/libqrencode) hasn't seen any activity for ~4 years, so the odds of getting anything upstream seems low, but I've made two minor changes to the source here, which I will PR in any case. From an initial look I couldn't find any significant difference between the Autotools and CMake produced libs. As part of this change we move cmake-minimal in Guix into the global package set. ACKs for top commit: TheCharlatan: ACK 007ea32 Tree-SHA512: c784f790ddea958082c8ae96d3744bdf99331a8799765f9d44f00861b8e2cfcab1a88a3d64af5b10e51a8d5938d55eb6a3d271790b565e50492a39d00dc0e30f
427be69 to
ae6e0ad
Compare
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK ae6e0ad
knst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK ae6e0ad
| $(package)_config_opts += -DENABLE_DRAFTS=OFF -DZMQ_BUILD_TESTS=OFF | ||
| $(package)_cxxflags += -ffile-prefix-map=$($(package)_extract_dir)=/usr | ||
| $(package)_config_opts_mingw32 += -DZMQ_WIN32_WINNT=0x0601 -DZMQ_HAVE_IPC=OFF | ||
| $(package)_config_opts_mingw32 += -DCMAKE_RC_COMPILER=x86_64-w64-mingw32-windres |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix mingw32 build
why bitcoin doesn't need this option? maybe we have some missing package installed in our Dockerfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have all the packages needed because simply specifying an option is enough to fix it. It's some cmake (or maybe depends in general) related backport that is missing imo but I couldn't figure out which one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have TODO here to remove it? Otherwise it may stay forever (or almost forever)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way. Having this option here shouldn't be an issue though 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add todo in cmake-prereq-3 then
knst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK ae6e0ad
1b62294 Merge bitcoin#30743: depends: build libevent with `-D_GNU_SOURCE` (merge-script) 0f135dd Merge bitcoin#30522: ci: Add missing qttools5-dev install to Asan task (merge-script) d46e16c Merge bitcoin#30490: depends: bump libmultiprocess for CMake fixes (merge-script) 7a63c20 Merge bitcoin#29276: depends: Update libmultiprocess library to fix C++20 macos build error (fanquake) 630e767 Merge bitcoin#28907: depends: bump libmultiprocess to fix capnproto deprecation warnings (fanquake) 318471d Merge bitcoin#28735: depends: Bump to capnproto-c++-1.0.1 (fanquake) ad0c279 Merge bitcoin#26672: build: Update libmultiprocess library (fanquake) Pull request description: ## Issue being fixed or feature implemented depends on #6294 ## What was done? Batch of backports ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1b62294 Tree-SHA512: a0a01b1b4844725aa6c96304a4cddae61ec29b677a760f35648e7f39fb36f6f462d3a6d5e411e99f4db1fa59c01f6fffd87158cbef5e1ba1edb43e68fc362c77
, bitcoin#30387, bitcoin#31461, bitcoin#31484, bitcoin#31498, bitcoin#31552, bitcoin#31627, bitcoin#30774, bitcoin#31125, bitcoin#31661, bitcoin#31800, bitcoin#31500, bitcoin#33494, partial bitcoin#30940 (build backports: part 3) 8e07d33 merge bitcoin#33494: Update URL for `qrencode` package source tarball (Kittywhiskers Van Gogh) 3dd5c1c merge bitcoin#31500: Fix compiling `libevent` package on NetBSD (Kittywhiskers Van Gogh) be4e492 merge bitcoin#31800: Avoid using the `-ffile-prefix-map` compiler option (Kittywhiskers Van Gogh) 721da49 merge bitcoin#31661: Override default build type for `libevent` (Kittywhiskers Van Gogh) 802232d merge bitcoin#31125: add *FLAGS to gen_id (Kittywhiskers Van Gogh) 29c3c06 merge bitcoin#30774: Qt 5.15.16 (Kittywhiskers Van Gogh) 3dbc289 merge bitcoin#31627: Fix spacing issue (Kittywhiskers Van Gogh) 78ccada merge bitcoin#31552: Update capnproto to 1.1.0 (Kittywhiskers Van Gogh) aac17f3 merge bitcoin#31498: Ignore prefix directory on OpenBSD (Kittywhiskers Van Gogh) aa28a18 merge bitcoin#31484: update capnproto to 1.0.2 (Kittywhiskers Van Gogh) de2dc16 merge bitcoin#31461: add `-g` to *BSD_debug flags (Kittywhiskers Van Gogh) 31881e6 partial bitcoin#30940: Fix build with `MULTIPROCESS=1` in Guix environment (Kittywhiskers Van Gogh) f73021a merge bitcoin#30387: use c++ compiler rather than c compiler for binary checks (Kittywhiskers Van Gogh) 44ec95a merge bitcoin#29895: remove bzip2 from deps (Kittywhiskers Van Gogh) 86004c5 merge bitcoin#29249: add NM output to gen_id (Kittywhiskers Van Gogh) dc24acb refactor: point Qt URL to `archives` subdirectory (Kittywhiskers Van Gogh) 13e1bfe refactor: move `windres` fix to `hosts/mingw32.mk` (Kittywhiskers Van Gogh) b01440d merge bitcoin#28900: remove mingw-w64 install for "older" systems (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6919 * The current windres fix was introduced by [dash#6294](#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](bitcoin#31388)). To mitigate this, the fix has been generalised by setting the `WINDRES` variable, which is checked by `configure.ac` ([source](https://github.com/dashpay/dash/blob/f7dad69eab573c179060ff9eb1bbaccb317de6d3/configure.ac#L835)). * This fix had the effect of breaking detection (see error below) as `test -f` cannot traverse through `PATH` ([source](https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html)), this has been resolved by using `command -v`, which is a better fit ([source](https://pubs.opengroup.org/onlinepubs/9799919799/utilities/command.html)) ``` make[1]: Entering directory '/distsrc-base/distsrc-23.0.0-rc.3-125-gca749d4d0d58-x86_64-w64-mingw32/src' make[2]: Entering directory '/distsrc-base/distsrc-23.0.0-rc.3-125-gca749d4d0d58-x86_64-w64-mingw32/src' CXX dashd-bitcoind.o windres x86_64-w64-mingw32-windres not found, but is required to compile windows resource files ``` * Versions below Qt 6.5 are considered (as of this writing), archived ([source](https://download.qt.io/official_releases/qt/)), 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](https://download.qt.io/archive/qt/5.15/)). The URL has been updated to reflect the same. * While upstream reverted [bitcoin#33494](bitcoin#33494) with [bitcoin#33577](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 - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 8e07d33 Tree-SHA512: 3356603fb1f74470656f3df233100da8ff722dd95926c25a251fe542afc85876d6517a45bb015863003ffe485d3f17241e1e5e73fbbcc9b5e90729b429272300
Issue being fixed or feature implemented
depends on #6293
What was done?
Describe your changes in detail
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.
Breaking Changes
Please describe any breaking changes your code introduces
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.