Dynamically load libudev in linux-hidraw backend#788
Dynamically load libudev in linux-hidraw backend#788falkTX wants to merge 1 commit intolibusb:masterfrom
Conversation
|
The build failed under Ubuntu and Arch Linux. |
b86c642 to
de4b582
Compare
Related to more strict warnings, likely fixed now. |
Signed-off-by: falkTX <falktx@falktx.com>
Not yet... |
de4b582 to
8fc87a7
Compare
I built locally with the same pedantic flags and pushed another try. |
There was a problem hiding this comment.
I don't mind the idea of having libudev loaded dynamically in runtime, but I can't agree that the current aproach is best for that. A few points I'd ask to change if I where to accept it this way:
- licence shouldn't derive from libudev license - we don't copy-paste their code, and HIDAPI uses a bit different licencing sceme, and the new source that becomes a part of HIDAPI would potentially restrict its users;
- including the
.cfile for this purpose looks a bit ugly, and the namelibudev.csuggests that it actually is (i.e. implements) libudev, but in fact it is only a wrapper; what is best instead - have ahidapi_libudev_wrap.h, and all functions inside should be either static or inline; - the
C()macro... I checked - it is a valid C, but the fact that I had to go and double-check suggests that it looks like something that might break;HIDAPI_UDEV_RESOLVEwouldn't raise any naming suspicious RTLD_GLOBAL- that is a code smell to use it here, where passing the_lib(or ratherudev_lib_handletodlsym) explicitly is what I'd expect- no
dlclosewhich is technically a resource leak if HIDAPI itself is ever to be dynamically loaded
At the end of the day - I'd be much happier to see a more common way of dynamically loading a library during the initialisation, saving and using function pointers explicitly and unloading a library during the hid_exit, e.g. like we did it for windows backend.
Yes that would cause a lot more changes in linux/hid.c - but that's absolutely fine.
| find_package(Threads REQUIRED) | ||
|
|
||
| include(FindPkgConfig) | ||
| pkg_check_modules(libudev REQUIRED IMPORTED_TARGET libudev) |
There was a problem hiding this comment.
the documentation and CI scripts also needs to be updated - now the libudev-dev no longer required and only the libudev1 is needed
|
That is all very fair, I mostly did this change as a way to get my cross-compilation builds working and decided to push it upstream because why not. You have good points for something that would be integrated officially in hidapi, but sorry to say my needs are covered already as-is, the steps you mention make sense but sound like it would be quite a bit more work than I am willing to spend just for a small feature. If you think the feature is worthwhile, take all the code here to adapt freely under any license, I give written consent. |
|
Thanks for the contribution. I'll leave it be for now, in case anyone wants to catch up. |
This PR allows applications using hidapi with linux-hidraw backend to not have any direct dependencies, making binaries more portable and easier to build.
For the loader file I used the same license as libudev (LGPL-2.1-or-later), and copied their way of doing the license header too.
The file is included in-place, just to keep the diff as small as possible.