trezor / cython-hidapi

:snake: Python wrapper for the HIDAPI
Other
285 stars 110 forks source link

Use pkg-config to find dependencies #162

Closed Youw closed 11 months ago

Youw commented 11 months ago

Fixes: #54 Closes: #55 Closes: #131 Fixes: #133

Youw commented 11 months ago

I've only properly tested this on Linux. ~Will check it on Windows/macOS later.~ UPD: checked Windows and macOS as well. Don't have any BSD to check it for now.

Youw commented 11 months ago

Just noticed #131 - I'll include that change in this PR as well as otherwise if would make a huge conflict in the code.

Youw commented 11 months ago

Just noticed https://github.com/trezor/cython-hidapi/pull/131 - I'll include that change in this PR as well as otherwise if would make a huge conflict in the code.

After going over #131 carefully, and taking into account that HIDAPI/libusb backend has a lot (even though very specific uses) and probably going to be available on Windows and macOS, I'm opting into keeping the behavior of --with-libusb as a complete fallback to "legacy" behavior (when both hid package is available for libusb and hidraw package for hidraw backend of HIDAPI).

As for future plans/default behavior - I'd consider explicitly providing a separate package (e.g. hid.libusb) on platforms that support it in addition to hid package, which should be considered as best/default backend for those who don't care explicitly.

Youw commented 11 months ago

Still haven't checked BSD-like systems, but I'm confident this is ready to be checked/reviewed.

NOTE: there is no special handling of BSD-like on HIDAPI/CMake build scripts (except NETBSD - recently there is a separate backend, but that's a separate story), so probably shouldn't be anything specific in cython-hidapi either.

Youw commented 11 months ago

~BTW: I also had to include (partially unrelated) fix, to be able to run it locally.~ ~It appears it fixes the "known" issue (https://github.com/trezor/cython-hidapi/actions/runs/6552643885/job/17796388898) and would unblock #160 as well.~ ~Surely can be contributed to master as a separate commit if requested by code owner.~ ~A separate PR just for that: #163 will rebase once merged.~ UPD: in master.

Youw commented 11 months ago

One more thing: if there're any code-style/indentation guides I should follow or a a tool to apply formatting - please do let me know. Python is not my primary language, I've formatted the code based on the existing code and my personal preferences.

prusnak commented 11 months ago

I merged all your work into the rework branch. Thank you for all your effort.

I will try to build the wheels via your new method and if everything works just fine I'll merge this branch into master.

Thanks again!

prusnak commented 11 months ago

The CI successfully built the rework branch here: https://github.com/trezor/cython-hidapi/actions/runs/6842485940

So I merged rework into master. Thanks again!

bearsh commented 11 months ago

As this PR changes the default backend on linux, wouldn't it make sense to adjust the wheels action to pass --with-libusb to be backwards compatible? otherwise it would make sense to raise the version to 1.x as this is kind of a breaking change as e.g. the users udev rules may not work anymore.

prusnak commented 11 months ago

As this PR changes the default backend on linux, wouldn't it make sense to adjust the wheels action to pass --with-libusb to be backwards compatible? otherwise it would make sense to raise the version to 1.x as this is kind of a breaking change as e.g. the users udev rules may not work anymore.

I want to keep the version number the same as hidapi version, so no bump to 1.x

@Youw what do you think about the broken udev rules? Should we keep using libusb as the default backend on Linux?

Youw commented 11 months ago

Totally agree on the version thing.

As for the backward compatibility - I completely get the potential frustration of users who would accidentally (during the upgrade) would get their environment broken without even possible workaround on the code level (only (re-)installing of the python package is an option as of now. Until we have a migration plan - it makes sense to keep libusb as a default for now.

I think it does makes sense to have hidraw as a default for setup.py but still have libusb as a default for wheels. Lets continue this conversation in #166.

prusnak commented 11 months ago

wouldn't it make sense to adjust the wheels action to pass --with-libusb to be backwards compatible?

Done in https://github.com/trezor/cython-hidapi/commit/690824080c360c60276e4da0f873bd3df223a502