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

Cleanup of host software CMake build system #664

Merged
merged 14 commits into from
Dec 3, 2021

Conversation

multiplemonomials
Copy link
Contributor

Hi all! Recently we got a HackRF at my lab (USC RPL), and while we're having a really fun time playing around with it, we did have a few issues at the start getting the host software set up -- especially on Windows with MSVC. It wasn't anything we couldn't deal with, but I did see a few places where the CMake build system could be improved, so I thought I'd try to clean it up a bit. I incorporated a few bits and pieces from other projects I've worked on (mainly LibraryUtils.cmake, and a much less sophisticated version of FindFFTW), but also wrote a fair amount of new logic from scratch. Hopefully the build system should run a bit smoother now!

There are a lot of small changes as you can see, but I'll try to list the major things here:

  • Clean up and consolidate logic that was split across multiple build files into a single clean block at the top level
    • Compile flags
    • Dependencies
    • Uninstall logic
  • Rewrite FindFFTW based on my earlier version. It's now smart enough to consider libfftw3 vs libfftw3f (as well as all the other fftw3 libraries) as separate components, and to generate good, clear error messages if one of them is missing.
  • Clean up FindUSB and FindHACKRF according to CMake find module best practices.
    • Hopefully the issues where the configuration continues even though libusb was not found (which should have been a fatal error) should stop now!
  • Add a proper find module for libm
  • Set up hackrf DLL export / import logic to be automatically managed by CMake.

Hope all this stuff seems useful! As you can see from the build instructions, it makes the Windows build quite a bit simpler because it enables all libraries to be found correctly just by setting the prefix path to where the libraries are installed. Hopefully it should improve life on other platforms too.

I'm happy to answer any other questions or make any other changes you need, just let me know.

@mossmann mossmann requested review from miek and ktemkin December 17, 2019 22:50
@mossmann mossmann requested review from Qyriad and removed request for ktemkin January 21, 2020 00:22
@straithe straithe removed the request for review from Qyriad June 21, 2021 18:40
@straithe straithe added the enhancement potential new feature label Nov 4, 2021
@multiplemonomials
Copy link
Contributor Author

Is this going to be looked at?

@straithe
Copy link
Member

straithe commented Nov 10, 2021

This has been looked at internally, but there are some conflicts with the existing repository that need to be addressed. We've been discussing how we would like to update this so there are no conflicts.

We do really appreciate that you have submitted this PR and we are interested in accepting it. Thank you for checking in.

If you are interested in updating this PR so it does not have any merge conflicts, it might help us merge it sooner.

@multiplemonomials
Copy link
Contributor Author

Good to hear! I went ahead and rebased it.

@straithe
Copy link
Member

Thank you! I really appreciate it.

@multiplemonomials
Copy link
Contributor Author

Actually I looked over the code again and realized that FindFFTW was quite out of date from the "upstream" version (I originally made it for the Amber MD project). So, I copied in the new version, as well as another really useful module it uses, TryLinkLibraries, which is used to make sure that libraries found by CMake are properly linkable.

@straithe
Copy link
Member

Thanks for the updates! We ran our CI and there was a CMake error on Ubuntu. Would you please dig into that and address that issue?

@straithe straithe self-requested a review November 14, 2021 19:17
@multiplemonomials
Copy link
Contributor Author

multiplemonomials commented Nov 15, 2021

Oops missed a

include(CMakePushCheckState)

Should be fixed now.

@straithe straithe removed their request for review November 15, 2021 17:09
@straithe
Copy link
Member

Thank you for the code updates! Your PR passes all the preliminary checks. I'll make sure the reviewing team takes a look when they can.

@straithe straithe merged commit d60fb83 into greatscottgadgets:master Dec 3, 2021
@straithe
Copy link
Member

straithe commented Dec 3, 2021

Thank you very much for this PR!

@mossmann
Copy link
Member

mossmann commented Dec 9, 2021

We decided to revert this PR after discovering a couple problems:

  1. This broke the ability to build at the subdirectory level (libhackrf or hackrf-tools) in addition to the top (host) level (discovered in [FreeBSD] host/hackrf-tools: cmake set_release problem #1007). We require this ability in order to support distributions that package the library and the command-line tools separately.

  2. This changed the libhackrf.so versioning from MAJOR.MINOR.PATCH to MAJOR.MINOR which can cause some installation problems if people make install both before and after the PR is merged. We're not sure, but this might also be troublesome for distro packagers, so we think it would be safest to continue using MAJOR.MINOR.PATCH.

The second problem looks like it could be fixed easily, but the first one appears to be more difficult and is a more significant problem.

@martinling
Copy link
Member

2. This changed the libhackrf.so versioning from MAJOR.MINOR.PATCH to MAJOR.MINOR which can cause some installation problems if people make install both before and after the PR is merged.

To give the specifics of this problem - this was the result after make install on my development system, when working on my branch for #982 after merging in these changes:

$ ls -l /usr/local/lib/libhackrf.*
-rw-r--r-- 1 root staff  33388 Dec  7 23:56 /usr/local/lib/libhackrf.a
lrwxrwxrwx 1 root staff     14 Nov  3 18:01 /usr/local/lib/libhackrf.so -> libhackrf.so.0
lrwxrwxrwx 1 root staff     18 Dec  8 22:22 /usr/local/lib/libhackrf.so.0 -> libhackrf.so.0.7.0
-rw-r--r-- 1 root staff  42496 Dec  7 23:53 /usr/local/lib/libhackrf.so.0.6
-rw-r--r-- 1 root staff  42496 Dec  7 23:56 /usr/local/lib/libhackrf.so.0.6.0
-rw-r--r-- 1 root staff 129696 Dec  8 22:22 /usr/local/lib/libhackrf.so.0.7
-rw-r--r-- 1 root staff  42648 Dec  3 18:40 /usr/local/lib/libhackrf.so.0.7.0

The modifiied build system installed libhackrf.so.0.7, but left libhackrf.so.0 symlinked to a libhackrf.so.0.7.0 from several days before, so the make install did not actually cause the version in use to change. This caused a lot of confusion when trying to debug why my changes weren't working!

@multiplemonomials
Copy link
Contributor Author

multiplemonomials commented Dec 9, 2021

Ok I'll work on fixing these issues.
One thing I just realized is, would it be OK if, to build only the library or the applications, you had to run cmake at the top level directory but then only run make in a subdirectory? e.g. to build just libhackrf you would do:

cd host
mkdir build && cd build
cmake ..
cd libhackrf
make -j4 && sudo make install

If that's OK then the build system actually already supported this feature.

@martinling
Copy link
Member

would it be OK if, to build only the library or the applications, you had to run cmake at the top level directory but then only run make in a subdirectory?

It's not just about our own preference. At this point, there are quite a few distributions out there which will already have packaging set up to work with the build system as it was before - as well as any number of scripts etc that users may have set up privately.

I think we should avoid doing anything that will require changes to existing workflows, unless there's a really strong reason.

multiplemonomials added a commit to USCRPL/hackrf that referenced this pull request Dec 10, 2021
multiplemonomials added a commit to USCRPL/hackrf that referenced this pull request Dec 13, 2021
multiplemonomials added a commit to USCRPL/hackrf that referenced this pull request Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement potential new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hackrf host 2017.02.1 FreeBSD build problem (fftw3f)
4 participants