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

Poetry migration #370

Closed
wants to merge 6 commits into from
Closed

Conversation

Ovsyanka
Copy link

@Ovsyanka Ovsyanka commented Jan 2, 2024

There is inconsistency in dependency management in the project. Some processes use setup.py with almost no version restrictions and some use requirements.txt with exact versions (that is too tight).

Workflow to build the package relies on direct invocation of setup.py, and that is deprecated.

In that PR I address that two issues by replacing setup.py + setuptools by pyproject.toml + poetry.

Details/explanations of the changes

Required python wasn't be specified so I took it from .github/workflows/ci.yaml (meaning 3.6+)

Dependency versions I took from the requirements.txt, but replaced == for ^ because it should be safe enough and not restrict user to outdated versions of the dependencies.
Exact dependencies versions proven to work should be managed by poetry.lock file, that is created/updated on changing (adding, removing, updating) dependencies.
The only version restriction in setup.py was "pycryptodomex=>3.6.2" and I replaced it with "pycryptodomex^3.6.2" because I think it is safer.

I removed __version__ from the init.py because it would require to use some library like importlib.metadata (available from python 3.8) to get version from the package metadata and there could be different options depending on the python version. As I understand, the library by itself doesn't need that functionality and if someone who uses the library wants to get it's version he can use such a library instead of pykeepass.__version__.
As it was used in the make pypi command, I made it upload all new releases to the PyPI instead of just current one. Probably it will be replaced by poetry publish in the future.

Current (1.7.1) version of poetry doesn't support python < 3.8, and poetry 1.1.13 is the last version with python 3.6 support. So in the github CI I use poetry 1.1.13 for both python 3.6 and 3.7, so It will be fine to use it for use the library, but for development current version of poetry expected.

I made the configuration that can be used insted of setup.py. One of the reasons I decided to do that is because setup.py doesn't have the versions of the dependencies and the state of the project can't be perfectly replicated without knowing the versions.
Direct invocation of setup.py is [deprecated](https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html), so I replace it with using poetry.
I removed `version` from the `init.py` because it would require to use some library like [importlib.metadata](https://docs.python.org/3/library/importlib.metadata.html) (available from python 3.8) to get version from the package metadata and there could be different options depending on the python version. As I understand, the library by itself doen't need that functionality and if someone who uses the library wants to get it's version he can use such a library instead of `pykeepass.version`

As it was used in the `make pypi` command, I made it upload all new releases to the PyPI instead of just current one. Probably twine will be replaced by `poetry publish` in the future anyway.
requirements-rtd.txt, requirements.txt, and setup.py isn't used anymore
@Evidlo
Copy link
Member

Evidlo commented Jan 2, 2024

Why should running tests depend on Poetry?

@Ovsyanka
Copy link
Author

Ovsyanka commented Jan 2, 2024

Why should running tests depend on Poetry?

Because to run tests we need to install package dependencies and to install dependencies we need poetry. Before there was requirements.txt, that held dependencies and pip was used directly, now there is poetry.lock instead.

upd: I think I misread your question at first. So, I think you are talking not about why do we need poetry installed, but why do we run tests using poetry run command, right?

This command run it's argument in activated virtual environment, where all the dependencies installed. (see https://python-poetry.org/docs/basic-usage/#using-poetry-run)
We are not installing packages globally, like it was before, but in the .venv directory.

@Evidlo
Copy link
Member

Evidlo commented May 21, 2024

I've grown a bit uncertain about poetry over the last few months, particularly in regards to how it seems to often break pip editable-installs and makes supporting older versions of Python more complicated.

Sorry for drawing this out for so long. I see that you put in significant effort on this PR and even joined the chatroom to ask questions, so I feel bad about closing this. Hopefully this doesn't discourage you from contributing to this project or other FOSS projects like it in the future.

@Evidlo Evidlo closed this May 21, 2024
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.

None yet

2 participants