cmake: Build `bitcoin-qt` executable #77

hebasto · 2024-01-17T16:15:35Z

A new configuration option WITH_GUI has been added. Its valid values are AUTO, Qt5, OFF.

hebasto · 2024-01-17T19:19:32Z

cc @fanquake @TheCharlatan @theuni @vasild @luke-jr @willcl-ark @stickies-v @pablomartin4btc @m3dwards

hebasto · 2024-01-18T09:49:50Z

The branch fails to link on macOS (

Working on it.

hebasto · 2024-01-18T10:26:52Z

The branch fails to link on macOS (

Working on it.

Fixed.

Also the GUI has been enabled in the CI (except for MSVC, for now).

This reverts commit 1155978.

hebasto · 2024-01-18T13:08:35Z

Also the GUI has been enabled in the CI (except for MSVC, for now).

Now, the GUI is enabled for the MSVC CI job as well.

Ready for review :)

fanquake · 2024-01-19T11:54:38Z

depends/toolchain.cmake.in

@@ -84,15 +84,19 @@ set(OTOOL "@OTOOL@")

 set(CMAKE_FIND_ROOT_PATH "@depends_prefix@")
 set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)
-set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)
+set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY BOTH)


Follow up from yesterday: Why is this scoped globally, and what is it actually needed for?

De-globalized. An explanation comment added:

bitcoin/src/qt/CMakeLists.txt

Lines 18 to 26 in c6c2cf9

if(CMAKE_CROSSCOMPILING)

# The find_package(Qt ...) function internally uses find_library()

# calls for all dependencies to ensure their availability.

# In turn, the find_library() inspects the well-known locations

# on the file system; therefore, it must be able to find

# platform-specific system libraries, for example:

# /usr/x86_64-w64-mingw32/lib/libm.a or /usr/arm-linux-gnueabihf/lib/libm.a.

set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY BOTH)

endif()

I agree with this change, moving it to CMakeLists.txt to de-globalized, but it was copied and left in toolchain.cmake.in, is this correct for when the dependencies are built?

Also, set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY BOTH) is required not only for cross-compilation but also for Mac or Apple devices, as I found on "Professional CMake - A Practical Guide by Craig Scott" (2023):

On Apple platforms, consider carefully before setting CMAKE_FIND_ROOT_PATH_MODE_LIBRARY to ONLY, as libraries may be built as fat binaries which support multiple target platforms. These fat binaries may not reside under target platform-specific paths, so it may still be necessary to search host platform paths to find them.

Do we need set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE ONLY) to change it to BOTH as well?

Do we need set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE ONLY) to change it to BOTH as well?

I don't think so, because our both native macOS CI jobs are green. And we set the CMAKE_FIND_ROOT_PATH only for cross-compiling (please note that the CMAKE_FIND_ROOT_PATH_MODE_* variables values affect how the CMAKE_FIND_ROOT_PATH is applied during searching routines).

I spent most of yesterday poking at this. It's extremely annoying. I don't understand why the platform libs are lumped in here.

Is QT the only offender? Do other cmake projects also search for their platform dependencies like this? Afaik it makes CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY completely useless.

If qt is the only lib with this behavior, I'm wondering if we should patch around it and/or upstream a change?

Building with -DQt5_EXCLUDE_STATIC_DEPENDENCIES:BOOL=ON almost fixes the problem by letting us link the required libs ourselves. Except that the static plugins don't honor it :\

Is QT the only offender?

It seems so.

If qt is the only lib with this behavior, I'm wondering if we should patch around it and/or upstream a change?

The Qt active development happens for version 6 now. Suggesting to postpone this decision until we also bump our Qt package to the next major version. I'm assuming that things might change to better in there.

Heh, we've been burned too many times by qt in the past to assume that it will actually become more reasonable in the future. It's more likely that they'll invent yet another wonky buildsystem :p

As far as this goes though, does the fact that this lives in src/qt/CMakeLists.txt mean that the CMAKE_FIND_ROOT_PATH_MODE_LIBRARY gets scoped to QT only? Or does that apply the option globally?

does the fact that this lives in src/qt/CMakeLists.txt mean that the CMAKE_FIND_ROOT_PATH_MODE_LIBRARY gets scoped to QT only?

Yes, it does.

Ok, great. In that case I care much less about this and am ok with punting.

fanquake · 2024-01-19T11:55:45Z

src/qt/winshutdownmonitor.h

