Skip to content
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.

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

Adjust CMake build behavior to match Qt project files #1636

Merged
merged 4 commits into from
Nov 10, 2024

Conversation

cmuellner
Copy link
Contributor

I'm building qlcplus for Fedora using my own SPEC file, where I invoke qt-make (similar to the SPEC file in the qlcplus repo).
My assumption is, that the CMakeFile scripts should be the preferred build scripts, so there is a motivation to move there.
I tried to switch to cmake several times but always gave up as I ran into weird build issues.
This time, I decided to fix them.

I also switched my build from Qt5 to Qt6.

In total, I had to solve four issues, resulting in the four commits of this PR:

  1. Hard-coded "qt5" in the PLUGINDIR path definition
  2. Hard-coded Debian-style path in the LIBSDIR path definition
  3. Missing build of the HTML docs
  4. Installation of velleman library

One thing, that I did not fix is, that QLC+ can't be built out-of-tree because of generated header files (the include directory is relative to the source root and not the build root). I did not fix that, as it was not really a problem to do an in-tree build.

There is also a difference in the version-postfix of shared libraries (foo.so vs foo.so.1.0.0) between the CMakeFile scripts (creating foo.so) and the Qt project files (creating foo.so.1.0.0). I did not change that.

@coveralls
Copy link

coveralls commented Nov 6, 2024

Coverage Status

coverage: 31.978%. remained the same
when pulling c5a5d5b on cmuellner:cmake-build-fedora
into 8a5a946 on mcallegari:master.

@mcallegari
Copy link
Owner

mcallegari commented Nov 6, 2024

Hi, thanks for this but:

@cmuellner
Copy link
Contributor Author

Hi, thanks for this but:

Ok.

Should I add a commit to remove doc generation from the Qt project (resources/resources.pro includes SUBDIRS += docs)?

  • velleman plugin is being phased out, so sooner or later it will be removed from build/installation

I need to handle it somehow.
If it is built and installed, then rpmbuild complains if it is not packaged.
So, if I don't want it in the package, then I must prevent the build (that's what the commit does).

@mcallegari
Copy link
Owner

Should I add a commit to remove doc generation from the Qt project (resources/resources.pro includes SUBDIRS += docs)?

Yes please.

  • velleman plugin is being phased out, so sooner or later it will be removed from build/installation

I need to handle it somehow. If it is built and installed, then rpmbuild complains if it is not packaged. So, if I don't want it in the package, then I must prevent the build (that's what the commit does).

Up to you, but as I said, at some point I will remove it everywhere.

As for hardcoded 'qt5' paths I think there will be many more to fix. I checked also the qmltoqt6 branch and I think many changes are missing there too

@cmuellner
Copy link
Contributor Author

As for hardcoded 'qt5' paths I think there will be many more to fix. I checked also the qmltoqt6 branch and I think many changes are missing there too

Most of which I have seen are gated by appimage.
For a UNIX/Linux build, I did not see any other qt5 issues.
Building and running a quick test with audio and video worked fine.

@cmuellner
Copy link
Contributor Author

I've udpated the PR:

  • replace the commit that adds the docs to the CMake build with a commit that removes the docs from the Qt project

@@ -89,7 +89,7 @@ if (WIN32)
elseif (APPLE)
set(LIBSDIR "Frameworks")
elseif (UNIX)
set(LIBSDIR "lib/${CMAKE_C_LIBRARY_ARCHITECTURE}")
set(LIBSDIR "${CMAKE_INSTALL_LIBDIR}")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check if this works on the Raspberry Pi and in general on Debian package creation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Fedora this gives lib64.

The CMake implementation explicitly mentions that it works different for Debian and does so here: https://gitlab.kitware.com/cmake/cmake/-/blob/v3.26.4/Modules/GNUInstallDirs.cmake?ref_type=tags#L276

But yes, testing it is the best thing to do (I don't have a RasPi, so I cannot do that).

Copy link
Owner

@mcallegari mcallegari Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked on Ubuntu

---> CMAKE_C_LIBRARY_ARCHITECTURE x86_64-linux-gnu
---> CMAKE_INSTALL_LIBDIR lib/x86_64-linux-gnu

Checked also on the Raspberry Pi

---> CMAKE_C_LIBRARY_ARCHITECTURE=aarch64-linux-gnu
---> CMAKE_INSTALL_LIBDIR=lib/aarch64-linux-gnu

So this change can actually work on Debian-like systems.

@@ -1,6 +1,6 @@
TEMPLATE = subdirs

!qmlui: SUBDIRS += docs
!qmlui:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the whole line

variables.cmake Outdated
@@ -289,7 +289,7 @@ elseif (UNIX)
if (appimage)
set(PLUGINDIR "../lib/qt5/plugins/qlcplus")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd port the change also to this line

Currently, we set a PLUGINDIR path that includes a hard-coded "qt5"
subdir name in it. This obviously fails for Qt6.
Luckily, we already have $QT_MAJOR_VERSION variables, which can be
used to replace the hard-coded 5 by the actual version to build for.

Signed-off-by: Christoph Müllner <[email protected]>
We already get a properly defined library directory path
in the variable CMAKE_INSTALL_LIBDIR from invoking
`include(GNUInstallDirs)` in the toplevel CMakeLists.txt.
This is already addressing differences between different Linux
distros. E.g., for Debian we get "lib/${CMAKE_LIBRARY_ARCHITECTURE}"
and for Fedora we get "lib" or "lib64".
So, let's use that variable.

Signed-off-by: Christoph Müllner <[email protected]>
Docs are no longer bundled with QLC+.
The CMakeLists.txt does not need to be adjusted as it
does not include the corresponding subdirectory.

Signed-off-by: Christoph Müllner <[email protected]>
On platforms other than Windows, libvelleman is just a mockup
implementation for testing purposes. Therefore, the Qt-project file
excludes this library from being installed on non-Windows platforms.
Let's do the same in the CMakeLists.txt file.

Signed-off-by: Christoph Müllner <[email protected]>
@cmuellner
Copy link
Contributor Author

I've updated the PR as requested.

@mcallegari mcallegari merged commit 9e273b5 into mcallegari:master Nov 10, 2024
9 checks passed
@mcallegari
Copy link
Owner

Merged, thanks 👍

@ktdreyer
Copy link

Hi @cmuellner, I'm testing building qlcplus for Fedora also. My latest Git snapshot is at https://copr.fedorainfracloud.org/coprs/ktdreyer/qlcplus/ , using the in-tree platforms/linux/qlcplus.spec file.

@mcallegari
Copy link
Owner

We've got Fedora builds since years here
https://build.opensuse.org/project/show/home:mcallegari79
triggered at every commit

@cmuellner
Copy link
Contributor Author

Hi @cmuellner, I'm testing building qlcplus for Fedora also. My latest Git snapshot is at https://copr.fedorainfracloud.org/coprs/ktdreyer/qlcplus/ , using the in-tree platforms/linux/qlcplus.spec file.

I'm using my own spec file. You can find all details here: https://copr.fedorainfracloud.org/coprs/cmuellner/qlcplus/

@ktdreyer
Copy link

ktdreyer commented Nov 12, 2024

We've got Fedora builds since years here

Would you please consider updating to the latest Fedora versions? I see Fedora 37 and 38 there, and those are EOL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants