[FIX] Avoid updating dependencies if pinned by odoo

[yajo]

Around https://github.com/OCA/l10n-spain/actions/runs/7707368084/job/21004431015?pr=3389#step:4:347 you can see these logs:

+ cat test-requirements.txt
[...]
cryptography
[...]
Requirement already satisfied: cryptography in /opt/odoo-venv/lib/python3.10/site-packages (from -r test-requirements.txt (line 3)) (3.4.8)
[...]
Collecting cryptography (from -r test-requirements.txt (line 3))
  Downloading cryptography-42.0.1-cp39-abi3-manylinux_2_28_x86_64.whl.metadata (5.3 kB)
[...]
  Attempting uninstall: cryptography
    Found existing installation: cryptography 3.4.8
    Uninstalling cryptography-3.4.8:
      Successfully uninstalled cryptography-3.4.8
Successfully installed [...] cryptography-42.0.1 [...]

(Full logs here: https://github.com/moduon/oca-ci/pull/1#issuecomment-1916446873).

These reveal that for some reason, adding cryptography to test-requirements.txt makes pip update it, when it should just stay with the version pinned by odoo.

Here I'm adding upstream requirements as a constraints file, so that OCA workloads don't attempt to install any dependency that is pinned there.

@moduon MT-4799 @EmilioPascual

[yajo]

@sbidoul could you please review this one?

I found https://github.com/OCA/l10n-france/pull/515 after applying it in our CI.

[sbidoul]

Could you test that first in your problematic repo by adding the constraint in test-requirements.txt. I suspect this will fail due to conflicting constraints.

Actually, I think we should not do this. Odoo is pinning very old versions and of external libs (for reasons I don't quite understand). In many cases we need more recent ones, and Odoo actually works fine with later versions.

[yajo]

But doesn't it make sense to assert that OCA works with the minimal versions pinned by Odoo, just like we are supposed to work with its minimal python version?

[sbidoul]

That might be nice, but I'm not sure its feasible, and we don't ensure it at the moment, so I suspect this PR will break a lot of repos.

Also at some point there were security issues on the versions of these libs that are in odoo's requirements.txt (I have not checked recently). Debian/ubuntu backport patches in their .deb packaging of these versions, but I suspect it may even be unsafe to source these versions from PyPI.

To me the real meaning of this odoo/requirements.txt is unclear. I'm not even sure Odoo's runbot use these version. And I have seen odoo.sh instances with different versions.

In an ideal world Odoo's setup.py should specify minimum versions of their dependencies. And also place upper bounds on dependencies they know are incompatible (such as pypdf2<3, reportlab<4, xlrd<2 for Odoo 16).

[yajo]

Could you test that first in your problematic repo by adding the constraint in test-requirements.txt. I suspect this will fail due to conflicting constraints.

It doesn't seem to work: https://github.com/OCA/l10n-spain/actions/runs/7724809391/job/21057695751

[sbidoul]

Ah but that's just the check for unreleased dependencies. The installation actually works. And the log does show cryptography==3.4.8.

So this mean I don't understand the root cause of the cryptography upgrade. Can you provide a link to a similar log without the constraint?

[yajo]

Yes, you have it above: https://github.com/OCA/oca-ci/pull/66#issue-2108185584

[yajo]

So this mean I don't understand the root cause of the cryptography upgrade.

FWIW I also don't understand it. But it started happening both on OCA and our internal CI, and this constraint fixed it.

[sbidoul]

Ok I got it. It's the last version of requests-pkcs12 (which is a dependency of something in l10n-spain) that requires cryptography>=42. When you add a constraint, pip goes out of its way to find an older version of request-pkcs12 that satisfies the constraints.

You can see that in the log (https://github.com/OCA/l10n-spain/actions/runs/7724809391/job/21057695944#step:4:232), it finally finds version 1.12 which satisfies the constraints.

I still think we'll get more troubles than benefits if we merge this.

Actually, what's the problem with cryptography>=42 ?

[yajo]

Actually, what's the problem with cryptography>=42 ?

It's incompatible with the preinstalled version of pyopenssl. Just see the failure in the logs: https://github.com/OCA/l10n-spain/actions/runs/7707368084/job/21004431015?pr=3389#step:7:48

I still think we'll get more troubles than benefits if we merge this.

🤷🏼‍♂️ OK, but then we need some sort of fix... Getting a red build as explained in https://github.com/OCA/oca-ci/pull/66#issuecomment-1918846370 isn't a fix neither.

[sbidoul]

One thing we could do is declaring the upper bound constraints somewhere. Maybe something like requirements-upperbounds.txt that we could store in OCB.

For 16 it could look like this

reportlab<4
pypdf2<3
# xlrd<2 because Odoo relies on it to import xlsx files and xlrd 2 dropped that capability
xlrd<2
# the last version of cryptography that is compatible with the pyopenssl version in Odoo's requirements.txt
cryptography<=XXX

Longer term, we could try convincing Odoo to put these upper bounds in its setup.py.

Need to think about it a little bit.

[yajo]

What about supporting -c in test-requirements.txt? Because all would work too if we upgraded pyopenssl, but it makes no sense to add that requirement.

[sbidoul]

I propose #73 as an alternative.