search

OCA / pylint-odoo

Odoo plugin for Pylint
http://www.pylint.org
143 stars 168 forks source link

[IMP] Migrate to poetry to manage package #329

Closed joao-p-marques closed 1 year ago

joao-p-marques commented 3 years ago

The fix from https://github.com/OCA/pylint-odoo/pull/328 is incomplete. The problem still persists and will remain (and possibly escalate) while using deprecated packaging methods.

As mentioned in the following references:

It is recommended to update the packaging method not to depend on things like the setup_requires keyword, but instead manage the build dependencies from a pyproject.toml file.

To avoid complications when migrating to a different Python packaging method, poetry can facilitate the move. It can fully replace setup.py and setup.cfg, while still integrating with current tools like uniitest and tox. With this PR, I try to make that transition, while keeping the current functionality.

The only current functionality I didn't manage to incorporate here was the automatic changelog generation from git. If it is an important thing to keep, it can be done with most up-to-date tools based on https://www.conventionalcommits.org/en/v1.0.0/, like https://commitizen-tools.github.io/commitizen/ to read from git history and generate the changelog, like now. It would need some configuration to adapt it to the current OCA commit guidelines, but it could be done and would allow a bunch of cool features like https://commitizen-tools.github.io/commitizen/#commitizen-features WDYT @moylop260?

@Tecnativa TT29203

ping @Yajo

joao-p-marques commented 3 years ago

@Yajo changes made. Thanks!

yajo commented 3 years ago

Please could you review @moylop260?

ap-wtioit commented 3 years ago

This seems to break our doobda tests when running 11.0 and 12.0 with python 3.6 (instead of 3.5), it works fine when installing the pypi pylint-odoo version 3.7.1:

doodba INFO: Executing /qa/venv/bin/python --version
Python 3.6.13
INFO:root:Subtest execution: test_qa (tests.ScaffoldingCase) (DB_VERSION='11', ODOO_MINOR='11.0', PWD='/home/gitlab-runner/builds/DAvFejyg/0/odoo_tools/docker-odoo-base/tests/scaffoldings/settings', command=('/qa/venv/bin/python', '-c', 'import pylint_odoo'))
Creating settings-295419_odoo_run ... 
Creating settings-295419_odoo_run ... done
doodba INFO: Waiting until postgres is listening at postgresql...
doodba WARNING: Attempt to install unaccent in prod@postgresql failed with this message: psql: error: FATAL: database "prod" does not exist
doodba INFO: Linking all addons from /opt/odoo/custom/src/addons.yaml in /opt/odoo/auto/addons
doodba INFO: Generating /opt/odoo/auto/odoo.conf file. Overriding any existing...
doodba INFO: Merging found configuration files in /opt/odoo/auto/odoo.conf
doodba INFO: Executing /qa/venv/bin/python -c import pylint_odoo
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/qa/venv/lib/python3.6/site-packages/pylint_odoo/__init__.py", line 2, in <module>
    from . import checkers
  File "/qa/venv/lib/python3.6/site-packages/pylint_odoo/checkers/__init__.py", line 1, in <module>
    from . import modules_odoo
  File "/qa/venv/lib/python3.6/site-packages/pylint_odoo/checkers/modules_odoo.py", line 13, in <module>
    from .. import misc, settings
  File "/qa/venv/lib/python3.6/site-packages/pylint_odoo/misc.py", line 14, in <module>
    from pylint.utils import _basename_in_blacklist_re
ImportError: cannot import name '_basename_in_blacklist_re'
1
INFO:root:Subtest execution: test_qa (tests.ScaffoldingCase) (DB_VERSION='11', ODOO_MINOR='11.0', PWD='/home/gitlab-runner/builds/DAvFejyg/0/odoo_tools/docker-odoo-base/tests/scaffoldings/settings')
Stopping settings-295419_postgresql_1 ... 
Stopping settings-295419_postgresql_1 ... done
Removing settings-295419_postgresql_1 ... 
Removing settings-295419_postgresql_1 ... done
Removing network settings-295419_default
Removing volume settings-295419_db
Removing volume settings-295419_filestore
joao-p-marques commented 3 years ago

