Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jun 3, 2021 •
edited
Loading

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 fanquake marked this pull request as draft June 3, 2021 14:48
@hebasto
Copy link
Member

hebasto commented Jun 3, 2021

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
Copy link
Contributor

DrahtBot commented Jun 4, 2021 •
edited
Loading

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

Conflicts

Reviewers, 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.

This was referenced Jun 4, 2021
Closed
Closed
Closed
@hebasto
Copy link
Member

hebasto commented Jun 7, 2021

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

@DrahtBot DrahtBot mentioned this pull request Jun 8, 2021
Merged
@hebasto
Copy link
Member

hebasto commented Jun 8, 2021

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

...
Configure summary:

Building on: linux-g++ (x86_64, CPU features: mmx sse sse2)
Building for: linux-g++-32 (i386, CPU features: <none>)
...

with the following patch:

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

@hebasto
Copy link
Member

hebasto commented Jun 8, 2021 •
edited
Loading

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

@hebasto
Copy link
Member

hebasto commented Jun 8, 2021

The branch with applied fixup is here.

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

hebasto
hebasto reviewed Jun 8, 2021
View reviewed changes
hebasto
hebasto reviewed Jun 8, 2021
View reviewed changes
@hebasto
Copy link
Member

hebasto commented Jun 8, 2021

The linguist stuff could be simplified:

--- a/depends/packages/native_qtbase.mk
+++ b/depends/packages/native_qtbase.mk
@@ -162,9 +162,7 @@ define $(package)_config_cmds
   qtbase/bin/qmake -o qtbase/src/tools/qfloat16-tables/Makefile qtbase/src/tools/qfloat16-tables/qfloat16-tables.pro && \
   qtbase/bin/qmake -o qtbase/src/tools/qvkgen/Makefile qtbase/src/tools/qvkgen/qvkgen.pro && \
   qtbase/bin/qmake -o qtbase/qmake/Makefile qtbase/qmake/qmake-aux.pro && \
-  qtbase/bin/qmake -o qttools/src/linguist/lrelease/Makefile qttools/src/linguist/lrelease/lrelease.pro && \
-  qtbase/bin/qmake -o qttools/src/linguist/lupdate/Makefile qttools/src/linguist/lupdate/lupdate.pro && \
-  qtbase/bin/qmake -o qttools/src/linguist/lconvert/Makefile qttools/src/linguist/lconvert/lconvert.pro
+  qtbase/bin/qmake -o qttools/src/linguist/Makefile qttools/src/linguist/linguist.pro
 endef
 
 define $(package)_build_cmds
@@ -174,9 +172,7 @@ define $(package)_build_cmds
   $(MAKE) -C qtbase/src/tools/qvkgen && \
   $(MAKE) -C qtbase/src/tools/rcc && \
   $(MAKE) -C qtbase/src/tools/uic && \
-  $(MAKE) -C qttools/src/linguist/lrelease && \
-  $(MAKE) -C qttools/src/linguist/lupdate && \
-  $(MAKE) -C qttools/src/linguist/lconvert
+  $(MAKE) -C qttools/src/linguist
 endef
 
 define $(package)_stage_cmds

Verified with 9046de8 (from #21995).

@fanquake fanquake changed the title [WIP] build: split depends Qt into native and target builds build: split depends Qt into native and target builds Jun 9, 2021
@fanquake
Copy link
Member Author

fanquake commented Jun 9, 2021

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.

fanquake
fanquake commented Jun 9, 2021
View reviewed changes
@fanquake fanquake marked this pull request as ready for review June 10, 2021 02:40
@DrahtBot DrahtBot mentioned this pull request Jun 10, 2021
Merged
@hebasto
Copy link
Member

hebasto commented Jun 10, 2021

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 WIP: Qt: add QML based mobile GUI #16883, or QtMultimedia for Qr-code scanning)

@hebasto hebasto mentioned this pull request Jun 11, 2021
Merged
@fanquake
Copy link
Member Author

compatibility with possible including other modules in the future

Is this approach actually incompatible with doing that?

@hebasto
Copy link
Member

hebasto commented Jun 11, 2021

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
Copy link
Member Author

Closing this in favour of #20641 for now.

@fanquake fanquake closed this Jun 18, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
@fanquake fanquake deleted the split_qt_again branch November 9, 2022 16:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto left review comments

Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

build: Fail to build Qt in depends on ARM 32bit
3 participants
@fanquake @hebasto @DrahtBot