build: Try posix-specific CXX first for mingw32 host #22093
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
| @@ -1,3 +1,7 @@ | |||
| ifneq ($(shell which $(host)-g++-posix),) | |||
| mingw32_CXX := $(host)-g++-posix | |||
There was a problem hiding this comment.
Do we need := here? Can we just do =?
There was a problem hiding this comment.
Do we need
:=here?
No, we don't.
Can we just do
=?
Yes, we can.
But I do prefer simply expanded make variables to recursively expanded ones. They make debugging and reasoning about Makefiles much easier. Actually, I think the choice of a simply expanded variable is the default, while the usage of a recursively expanded variable requires justification.
In this particular case the host variable is simply expanded, and all code is placed in an included file. Honestly, I cannot see advantages for mingw32_CXX being a recursively expanded variable here.
Or did I miss something?
There was a problem hiding this comment.
I do not know enough of the make assignment semantics to weigh in here.
|
Rebased 6f44c42 -> 2fda0c7 (pr22093.01 -> pr22093.02) due to the conflict with #22088. |
There was a problem hiding this comment.
re-ACK 2fda0c7
I think it is better to automatically set this rather than have a user manually configure this. Tested on Ubuntu 20.04.
Note that on another system where the mingw-cxx is not bundled with different versions like the ubuntu package is, such as Arch Linux, the following line will be in the depends output:
which: no x86_64-w64-mingw32-g++-posix in ...
Guix Hashes:
$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 guix-build-2fda0c785188/output/dist-archive/SKIPATTEST.TAG
129fe7c0234cf1e1d2b8c71dc36469a7dcb07e1a7c1806c547ecb29824a43c78 guix-build-2fda0c785188/output/dist-archive/bitcoin-2fda0c785188.tar.gz
0e13399126a5acbc1ab62d32700dd37ae7a43490c0ef74aa35d347bb9e1dadb9 guix-build-2fda0c785188/output/x86_64-w64-mingw32/bitcoin-2fda0c785188-win-unsigned.tar.gz
5d0468c34d947251e472ac9bf6182d9bb1a48be16c1d8fe48ad6f6ed423f87b3 guix-build-2fda0c785188/output/x86_64-w64-mingw32/bitcoin-2fda0c785188-win64-debug.zip
d757fa29ae8e7ee6ba709efca19549a9837df4173633c3b9fbf1753b156cd29a guix-build-2fda0c785188/output/x86_64-w64-mingw32/bitcoin-2fda0c785188-win64-setup-unsigned.exe
49057728c99484ca7b96727183a368ea5ffd0b8f58e2d51e806c17946ba8caf4 guix-build-2fda0c785188/output/x86_64-w64-mingw32/bitcoin-2fda0c785188-win64.zip
10e3dda06008af8c7db65f1ee4d943df7f6f5138d35eaa3931676ed25648fcf5 guix-build-2fda0c785188/output/x86_64-w64-mingw32/inputs.SHA256SUMS
Guix builds
|
Gitian builds
|
|
I wonder if the mingw developers plan on supporting the C++ threading stuff in the That said, this makes sense for now. Concept ACK. |
|
|
||
| The first step is to install the mingw-w64 cross-compilation tool chain: | ||
|
|
||
| sudo apt install g++-mingw-w64-x86-64 |
There was a problem hiding this comment.
This always installs both variants?
There was a problem hiding this comment.
Apparently yes: https://packages.ubuntu.com/hirsute/g++-mingw-w64-x86-64
g++-mingw-w64-x86-64 is an umbrella package that depends on g++-mingw-w64-x86-64-posix and g++-mingw-w64-x86-64-win32. It does not contain any compiler itself, just some documentation files.
shaavan
left a comment
There was a problem hiding this comment.
tACK 2fda0c7
Tested on Ubuntu 20.04
This PR adds functionality to automatically select the POSIX model for cross-compilation of Windows bitcoin-core on Ubuntu. It is achieved by modifying mingw32.mk file so that if the POSIX thread model is present in the system, then declare ming32_CXX variable using Simply Expanded Variable declaration and equals it to the POSIX model’s location.
Tested the PR by first selecting the POSIX model, and trying to cross-compile the PR. And then cross-compiling the PR again while setting a non-POSIX(win32) model. I was able to successfully cross-compile the PR in both cases.
On master:
The cross-compilation was successful when the POSIX model was selected but failed when (win32) model was selected.
Error message while using (win32) model:
make[2]: *** [Makefile:10778: qt/libbitcoinqt_a-bitcoin.o] Error 1
make[2]: Leaving directory '/home/shaman/Documents/summerofbitcoin/window_cross/bitcoin/src'
make[1]: *** [Makefile:16271: all-recursive] Error 1
make[1]: Leaving directory '/home/shaman/Documents/summerofbitcoin/window_cross/bitcoin/src'
make: *** [Makefile:823: all-recursive] Error 1
On PR:
The cross-compilation was successful in both cases when the POSIX model was selected, and when the non-POSIX (win32) model was selected.
I agree with the changes suggested by op. These changes will lead to successful cross-compilation of bitcoin-core, even when the POSIX thread model is not selected. I think this particular change will help in two cases:
- Ease the windows cross-compilation process on Ubuntu a little bit easier.
- If the user is using some other software that requires win32 based thread model (non-POSIX model) for their working, they need not have to worry about changing the thread model, each time they cross-compile bitcoin-core.
|
My apologies about this. While this PR worked with Qt 5.12.11, it got broken with Qt 5.15.2. Even though the correct compiler has been chosen it appears that without Our CI task passes due to bitcoin/ci/test/05_before_script.sh Lines 46 to 48 in 6d859cb I have no explanation for now, but this PR must be reverted.
This is not the case for bionic or focal. But for hirsute, impish and jammy, if cc @laanwj @dongcarl @jarolrod @shaavan UPDATE: Another possible solution: --- a/depends/packages/qt.mk
+++ b/depends/packages/qt.mk
@@ -158,6 +158,7 @@ $(package)_config_opts_mingw32 += -no-dbus
$(package)_config_opts_mingw32 += -no-freetype
$(package)_config_opts_mingw32 += -xplatform win32-g++
$(package)_config_opts_mingw32 += "QMAKE_CFLAGS = '$($(package)_cflags) $($(package)_cppflags)'"
+$(package)_config_opts_mingw32 += "QMAKE_CXX = '$($(package)_cxx)'"
$(package)_config_opts_mingw32 += "QMAKE_CXXFLAGS = '$($(package)_cflags) $($(package)_cppflags)'"
$(package)_config_opts_mingw32 += "QMAKE_LFLAGS = '$($(package)_ldflags)'"
$(package)_config_opts_mingw32 += -device-option CROSS_COMPILE="$(host)-"UPDATE 2: A fix suggested in #24131. |
…ith Qt 5.15 9796dca doc: Install only "-posix" MinGW compiler when possible (Hennadii Stepanov) 0bbae23 ci: Drop no longer needed `update-alternatives` (Hennadii Stepanov) 01d1845 build, qt: Specify QMAKE_CXX explicitly (Hennadii Stepanov) Pull request description: While changes introduced in bitcoin/bitcoin#22093 worked fine with Qt 5.12, after bumping Qt up to 5.15 the cross-compiling of `qt` package for Windows fails with `error: ‘mutex’ in namespace ‘std’ does not name a type`. The first commit fixes this bug. The second commit cleans up a related CI script. The third commit improves related docs (see bitcoin/bitcoin#22093 (comment)). ACKs for top commit: prusnak: ACK 9796dca Tree-SHA512: 0dc46c5dfab85bd6d2901052cd630e86f9b4e09c08ef87136b44ddecb1783cdf3cd0a6e67b95ac7a78da24cd7adedc88745f61f9a8d9993fbff26d33bf88d874
9796dca doc: Install only "-posix" MinGW compiler when possible (Hennadii Stepanov) 0bbae23 ci: Drop no longer needed `update-alternatives` (Hennadii Stepanov) 01d1845 build, qt: Specify QMAKE_CXX explicitly (Hennadii Stepanov) Pull request description: While changes introduced in bitcoin#22093 worked fine with Qt 5.12, after bumping Qt up to 5.15 the cross-compiling of `qt` package for Windows fails with `error: ‘mutex’ in namespace ‘std’ does not name a type`. The first commit fixes this bug. The second commit cleans up a related CI script. The third commit improves related docs (see bitcoin#22093 (comment)). ACKs for top commit: prusnak: ACK 9796dca Tree-SHA512: 0dc46c5dfab85bd6d2901052cd630e86f9b4e09c08ef87136b44ddecb1783cdf3cd0a6e67b95ac7a78da24cd7adedc88745f61f9a8d9993fbff26d33bf88d874
On master (1186910), when cross-compiling for Windows using our depends build system, we must manually choose the POSIX thread model for the
x86_64-w64-mingw32-g++compiler.This PR improves the build system to make this choice automagically.