CMake: fix detection of hidapi #4200

Be-ing · 2021-08-14T21:32:48Z

Mixxx was building its vendored copy of hidapi even when the system had hidapi 0.10.1.

CMakeLists.txt

uklotzde · 2021-08-14T23:35:09Z

I suppose we are not able to resolve this issue in 2.3 without touching the macOS and Windows environments. Our include paths are wrong.

uklotzde

This went smoother than expected.

CMakeLists.txt

daschuer

I think the version detection in findhidapi.cmake requires an update to fix the issue.

daschuer · 2021-08-15T09:35:36Z

CMakeLists.txt

-  find_package(hidapi)
-  # hidapi_VERSION is only available starting with 0.10.0
-  if(NOT hidapi_FOUND OR NOT hidapi_VERSION OR hidapi_VERSION VERSION_LESS 0.10.1)
+  find_package(hidapi 0.10.1)


What was wrong with this expression above?
Find_package(hidapi 0.10.1) succeeds on Ubuntu Bionic, because it is no version number set.

Oof, why does CMake let this succeed...

I fixed this by adding hidapi_VERSION to the find_package_handle_standard_args call.

I think the error makes it clear what is happening:

-- Could NOT find hidapi (missing: hidapi_VERSION) (Required is at least version "0.10.1") -- Linking internal libhidapi statically

daschuer · 2021-08-15T21:25:44Z

Now it prints:

-- Could NOT find hidapi (missing: hidapi_VERSION)

which is misleading. There is a Version: 0.9.0 entry in the pc file. Cant we read that?

Be-ing · 2021-08-15T21:30:56Z

I think it is good enough how it is. It prints the required version already.

daschuer · 2021-08-15T21:34:04Z

cmake/modules/Findhidapi.cmake

 # Version detection
-if (EXISTS "${hidapi_INCLUDE_DIR}/hidapi.h")
-    file(READ "${hidapi_INCLUDE_DIR}/hidapi.h" hidapi_H_CONTENTS)
+if (EXISTS "${hidapi_INCLUDE_DIR}/hidapi/hidapi.h")


We are not sure if the header is in the /hidapi sub-directory.

I am sure the original code was working when it has been written by @uklotzde here:
#3730

So for a reliable version detection we need to use the
Cflags: -I${includedir}/hidapi
Info.

We are not sure if the header is in the /hidapi sub-directory.

Yes we are. This is easily verifiable by looking up the list of files installed by packages. Here is the libhidapi-dev package from Ubuntu 18.04:
https://packages.ubuntu.com/bionic/amd64/libhidapi-dev/filelist

It installs /usr/include/hidapi/hidapi.h.

I am not sure this code ever worked before.

I can confirm that for Ubuntu Groovy. Maybe @uklotzde can explain why the code was written like that. Is it different with fedora?

No, it is the same on Fedora:

❯ rpm -ql hidapi-devel /usr/include/hidapi /usr/include/hidapi/hidapi.h /usr/lib64/libhidapi-hidraw.so /usr/lib64/libhidapi-libusb.so /usr/lib64/pkgconfig/hidapi-hidraw.pc /usr/lib64/pkgconfig/hidapi-libusb.pc

Same on Arch:

$ yay -Ql hidapi | grep -F hidapi.h hidapi /usr/include/hidapi/hidapi.h

daschuer · 2021-08-15T21:55:20Z

Why don't we just use PC_hidapi_VERSION?

daschuer · 2021-08-15T21:57:03Z

I think it is good enough how it is. It prints the required version already.

No, sorry. "Could NOT find hidapi" is wrong and misleading.

Be-ing · 2021-08-15T21:59:10Z

I don't really care about perfect error messages for outdated operating systems. It still works and it gives a hint what is wrong:

Required is at least version "0.10.1"

uklotzde · 2021-08-15T22:09:59Z

@daschuer If you need better error messages for an issue that will disappear in the future and that probably affects only a minority of developers please take care of this later.

daschuer · 2021-08-15T22:15:13Z

What is the reason not to use "PC_hidapi_VERSION" that the error message would be reasonable by default.

Be-ing · 2021-08-15T22:20:10Z

That requires rewriting more code to handle the case where there is no pkgconfig.

daschuer · 2021-08-15T22:24:42Z

We have it on all out targets. At least use the "PC_hidapi_VERSION" and fall back to reading the header file, with the code guessing the content of the pc file.

Be-ing · 2021-08-16T15:42:19Z

@daschuer you can improve the error message for outdated operating systems if you want after merging this but I don't think there's any reason not to merge this now. I have a million other things to do.

daschuer · 2021-08-16T16:53:34Z

This can instantly merged, if you revert the changes in the CMakeList.txt, and making hidapi_VERSION not a requirement in findhidapi.cmake.
These changes are unnecessary and are leading to the noted issues.

The required bugfix is just to look into the hidapi sub folder, that's all and sufficient for the fix.

Be-ing · 2021-08-16T17:29:15Z

I don't have energy for this bikeshedding.

uklotzde · 2021-08-16T19:17:38Z

@daschuer Instead of starting a discussion it would have been more helpful to simply provide the changes you deem necessary. Then we could test if it still works for us.

daschuer · 2021-08-16T22:19:25Z

@uklotzde It is no problem for me to issue an alternative solution that makes use of the PC_hidapi_VERSION or just a simply fix of the header path. But for my understanding it is the normal way in a review to discuss things with the author. In this case l have discovered that he original PR was not working at all. Than I have clearly pointed out the later fix introduced an unecessary regression and suggested alternatives. It feels bad to being blamed for bikeshedding because of this.

Since @Be-ing is stepped back from this let's discuss what would be a merge-able. Should I make use of PC_hidapi_VERSION or is it preferred to just fix the include path based on your original solution?

uklotzde · 2021-08-16T22:32:52Z

@daschuer From what I understood the outcome is expected, i.e. the bundled/vendored version is used if the system version is too old and detection fails. The exact error message is a minor detail if it works.

uklotzde · 2021-08-16T22:38:26Z

Why do we need to discuss this if the CI builds demonstrate that there are no actual regressions? This PR fixes the include paths and dynamic linking for all Linux distributions except Ubuntu LTS.

What did I miss?

daschuer · 2021-08-16T22:46:50Z

The exact error message is a minor detail if it works.

Yes, but we have a solution in the 2.3 branch that has not the wrong error message. This solution was removed here along with the actual fix. I am happy to issue an alternative PR without this regression. I just need a conses before I start to work, because I have no interest to put work into a third solution.

daschuer · 2021-08-16T22:48:08Z

CMakeLists.txt

@@ -2605,12 +2605,12 @@ find_package(LibUSB)
 # USB HID controller support
 option(HID "USB HID controller support" ON)
 if(HID)
-  find_package(hidapi)
-  # hidapi_VERSION is only available starting with 0.10.0
-  if(NOT hidapi_FOUND OR NOT hidapi_VERSION OR hidapi_VERSION VERSION_LESS 0.10.1)


This change is unrelated to the fix and can be reverted.

It is not unrelated. find_package(hidapi 0.10.1) creates the error message hinting the version is too old.

daschuer · 2021-08-16T22:50:42Z

cmake/modules/Findhidapi.cmake

+  DEFAULT_MSG
+  hidapi_LIBRARY
+  hidapi_INCLUDE_DIR
+  hidapi_VERSION


This makes the hidapi_VERSION version required to "find" the package which is an issue because the code above does not work for versions < 0.10.0. We can make it work by using the version from the PC file or just stick with the original solution.

Please give a hint what you prefer and I will prepare a PR.

This is intentional. We require 0.10.1, so if there is no hidapi_VERSION it is too old anyway.

uklotzde · 2021-08-17T06:46:07Z

Before: The code was silently broken
After: The code is fixed with slight inconveniences for some special cases

Why do we need to pile up more work on top of it? Changing 1 or 2 lines would be ok, if it is as easy as claimed. But anything else could and should be done later.

daschuer · 2021-08-17T07:09:22Z

I was only asking for reverting a few lines, that's all.

Let's move one and consider a solution ..

After looking deeper into it I think the Findhidapi.cmake file is still broken.
It aims to work with and without a PC file available, but now the path without PC file will return the wrong include directory
just "/usr/include" instead" of "/usr/include/hidapi". It still woks in our case, because we use the cflags from the PC file, which comes with the correct include derective.

Do we know why using PATH_SUFFIXES was broken in the first place? It works on Linux.

daschuer · 2021-08-17T10:05:39Z

It was not broken, #4215 fixes all issues.

Be-ing · 2021-08-17T12:16:56Z

just "/usr/include" instead" of "/usr/include/hidapi"

/usr/include is correct. The file is at <hidapi/hidapi.h> relative to the installation prefix.

Holzhaus · 2021-08-17T12:44:55Z

just "/usr/include" instead" of "/usr/include/hidapi"

/usr/include is correct. The file is at <hidapi/hidapi.h> relative to the installation prefix.

Nope:

$ grep -HrnF hidapi.h src
src/controllers/hid/hidcontroller.cpp:3:#include <hidapi.h>
src/controllers/hid/hidenumerator.cpp:3:#include <hidapi.h>
src/controllers/hid/hiddevice.h:3:#include <hidapi.h>

Be-ing · 2021-08-17T12:45:44Z

@Holzhaus yes that is what I mean. The #include paths in our C++ code were wrong. I fixed that in this PR.

Holzhaus · 2021-08-17T12:47:01Z

This is not what the hidapi README suggests: https://github.com/libusb/hidapi#what-does-the-api-look-like

Be-ing · 2021-08-17T12:48:10Z

Well then the documentation is misleading. hidapi's build system has always installed the header to <hidapi/hidapi.h> relative to the installation prefix.

Be-ing · 2021-08-17T12:50:22Z

Fix for the hidapi documentation: libusb/hidapi#323

Be-ing · 2021-08-17T13:13:34Z

I have already implemented a fully working solution. It does not create a welcoming environment to have every unimportant detail nitpicked to shreds then reimplemented in a worse way that does not solve all the problems, and makes the minuscule imperfections that were raised worse.

Be-ing · 2021-08-17T13:43:55Z

