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

Replace cryptography with hashlib #278

Merged
merged 2 commits into from
Aug 27, 2023
Merged

Replace cryptography with hashlib #278

merged 2 commits into from
Aug 27, 2023

Conversation

InnoredFR
Copy link
Contributor

I had issues trying to build a Docker image form linux/arm/v7 that runs a http server using your library. This was due to the fact that cryptography does not have prebuilt binaries for armv7, and it needs to be compiled during he pip install phase.

Problem is that it also needs a rust compiler to do so (as cryptography uses rust), and after trying for two days, I was not able to install it in the image and buid cryptography in the image.

Also cryptography is only used in a small portion of the code, and I figured switching to hashlib will lead to an improved cross architecture compatibily and improve my own workflow (which is cross compiling docker images) but also, and I hope, others workflow.

shyne99 and others added 2 commits August 26, 2023 15:49
Cryptography uses rust as engine. This can lead to trouble when trying to build this package for
arm/v7 as cryptography as no prebuilt binaries for this platform. In general, this will help with
cross compatibility across architectures. Also it was needed, when i tried to build a docker image
of an app using xknxproject as a lib. cross building for linux/arm/v7 would always fail in this case
due to having to setup a rust compiler and not succeeding to do so.
feat(Deps): Replace crytpography with hashlib
@codecov
Copy link

codecov bot commented Aug 26, 2023

Codecov Report

Merging #278 (6cd4a18) into main (824d415) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
- Coverage   97.34%   97.34%   -0.01%     
==========================================
  Files          21       21              
  Lines         943      942       -1     
==========================================
- Hits          918      917       -1     
  Misses         25       25              
Files Changed Coverage Δ
xknxproject/zip/extractor.py 93.82% <100.00%> (-0.08%) ⬇️

@farmio
Copy link
Member

farmio commented Aug 26, 2023

Hi 👋!
We chose cryptography because it was already used in the mother-project xknx too. So wherever one wants to use both xknx and xknxproject, cryptography needs to be working anyway - and we minimize the overall dependency footprint.

What is your use case for xknxproject on 32bit Arm?

Copy link
Member

@farmio farmio left a comment

Choose a reason for hiding this comment

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

Oh dear, I didn't notice that hashlib is part of pythons standard library. So this actually reduces dependencies.

And is even faster than the cryptography implementation on a quick test I made on 1 machine 🙃 (not that it matters for this use case here 😛). Probably due to overhead, as both are using OpenSSL afaik

Deprecated since version 3.10: Slow Python implementation of pbkdf2_hmac is deprecated. In the future the function will only be available when Python is compiled with OpenSSL.

Anyway, I think this is a good idea! Thank you very much 👍

@farmio farmio merged commit f000680 into XKNX:main Aug 27, 2023
6 checks passed
@InnoredFR
Copy link
Contributor Author

I am very happy that my PR was merged.

Totally forgot to mention that I just removed these deps without adding a new one.

Not every wheel is available for arm. Image build now takes a lot of time, again, no prebuilt wheel for pyzipper, but it is based on C, so I can build it fine, it just takes a full 10 minutes to build the image.... But at least it builds and runs.

Thank you for the work on this, I made my own .knxproj parser in Go a while back... Yours works wayyy better :)

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

Successfully merging this pull request may close these issues.

3 participants