@@ -10,7 +10,7 @@
 #include <QString>
 #include <functional>

-#include <windef.h> // for HWND
+#include <windows.h> // for HWND


This is odd. If this change is needed shouldn't it already be needed in master; why does CMake change this?

A build error happens only when the vcpkg's Qt packages are used. It is a not CMake-specific issue, rather vcpkg-specific one.

If this change is needed shouldn't it already be needed in master...?

According to the provided explanation, the change is correct. However, I was a bit vague about whether the motivation is compelling enough for the master branch in the non-CMake context.

If this change is needed shouldn't it already be needed in master...?

Proposed in bitcoin-core/gui#789.

Could you explain what's different about the vcpkg qt? Does that imply that the qt headers installed by depends include more windows headers than vcpkg's? That's the only way I could see this making a difference.

Again, I'm not arguing about correctness here, this change seems fine to me. But I'm trying to understand why it's necessary in one place and not another, and if that means that our build (or vcpkg's) is suboptimal.

Could you explain what's different about the vcpkg qt?

Not exactly (

Does that imply that the qt headers installed by depends include more windows headers than vcpkg's? That's the only way I could see this making a difference.

Could it be related to pre-compiled headers as well?

I don't think that the comparison to our depends is correct because the minimum code example (bitcoin-core/gui#789 (comment)) cross compiles fine, but fails with MSVC.

Yes, PCH could definitely affect the include order and it's on for mingw. So that's at least a plausible explanation. I'll do a quick check with it disabled.

Actually, a quick test shows that the pch option only affects the qt build itself. So I doubt that's the culprit.

This isn't interesting enough to hold up merge.

fanquake · 2024-01-19T12:00:42Z

depends/toolchain.cmake.in

 set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)
 set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE ONLY)
 set(PKG_CONFIG_PATH "@depends_prefix@/lib/pkgconfig")
 set(PKG_CONFIG_LIBDIR "${PKG_CONFIG_PATH}")
 set(QT_TRANSLATIONS_DIR "@depends_prefix@/translations")

+if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND NOT CMAKE_HOST_APPLE)
+  set(CMAKE_FRAMEWORK_PATH "@OSX_SDK@/System/Library/Frameworks")


Can't remember the exact comments from yesterday, but I think this could use some docs to explain why the SDK is leaking out of the compiler invocation.

I've added an explanation comment:

bitcoin/depends/toolchain.cmake.in

Lines 95 to 99 in b0a1bae

# The find_package(Qt ...) function internally uses find_library()

# calls for all dependencies to ensure their availability.

# In turn, the find_library() inspects the well-known locations

# on the file system; therefore, a hint is required.

set(CMAKE_FRAMEWORK_PATH "@OSX_SDK@/System/Library/Frameworks")

I'm still confused by this. Does this relate to the previous commit? If I unset CMAKE_FRAMEWORK_PATH and drop the previous commit, it still configures, but then fails to link bitcoin-qt.

I'm still confused by this.

What exactly is the source of your confusion: the code or the comment?

The code, but I guess I should just look at what cmake does here.

https://cmake.org/cmake/help/latest/command/find_library.html describes the role of the CMAKE_FRAMEWORK_PATH variable.

For Qt's part, please see https://github.com/qt/qtbase/blob/da6e958319e95fe564d3b30c931492dd666bfaff/mkspecs/features/data/cmake/Qt5BasicConfig.cmake.in#L77-L134

The second invocation:

find_library(_Qt5$${CMAKE_MODULE_NAME}_${Configuration}_${_lib}_PATH ${_lib} HINTS ${CMAKE_CXX_IMPLICIT_LINK_DIRECTORIES})

actually uses the CMAKE_FRAMEWORK_PATH variable.

Does this relate to the previous commit? If I unset CMAKE_FRAMEWORK_PATH and drop the previous commit, it still configures, but then fails to link bitcoin-qt.

No, it doesn't. The Revert "build, qt: Do not install *.prl files" commit does not actually made the "prl" files available, which is required for a proper configuration. This is done in the cmake: Build bitcoin-qt executable commit. The first commit is supposed to make further changes more meaningful and clear.

Unsetting CMAKE_FRAMEWORK_PATH would result in a cross-compiling for macOS failure regardless of the commit.

m3dwards · 2024-01-19T17:37:01Z

I've successfully built and ran bitcoin-qt on Mac (aarch64) and Debian 12 (x86) using system libraries.