The pkgconfig file says the #include path is /usr/include:

 ❯ cat /usr/lib64/pkgconfig/hidapi-libusb.pc
prefix=/usr
exec_prefix=/usr
libdir=/usr/lib64
includedir=/usr/include

Name: hidapi-libusb
Description: C Library for USB HID device access from Linux, Mac OS X, FreeBSD, and Windows. This is the libusb implementation.
Version: 0.10.1
Libs: -L${libdir} -lhidapi-libusb
Cflags: -I${includedir}/hidapi

#include <hidapi/hidapi.h> is correct.

Be-ing · 2021-08-17T13:48:36Z

Oh, wait the pkgconfig file is doing weird stuff:

includedir=/usr/include

That is normal.

Cflags: -I${includedir}/hidapi

That I did not expect.

uklotzde · 2021-08-17T14:05:16Z

The example in test.c doesn't even show how hidapi.h is supposed to be included from a system path:

#include "hidapi.h"

This is NOT how a consumer should include an external library.

Holzhaus · 2021-08-17T14:13:35Z

I think #include <hidapi.h> is the standard. Looking at some other prominent projects, they use that, too:

Yubico's libfido2: https://github.com/Yubico/libfido2/search?l=C&q=hidapi
Ardour: https://github.com/Ardour/ardour/search?l=C%2B%2B&q=hidapi
Dolphin Emu: https://github.com/dolphin-emu/dolphin/search?l=C%2B%2B&q=hidapi

Youw · 2021-08-17T14:13:36Z

src/controllers/hid/hidcontroller.h

@@ -1,6 +1,6 @@
 #pragma once

-#include <hidapi.h>
+#include <hidapi/hidapi.h>


this is not correct
as per HIDAPI design, you should only include <hidapi.h>

Be-ing · 2021-08-17T14:17:06Z

I reverted the change to the #include path.

github-actions bot added the build label Aug 14, 2021

uklotzde suggested changes Aug 14, 2021

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

Be-ing force-pushed the hidapi_system branch 4 times, most recently from ea9ebb7 to beda3b3 Compare August 15, 2021 05:01

uklotzde approved these changes Aug 15, 2021

View reviewed changes

uklotzde reviewed Aug 15, 2021

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

daschuer requested changes Aug 15, 2021

View reviewed changes

Be-ing force-pushed the hidapi_system branch from beda3b3 to a6bb14e Compare August 15, 2021 11:45

Be-ing requested a review from daschuer August 15, 2021 15:16

daschuer reviewed Aug 15, 2021

View reviewed changes

Be-ing closed this Aug 16, 2021

uklotzde reopened this Aug 16, 2021

daschuer reviewed Aug 16, 2021

View reviewed changes

daschuer mentioned this pull request Aug 17, 2021

Fix version detection in Findhidapi.cmake #4215

Merged

Youw reviewed Aug 17, 2021

View reviewed changes

CMake: fix detection of hidapi

f67bcc6

Be-ing force-pushed the hidapi_system branch from a6bb14e to f67bcc6 Compare August 17, 2021 14:16

Be-ing closed this Aug 17, 2021

CMake: fix detection of hidapi #4200

CMake: fix detection of hidapi #4200

Conversation

Be-ing commented Aug 14, 2021

uklotzde commented Aug 14, 2021

uklotzde left a comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Aug 15, 2021

Be-ing commented Aug 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Aug 15, 2021

daschuer commented Aug 15, 2021

Be-ing commented Aug 15, 2021

uklotzde commented Aug 15, 2021

daschuer commented Aug 15, 2021

Be-ing commented Aug 15, 2021

daschuer commented Aug 15, 2021

Be-ing commented Aug 16, 2021

daschuer commented Aug 16, 2021

Be-ing commented Aug 16, 2021

uklotzde commented Aug 16, 2021

daschuer commented Aug 16, 2021

uklotzde commented Aug 16, 2021 • edited Loading

uklotzde commented Aug 16, 2021

daschuer commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Aug 17, 2021

daschuer commented Aug 17, 2021 • edited Loading

daschuer commented Aug 17, 2021

Be-ing commented Aug 17, 2021

Holzhaus commented Aug 17, 2021

Be-ing commented Aug 17, 2021

Holzhaus commented Aug 17, 2021

Be-ing commented Aug 17, 2021

Be-ing commented Aug 17, 2021

Be-ing commented Aug 17, 2021 • edited Loading

Be-ing commented Aug 17, 2021 • edited Loading

Be-ing commented Aug 17, 2021 • edited Loading

uklotzde commented Aug 17, 2021 • edited Loading

Holzhaus commented Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Be-ing commented Aug 17, 2021

uklotzde commented Aug 16, 2021 •

edited

Loading

daschuer commented Aug 17, 2021 •

edited

Loading

Be-ing commented Aug 17, 2021 •

edited

Loading

Be-ing commented Aug 17, 2021 •

edited

Loading

Be-ing commented Aug 17, 2021 •

edited

Loading

uklotzde commented Aug 17, 2021 •

edited

Loading

Holzhaus commented Aug 17, 2021 •

edited

Loading