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

Consider unlocking cryptography requirement in setup.py #467

Closed
smarlowucf opened this issue Feb 18, 2022 · 3 comments
Closed

Consider unlocking cryptography requirement in setup.py #467

smarlowucf opened this issue Feb 18, 2022 · 3 comments

Comments

@smarlowucf
Copy link
Contributor

smarlowucf commented Feb 18, 2022

I added a comment in #452 but figured it's better to open a new discussion.

Is there a reason (compatibility wise) to lock the version bounds for cryptography requirement? If flask-jwt-extended works with older versions of cryptography it would be ideal to support that option. Unnecessarily locking requirement bounds makes it difficult to package downstream where a given distribution might have an older version of cryptography.

Having the version locked like this conveys that you "need" cryptography>={some_version} for flask-jwt-extended to work but based on the commit history it doesn't seem like this is actually the case.

In reference to https://github.com/vimalloc/flask-jwt-extended/blob/master/setup.py#L33

@smarlowucf
Copy link
Contributor Author

Looking at the code I don't even see the cryptography package being used. Am I missing something in the code? It seems like cryptography shouldn't even be a requirement.

There's:

from jwt.algorithms import requires_cryptography

But that is coming from PyJWT. And that package handles it's own requirements via https://github.com/jpadilla/pyjwt/blob/master/setup.cfg#L42. Thus installing it with the crypto option would pull in cryptography>=3.3.1.

It seems like flask-jwt-extended would only inherit this. Even if it's explicitly set then it seems like it would match the requirement that it's duplicating from PyJWT rather than requiring a newer version than what's actually needed.

@vimalloc
Copy link
Owner

That in an extra_requires both in pyjwt and in here, so it wont get installed by default unless it's specifically asked for. I have it in my repository so that end users don't have to know the implementation details of how to install pyjwt with that addon in order for asymmetric crypto to work, and it allows me to change that dependency behind the scenes if I need to without causing breaking changes (in theory, there is actually some pyjwt stuff leaking through my abstraction layers that I need to clean up at some point, but it will be a breaking change so I haven't done so yet).

I'm not opposed to changing the lower band for crypto though. I'll move it to cryptography>=3.3.1 to match pyjwt 👍

@smarlowucf
Copy link
Contributor Author

Thanks @vimalloc that would be helpful. I understand with the extras requirement in PyJWT it's a harder requirement to manage.

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

No branches or pull requests

2 participants