I've been trying to build on Windows but the vcpkg is so large I keep running out of hard disk space on my small laptop. Will post update if I get it built on Windows.

m3dwards · 2024-01-19T19:42:54Z

@hebasto I did get a Windows build completed using your tip of not doing debug vcpkg builds.

I don't know if this is expected or not but when I run bitcoin-qt I also get a blank cmd prompt run which if I close it closes the qt GUI. Expected?

Edit: I have downloaded a prebuild 26.0 qt binary and I'm not getting the same behaviour. Apologies I'm a bit out of time to look into it further but here are the build commands I used:

mkdir build
cd build
Add-Content -Path "$env:VCPKG_ROOT\triplets\x64-windows.cmake" -Value "set(VCPKG_BUILD_TYPE release)"
Add-Content -Path "$env:VCPKG_ROOT\triplets\x64-windows-static.cmake" -Value "set(VCPKG_BUILD_TYPE release)"
cmake -S .. -A x64 --toolchain $env:VCPKG_ROOT\scripts\buildsystems\vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows-static
cmake --build . -j $env:NUMBER_OF_PROCESSORS --config Release

pablomartin4btc

tACK c6c2cf9

Tested on Ubuntu 22.04, ran all tests and executed bitcoin-qt successfully

Pending of testing it on cross-compilation for both Windows and Android.

hebasto · 2024-01-19T20:48:24Z

@pablomartin4btc

Pending of testing it on cross-compilation for both Windows and Android.

I don't think this branch supports cross-compilation for Android. Such a feature is planned for the later.

vasild

ACK c6c2cf9

bitcoin-qt is build and works on FreeBSD 14 with clang 18.

However, some tweaks were needed to get the compilation to succeed (I guess not due to this PR, but due to the C++20 switch from some time ago?).

clang-scan-deps is not found. The default compiler /usr/bin/cc is clang 16 and I do not think clang-scan-deps is installed, or at least I could not find under what name, nor did cmake. Anyway, I prefer compiling with the latest compiler clang 18 which installs the tool under /usr/local/bin/clang-scan-deps-devel, thus I had to use cmake ... -DCMAKE_CXX_COMPILER_CLANG_SCAN_DEPS:PATH=/usr/local/bin/clang-scan-deps-devel. The error is:

        "CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND" -format=p1689 -- /usr/local/bin/clang++-devel -DSTD_ATOMIC_NEEDS_LINK_TO_LIBATOMIC  -std=c++20 -x c++ /tmp/b/CMakeFiles/CMakeScratch/TryCompile-VQkc4W/src.cxx -c -o CMakeFiles/cmTC_adea9.dir/src.cxx.o -MT CMakeFiles/cmTC_adea9.dir/src.cxx.o.ddi -MD -MF CMakeFiles/cmTC_adea9.dir/src.cxx.o.ddi.d > CMakeFiles/cmTC_adea9.dir/src.cxx.o.ddi.tmp && mv CMakeFiles/cmTC_adea9.dir/src.cxx.o.ddi.tmp CMakeFiles/cmTC_adea9.dir/src.cxx.o.ddi
        /bin/sh: CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND: not found

It cannot find <cpuid.h> included from src/compat/cpuid.h. That file is installed in /usr/local/llvm-devel/lib/clang/18/include/cpuid.h, thus cmake ... -DCMAKE_CXX_FLAGS:STRING="-I/usr/local/llvm-devel/lib/clang/18/include" was needed. I am not sure how this works with autotools or how it worked recently with cmake (I did not had to explicitly provide this include directory, maybe it is related to clang-scan-deps?).

depends/patches/qt/qt.pro

depends/packages/qt.mk

hebasto · 2024-01-22T16:44:12Z

... when I run bitcoin-qt I also get a blank cmd prompt run which if I close it closes the qt GUI.

Fixed.

vasild

ACK 71aa125

theuni · 2024-01-22T20:18:22Z

Before reviewing today I've been doing compile tests for various platforms. Working so far for:
linux-x86_64 target on linux-x86_64 builder
mingw-w64 on linux-x86_64 builder
macos on linux-x86_64 builder
native macos (aarch64-apple-darwin22.5.0)

In order to build for native macos with no homebrew, I used the following branch to remove some cmake/pkg-config dependencies: https://github.com/theuni/bitcoin/commits/depends-less-pkgconfig/

