-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Mait/modern cmake #1578
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
Mait/modern cmake #1578
Conversation
Refactor HackRF build system with a more modern CMake style. Install files to allow library users using CMake to find_project(HackRF CONFIG) and obtain HackRF::hackrf and/or HackRF::hackrf_static library targets. Add options ENABLE_STATIC_LIB and ENABLE_SHARED_LIB to allow shared or static library to be disabled. (Default builds both shared and static libraries.) Add option ENABLE_HACKRF_SWEEP, when disabled allows building without FFT library. (Default enabled.) Add option DISABLE_USB_DEVICE_DISCOVERY for a compile definition which could be used to alter libusb usage as needed on Android.
Modernize CMake usage in scripts to define a build directory using the cmake -B flag, use cmake --build rather than just make.
martinling
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @maitbot,
I'm working on reviewing this and have a few questions & notes:
-
It's a bit awkward having most of this in one commit, with multiple unrelated changes including a lot of reformatting. Was this by any chance squashed from a longer branch, and if so, would it be possible to see that branch - even if it's messy? I'd prefer to have these changes broken down a bit more, and I'm happy to help with doing that, but it would be easier if I had more history.
-
You specify that this is intended to replace #1016. In your view, are there any benefits of that earlier PR which are not included in this one, or do you see it as a complete replacement at this point?
-
Since you posted this, we've bumped our CMake minimum version to 3.10 rather than 3.16, for the reasons discussed in #1514 (comment). We've removed the obsolete Appveyor build that was failing on this PR, and I've added matrix testing across CMake versions to Github Actions.
-
I have a rebase of your PR at #1586 with some changes that I'd appreciate your comments on - these were primarily to keep compatibility to CMake 3.10, since that turned out to be fairly straightforward to do, but I also spotted one bug:
- Stick to
project(HackRF C)rather than using the longer signature includingHOMEPAGE_URL(added in 3.12). I don't think we gain anything from adding the URL there. - Avoid the use of
GenerateExportHeader, which only supported C++ projects until 3.12. Unless I'm missing something, this wasn't actually being used, since the line installinghackrf_export.hwas commented out. - Add an
ARCHIVE DESTINATIONargument to the Unixinstallcommand inlibhackrf_common_settingsfunction (needed for both 3.10 and 3.12, at least). - Correct an error spotted in
FindFFTW3f.cmake: theHINTSforFFTW3f_INCLUDE_DIRSshould includePC_FFTW3f_INCLUDE_DIRS(notDIR), as per FindPkgConfig docs. This broke some builds on macOS with Homebrew, where each package gets its own prefix so the hint frompkg-configis essential.
- Stick to
|
I think your revision (PR #1586) is better.
I used my trial hackrf consuming application with an install of your PR 1586 branch, and it works |
|
Thanks for the answers. Let's move forward with #1586 then - I'd like to make some further edits to it to split up the changes, and incorporate your notes from above about what has changed and which issues that addresses. |
modern cmake
Refactor host CMake build.
Replace pull request 1016
"Host software CMake cleanup: attempt #2 #1016"
#1016
to adress more issues.
Update cmake_minimum_required.
(GNU Radio picked 3.16, because Ubuntu 20.04 used 3.16.3, and that is
now more than 5 years ago.)
Like pull request 1514
"updated minimum required cmake version to 3.16 #1514"
#1514
but just for the host (not firmware) build.
Update project() call.
CMake project handles VERSION now too.
Defines PROJECT_VERSION and PROJECT_VERSION_MAJOR etc., which will be used
to set CMake library target properties for VERSION and SOVERSION.
Add some overall build defaults,
set(CMAKE_VERBOSE_MAKEFILE ON)
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
These settings help to not hide details from the build logs, and
editors using LSP will want to consume the generated compile_commands.json file.
(vim can use LSP with clangd, and emacs has LSP and eglot modes.)
Make use of CMake's GNUInstallDirs.
Smart way to set install DESTINATIONs.
Update all DESTINATION to use GNUInstallDirs variables.
(except MSVC builds - someone else want to change this?)
Just use CMake's FindThreads.
Use the Threads::Threads CMake target.
Remove cmake/modules/FindThreads.cmake
On Windows, use find_package(PThreads4W) and the
resulting PThreads4W::PThreads4W also used by the
Windows USB library.
Rename FindUSB1 to FindLIBUSB, and have it provide the
LIBUSB::LIBUSB CMake target. (This also avoids warnings about
confusion between LIBUSB vs USB1 definitions.) Solving
"cmake dev warning FindUSB1.cmake -- find_package(USB1 REQUIRED) #1199"
#1199
libhackrf:
Remove c_sources and c_headers CMake variables, they just add a
potentially confusing layer of indirection. Just be explicit when
referencing source files.
See CMake documentation for cmake-packages(7)
Install CMake Config-file package files for users.
Maybe include(GenerateExportHeader)? Added in version 3.12: Added
support for C projects. Previous versions supported C++ project only.
Solves issue "Install project config files for cmake projects to use libhackrf #391"
#391
hackrf-tools:
Use FindFFTW3.cmake from GNU Radio instead of FindFFTW and use the
modern CMake target fftw3f::fftw3f it provides.
Build with Modern CMake targets.
CMake options:
Keep option INSTALL_UDEV_RULES, "Install udev rules for the HackRF".
Add options to allow shared or static library to be disabled,
ENABLE_STATIC_LIB
ENABLE_SHARED_LIB.
Solves "[Bug Report]: static libs are unconditionally built/installed on linux #1193"
#1193
Add option to disable hackrf_sweep - which allows building without FFT library.
option(ENABLE_HACKRF_SWEEP
Solves issue "Building hackrf host code w/out fftw #729"
#729
Add option to control a DISABLE_USB_DEVICE_DISCOVERY compile definition,
which could be used to alter libusb usage as needed on Android.
+option(DISABLE_USB_DEVICE_DISCOVERY
Like the CMake portion of pull request 1150
"Non-root android support #1150"
#1150
update scripts
Also add a commit for a more modern use of CMake in a script:
cmake -B builddir
cmake --build builddir
This commit is only CMake build system changes. No modified C code.
Checked to ensure project build with both Makefile and Ninja generators.
CI builds OK, including Windows build.