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

Introduce Poetry as build system #620

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Conversation

pehala
Copy link
Contributor

@pehala pehala commented Oct 4, 2024

Overview

  • Introduces Poetry as both dependency management and easy building tool.
  • First step to solving Release to PyPi #555
  • Add main() method to ccm script
    • Easier to refer to it in poetry
    • Could be solved even without it, if I investigated more, but I like main() method better anyway
  • Update workflows action versions
    • Solves deprecated warnings about node12
    • Unnecessary for this Pr, I can extract it to a separate one if needed

Why Poetry

Poetry enables to do both dependency management and building in one package. Setuptools could be used as well, but I find Poetry easier to use and it solves both problems at once. It is a modern build system with nice features like dependency groups (so we dont need multiple requirements files) etc. It also has good IDE support, making installing and managing dependencies much easier than it is today.

How to use it

Main difference between using requirements files or setup.py and Poetry is that poetry by default installs everything into venv and manages that env as well. Local development can be setup with simple poetry install, then you can either use poetry run <your_cmd> for running anything inside the poetry venv or you can simply use poetry shell to get into venv shell.

If you want to also install test dependencies you can use poetry install --with=test instead. Building is done through poetry build and poetry publish and you can change CCM version with poetry version.

More importantly, CCM can be installed with pip install git+... even if you dont have poetry installed, so this change should not break any existing workflows.

TODO

  • Test properly poetry publish
  • Clean up project metadata
  • Migrate pytest.ini into pyproject.toml
  • Add build & publish workflow

Testing

Questions for reviewers

  • Do you agree with Poetry usage?
  • What should the metadata be? (i.e. author, maintainers, etc)
  • Should we lock dependencies?

pehala added 3 commits October 4, 2024 09:09
Introduces main() method to ccm and then common if __name__ == '__main__': to run it
@pehala pehala marked this pull request as draft October 4, 2024 10:05
@pehala pehala requested review from fruch and roydahan October 4, 2024 10:05
@fruch
Copy link
Contributor

fruch commented Oct 6, 2024

Testing should cover:

  • running this ccm with dtest pipelines (gating / dtest CI)
  • driver matrices tests pipelines with this version (python/java)

@pehala
Copy link
Contributor Author

pehala commented Oct 9, 2024

  • running this ccm with dtest pipelines (gating / dtest CI)

https://jenkins.scylladb.com/view/master/job/scylla-master/job/byo/job/dtest-byo/640/. Works with no issues (the one fail is unrelated for sure)

  • driver matrices tests pipelines with this version (python/java)

https://jenkins.scylladb.com/job/scylla-staging/job/phala/job/python-driver-tests/2/consoleText. Not sure if tested
correctly, I was able to force custom ccm through my own pipelines fork, but I still see that it installed CCM (probably from dtest requirements.py) from https://github.com/scylladb/scylla-ccm/archive/master.zip.

@pehala pehala marked this pull request as ready for review October 14, 2024 08:10
@fruch
Copy link
Contributor

fruch commented Dec 4, 2024

Questions for reviewers
Do you agree with Poetry usage?

I'm not sure, my personal experience with it isn't that great (we use in Argus for example)

What should the metadata be? (i.e. author, maintainers, etc)

even me myself wasn't very consistent with that:
https://github.com/scylladb/python-driver/blob/2af442b0e13b92f7406b887667551b1dcf1b2dad/setup.py#L439C3-L439C27
https://github.com/scylladb/scylla-cqlsh/blob/5e20a4b83654d8f9462400cf67e0bc4d68f3b8cd/setup.cfg#L3

I tend to the first, just naming scylla and that's it
same as in scylla itself, we don't name the individuals on the RPM/DEB packages for example

Should we lock dependencies?

no we shouldn't lock, it's a library, and with locking it can complex the user of that library.
unless we know of something broken specify, that ccm won't work with, and then not pinning down
skip, or up from version.

@pehala
Copy link
Contributor Author

pehala commented Dec 4, 2024

I'm not sure, my personal experience with it isn't that great (we use in Argus for example)

Would love to hear it, my personal experience is positive, but there are plethora of other solutions we could use

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.

2 participants