A few of those patches have been proposed in other PRs, but not merged due to parity issues with their autotools equivalents, but they were good enough for testing this branch.

Will continue with actual review tomorrow.

pablomartin4btc

tACK 71aa125

I can't make it work on cross-building for Windows, I keep getting missing references. Tried WSL with Ubuntu 22.04 ad Ubuntu 22.04 itself.

make -C depends -j $(nproc) HOST=x86_64-w64-mingw32
mkdir build
cd build
cmake -S .. --toolchain ../depends/x86_64-w64-mingw32/share/toolchain.cmake
cmake --build . -j $(nproc)
ctest -j $(nproc)

e.g.:

[ 36%] Linking CXX static library libleveldb.a
CMakeFiles/bitcoin-tx.dir/objects.a(bitcoin-tx.cpp.obj): in function `void std::call_once<void (&)()>(std::once_flag&, void (&)())':
/usr/lib/gcc/x86_64-w64-mingw32/10-posix/include/c++/mutex:724: undefined reference to `std::__get_once_mutex()'
/usr/bin/x86_64-w64-mingw32-ld: /usr/lib/gcc/x86_64-w64-mingw32/10-posix/include/c++/mutex:726: undefined reference to `std::__set_once_functor_lock_ptr(std::unique_lock<std::mutex>*)'
/usr/bin/x86_64-w64-mingw32-ld: CMakeFiles/bitcoin-tx.dir/objects.a(bitcoin-tx.cpp.obj):/usr/lib/gcc/x86_64-w64-mingw32/10-posix/include/c++/x86_64-w64-mingw32/bits/gthr-default.h:700: undefined reference to `__once_proxy'
/usr/bin/x86_64-w64-mingw32-ld: CMakeFiles/bitcoin-tx.dir/objects.a(bitcoin-tx.cpp.obj): in function `void std::call_once<void (&)()>(std::once_flag&, void (&)())':
/usr/lib/gcc/x86_64-w64-mingw32/10-posix/include/c++/mutex:733: undefined reference to `std::__set_once_functor_lock_ptr(std::unique_lock<std::mutex>*)'
/usr/bin/x86_64-w64-mingw32-ld: CMakeFiles/bitcoin-tx.dir/objects.a(bitcoin-tx.cpp.obj):bitcoin-tx.cpp:(.rdata$.refptr.__once_proxy[.refptr.__once_proxy]+0x0): undefined reference to `__once_proxy'
/usr/bin/x86_64-w64-mingw32-ld: CMakeFiles/bitcoin-tx.dir/objects.a(bitcoin-tx.cpp.obj):bitcoin-tx.cpp:(.rdata$.refptr._ZSt14__once_functor[.refptr._ZSt14__once_functor]+0x0): undefined reference to `std::__once_functor'
/usr/bin/x86_64-w64-mingw32-ld: [ 36%] Built target leveldb
libbitcoin_common.a(system.cpp.obj): in function `GetNumCores()':
/home/pablo/workspace/bitcoin-hebasto/src/common/system.cpp:101: undefined reference to `std::thread::hardware_concurrency()'
[ 36%] Built target bitcoin_wallet
collect2: error: ld returned 1 exit status
gmake[2]: *** [src/CMakeFiles/bitcoin-tx.dir/build.make:107: src/bitcoin-tx.exe] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1575: src/CMakeFiles/bitcoin-tx.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2

