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

[IMP] Migrate to poetry to manage package #329

Closed
wants to merge 5 commits into from

Conversation

joao-p-marques
Copy link
Member

@joao-p-marques joao-p-marques commented Apr 13, 2021

The fix from #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 joao-p-marques force-pushed the migrate-poetry branch 8 times, most recently from ad72751 to 7afd6a3 Compare April 15, 2021 06:47
@joao-p-marques joao-p-marques marked this pull request as ready for review April 15, 2021 06:58
joao-p-marques added a commit to Tecnativa/doodba that referenced this pull request Apr 15, 2021
Install version from OCA/pylint-odoo#329 with a more modern packaging method, avoiding errors with setuptools
.travis.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

In general terms, poetry is pure pleasure.

Besides, it fixes what #328 was trying to fix (and didn't).

You have my blessing.

@joao-p-marques
Copy link
Member Author

@yajo changes made. Thanks!

ap-wtioit pushed a commit to ap-wtioit/docker-odoo-base that referenced this pull request Apr 19, 2021
Install version from OCA/pylint-odoo#329 with a more modern packaging method, avoiding errors with setuptools
@yajo
Copy link
Member

yajo commented Apr 20, 2021

Please could you review @moylop260?

joao-p-marques added a commit to Tecnativa/doodba that referenced this pull request Apr 20, 2021
Install version from OCA/pylint-odoo#329 with a more modern packaging method, avoiding errors with setuptools
yajo pushed a commit to Tecnativa/doodba that referenced this pull request Apr 20, 2021
Install version from OCA/pylint-odoo#329 with a more modern packaging method, avoiding errors with setuptools
@@ -0,0 +1,1013 @@
[[package]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did you make this file?

What extra considerations should we take now with this new way for each release?

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is managed by poetry. This is the file poetry uses to resolve all the dependencies for the environment and pin them to a specific version.

The only thing to take into account in a new release would be bumping the version in the pyproject.toml file. However, I think you can even enable another plugin in poetry to enable this automatically from git tags. @yajo knows this better 😄

Copy link
Member

Choose a reason for hiding this comment

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


after_success:
- coveralls
- python setup.py sdist # Build ChangeLog file from git log
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is building the package to upload to pypi?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry. I think I saw your comment there and assumed that step only enabled the changelog generation from git, so I deleted it.

We can build and push with poetry publish

I will be adding this.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can either use poetry build to build the package and use the current deploy stage to push it, or use poetry publish --build which will do both in one step. Which do you prefer @moylop260?

@ap-wtioit
Copy link

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
Copy link
Member Author

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
Copy link
Member

yajo commented Apr 22, 2021

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

@joao-p-marques
Copy link
Member Author

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

Ok 👍 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
Copy link
Collaborator

Sorry, I didn't get you

What pinned version?

@joao-p-marques
Copy link
Member Author

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 8a69151
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 f049dd4

@moylop260
Copy link
Collaborator

@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
Copy link
Member Author

Ok, thanks @moylop260

With f049dd4, 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
Copy link
Member Author

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 #329 (comment) which approach do you prefer to follow then?

@moylop260
Copy link
Collaborator

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
Copy link
Member Author

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 🙂

Remove setup.py and setup.cfg
These tools are becoming old and deprecated, and can introduce errors:
- https://pip.pypa.io/en/latest/cli/pip_install/#controlling-setup-requires
- https://setuptools.readthedocs.io/en/latest/userguide/dependency_management.html?highlight=dependencies#id3
- pypa/setuptools#1320 (comment)

Python-poetry can replace that, making it easier to manage dependencies
Also fix test module to allow being discovered by unittest
Remove auto generation of change log (temporary)
Remove install script (all dependencies handled by poetry)
Solves import error for higher versions as functions are renamed
@moylop260
Copy link
Collaborator

@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

@moylop260 moylop260 closed this in feacd10 Oct 21, 2022
IJOL pushed a commit to BITVAX/doodba that referenced this pull request Apr 16, 2024
Install version from OCA/pylint-odoo#329 with a more modern packaging method, avoiding errors with setuptools

(cherry picked from commit 68fd24e)
@pedrobaeza pedrobaeza deleted the migrate-poetry branch August 28, 2024 15:25
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

Successfully merging this pull request may close these issues.

4 participants