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

Host software CMake cleanup: attempt #2 #1016

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

multiplemonomials
Copy link
Contributor

@multiplemonomials multiplemonomials commented Dec 10, 2021

Apologies for the issues that were encountered with the first version of my PR (#664), turns out that this build system was supposed to operate in ways I wasn't aware of. I went through all the feedback and made adjustments, and I believe I addressed all the issues with the original version. Summary of changes made:

  • Fixed SOVERSION bug, SOVERSION has 3 digits again
  • Adjusted build system to handle being run from all three possible top dirs: host/, host/hackrf-tools/, and host/libhackrf/
    • Moved common logic (compile flags, common dependencies, etc) into a script that is called from wherever the top directory is
    • Added back FindLIBHACKRF module (used by a standalone tools build to locate libhackrf) and improved it to better handle static libraries and Windows builds
  • Fixed some incorrect library install dirs

To see the full set of changes vs the original PR, just look at all commits after the first commit in this PR.

To make sure things were working, I ran builds this PR on Windows MinGW, Windows MSVC, and Ubuntu. For each environment, I tried a build from each of the three supported top directories. I'm fairly confident that it should be working the way you want it now!

@mossmann
Copy link
Member

Thank you so much for taking the time to work on this! We will review it soon.

@multiplemonomials
Copy link
Contributor Author

Had a chance to look at this yet?

@mossmann
Copy link
Member

I'm planning to make a release shortly and then review and merge this PR right after the release.

@multiplemonomials
Copy link
Contributor Author

@mossmann Pinging again one year later, I rebased this to be compatible with latest master branch. Everything look ok to merge?

I also updated the new github actions windows job to be compatible with this MR. Now, it uses CMAKE_PREFIX_PATH to pick up everything that vcpkg installs in one single swoop. To make this work, I had to update the library names searched for by the pthread and libusb find modules to match what vcpkg installs.

Last but not least, I'm seeing something odd where my local clang-format and the CI job are disagreeing about how to format one specific block of code (#define true 1). So running the clang format script does not generate code that passes the CI.

@straithe
Copy link
Member

Thank you for updating this PR again! We really appreciate your patience.

Our team is stretched very thin and will continue to be for a while. I'll do my best to see if I can get some community reviews and free up some time from the reviewers within GSG.

@multiplemonomials
Copy link
Contributor Author

@straithe any updates?

@straithe
Copy link
Member

I don't have the skillset to review this particularly well myself and the senior reviewers on my team are booked up for a couple months. I've asked in the GSG Discord for community reviews so I can ensure this solution works for a variety of systems.

@jLynx
Copy link

jLynx commented Jun 2, 2023

I tried building this, it was successful for windows, but the exe's didn't actually work for whatever reason. any thoughts @multiplemonomials ?

@multiplemonomials
Copy link
Contributor Author

What exactly didn't work? Did they fail to run? If so, that can be cause by having missing or incorrect DLLs in your system PATH. I recommend using Dependencies to load the executables and see what DLLs they are trying to load.

@jLynx
Copy link

jLynx commented Jun 5, 2023

@multiplemonomials you can download the artifacts here to try run it https://github.com/jLynx/hackrf/actions/runs/5151822083

And here is the workflow file https://github.com/jLynx/hackrf/actions/runs/5151822083/workflow

And when I said it didn't run, i mean when I called the exe via cli, nothing was outputted, so errors, nothing. -h didn't return anything either

@multiplemonomials
Copy link
Contributor Author

multiplemonomials commented Jun 6, 2023

Ah, I think the issue is that the artifacts don't include libhackrf.dll, which is necessary to run the executables. (they don't include the fftw dll either, which is needed for hackrf_sweep).

You can see this by running Dependencies on the executables:
image

I'm not a github actions expert, but I think the artifacts need to include the ${{runner.workspace}}/host/libhackrf/build folder as well, which will contain the DLL.

@jLynx
Copy link

jLynx commented Jun 6, 2023

Do the dll's get updated too, or are the the same ones from a few years ago? The reason I ask is because I copied some old ones in the dir to test and it didn't work. But I'm assuming they did get updated and that's the reason why @multiplemonomials

@multiplemonomials
Copy link
Contributor Author

Most likely, yeah.

@dmaltsiniotis
Copy link
Contributor

dmaltsiniotis commented Jun 6, 2023

Do the dll's get updated too, or are the the same ones from a few years ago? The reason I ask is because I copied some old ones in the dir to test and it didn't work. But I'm assuming they did get updated and that's the reason why @multiplemonomials

As far as I know, the same dependency DLL are used. These versions are the ones I use when building the latest host software on Windows and need to be included for libhackrf to operate:

fftw-3.3.5-dll64
libusb-1.0.24
pthreads-w32-2-9-1-release

@mossmann
Copy link
Member

mossmann commented Feb 1, 2024

@multiplemonomials Is there is a reason that cmake version 3.8 is required for this?

@multiplemonomials
Copy link
Contributor Author

Yes, I believe that it does use some CMake 3.8 specific features. But 3.8 should be available on any modern linux distro at this point, it's a years old release.

btw, want me to rebaae this MR?

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.

5 participants