qmk_cli icon indicating copy to clipboard operation
qmk_cli copied to clipboard

[Bug] Python libraries hid and hidapi occupy the same namespace with different APIs

Open Foxboron opened this issue 2 years ago • 30 comments

Describe the Bug

The two python dependencies hid and hidapi occupies the same namespace in Python with different APIs.

This implies one can only install one version globally on a system and you enter an either/or scenario.

From looking at qmk it seems hid is only used by https://github.com/qmk/qmk_firmware/blob/9dea6f772077dc5c09daf40378e45884d29ab2e2/keyboards/dekunukem/duckypad/keymaps/m4cs/sysinfo.py, but the dependency is still enforced top-level in qmk_cli.

Preferably it should be optional or one should just go with the more popular hidapi library.

https://github.com/trezor/cython-hidapi

https://github.com/apmorton/pyhidapi

Additional Context?

No response

Foxboron avatar Jul 20 '21 13:07 Foxboron

Also see https://github.com/liquidctl/liquidctl/issues/361#issuecomment-883384395

Seems like the actual issue is that the hid library was never suppose to be used, but hidapi should be instead.

Foxboron avatar Jul 20 '21 13:07 Foxboron

pyhidapi is what we have gone with; the CLI uses it (on the qmk_firmware side) for qmk console: https://github.com/qmk/qmk_firmware/blob/master/lib/python/qmk/cli/console.py

If @m4cs is using the other library then they will need to update their Python script. It's unfortunate that these two libraries share the same namespace, but that's probably something they will have to work out between them.

fauxpark avatar Jul 20 '21 13:07 fauxpark

Right, but this actually means that one system can only have pyhidapi or hidapi. That implies there are going to be system conflicts for all distributions utilizing these two libraries when packaging up qmk_cli.

Foxboron avatar Jul 20 '21 13:07 Foxboron

This also applies to pip installing the libraries for different tools.

Foxboron avatar Jul 20 '21 13:07 Foxboron

I'm not sure what the proposed solution to this would be. Whichever library we choose, it's going to conflict with the other. It's really up to the maintainers of those two libraries to negotiate which of them get to keep the hid namespace.

Just in case, I will ping them here to see if they have anything to say:

@apmorton @prusnak

fauxpark avatar Jul 20 '21 13:07 fauxpark

I'd use cython-hidapi and drop pyhidapi from the codebase. The other option is to vendor pyhidapi since it's 215 lines of code with no changes for two years.

The actual keymap script in qmk_firmware can just stay in both of those cases.

Foxboron avatar Jul 20 '21 13:07 Foxboron

I don't think this is a problem - why would anyone want to use both cython-hidapi and pyhidapi in the same project?

prusnak avatar Jul 20 '21 13:07 prusnak

No, same system. You can only pip install or use a package manager to install cython-hidapi or pyhidapi. The other issue, local to qmk, is that it uses both.

Foxboron avatar Jul 20 '21 13:07 Foxboron

The other issue, local to qmk, is that it uses both.

Not really. M4cs's keymap was merged before we had settled on pyhidapi: https://github.com/qmk/qmk_firmware/commit/ac33dc12dacee480eba67462d985cb4dc7589c7f https://github.com/qmk/qmk_cli/commit/1c6d81fc3b14566c5a13d702134b7470e6b9574c

At that point there was no opinion either way as to which one to use, since qmk console did not exist yet. And we try not to modify user code without at least a heads up; it's generally their responsibility to keep their stuff maintained.

fauxpark avatar Jul 20 '21 14:07 fauxpark

Right, but this actually means that one system can only have pyhidapi or hidapi. That implies there are going to be system conflicts for all distributions utilizing these two libraries when packaging up qmk_cli.

As @fauxpark points out we require pyhidapi. When working on qmk console I tried both, and this is the one that works on more platforms with a more consistent API. It's unfortunate that keymap has a script that uses cython-hidapi, but I didn't even know it was there tbh. Things inside keymap directories are not part of core and we don't go looking there on a regular basis.

That implies there are going to be system conflicts for all distributions utilizing these two libraries when packaging up qmk_cli.

The real solution here is to use a venv to install qmk CLI. If you use homebrew to install you get this for free. I'd like to see other system packages start to do the same, and have been looking at the possibility of using stdeb to provide debian packages that build a venv.

skullydazed avatar Jul 20 '21 15:07 skullydazed

The real solution here is to use a venv to install qmk CLI.

This is just going to prevent it being redistributeable in larger repositories and requires your users instead to have knowledge about Python development concepts. It's a band-aid, not a solution.

I'll submit a pull-request that vendors this dependency and you can do with that what you want.

Foxboron avatar Jul 20 '21 16:07 Foxboron

