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

Fix version detection in Findhidapi.cmake #4215

Merged
merged 5 commits into from
Aug 17, 2021
Merged

Conversation

daschuer
Copy link
Member

The first commit is the bug fix, taken from #4200, the second commit is for the improved messages.

@github-actions github-actions bot added the build label Aug 17, 2021
@daschuer daschuer changed the base branch from main to 2.3 August 17, 2021 09:16
@daschuer daschuer requested a review from uklotzde August 17, 2021 09:16
Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't fix the incorrect #include path which was fixed already in #4200.

Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a misleading error message in case hidapi is too old:

-- Found hidapi: /usr/lib/x86_64-linux-gnu/libhidapi-libusb.so  
-- Found libhidapi version < 0.10.0
-- Linking internal libhidapi 0.10.1 statically

Found libhidapi version < 0.10.0 is misleading. While it is technically correct, we require 0.10.1 and this is what the user should be told. find_package(hidapi 0.10.1) as in #4200 does this.

CMakeLists.txt Outdated
message(STATUS "Found libhidapi version < 0.10.0")
endif()
endif()
message(STATUS "Linking internal libhidapi 0.10.1 statically")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is error prone. I do not anticipate that anyone would remember to update this line when updating the vendored library.

@daschuer
Copy link
Member Author

This doesn't fix the incorrect #include path which was fixed already in #4200.

For my understanding that was a regression by #4300 itself.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 17, 2021

No, the Mixxx code has been wrong. #4200 fixes it.

@daschuer
Copy link
Member Author

daschuer commented Aug 17, 2021

No, the Mixxx code has been wrong. #4200 fixes it.

Please explain how it has worked since than.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 17, 2021

It worked because the build system fudged the include paths by treating /usr/include/hidapi as the include path. It does not do this for any other library; hidapi should not be an exception.

@daschuer
Copy link
Member Author

This is the way it is used in the examples on https://github.com/libusb/hidapi and in many other Findhidapi.cmke implementations.
The PC file has the

Cflags: -I${includedir}/hidapi

We should use the library in the recommended way. This is by the way an extra topic not related to the bug fix in this PR.

@daschuer
Copy link
Member Author

OK, all findings are resolved, merge?

@Be-ing
Copy link
Contributor

Be-ing commented Aug 17, 2021

I don't care which of this or #4200 gets merged. They both do the same thing. It was frustrating to have to review this.

@uklotzde
Copy link
Contributor

I'm slightly in favor of merging #4200, because it is simpler and the error message is comprehensible for me. The special case handling is restricted to Findhidapi.cmake and does not creep into CMakeLists.txt.

@daschuer
Copy link
Member Author

OK, here the full fix as discussed as second option.

Now a it utilizes PC_hidapi_VERSION and falls back reading the header if the PC file is not available.
The error message is generated using FindPackageHandleStandardArgs() correctly with the default error message.

In case of Ubuntu Bionic it prints now:

-- Could NOT find hidapi: Found unsuitable version "0.8.0-rc1", but required is at least "0.10.1" (found /usr/lib/x86_64-linux-gnu/libhidapi-libusb.so)
-- Linking internal libhidapi statically

@Be-ing
Copy link
Contributor

Be-ing commented Aug 17, 2021

I was not aware of the VERSION_VAR parameter of FindPackageHandleStandardArgs. That is the best solution.

@Be-ing Be-ing merged commit db4b911 into mixxxdj:2.3 Aug 17, 2021
@daschuer
Copy link
Member Author

@Be-ing:
We have the policy to use merge commits!
It looks like you have changed you habit recently. #4172 #4121 #4163 #4058
Please use merge commits in future to not information like the relationship of commit messages to change in the code.

daschuer added a commit to daschuer/mixxx that referenced this pull request Aug 18, 2021
@Be-ing Be-ing added the changelog This PR should be included in the changelog label Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build changelog This PR should be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants