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

Pipenv to poetry #623

Merged
merged 18 commits into from
May 16, 2021
Merged

Pipenv to poetry #623

merged 18 commits into from
May 16, 2021

Conversation

jb3
Copy link
Member

@jb3 jb3 commented Mar 10, 2021

This PR migrates the dependency manager for Sir Lancebot to Poetry.

This is not intended to be merged anytime soon but is more of an experiment to see whether the tool integrates well and has good support across platforms, hence why it is currently a draft.

There are minor things that have had to be altered in order to make support work, as it's building on Python 3.9.

If we decide to go through with this then we'll probably merge this and then make alterations to new Python 3.9 syntax (i.e. typing with generics) in a later PR.

Copy link
Contributor

@Akarys42 Akarys42 left a comment

Choose a reason for hiding this comment

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

Very cool, thanks a lot joe

pyproject.toml Outdated Show resolved Hide resolved
@Xithrius Xithrius added area: backend Related to internal functionality and utilities area: CI Related to continuous intergration and deployment area: dependencies Related to package dependencies and management area: tools Development related to our toolchain (Docker, pipenv, flake8) status: WIP Work In Progress type: enhancement Changes or improvements to existing features type: feature Relating to the functionality of the application. labels Mar 10, 2021
Base automatically changed from master to main March 13, 2021 20:09
Copy link
Contributor

@Kronifer Kronifer left a comment

Choose a reason for hiding this comment

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

Looks good, but there doesn't seem to be any need for taskipy as shown in the docs here

@HassanAbouelela
Copy link
Member

@Kronifer The default task system is meant to run python modules, such as the main bot for example, it doesn't seem able to run arbitrary commands like taskipy. Taskipy is closer in functionality to pipenv scripts, and scripts from other languages' package managers. This is needed for tasks such as linting with flake8. A possible alternative that uses the builtin system would be defining all of our tasks as python modules, but that seems a little pointless.

@janine9vn
Copy link
Contributor

Tested it out. It's pretty nice and locking deps didn't take 5 years. So +1 for a basic Windows user.

@HassanAbouelela
Copy link
Member

For something that may not be noticed in testing: poetry does not support automatic selection of python interpreter (even though it claims it does). This is something that has bitten us in the staff pyweek project for some people, and something I've dealt with on my own projects.

Poetry claims to be able to select the correct python version from your path, like pipenv does, but that doesn't seem to work (from my testing on windows, and from reports from linux users). Since I have python 3.9 as the base interpreter on my device, I had no problem with projects such as pyweek, or forms (they both use 3.9), or this PR since it bumps python version up to 3.9. Bumping back down to 3.8 causes it to fail (I tried: 3.8.* & 3.8 & >=3.8 <=3.9, the star format being something I can confirm works for 3.9. Locking to my specific python version does not work correctly, even if fully specified 3.8.6.)

Installation with 3.8 for the python version results in:

The currently activated Python version 3.9.4 is not supported by the project (3.8.*).
Trying to find and use a compatible version.

  NoCompatiblePythonVersionFound

  Poetry was unable to find a compatible version. If you have one, you can explicitly use it via the "env use" command.

  at c:\python\python39\lib\site-packages\poetry\utils\env.py:735 in create_venv
       731python_minor = ".".join(python_patch.split(".")[:2])
       732break
       733734if not executable:
    →  735raise NoCompatiblePythonVersionFound(
       736self._poetry.package.python_versions
       737│                 )
       738739if root_venv:

(The first line in that error matches whatever python version I specify in the pyproject.toml.)

For more info about my setup: I have 3.8.6 from the default python.org install. I have it on my path. I can access it with py -3.8. I can not get poetry env to pick it up, unless I explicitly pass in the executable path (poetry env use C:\Python\python38\python.exe).

Ultimately, that does work, but it is nowhere near as simple as pipenv. The main selling point for poetry for me was the ludicrous speed at which it does locks and relocks, but in the grand scheme of things, we may relock once every blue moon, and a regular contributor may never relock at any point. All contributors will have to use the install command at one point or another. If pipenv does that better, and poetry's only selling point is speed, it does not make sense to switch to poetry.

@ChrisLovering ChrisLovering marked this pull request as ready for review May 16, 2021 14:42
d.py 1.5.1 depending on multidict, which needs gcc installed. Bumping d.py
to 1.7.2 removes the need fo rbuild tools in the image. d.py 1.7 introduced
the line sep are on the paginator, so this needs to be added to our
linepaginator subclass.
@ChrisLovering
Copy link
Member

d.py 1.5.1 depends on multidict, which needs gcc installed. Bumping d.py to 1.7.2 removes the need for build tools in the image.

d.py 1.7 introduced the linesep param in the paginator, so this needed to be added to our linepaginator subclass.

Copy link
Contributor

@ToxicKidz ToxicKidz left a comment

Choose a reason for hiding this comment

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

:shipit: LGTM, works well. Just one comment.

bot/utils/pagination.py Outdated Show resolved Hide resolved
Copy link
Member

@HassanAbouelela HassanAbouelela left a comment

Choose a reason for hiding this comment

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

This looks a-ok to me.

@Akarys42 Akarys42 merged commit 3d3a035 into main May 16, 2021
@Akarys42 Akarys42 deleted the pipenv-to-poetry branch May 16, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: backend Related to internal functionality and utilities area: CI Related to continuous intergration and deployment area: dependencies Related to package dependencies and management area: tools Development related to our toolchain (Docker, pipenv, flake8) status: WIP Work In Progress type: enhancement Changes or improvements to existing features type: feature Relating to the functionality of the application.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants