build: split depends Qt into native and target builds

fanquake commented at 2:48 pm on June 3, 2021: member

This splits our Qt build in depends into two parts. The first builds qmake, qt tools (uic, moc, rcc etc) as well as translation tools (lconvert, lupdate etc). The second builds the libs we want for bitcoin-qt.

Splitting the build in this way has a few advantages. For example, we can enable the xml module while building the tools, and ideally fix issues like #14648 or #18536 without needing changes like #21589 or #21420, or having to build it alongside our libs.

This has been tested a little bit. GUIX builds are working for macOS and Windows.

Some testing still needs to be done in regards to passing compiler flags, DEBUG=1 builds etc. No doubt some improvements can still be made to what is currently here.

Fixes #18536.

fanquake added the label Build system on Jun 3, 2021

fanquake marked this as a draft on Jun 3, 2021

hebasto commented at 2:57 pm on June 3, 2021: member

Concept ACK. The same idea (a separated making of tools) is used for other dependencies.

Need more investigation to find out if this approach is compatible with adding additional Qt modules.

DrahtBot commented at 3:29 am on June 4, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20641 (depends: Use Qt top-level build facilities by hebasto)

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.

hebasto commented at 3:04 am on June 7, 2021: member

I think the reason of the failure on Centos 8 is wrong architecture the qmake is configured for – x86_64 instead of i386.

hebasto commented at 4:44 pm on June 8, 2021: member

I’m able to make -C depends qt_configured HOST=i686-pc-linux-gnu with the correct architecture detection:

0...
1Configure summary:
2
3Building on: linux-g++ (x86_64, CPU features: mmx sse sse2)
4Building for: linux-g++-32 (i386, CPU features: <none>)
5...

with the following patch:

 0--- a/depends/packages/qt.mk
 1+++ b/depends/packages/qt.mk
 2@@ -111,10 +111,6 @@ $(package)_config_opts += -no-feature-vulkan
 3 $(package)_config_opts += -no-feature-wizard
 4 $(package)_config_opts += -no-feature-xml
 5 
 6-$(package)_config_opts += "QMAKE_CFLAGS = '$($(package)_cflags) $($(package)_cppflags)'"
 7-$(package)_config_opts += "QMAKE_CXXFLAGS = '$($(package)_cflags) $($(package)_cppflags)'"
 8-$(package)_config_opts += "QMAKE_LFLAGS = '$($(package)_ldflags)'"
 9-
10 $(package)_config_opts_darwin = -no-feature-corewlan
11 $(package)_config_opts_darwin += QMAKE_MACOSX_DEPLOYMENT_TARGET=$(OSX_MIN_VERSION)
12 
13@@ -217,6 +213,9 @@ define $(package)_preprocess_cmds
14   cp -f $($(package)_patch_dir)/mac-qmake.conf qtbase/mkspecs/macx-clang-linux/qmake.conf && \
15   cp -r qtbase/mkspecs/linux-arm-gnueabi-g++ qtbase/mkspecs/bitcoin-linux-g++ && \
16   sed -i.old "s/arm-linux-gnueabi-/$(host)-/g" qtbase/mkspecs/bitcoin-linux-g++/qmake.conf && \
17+  echo "!host_build: QMAKE_CFLAGS     += $($(package)_cflags) $($(package)_cppflags)" >> qtbase/mkspecs/common/gcc-base.conf && \
18+  echo "!host_build: QMAKE_CXXFLAGS   += $($(package)_cxxflags) $($(package)_cppflags)" >> qtbase/mkspecs/common/gcc-base.conf && \
19+  echo "!host_build: QMAKE_LFLAGS     += $($(package)_ldflags)" >> qtbase/mkspecs/common/gcc-base.conf && \
20   sed -i.old "s|QMAKE_CC                = \$$$$\$$$${CROSS_COMPILE}clang|QMAKE_CC                = $($(package)_cc)|" qtbase/mkspecs/common/clang.conf && \
21   sed -i.old "s|QMAKE_CXX               = \$$$$\$$$${CROSS_COMPILE}clang++|QMAKE_CXX               = $($(package)_cxx)|" qtbase/mkspecs/common/clang.conf && \
22   sed -i.old "s/LIBRARY_PATH/(CROSS_)?\0/g" qtbase/mkspecs/features/toolchain.prf

hebasto commented at 5:22 pm on June 8, 2021: member

The branch with applied fixup is here. CI – https://cirrus-ci.com/build/4834341327994880 – is green :tiger2:

hebasto commented at 7:07 pm on June 8, 2021: member

The branch with applied fixup is here.

Tested on Armbian 21.02.3 – #18536 is fixed, and #21589 could be dropped.

in depends/packages/native_qtbase.mk:23 in 6e861db849 outdated

18+$(package)_config_opts_debug += -optimized-tools
19+$(package)_config_opts += -bindir $(build_prefix)/bin
20+$(package)_config_opts += -c++std c++1z
21+$(package)_config_opts += -confirm-license
22+$(package)_config_opts += -hostprefix $(build_prefix)
23+$(package)_config_opts += -hostdatadir $(build_prefix)/share

hebasto commented at 8:25 pm on June 8, 2021:

This line seems unneeded. Verified with 9046de8a4cbc3899fed9eae084115f423e7ac5bd (from #21995).

fanquake commented at 7:31 am on June 9, 2021:

Addressed in next push.

in depends/packages/native_qtbase.mk:142 in 6e861db849 outdated

138+endef
139+
140+define $(package)_extract_cmds
141+  mkdir -p $($(package)_extract_dir) && \
142+  echo "$($(package)_sha256_hash)  $($(package)_source)" > $($(package)_extract_dir)/.$($(package)_file_name).hash && \
143+  echo "$($(package)_qttools_sha256_hash)  $($(package)_source_dir)/$($(package)_qttools_file_name)" >> $($(package)_extract_dir)/.$($(package)_file_name).hash && \

hebasto commented at 8:26 pm on June 8, 2021:

style nit, extra spaces:

0  echo "$($(package)_sha256_hash) $($(package)_source)" > $($(package)_extract_dir)/.$($(package)_file_name).hash && \
1  echo "$($(package)_qttools_sha256_hash) $($(package)_source_dir)/$($(package)_qttools_file_name)" >> $($(package)_extract_dir)/.$($(package)_file_name).hash && \

fanquake commented at 7:31 am on June 9, 2021:

Addressed in next push.

hebasto commented at 8:54 pm on June 8, 2021: member

The linguist stuff could be simplified:

 0--- a/depends/packages/native_qtbase.mk
 1+++ b/depends/packages/native_qtbase.mk
 2@@ -162,9 +162,7 @@ define $(package)_config_cmds
 3   qtbase/bin/qmake -o qtbase/src/tools/qfloat16-tables/Makefile qtbase/src/tools/qfloat16-tables/qfloat16-tables.pro && \
 4   qtbase/bin/qmake -o qtbase/src/tools/qvkgen/Makefile qtbase/src/tools/qvkgen/qvkgen.pro && \
 5   qtbase/bin/qmake -o qtbase/qmake/Makefile qtbase/qmake/qmake-aux.pro && \
 6-  qtbase/bin/qmake -o qttools/src/linguist/lrelease/Makefile qttools/src/linguist/lrelease/lrelease.pro && \
 7-  qtbase/bin/qmake -o qttools/src/linguist/lupdate/Makefile qttools/src/linguist/lupdate/lupdate.pro && \
 8-  qtbase/bin/qmake -o qttools/src/linguist/lconvert/Makefile qttools/src/linguist/lconvert/lconvert.pro
 9+  qtbase/bin/qmake -o qttools/src/linguist/Makefile qttools/src/linguist/linguist.pro
10 endef
11 
12 define $(package)_build_cmds
13@@ -174,9 +172,7 @@ define $(package)_build_cmds
14   $(MAKE) -C qtbase/src/tools/qvkgen && \
15   $(MAKE) -C qtbase/src/tools/rcc && \
16   $(MAKE) -C qtbase/src/tools/uic && \
17-  $(MAKE) -C qttools/src/linguist/lrelease && \
18-  $(MAKE) -C qttools/src/linguist/lupdate && \
19-  $(MAKE) -C qttools/src/linguist/lconvert
20+  $(MAKE) -C qttools/src/linguist
21 endef
22 
23 define $(package)_stage_cmds

Verified with 9046de8a4cbc3899fed9eae084115f423e7ac5bd (from #21995).

fanquake renamed this:
~~[WIP] build: split depends Qt into native and target builds~~
build: split depends Qt into native and target builds
on Jun 9, 2021

fanquake force-pushed on Jun 9, 2021

fanquake commented at 7:44 am on June 9, 2021: member

I’m able to make -C depends qt_configured HOST=i686-pc-linux-gnu with the correct architecture detection: The linguist stuff could be simplified:

Thanks, I’ve addressed both of these points. Also addressed the comments you left inline and rebased for #22174. I think we can merge #22186 before this. I’m not really happy with how qt patches end up duplicated, but we should be able to fix that.

in depends/packages/qt.mk:214 in 38d1583fe9 outdated

201+# 3. After making a copy of the mkspec for the linux-arm-gnueabi host, named
202 # bitcoin-linux-g++, replace instances of linux-arm-gnueabi with $(host). This
203 # way we can generically support hosts like riscv64-linux-gnu, which Qt doesn't
204 # ship a mkspec for. See it's usage in config_opts_* above.
205 #
206-# 5. Put our C, CXX and LD FLAGS into gcc-base.conf. Only used for non-host builds.

fanquake commented at 1:26 pm on June 9, 2021:

needs to be restored here.

fanquake force-pushed on Jun 9, 2021

build, qt: Fix wrong cross-compiling detection on macOS ad6ea2f088

build: split qt into native and host builds ea4cdeba3a

fanquake marked this as ready for review on Jun 10, 2021

fanquake force-pushed on Jun 10, 2021

hebasto commented at 8:25 pm on June 10, 2021: member

Splitting the build in this way has a few advantages. For example, we can enable the xml module while building the tools, and ideally fix issues like #14648 or #18536 without needing changes like #21589 or #21420, or having to build it alongside our libs.

It is true that avoiding intrusive fixes to the build system–i.e., patching Qt code base–makes it more maintainable and more comprehensible. That is why Concept ACKed this PR.

But considering different approaches, I still prefer the #20641. It is better than this one for the following reasons:

no qmake building duplication
no patch duplication
using Qt “blessed” approach, that mimics building from the single source tree that includes all modules
compatibility with possible including other modules in the future (e.g., QtQml for #16883, or QtMultimedia for Qr-code scanning)

fanquake commented at 0:49 am on June 11, 2021: member

compatibility with possible including other modules in the future

Is this approach actually incompatible with doing that?

hebasto commented at 0:56 am on June 11, 2021: member

compatibility with possible including other modules in the future

Is this approach actually incompatible with doing that?

If new modules depends on core and tools are built in the native_qtbase.mk only – that is ok. But if they have mutual dependencies, this approach could fail (without additional hacks).

fanquake commented at 7:23 am on June 18, 2021: member

Closing this in favour of #20641 for now.

fanquake closed this on Jun 18, 2021

DrahtBot locked this on Aug 18, 2022

fanquake deleted the branch on Nov 9, 2022

build: split depends Qt into native and target builds #22142

Conflicts