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

First CMake files #780

Merged
merged 57 commits into from
Sep 4, 2022
Merged

First CMake files #780

merged 57 commits into from
Sep 4, 2022

Conversation

lizardgai4
Copy link
Contributor

@lizardgai4 lizardgai4 commented Aug 26, 2022

Fixes #769

Plus new gitignore parameters for CMake-generated files, It's a good start.

Current flaws:

  • The program only works on Linux (but porting is very hard)

Fixed flaws:

  • Requires code modifications to work on multiple Linux distros
  • -ld-analyse requires modifications to the generated Makefile to compile Fixed (on Ubuntu at least)
  • FindQWT code and ld-analyse code currently occupy the same CMakeLists.txt file.
  • Having Qt6 installed at all might break installation (explicitly mentioning Qt5 instead of just Qt in the CMake files may fix it)
  • library install locations are hard-coded to /usr/lib64
  • ld-analyse compiles and runs on Ubuntu, but visual assets (png and svg files) currently don't install with the program.

Current flaws:

The thing only works on Linux, and library install locations are manually set to /usr/lib64

ld-analyse requires modifications to the makefile to compile

FindQWT code and ld-analyse code currently occupy the same CMakeLists.txt file.
@happycube
Copy link
Owner

I'm still on Ubutu 20.04, this doesn't work for me:

"
-- ---------Beginning cmake build of ld-ldf-reader---------
CMake Error at CMakeLists.txt:3 (cmake_minimum_required):
CMake 3.22 or higher is required. You are running version 3.16.3
"

@lizardgai4
Copy link
Contributor Author

@happycube is it possible to upgrade to CMake 3.22 on your machine?

Copy link
Collaborator

@atsampson atsampson left a comment

Choose a reason for hiding this comment

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

This isn't a detailed review, but a few comments from a first look through this...

The GitHub CI config needs updating to build the project with CMake - you should be able to copy the one from DomesdayDuplicator, which builds on both the oldest and newest versions of Ubuntu (do it with Qt 5 only for now).

The test rules from the top-level Makefile need converting to CMake tests - this should be easy enough since they're all just running a command and checking it exits 0.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
# Include all the paths
message( STATUS "--------- Finding packages---------")

find_package(Qt5 REQUIRED COMPONENTS Core)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd definitely get it working with Qt 5 first, but we will need Qt 6 support as well (see the DomesdayDuplicator commits for how we handled it there without needing to make too much stuff conditional).

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/CMakeLists.txt Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/encoder/CMakeLists.txt Outdated Show resolved Hide resolved
tools/ld-disc-stacker/CMakeLists.txt Outdated Show resolved Hide resolved
tools/library/filter/CMakeLists.txt Outdated Show resolved Hide resolved
tools/library/filter/CMakeLists.txt Outdated Show resolved Hide resolved
@lizardgai4
Copy link
Contributor Author

@atsampson I will make those changes tonight and test them before committing

atsampson and others added 19 commits September 1, 2022 03:48
I don't see any evidence that this is needed.
Only ld-process-efm needs this, since it has headers in subdirs.
Without this, it won't actually pass -std= to the compiler.
This largely follows what gnuradio does -- there is a single project()
for the whole thing, and the subdirs just declare targets within it.
This follows gnuradio's conventions.
I've put these in a separate file since they're quite long.
Verified to work on Ubuntu and Manjaro
Use gnuradio's FindQwt module (slightly modified).
@lizardgai4
Copy link
Contributor Author

lizardgai4 commented Sep 1, 2022

After some collaboration, it works with nearly every Linux distro I tested, no source code tweaking required!

Working:

  • Ubuntu 22.04 LTS
  • Fedora 36
  • Arch BTW (rolling release)
  • Manjaro 21.3.7 Ruah
  • OpenSUSE Leap 15.4
  • Solus 4.3

Not working: Slackware 15.0

UPDATE: I have declared it to difficult to support Slackware (unless we distribute binaries, or don't update library versions until Slackware catches up)

atsampson and others added 8 commits September 2, 2022 00:11
Only the lddecode-chroma library needs it, and it should use the result
of pkgconfig rather than specifying the name directly.
It looks like nearly all distributions that ship a recent Qwt also
include the pkgconfig file -- the only one I've tested that doesn't is
FreeBSD, which was broken anyway.
@happycube happycube merged commit cbb0693 into happycube:master Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for more Linux distros
3 participants