I can cross-build successfully with previous PRs with NO_QT=1 (e.g.: #31, #75).

clang --version
Ubuntu clang version 14.0.0-1ubuntu1.1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

mingw-w64 is already the newest version (8.0.0-1).

gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

GNU Make 4.3

cmake version 3.22.1

hebasto · 2024-01-23T15:16:46Z

@pablomartin4btc

I can't make it work on cross-building for Windows, I keep getting missing references.

Could you make another try in the clean Docker container:

apt update
apt install --no-install-recommends git ca-certificates cmake curl make automake libtool pkg-config patch lbzip2 xz-utils g++-mingw-w64-x86-64-posix g++
git clone --depth=1 --branch 240117-cmake-S https://github.com/hebasto/bitcoin.git
cd bitcoin
make -C depends -j $(nproc) HOST=x86_64-w64-mingw32
mkdir build
cd build
cmake -S .. --toolchain ../depends/x86_64-w64-mingw32/share/toolchain.cmake
cmake --build . -j $(nproc)

?

pablomartin4btc

I can't make it work on cross-building for Windows, I keep getting missing references.

Could you make another try in the clean Docker container?

That works just fine, thanks! (edited)

~/workspace/bitcoin-hebasto-docker$ docker build -t bitcoin-builder .
[+] Building 1375.0s (13/13) FINISHED                                                                     docker:default
 => [internal] load build definition from Dockerfile                                                                0.0s
 => => transferring dockerfile: 788B                                                                                0.0s
 => [internal] load metadata for docker.io/library/ubuntu:22.04                                                     5.0s
 => [internal] load .dockerignore                                                                                   0.1s
 => => transferring context: 2B                                                                                     0.0s
 => [1/9] FROM docker.io/library/ubuntu:22.04@sha256:e6173d4dc55e76b87c4af8db8821b1feae4146dd47341e4d431118c7dd060  8.7s
 => => resolve docker.io/library/ubuntu:22.04@sha256:e6173d4dc55e76b87c4af8db8821b1feae4146dd47341e4d431118c7dd060  0.0s
 => => sha256:e6173d4dc55e76b87c4af8db8821b1feae4146dd47341e4d431118c7dd060a74 1.13kB / 1.13kB                      0.0s
 => => sha256:cb2af41f42b9c9bc9bcdc7cf1735e3c4b3d95b2137be86fd940373471a34c8b0 424B / 424B                          0.0s
 => => sha256:e34e831650c1bb0be9b6f61c6755749cb8ea2053ba91c6cda27fded9e089811f 2.30kB / 2.30kB                      0.0s
 => => sha256:29202e855b2021a2d7f92800619ed5f5e8ac402e267cfbb3d29a791feb13c1ee 29.55MB / 29.55MB                    7.9s
 => => extracting sha256:29202e855b2021a2d7f92800619ed5f5e8ac402e267cfbb3d29a791feb13c1ee                           0.6s
 => [2/9] RUN apt-get update &&     apt-get install -y --no-install-recommends     git ca-certificates cmake curl  70.0s
 => [3/9] RUN git clone --depth=1 --branch 240117-cmake-S https://github.com/hebasto/bitcoin.git /bitcoin          16.0s 
 => [4/9] WORKDIR /bitcoin                                                                                          0.1s 
 => [5/9] RUN make -C depends -j $(nproc) HOST=x86_64-w64-mingw32                                                 696.8s 
 => [6/9] RUN mkdir build                                                                                           0.3s 
 => [7/9] WORKDIR /bitcoin/build                                                                                    0.1s 
 => [8/9] RUN cmake -S .. --toolchain ../depends/x86_64-w64-mingw32/share/toolchain.cmake                          43.6s 
 => [9/9] RUN cmake --build . -j $(nproc)                                                                         524.5s 
 => exporting to image                                                                                              9.7s 
 => => exporting layers                                                                                             9.7s 
 => => writing image sha256:0af226fe285bd8430937bd657f4a059b231ddb0f2a7355040a5256171292e4c3                        0.0s 
 => => naming to docker.io/library/bitcoin-builder

edit:

Copied file bitcoin-qt.exe to Windows (using command docker cp container_id:docker_image_filepath) and ran it in there successfully.

I'll give it a go again tmw on WSL as well.

pablomartin4btc

I'll give it a go again tmw on WSL as well.

tACK 71aa125

Tested cross-build for win on WSL Ubuntu 22.04 usind Docker and running generated bitcoin-qt.exe on Windows successfully.

theuni · 2024-01-24T19:08:10Z

src/qt/CMakeLists.txt

+message(STATUS "Found Qt: ${Qt5_DIR} (found suitable version \"${Qt5_VERSION}\", minimum required is \"${qt_minimum_required_version}\")")
+unset(qt_minimum_required_version)
+
+file(GLOB ts_files RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} locale/*.ts)


Why glob rather than listing explicitly? This introduces potential non-determinism based on the builder's environment.

You are right. I would consider this approach as a temporary one until the https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py script is updated. Specifically, its update_build_systems function.

Also the bitcoin_locale.qrc file could be a build artifact rather than a source tree file.

Maybe add a comment to make this clear?

Maybe add a comment to make this clear?

Added.

vasild

ACK 6f1f8b0

theuni · 2024-01-26T17:18:59Z

depends/packages/qt.mk

@@ -281,12 +281,12 @@ define $(package)_build_cmds
 endef

 define $(package)_stage_cmds
-  $(MAKE) -C qtbase/src INSTALL_ROOT=$($(package)_staging_dir) $(addsuffix -install_subtargets,$(addprefix sub-,$($(package)_qt_libs))) && \
-  $(MAKE) -C qttools/src/linguist INSTALL_ROOT=$($(package)_staging_dir) $(addsuffix -install_subtargets,$(addprefix sub-,$($(package)_linguist_tools))) && \
+  $(MAKE) -C qtbase INSTALL_ROOT=$($(package)_staging_dir) install && \


Can you explain this change? It's been ages since I've messed with the qt build, but IIRC this was done to keep the installed libs to a minimum. Now we just install everything?

Otherwise, the CMake-driven configuration fails. I cannot point at the exact error as I did that change a year ago and just don't remember the exact problem.

Ok. Sounds like maybe it just added some additional cmake targets added to our list? Otherwise I believe this means $(package)_qt_libs is now unused.

Mind adding a comment?

"TODO: Investigate whether specific targets can be used here to minimize the amount of files/components installed.".

Mind adding a comment?

"TODO: Investigate whether specific targets can be used here to minimize the amount of files/components installed.".

Added.

theuni

ACK (after adding the depends comment) to keep things moving.

See: https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio

hebasto · 2024-01-26T19:26:23Z

ACK (after adding the depends comment) to keep things moving.

The comment has been added.

While approving this PR, do you mind to approve bitcoin-core/gui#789 as well?

8023640 qt: Avoid non-self-contained Windows header (Hennadii Stepanov) Pull request description: Using the `windows.h` header guarantees correctness regardless of the content of other headers. For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager. Related to hebasto#77. ACKs for top commit: theuni: ACK 8023640. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless. Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c

TheCharlatan · 2024-01-26T22:24:16Z

Post-merge ACK

63c1bb5 fixup! cmake: Add fuzzing options (Hennadii Stepanov) d31abc2 fixup! ci: Test CMake edge cases (Hennadii Stepanov) Pull request description: From #85 (comment): > **Known Bugs** > > Unfortunately, due to a silent conflict between #43 and #77, providing `-DFUZZ=ON` does not disable the `bitcoin-qt` target. It will be fixed shortly after pushing this branch. Fixed. ACKs for top commit: vasild: ACK 63c1bb5 Tree-SHA512: b3cc2889d0239913de64c170880c97b37966a890d8c4e05f9090485a016b7f9cdf4880d770a234f323d3191b9adda8ed0343c29dfa49b5bb99b0b54481d4335e

pablomartin4btc

post-merge ACK 7f4babc

After investigating and debugging cmake building process with @hebasto's help, failures reported on previous comment were not related to the QT module. Root cause of that problem is still under investigation.

pablomartin4btc

After investigating and debugging cmake building process with @hebasto's help, failures reported on previous comment were not related to the QT module. Root cause of that problem is still under investigation.

Just for reference the issue was inconsistency in the binary building due to the linker using incorrect library path, so first thing is that I was having installed both packages g++-mingw-w64-x86-64 and g++-mingw-w64-x86-64-win32, as per our doc we only require posix version for win cross-compilation (I've removed the win32 version and I managed to build succesfully), digging on where the incorrect path was determined is that @hebasto found a second issue that's related with cmake itself and the way the implicit directories are set, so he raised it in cmake gitlab repo.

8023640 qt: Avoid non-self-contained Windows header (Hennadii Stepanov) Pull request description: Using the `windows.h` header guarantees correctness regardless of the content of other headers. For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager. Related to hebasto#77. ACKs for top commit: theuni: ACK 8023640. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless. Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c

hebasto force-pushed the 240117-cmake-S branch 2 times, most recently from c1e7e37 to fa34d99 Compare January 17, 2024 19:16

hebasto mentioned this pull request Jan 17, 2024

CMake-based build system tracking issue bitcoin/bitcoin#28607

Closed

16 tasks

hebasto force-pushed the 240117-cmake-S branch from fa34d99 to 14b9171 Compare January 18, 2024 10:13

Revert "build, qt: Do not install *.prl files"

adcb895

This reverts commit 1155978.

hebasto force-pushed the 240117-cmake-S branch from 14b9171 to ce9dcfd Compare January 18, 2024 13:07

fanquake reviewed Jan 19, 2024

View reviewed changes

hebasto force-pushed the 240117-cmake-S branch from ce9dcfd to b0a1bae Compare January 19, 2024 17:20

fixup! build: Generate share/toolchain.cmake in depends

2609377

hebasto force-pushed the 240117-cmake-S branch from b0a1bae to c6c2cf9 Compare January 19, 2024 19:08

hebasto mentioned this pull request Jan 19, 2024

Avoid non-self-contained Windows header bitcoin-core/gui#789

Merged

pablomartin4btc reviewed Jan 19, 2024

View reviewed changes

vasild approved these changes Jan 22, 2024

View reviewed changes

depends/patches/qt/qt.pro Show resolved Hide resolved

depends/packages/qt.mk Show resolved Hide resolved

hebasto force-pushed the 240117-cmake-S branch from c6c2cf9 to 71aa125 Compare January 22, 2024 16:43

vasild approved these changes Jan 22, 2024

View reviewed changes

pablomartin4btc reviewed Jan 23, 2024

View reviewed changes

pablomartin4btc reviewed Jan 24, 2024

View reviewed changes

pablomartin4btc approved these changes Jan 24, 2024

View reviewed changes

theuni reviewed Jan 24, 2024

View reviewed changes

hebasto force-pushed the 240117-cmake-S branch from 71aa125 to 6f1f8b0 Compare January 25, 2024 10:24

vasild approved these changes Jan 25, 2024

View reviewed changes

theuni reviewed Jan 26, 2024

View reviewed changes

theuni approved these changes Jan 26, 2024

View reviewed changes

hebasto added 4 commits January 26, 2024 19:23

cmake: Build bitcoin-qt executable

b342887

msvc: Fix building with vcpkg's Qt packages

923d883

See: https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio

fixup! cmake: Add vcpkg manifest file

de62901

fixup! ci: Test CMake edge cases

7f4babc

hebasto force-pushed the 240117-cmake-S branch from 6f1f8b0 to 7f4babc Compare January 26, 2024 19:24

hebasto merged commit ff2f574 into cmake-staging Jan 26, 2024
22 checks passed

hebasto mentioned this pull request Jan 26, 2024

cmake: Regular rebasing of the cmake-staging branch #85

Closed

hebasto mentioned this pull request Jan 27, 2024

cmake: Bug fixes #87

Merged

pablomartin4btc reviewed Feb 4, 2024

View reviewed changes

pablomartin4btc reviewed Feb 21, 2024

View reviewed changes

	if(CMAKE_CROSSCOMPILING)
	# The find_package(Qt ...) function internally uses find_library()
	# calls for all dependencies to ensure their availability.
	# In turn, the find_library() inspects the well-known locations
	# on the file system; therefore, it must be able to find
	# platform-specific system libraries, for example:
	# /usr/x86_64-w64-mingw32/lib/libm.a or /usr/arm-linux-gnueabihf/lib/libm.a.
	set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY BOTH)
	endif()

cmake: Build bitcoin-qt executable #77

cmake: Build bitcoin-qt executable #77

Conversation

hebasto commented Jan 17, 2024

hebasto commented Jan 17, 2024

hebasto commented Jan 18, 2024

hebasto commented Jan 18, 2024

hebasto commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

hebasto Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

m3dwards commented Jan 19, 2024 • edited Loading

m3dwards commented Jan 19, 2024 • edited Loading

pablomartin4btc left a comment

Choose a reason for hiding this comment

hebasto commented Jan 19, 2024

vasild left a comment

Choose a reason for hiding this comment

hebasto commented Jan 22, 2024

vasild left a comment

Choose a reason for hiding this comment

theuni commented Jan 22, 2024 • edited Loading

pablomartin4btc left a comment

Choose a reason for hiding this comment

hebasto commented Jan 23, 2024

pablomartin4btc left a comment • edited Loading

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

hebasto commented Jan 26, 2024

TheCharlatan commented Jan 26, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

cmake: Build `bitcoin-qt` executable #77

cmake: Build `bitcoin-qt` executable #77

hebasto Jan 19, 2024 •

edited

Loading

hebasto Jan 22, 2024 •

edited

Loading

hebasto Jan 22, 2024 •

edited

Loading

m3dwards commented Jan 19, 2024 •

edited

Loading

m3dwards commented Jan 19, 2024 •

edited

Loading

theuni commented Jan 22, 2024 •

edited

Loading

pablomartin4btc left a comment •

edited

Loading

hebasto Jan 24, 2024 •

edited

Loading