This seems to break our doobda tests when running 11.0 and 12.0 with python 3.6 (instead of 3.5), it works fine when installing the pypi pylint-odoo version 3.7.1

Thanks @ap-wtioit Indeed, for python > 3.5 it was installing a latest version of pylint, which was incompatible with this plugin. We hadn't noticed because we only added it in Doodba for v11 and v12 for now (which use Python 3.5) and the CI uses the poetry lock file to install the dependencies, which had the correct version pinned. Now that I pinned the version in the pyproject.toml and also enabled the build in CI, it shouldn't cause any problems.

yajo commented 3 years ago

I pushed the fix, you can undo the pin if you prefer.

joao-p-marques commented 3 years ago

I pushed the fix, you can undo the pin if you prefer.

Ok :+1: Thanks! Aren't there any other potential changes that can introduce an error by upgrading the version? @moylop260 is there another reason to have that version pinned?

moylop260 commented 3 years ago

Sorry, I didn't get you

What pinned version?

joao-p-marques commented 3 years ago

What pinned version?

Currently, the requirements.txt file pins the version of pylint with a strict ==2.6.0: https://github.com/OCA/pylint-odoo/blob/master/requirements.txt#L9 When I adapted to poetry, I added a different constraint of ^2.6.0, which was making all the versions up to 2.7.0 eligible, and led to some problems that @Yajo solved in https://github.com/OCA/pylint-odoo/pull/329/commits/8a6915146fa441e326521fef2800730185598af2 Now, we can keep that constraint slightly more loose or, if there were other reasons to keep that strict version number, we can also do that with https://github.com/OCA/pylint-odoo/pull/329/commits/f049dd427394a99535d30c1945d557c97df2d574

moylop260 commented 3 years ago

@joao-p-marques

Ok, I got it

The reason is that this project is used for all projects of the OCA So, if there is a new change in pylint incompatible with pylint-odoo so all CI will be red

We liked avoid it using only pinned version and bumping version using a new PR in order to check all is fine with the CI or fix the compatibility matter.

So, you can bump version in this PR in order to check all is working well or fix it if it is not

joao-p-marques commented 3 years ago

Ok, thanks @moylop260

With f049dd427394a99535d30c1945d557c97df2d574, poetry limits the version of pylint to only patch updates, you can see it in the docs: https://python-poetry.org/docs/dependency-specification/#tilde-requirements

So, in this case, it would limit to versions 2.6.0 up to 2.6.2. I’ll do some more testing to make sure it doesn’t introduce any problem and report back.

joao-p-marques commented 3 years ago

Well, taking a better look at it, the only 2.6.* release in Github is 2.6.2 (2.6.1 must have been deleted) and the only change is a fix in the Astroid version: image https://github.com/PyCQA/pylint/releases/tag/pylint-2.6.2 A quick test in a docker container and everything seems to work...

About this https://github.com/OCA/pylint-odoo/pull/329#discussion_r617233061 which approach do you prefer to follow then?

moylop260 commented 3 years ago

Sorry, I'm not familiar neither poetry and ^ and ~ dependencies structure

I see this PR hard to maintain from me in the time

@guewen What do you think?

joao-p-marques commented 3 years ago

Sorry, I'm not familiar neither poetry and ^ and ~ dependencies structure I see this PR hard to maintain from me in the time

Sorry, poetry also allows to specify dependency version constraints in a more conventional way, like pip. However, it does provide some extra syntax to make it easier, you can find more here: https://python-poetry.org/docs/dependency-specification/#version-constraints

Overall, it is a change to make it easier to maintain IMHO, specially with the changes in PEP standards I mention above and that will only cause more problems over time. I understand this is a big change, so it's up to you if it is worth it to invest in a tool like this 🙂

moylop260 commented 1 year ago

@yajo @joao-p-marques

I have created a new python packages here:

I just deleted the requirement to pbr

Now it is used only from tox in order to create the fancy auto-generated "ChangeLog" based on commits and tags

So, it should not be a problem

Could you check if it fix your current issue

FYI we will copy many things from that project to pylint-odoo

I have checked pylint, flake8, pre-commit and other projects and they are using the out-of-the-box packages way