This is just going to prevent it being redistributeable in larger repositories

Using a virtualenv is a distro package level decision. The pypi package will not be using it directly.

and requires your users instead to have knowledge about Python development concepts.

It does not require users to have knowledge about Python development concepts, but it does mean creating OS specific packages. We already do this for homebrew, and I'm actually in the middle of producing a .deb that can be used on those distributions. For other distributions the community can create packages for their OS if it doesn't exist.

I'll submit a pull-request that vendors this dependency and you can do with that what you want.

We will not be vendoring any dependency. We have specified only one package and people who need the other one installed as well will need to use virtualenv or another solution that allows them to resolve the namespace conflict.

skullydazed avatar Jul 20 '21 16:07 skullydazed

Using a virtualenv is a distro package level decision. The pypi package will not be using it directly.

It's an upstream decisions if there are no other options.

For other distributions the community can create packages for their OS if it doesn't exist.

I maintain the Arch Linux package. I'm telling you this is a problem.

We will not be vendoring any dependency. We have specified only one package and people who need the other one installed as well will need to use virtualenv or another solution that allows them to resolve the namespace conflict.

Distributing qmk as a venv is vendoring. You have just moved the responsibility of the vendored dependencies from upstream to the distributor.

You also vendor 7 other projects as git submodules. https://github.com/qmk/qmk_firmware/tree/master/lib

A submodule, lufa also depends on cython-hidapi. But I think these are all test files? So probably fine?

Foxboron avatar Jul 20 '21 16:07 Foxboron

A submodule, lufa also depends on cython-hidapi. But I think these are all test files? So probably fine?

Again, a single Python script, which qmk_firmware core does not use.

fauxpark avatar Jul 20 '21 16:07 fauxpark

I maintain the Arch Linux package. I'm telling you this is a problem.

I believe you that this is a problem, but you aren't convincing us that it's our problem.

Distributing qmk as a venv is vendoring. You have just moved the responsibility of the vendored dependencies from upstream to the distributor.

Which is where that responsibility belongs.

You also vendor 7 other projects as git submodules. https://github.com/qmk/qmk_firmware/tree/master/lib

Those are C development dependencies, not python.

A submodule, lufa also depends on cython-hidapi. But I think these are all test files? So probably fine?

Those are not part of the QMK codebase. Random python files in keymap directories aren't part of the QMK codebase. There is no part of the QMK codebase that relies on cython-hidapi.

skullydazed avatar Jul 20 '21 17:07 skullydazed

I believe you that this is a problem, but you aren't convincing us that it's our problem.

I think picking a library which hasn't seen a release for 2 years with a colliding import path is an upstream problem when the other one is in much wider use. The simplest thing is to stick the single file into the library.

Which is where that responsibility belongs.

You are enforcing a responsibility onto the distributor though. You are not giving anyone a choice to package this correctly but enforce what you think we should be doing.

Those are C development dependencies, not python.

Then there is no problem adding a 200 line python file? It's going to be easier for you regardless when you figure out you need new features.

Foxboron avatar Jul 20 '21 17:07 Foxboron

Please consider the human on the other side of this conversation. You have been very aggressive about what is right and what is wrong here, when a lot of this comes down to conflicting goals and practices.

I think picking a library which hasn't seen a release for 2 years with a colliding import path is an upstream problem when the other one is in much wider use. The simplest thing is to stick the single file into the library.

There are always multiple perspectives on which is the colliding module.

The module I picked has a pypi package name that matches its namespace. It's also the library that worked on all 3 platforms more consistently and with fewer bugs. I know that it's not a requirement that python package namespaces match the pypi package name, but it's rare that a package doesn't follow that convention.

You are enforcing a responsibility onto the distributor though. You are not giving anyone a choice to package this correctly but enforce what you think we should be doing.

I am following standard python packaging conventions here, supplying a package to pypi with my requirements laid out. As this is a application it's best practice to pin versions so that the package can always be installed, even when its dependencies release broken packages. Pinning our dependencies in this way has been requested several times.

We are not forcing package maintainers to do anything specific WRT resolving version and/or dependency conflicts. While we will do what we can to reduce the pain of package maintainers in this area sometimes there are conflicting requirements from different groups. We do what we can to juggle those requirements.

Then there is no problem adding a 200 line python file? It's going to be easier for you regardless when you figure out you need new features.

Please don't condescend.

skullydazed avatar Jul 20 '21 17:07 skullydazed

My ctypes hid library is unlikely to see any future updates unless there are community contributions or critical issues that crop up from newer hidapi library versions.

Vendoring the ctypes bindings may be the simplest solution here unless somebody from qmk wants to fork the project and publish it to pypi under a non-conflicting import path.

apmorton avatar Jul 20 '21 17:07 apmorton

My ctypes hid library is unlikely to see any future updates unless there are community contributions or critical issues that crop up from newer hidapi library versions.

QMK isn't likely to need any future updates either, but if we do we are more likely to contribute them upstream than want to maintain them in our own repo.

Vendoring the ctypes bindings may be the simplest solution here unless somebody from qmk wants to fork the project and publish it to pypi under a non-conflicting import path.

We are disinclined to do that, generally we like to rely on stock upstream distributions as much as possible.

skullydazed avatar Jul 20 '21 17:07 skullydazed

Please consider the human on the other side of this conversation. You have been very aggressive about what is right and what is wrong here, when a lot of this comes down to conflicting goals and practices.

Yes, I apologize.

There are always multiple perspectives on which is the colliding module. The module I picked has a pypi package name that matches its namespace. It's also the library that worked on all 3 platforms more consistently and with fewer bugs. I know that it's not a requirement that python package namespaces match the pypi package name, but it's rare that a package doesn't follow that convention.

Right, but the ctypes one predates it with 4-5 years at this point and I'd widely packaged in distributions.

https://repology.org/project/python:hid/versions

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

You can do an intersection with https://repology.org/project/qmk/versions to see distributions this is a problem for.

At this point I'm of the opinion that vendoring has more upsides instead of downsides. It's also hard for downstream to patch anything qmk related since everything is provided by qmk_firmware. This makes it impossible to resolve the conflicting namespace unless we break our own package guidelines, and this applies equally for the 6 distributions (with the only exception being nixos I believe) that have packaged qmk.

Please don't condescend.

It wasn't meant to be condescending. It was a remark about the stale nature of the upstream. I apologize.

Foxboron avatar Jul 20 '21 18:07 Foxboron

Yes, I apologize.

It wasn't meant to be condescending. It was a remark about the stale nature of the upstream. I apologize.

Apologies accepted.

Right, but the ctypes one predates it with 4-5 years at this point and I'd widely packaged in distributions.

I think some naming got confused here, did you mean pyhidapi there?

Per the links you posted the hidapi package was showing up in the tracked repos earlier, but the hidapi package was only uploaded to pypi in 2015, while the hid package was first uploaded to pypi in 2013.

At this point I'm of the opinion that vendoring has more upsides instead of downsides.

You may be right, and yet we are loathe to vendor based on past experience.

It's also hard for downstream to patch anything qmk related since everything is provided by qmk_firmware.

That was a deliberate choice given how fast we are currently moving. Our data driven initiative means that changes inside the repo have to move in lockstep with the python code that consumes data inside the repo, and putting the code there was the last bad choice. When our data format settles down we can move a lot of that code over to the qmk_cli repo which will improve that situation for you, but as #13366 shows we still have a lot of work to do on that front.

This makes it impossible to resolve the conflicting namespace unless we break our own package guidelines, and this applies equally for the 6 distributions (with the only exception being nixos I believe) that have packaged qmk.

Do your package guidelines preclude the use of virtualenv or another way of installing self-contained packages?

skullydazed avatar Jul 20 '21 18:07 skullydazed

I think some naming got confused here, did you mean pyhidapi there? Per the links you posted the hidapi package was showing up in the tracked repos earlier, but the hidapi package was only uploaded to pypi in 2015, while the hid package was first uploaded to pypi in 2013.

No, I meant hidapi.

The current project have been forked and moved around. I'm not sure if when hidapi was uploaded to pypi is a indicator of when people started using that project, considering the wide distribution in linux distros.

You may be right, and yet we are loathe to vendor based on past experience.

Which is understandable, but at this point it's 200 lines of fairly simple code which is why I struggle to see the downsides when looking at the distribution issue downstream.

Do your package guidelines preclude the use of virtualenv or another way of installing self-contained packages?

We don't vendor downstream, so yes. We may accept upstream vendored dependencies if there is no way around it, but usually actively work on devendoring upstream projects, the opposite of what you see here.

Even packaging a self-contained python application with a venv would be painfull as we'd have to mock around installing into /opt and writing a shim binary for /usr/bin that starts the venv before calling qmk.

Foxboron avatar Jul 20 '21 18:07 Foxboron

The current project have been forked and moved around. I'm not sure if when hidapi was uploaded to pypi is a indicator of when people started using that project, considering the wide distribution in linux distros.

At any rate I'm not sure it's really germane to our discussion at this. My only point in all this is that who was "first" doesn't really matter, and can be looked at from different perspectives.

We don't vendor downstream, so yes. We may accept upstream vendored dependencies if there is no way around it, but usually actively work on devendoring upstream projects, the opposite of what you see here.

