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

[16.0][FIX] l10n_es_aeat + l10n_es_facturae + l10n_es_facturae_face: #3937

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

extrememicro
Copy link
Contributor

remove pinned cryptograph dependency

backport de #3936

Resuelve diferentes issues con problemas de versiones en la librería cryptography #3876 #3551 #3563 ... #3507
@etobella fijó la verisón por ser consistentes con Odoo en aquel momento ea23d96

Con la introducción de versiones de Python superiores a 3.10, que son las predeterminadas en sistemas como Debian 12 (Python 3.11) y Ubuntu 24.04 (Python 3.12), es probable que aumenten los problemas al instalar la localización.

Ahora, Odoo define dos versiones concretas aquí: https://github.com/odoo/odoo/blob/16.0/requirements.txt#L5
Además no veo incompatibilidad con versiones más modernas de cryptography y he testeado l10n_es_aeat y l10n_es_facturae con cryptography==42.0.8 sin problemas.
Por lo tanto, considero necesario ajustar la dependencia fijada en los módulos l10n_es_aeat, l10n_es_facturae y l10n_es_facturae_face para asegurar su compatibilidad con versiones más modernas de la librería.

@OCA-git-bot
Copy link
Contributor

Hi @etobella, @pedrobaeza,
some modules you are maintaining are being modified, check this out!

@etobella
Copy link
Member

Está asi por esto:

https://github.com/odoo/odoo/blob/16.0/requirements.txt#L5-L6

Talvez la solución seria poner las mismas restricciones o directamente quitar la dependencia porque ya viene de Odoo, no dejarlo así, ya que podemos tener problemas después.

@pedrobaeza
Copy link
Member

Teniéndolo en https://github.com/odoo/odoo/blob/16.0/requirements.txt#L5, creo que como dice Enrice, se puede eliminar directamente de aquí de external_dependencies (aunque se deje comentario que se utiliza, pero que se quita por compatibilidad).

@pedrobaeza pedrobaeza added this to the 16.0 milestone Jan 22, 2025
@extrememicro
Copy link
Contributor Author

La versión concreta ya viene forzada por el requirements de Odoo.
Al no especificar versión en los manifest, ya se cumple el requerimiento de la librería y se indica que se utiliza en los módulos. De esta manera no hay problemas de compatibilidad ya que la versión específica viene de Odoo.

Pensad que es lo mismo que pasa con requests de l10n_es_aeat_sii_oca. En odoo se fijan las versiones:
https://github.com/odoo/odoo/blob/16.0/requirements.txt#L69C1-L70C75
y en la localización se especifica que es un requerimiento sin forzar versión:
https://github.com/OCA/l10n-spain/blob/16.0/requirements.txt#L8
https://github.com/OCA/l10n-spain/blob/16.0/l10n_es_aeat_sii_oca/__manifest__.py#L39

@pedrobaeza
Copy link
Member

Pero si se incluye aquí, según despliegues, puede colocar una versión más moderna. Es mejor quitarlo. De hecho, ahora mismo está el CI en rojo y no sé si sería por está razón.

@etobella
Copy link
Member

Si miras lo que hace el runbot, al salir aquí intenta actualizar, no poner la versión sugerida por odoo. Debes eliminarlo o pinear igual que odoo

@extrememicro
Copy link
Contributor Author

Efectivamente en el CI se ha instalado cryptography==44.0.0, eso no debería haber pasado ya que la dependencia ya estaba satisfecha pero para evitar problemas lo mejor es quitarlo.

Ahora hago los rebase.

@extrememicro
Copy link
Contributor Author

@pedrobaeza aprovecho y quito del resto de módulos zeep y requests que ya vienen en Odoo?

@pedrobaeza
Copy link
Member

Vale, pone el comentario (en inglés) diciendo que se usan, pero que no se indican expresamente por la razón que hemos comentado aquí.

@extrememicro extrememicro force-pushed the 16.0-fix-l10n_es_facturae branch from 0cc023e to 25c4f8a Compare January 22, 2025 15:01
@extrememicro
Copy link
Contributor Author

Sigue dando error en el CI. He mirado donde está la librería y me imagino que se instala igualmente la última versión por algún otro módulo en repos como queue o reporting-engine:

Listado donde veo las dependencias en rama 16.0:

Details

❯ rg -e cryptography -e zeep -e 'requests$' -t txt -uuu
queue/requirements.txt
2:requests

reporting-engine/requirements.txt
2:cryptography

search-engine/requirements.txt
4:requests

l10n-belgium/requirements.txt
5:zeep

server-env/requirements.txt
2:cryptography

geospatial/requirements.txt
3:requests

server-tools/requirements.txt
4:cryptography

delivery-carrier/requirements.txt
4:zeep

social/requirements.txt
3:cryptography<37

l10n-spain/requirements.txt
3:cryptography==3.4.8
8:requests
14:zeep

server-auth/requirements.txt
2:cryptography

l10n-switzerland/requirements.txt
3:zeep

connector-telephony/requirements.txt
3:requests

l10n-france/requirements.txt
5:requests

@pedrobaeza
Copy link
Member

La solución creo que pasa por añadir en test-requirements.txt la versión específica de al menos cryptography.

@extrememicro extrememicro force-pushed the 16.0-fix-l10n_es_facturae branch from 25c4f8a to 8354392 Compare January 23, 2025 08:18
@pedrobaeza
Copy link
Member

Faltaría hacer lo del test-requirements.txt

…ments

The following dependencies: cryptography, zeep, and requests are used in
different modules, but we remove them from being required because they are
already included in Odoo's requirements.txt.
Having them in both places creates maintenance overhead and potential conflicts.
@extrememicro extrememicro force-pushed the 16.0-fix-l10n_es_facturae branch from 8354392 to cd0c8c7 Compare January 24, 2025 17:22
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Aunque el CI tiene versión Python 3.10 siempre, tampoco pasa nada por poner ambas versiones en test-requirements.txt.

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-3937-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 8280cb4. Thanks a lot for contributing to OCA. ❤️

@OCA-git-bot OCA-git-bot merged commit 6208762 into OCA:16.0 Jan 24, 2025
7 checks passed
@extrememicro extrememicro deleted the 16.0-fix-l10n_es_facturae branch January 25, 2025 08:28
@extrememicro extrememicro restored the 16.0-fix-l10n_es_facturae branch January 25, 2025 08:28
@extrememicro extrememicro deleted the 16.0-fix-l10n_es_facturae branch January 25, 2025 08:31
@extrememicro
Copy link
Contributor Author

extrememicro commented Jan 25, 2025

La dependencia venía de requests_pkcs12 que en su última versión necesita cryptography>=42.0.0:

requests-pkcs12==1.25
├── cryptography [required: >=42.0.0, installed: 44.0.0]
│   └── cffi [required: >=1.12, installed: 1.17.1]
│       └── pycparser [required: Any, installed: 2.22]
└── requests [required: >=2.26.0, installed: 2.32.3]
    ├── certifi [required: >=2017.4.17, installed: 2024.12.14]
    ├── charset-normalizer [required: >=2,<4, installed: 3.4.1]
    ├── idna [required: >=2.5,<4, installed: 2.10]
    └── urllib3 [required: >=1.21.1,<3, installed: 1.26.5]

Por lo que hay que ajustar de nuevo el requirements a requests_pkcs12==1.22 ; python_version < '3.12'
Con esto debería ir el CI sin tocar el test test-requirements.txt.

Miro de hacer otro PR

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.

4 participants