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

modernized build #2070

Merged
merged 3 commits into from
Apr 16, 2022
Merged

modernized build #2070

merged 3 commits into from
Apr 16, 2022

Conversation

mgor
Copy link
Contributor

@mgor mgor commented Apr 11, 2022

i mentioned in a previous PR #2040 that there was problems adding locust as a git dependency; pip or pip-tools got confused that there was a pyproject.toml, but no build-system section available. all it contained was settings for black.

there have been other projects having the same problem.

the underlying problem seems to have been fixed now, so it is indeed possible to have locust as a git dependency (which I realized after implementing this...). but there are probably other benefits of moderizing the build setup to be PEP517 and PEP518 compatible.

since most stuff already was present in setup.cfg, it was quite an easy transition to move away from setup.py, however editable installs is not yet supported by pyproject.toml and hence a bare bone setup.py is needed.

most work had to be done in the github workflow and tox settings.

regarding the changes of command to python -m <module> in test.yml:
https://snarky.ca/why-you-should-use-python-m-pip/

build packages based on metadata (pyproject.toml+setup.cfg).

barebone setup.py is needed for compability, e.g. `pip install -e`
test would fail one run (stopping instead of stopped) and pass the next.
Dockerfile Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
roundrobin >=0.0.2
typing-extensions >=3.7.4.3
Jinja2 <3.1.0
pywin32;platform_system=='Windows'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was unable to get it working with pywin32 being an extras_require as in the setup.py case, however this should be the same(?). pywin32 only needed when running on windows.
https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#platform-specific-dependencies

@mgor mgor changed the title moderized build modernized build Apr 12, 2022
@cyberw
Copy link
Collaborator

cyberw commented Apr 13, 2022

LGTM. Have you tested it on windows? I have a windows box somewhere, but I can't be bothered to fire it up :)

@cyberw cyberw requested a review from heyman April 13, 2022 12:18
tox.ini Show resolved Hide resolved
@mgor
Copy link
Contributor Author

mgor commented Apr 13, 2022

LGTM. Have you tested it on windows? I have a windows box somewhere, but I can't be bothered to fire it up :)

If you mean the changed way pywin32 is included?

I could've actually just tested it myself.. without writing an obscure comment like that, my bad.

on "linux" (Windows 10 WSL2 with ubuntu 21.04):

➜ pip install .
➜ pip freeze | grep pywin32
➜ 

on windows 10:

(locust-venv) C:\NoBackup\locust [feature/pep_517_518 ≡]> pip3 install .
(locust-venv) C:\NoBackup\locust [feature/pep_517_518 ≡]> pip3 freeze | Select-String pywin32

pywin32==303

@cyberw
Copy link
Collaborator

cyberw commented Apr 13, 2022

I’m gonna give Heyman a day or two to chime in, but it looks all good to me!

@cyberw cyberw merged commit 51b1d50 into locustio:master Apr 16, 2022
@mgor mgor deleted the feature/pep_517_518 branch April 16, 2022 08:50
@heyman
Copy link
Member

heyman commented Apr 19, 2022

Looks good to me 👍

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.

3 participants