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

Update hidapi to 0.12.0 #4777

Closed
wants to merge 1 commit into from

Conversation

JoergAtGithub
Copy link
Member

This PR updates hidapi to the latest version 0.12.0

Furthermore it increases the minimum required version to 0.11.2. This solves the open FIXME of #4054. Since this PR getInputReport is no longer working on Linux, because the hidapi command hid_get_input_report wasn't implemented for the Linux hidraw backend at this time.
This was fixed by libusb/hidapi#259 which was part of the hidapi 0.11.2 release.

Note: hid_get_input_report will only succeed with Linux Kernel 5.11 or later. With earlier Linux kernel versions, hid_get_input_report will return error code -1 as before this PR.

@github-actions github-actions bot added the build label May 30, 2022
@Holzhaus
Copy link
Member

Thanks. Could you remove the CI and documentation files from this PR and potentially add them as a gitignore pattern to avoid accidental commits in the future?

@uklotzde
Copy link
Contributor

Thanks. Could you remove the CI and documentation files from this PR and potentially add them as a gitignore pattern to avoid accidental commits in the future?

Please strip the archive down to the minimum as before, only required sources and license files.

@uklotzde
Copy link
Contributor

The commits should be squashed.

@daschuer
Copy link
Member

Can you squash all commits again, to remove the not used files entirely from the git history. Thanks.

Delete unused files from lib/hidapi
Updated hidapi from 0.10.1 to 0.12.0
Increased minimum required hidapi version to 0.11.2
@Be-ing
Copy link
Contributor

Be-ing commented May 31, 2022

Why is this still being vendored? IIRC it was in the past due to an old version in Ubuntu?

@daschuer
Copy link
Member

focal (20.04LTS) 0.9.0+dfsg-1
impish (21.10)] 0.10.1+dfsg-1
jammy (22.04LTS)] 0.11.2-1
kinetic (22.10) 0.11.2-1

Even fedora 36 is on 0.11.2

See:
https://repology.org/project/hidapi/versions

@daschuer
Copy link
Member

daschuer commented May 31, 2022

If we only need a safety net for our minimum required version, we may only update to 0.11.2. than we have the chance to remove the vendored library once we drop support for focal / impish / Fedora 35 (even Fedora 34 has been updated to 0.11.2).

@daschuer
Copy link
Member

daschuer commented May 31, 2022

I don't have a strong opinion here.

Without lifting the requirement to 0.12, aged distros will build with 0.12 and the recent versions with 0.11. Is this desired?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

I think before merge we should either lift the minimum requirements to 0.12 or downgrade the overlay to 0.11. that everything is consistent.

Do the 0.12 changes outwaight the regression risk of using a 7 days old library no one else is using (yet) ?

@uklotzde what do you think about this?

@uklotzde
Copy link
Contributor

uklotzde commented Jun 1, 2022

I think before merge we should either lift the minimum requirements to 0.12 or downgrade the overlay to 0.11. that everything is consistent.

Do the 0.12 changes outwaight the regression risk of using a 7 days old library no one else is using (yet) ?

@uklotzde what do you think about this?

I don't care. But let's please stop these back and forth edits.

@daschuer
Copy link
Member

daschuer commented Jun 1, 2022

For now I think it is best to downgrade to 0.11.
This allows us to de-vrndor this library earlier.
@JoergAtGithub shall I prepare an alternative PR with this change or do you have interest to do it her.

Sorry for the extra loop. I have original missed the missmatch between the required and the installed version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants