search

kennedyshead / aioasuswrt

MIT License
24 stars 24 forks source link

Change the crypto requirements #71

Closed JJdeVries closed 3 years ago

JJdeVries commented 3 years ago

We can remove the cryptography requirement as we don't require it directly (but through asyncssh).

This change does mean that we require users to upgrade their pip before you're able to install the requirements (The raised error is shown for example in this previously run workflow): https://github.com/kennedyshead/aioasuswrt/runs/1892985827?check_suite_focus=true#step:6:25

This also aligns the library with the requirements from HomeAssistent for including it as mentioned in home-assistant/core#48582

kennedyshead commented 3 years ago

Are you sure here? Reason for adding this was a bug where that was not installed in some cases (if I remember correctly)

JJdeVries commented 3 years ago

So, actually i'm not really sure why the linked pipeline failed, but now it succeeds. It could be that whatever platform the CI is actually running on now actually has a rust compiler etc. available. A more thorough discussion is here: https://github.com/pyca/cryptography/issues/5771

But for whatever reason it is, the pipelines now succeed, so we're able to install and test the library succesfully. Meaning I would rather not pin the cryptography library as for us there is no reason to do this. Any user/library that wants to install this on a non-rust supported platform could/should set the cryptography requirement themselves.

In the end, from the discussion on cryptography it does not seem that they will revert the dependency on rust, and keeping the cryptography pinned to the old version also does not really seem to be the way to go forward.

kennedyshead commented 3 years ago

👍 Lets do some testing :)

Tmin10 commented 3 years ago

Hello. Maybe it's better to merge it and release a new version? Original PR to HA looks abandoned and it's going to close due to inactivity.