Skip to content

Conversation

@reneleonhardt
Copy link

Chores

  • Update dependencies
  • Add Python 3.14, remove pypy3.9, add pypy3.11
  • Let dependabot update github-actions

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2025

CLA assistant check
All committers have signed the CLA.

@NyanKiyoshi
Copy link
Member

NyanKiyoshi commented Aug 26, 2025

@reneleonhardt thanks for the PR!

Seems like there are issues with PyPy3.11 and Python 3.14, would you have some time to look into resolving these? If not, we can remove the added support in this PR, and we could try to come back to that topic in the future

There is also one type check failing (mypy), a cast(int, port) should do the trick as we should never get bytes for port (https://github.com/saleor/requests-hardened/blob/04a37172174b1911b072817b4a2cca50fa2beca2/requests_hardened/ip_filter.py?1#L36)

@reneleonhardt
Copy link
Author

reneleonhardt commented Aug 27, 2025

I wonder why CI isn't allowed to run automatically, can you enable it for faster feedback?

pytest has not been released yet since all the Python 3.14 fixes over the last months, for example the mark / decorators support which is breaking tests here:
pytest-dev/pytest#13553
I used a mark (pun intended) to skip the 2 corresponding tests for the time being and asked for a new release.

Is pinning .poetry_version really necessary, why not just install the latest poetry compatible with the tested Python version?

Also locally I couldn't manage to convince poetry to update some transitive dependencies like cryptography which weren't buildable with Python 3.14.
After I migrated to uv it was a breeze, maybe you consider using uv in the future.

@NyanKiyoshi
Copy link
Member

NyanKiyoshi commented Sep 1, 2025

My apologies for the delays, I'm quite swamped currently thus it takes me a while to get back to you. I think the PR should be pretty close from done!

I wonder why CI isn't allowed to run automatically, can you enable it for faster feedback?

This is due to the fact any change must be reviewed before running them in workflows as they could contain malicious code (that's unfortunately the way GitHub designed actions)

pytest has not been released yet since all the Python 3.14 fixes over the last months, for example the mark / decorators support which is breaking tests here: pytest-dev/pytest#13553 I used a mark (pun intended) to skip the 2 corresponding tests for the time being and asked for a new release.

That sounds good to me if you can mark the failing tests as to be skipped for Python 3.14 meanwhile pytest releases a new version 🙂

Is pinning .poetry_version really necessary, why not just install the latest poetry compatible with the tested Python version?

Yes it's better to pin it in order to ensure the behavior of the workflows is always consistent (we especially want to prevent them from suddenly breaking which can happen however is very rare). I do see it's using a pretty old version of Poetry though thus should be updated.

Also locally I couldn't manage to convince poetry to update some transitive dependencies like cryptography which weren't buildable with Python 3.14.

It's caused by the fact cryptography>=44.0.0 doesn't support Python 3.9.0 and 3.9.1:

Poetry logs
$ poetry update -vvv cryptography
[…]
Source (PyPI): 64 packages found for cryptography >=3.1
   1: fact: cryptography (45.0.6) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==45.0.6)
   1: fact: cryptography (45.0.5) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==45.0.5)
   1: fact: cryptography (45.0.4) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==45.0.4)
   1: fact: cryptography (45.0.3) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==45.0.3)
   1: fact: cryptography (45.0.2) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==45.0.2)
   1: fact: cryptography (45.0.1) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==45.0.1)
   1: fact: cryptography (44.0.3) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==44.0.3)
   1: fact: cryptography (44.0.2) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==44.0.2)
   1: fact: cryptography (44.0.1) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==44.0.1)
   1: fact: cryptography (44.0.0) requires Python !=3.9.0,!=3.9.1,>=3.7
   1: derived: not cryptography (==44.0.0)
   1: fact: cryptography (43.0.3) depends on cffi (>=1.12)
   1: selecting cryptography (43.0.3)
   1: derived: cffi (>=1.12)
[…]

Doing the following fixes the issue (it updated cryptography to 45.0.6 which is latest):

diff --git a/pyproject.toml b/pyproject.toml
index 730a045..e95dd04 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -42,7 +42,7 @@ Issues = "https://github.com/saleor/requests-hardened/issues"
 Changelog = "https://github.com/saleor/requests-hardened/releases/"

 [tool.poetry.dependencies]
-python = ">=3.9,<4.0"
+python = ">=3.9.2,<4.0"
 # We require >=2.32.3 due to depending on `get_connection_with_tls_context`.
 requests = ">=2.32.3,<3.0.0"

I will open a PR to fix that issue (edit: #44) as it prevents us from using the latest version, thanks for noticing!

@reneleonhardt
Copy link
Author

Why did you duplicate my work into a new PR instead of helping here and merging first?

@NyanKiyoshi
Copy link
Member

NyanKiyoshi commented Sep 1, 2025

Why did you duplicate my work into a new PR instead of helping here and merging first?

Hey! It was because I considered the cryptography issue as an issue requiring immediate attention. You can safely rebase 🙂 (note: for poetry.lock conflict, just accept "their" or "ours", it doesn't matter which one, then you can run poetry lock which will fix the conflicts)

@NyanKiyoshi
Copy link
Member

Hey @reneleonhardt 👋

As this PR is stale and Python 3.14's release is very soon, I took it upon myself to add Python 3.14 support here: #50

Thank you once again for the changes, and for taking the time to make them 🙇 And sorry for the confusion earlier around the conflicts created by another PR due to implementing same/similar changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants