CMake not spotting missing qtbase5-private-dev

[zapashcanon]

Hi,

When trying to build LMMS on Debian Unstable, I got:

/home/zapashcanon/Programmation/C++/lmms/src/3rdparty/qt5-x11embed/src/X11EmbedContainer.cpp:40:10: fatal error: 'QtCore/private/qobject_p.h' file not found
#include <QtCore/private/qobject_p.h>
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [src/3rdparty/qt5-x11embed/CMakeFiles/qx11embedcontainer.dir/build.make:63: src/3rdparty/qt5-x11embed/CMakeFiles/qx11embedcontainer.dir/src/X11EmbedContainer.cpp.o] Error 1

I opened an issue against qt5-x11embed. Finally, @lukas-w told me I was missing qtbase5-private-dev, that was indeed the problem, which should have been spotted by CMake in LMMS.

[lukas-w]

I remember a discussion on Discord about this issue. We found that the problem is FIND_PACKAGE(Qt5 …) being called twice results in the second call being ineffective because of the way Qt's find modules work internally. One possible solution to this would be to use ExternalProject_Add instead of add_subdirectory to build qt5-x11embed.

[tresf]

Here's the archive of the conversation that @lukas-w is talking about:

tresf - 02/23/2018

@lukas-w is there a chance it's a regression caused by changing our syntax? https://github.com/LMMS/lmms/blob/master/CMakeLists.txt#L139
The submodule uses

find_package(Qt5Core COMPONENTS Private REQUIRED)
#               ^

whereas master uses

FIND_PACKAGE(Qt5 COMPONENTS Core Gui Widgets Xml REQUIRED)
#                           ^

This newer technique is needed for better Qt5 detection with the non-Linux platforms per https://blog.kitware.com/cmake-finding-qt5-the-right-way/

Lukas-W - 02/23/2018

@tresf I don't think so, find_package(Qt5 COMPONENTS Core) just calls find_package(Qt5Core) internally
@tresf The contents of Qt5CoreConfig.cmake are wrapped in a if (NOT TARGET Qt5::Core) condition, so it looks like my initial assumption is right

tresf - 02/23/2018

@lukas-w can we hack in a unset (<some_varialble> CACHE) to workaround the issue?

Lukas-W - 02/23/2018

@tresf I don't think so. There's no way to unset a target in CMake.
The "canonical" way of doing it would be to use ExternalProject_Add instead of add_subdirectory

tresf - 02/23/2018

@lukas-w Could we detect the private components are needed upfront and hack it in there?
I realize it would take it out of scope of the submodule, but it might help the users trying to build it.
@lukas-w I was able to compile qt5-x11embed fine with the CMAKE_PREFIX_PATH set to /opt/qt59 and without qtbase5-private-dev installed.
Qt59 standalone ships with the private headers, but from what I'm reading we shouldn't necessarily expose them to the entire project.
@lukas-w On my system, Ubuntu 14.04 qt-x11embed, it completely ignores CMAKE_PREFIX_PATH for some reason. I'm going to do a fresh clone from lmms/lmms/master and see if the syntax is an influencing factor. (Edit: It's not)
My theory is that this is why the additional Ubuntu package qtbase5-private-dev is incorrectly needed on master even when building with a custom Qt5 version, such as from /opt/qt59 (Edit: I did confirm it shouldn't be needed if the above bug were fixed)

[tresf]

At the end, my proposal was to hack in a way to pick up the variables introduced by the parent project and not call FIND_PACKAGE(Qt5) unless it wasn't already found. (e.g. anticipate the private-dev on the LMMS-side) @lukas-w can we hack something like that in or do you feel we need to switch it over to ExternalProject_Add ?

[lukas-w]

@tresf We can certainly anticipate the need for private components LMMS-side. There shouldn't be a need to change anything within qt5-x11embed because the second call to FIND_PACKAGE(Qt5) will be ineffective.

[PhysSong]

Can we work around this bug like this?

IF(LMMS_BUILD_LINUX)
	FIND_PACKAGE(Qt5Core COMPONENTS Private REQUIRED)
	FIND_PACKAGE(Qt5 COMPONENTS X11Extras REQUIRED)
	LIST(APPEND QT_LIBRARIES Qt5::X11Extras)
ELSE()
	FIND_PACKAGE(Qt5 COMPONENTS Core REQUIRED)
ENDIF()

FIND_PACKAGE(Qt5 COMPONENTS Gui Widgets Xml REQUIRED)

We should find the Core component before any others because they depend on Qt5::Core.

[PhysSong]

One possible solution to this would be to use ExternalProject_Add instead of add_subdirectory to build qt5-x11embed.

I also think we should eventually do that. If we do so, how about other submodules? Should we switch them to ExternalProject_Add as well?

[tresf]

@PhysSong both proposals make sense to me. The first one can probably be commented with a # TODO to be removed once the project has been switched to use ExternalProject_Add for all necessary submodules.

ExternalProject_Add may not be a good solution to all submodules though especially ones that don't have native CMake support like several of our plugins. Examples include OpulenZ, Xpressive, FreeBoy, LadspaEffect/tap, LadspaEffect/swh, LadspaEffect/calf. Most of the submodule candidates required some custom CMake logic on our side for compilation.

[PhysSong]

That's right. However, I think rpmalloc can also be a candidate.

[claell]

I just ran into this. Since this seems to be also caused by a missing package, should the corresponding Wiki pages with dependencies also updated?

[tresf]

If the environment your building for is missing the instructions please feel free to edit the wiki to fix them or feel free to propose the change here.

For example, in Ubuntu, it's in the VST section (not sure if that's the right place).

[claell]

I used Ubuntu, but did not install the things from the VST section as they seemed optional to me. So shell I change the section for that package?

[tresf]

So shell I change the section for that package?

Sure, thanks. 🐚 Go ahead and bump it up to the main dependencies block if that's what you believe is required.

Note, Qt5-downstream (from Ubuntu/Debian) repos don't offer the private headers by default and require the additional package however Qt5-upstream from Qt's website does, but this message still occurs when using Qt5-upstream due to the discussion above. Because of this, this building issue needs to remain open until we permanently fix this. A quick explanation is here above: #4273 (comment)

[claell]

Alright, done for Ubuntu.

[rdrpenguin04]

Still an issue it seems, but, since this thread exists, I at least knew which package to install. Thank you!