I don't know what to tell you then. For your policy to really work without someone bending their ideals you need to convince these two python modules to play nicely together. In an ideal world hidapi would have used the hidapi namespace from the start, but I'm sure they have reasons they didn't. I don't think this problem is severe enough to ask either project to move at this point, especially when python provides tools for resolving this situation.

Even packaging a self-contained python application with a venv would be painfull as we'd have to mock around installing into /opt and writing a shim binary for /usr/bin that starts the venv before calling qmk.

I think you may be have some of the same misconceptions I had before I started using venv. While for development purposes it's common to source VENV_PATH/bin/activate it's not required. You can specify the full path (VENV_PATH/bin/qmk) and it will execute in the virtualenv context, and you can also symlink from /usr/bin/qmk to VENV_PATH/bin/qmk without any special shim or other configuration.

From a packaging perspective it shouldn't too painful at all, adding 3 lines to your build script should do it:

python3 -m venv $FAKEROOT/opt/qmk-<VERSION>
source $FAKEROOT/opt/qmk-<VERSION>/bin/activate

# pip install actions here

ln -s ../../opt/qmk-<VERSION>/bin/qmk $FAKEROOT/usr/bin/qmk

# Rest of install here

skullydazed avatar Jul 20 '21 19:07 skullydazed

I don't know what to tell you then. For your policy to really work without someone bending their ideals you need to convince these two python modules to play nicely together. In an ideal world hidapi would have used the hidapi namespace from the start, but I'm sure they have reasons they didn't. I don't think this problem is severe enough to ask either project to move at this point, especially when python provides tools for resolving this situation.

In a normal situation we would just downstream patch this and you would never hear from us again, but the awkward project structure (as mentioned and explained) makes this hard for everyone involved.

I think you may be have some of the same misconceptions I had before I started using venv. While for development purposes it's common to source VENV_PATH/bin/activate it's not required. You can specify the full path (VENV_PATH/bin/qmk) and it will execute in the virtualenv context, and you can also symlink from /usr/bin/qmk to VENV_PATH/bin/qmk without any special shim or other configuration.

Right, it makes things easier indeed. But the technical issue is secondary to the policy issue. Downstream does not trust upstream to properly account for their dependencies. It's better for us to control the entire chain then rely on upstreams to follow things like API breakage, security issues and so on.

Foxboron avatar Jul 20 '21 20:07 Foxboron

In a normal situation we would just downstream patch this and you would never hear from us again, but the awkward project structure (as mentioned and explained) makes this hard for everyone involved.

What if we moved qmk console from firmware to cli? I was thinking about this at the dog park and we might be able to do that during this develop cycle. Of all our commands I think this one will likely remain standalone for the foreseeable future.

Right, it makes things easier indeed. But the technical issue is secondary to the policy issue. Downstream does not trust upstream to properly account for their dependencies. It's better for us to control the entire chain then rely on upstreams to follow things like API breakage, security issues and so on.

Is the policy that prevents you from using virtualenv documented anywhere? I went looking on https://wiki.archlinux.org/title/Python_package_guidelines but couldn't find anything.

skullydazed avatar Jul 20 '21 21:07 skullydazed

What if we moved qmk console from firmware to cli? I was thinking about this at the dog park and we might be able to do that during this develop cycle. Of all our commands I think this one will likely remain standalone for the foreseeable future.

That would solve my issues by patching inn the vendored library.

Is the policy that prevents you from using virtualenv documented anywhere? I went looking on >https://wiki.archlinux.org/title/Python_package_guidelines but couldn't find anything.

The guidelines explains how we are expected to packaged. If it's not mentioned, it's discouraged and/or rejected.

Foxboron avatar Jul 20 '21 21:07 Foxboron

That would solve my issues by patching inn the vendored library.

I won't promise it'll make it into this cycle (which closes to new PRs in 10 days) but I'll take a look and see where I can get with that.

skullydazed avatar Jul 20 '21 21:07 skullydazed

I won't promise it'll make it into this cycle (which closes to new PRs in 10 days) but I'll take a look and see where I can get with that.

Thanks! If you want to test things you can tag me and I'll try it out. If it's ready outside of your release cycle I can apply the patch downstream if you think it's a good idea.

But please do not stress with it. The current situation isn't perfect and I would expect some annoyances from users with the conflicts in place, but it's not going to break anything.

Foxboron avatar Jul 20 '21 22:07 Foxboron

Is this issue resolved now?

fauxpark avatar Jul 03 '22 00:07 fauxpark

This still appears to be a problem. Just attempted to install qmk on Manjaro via pacman -Sy qmk and ran into this exact dependency issue, as I have streamdeck-ui installed that relies on python-hidapi, so the qmk install fails.

littlediobolic avatar Jul 21 '22 23:07 littlediobolic