-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Implement Poetry build system (mainly so we don't have to commit dynamically generated front end bundles to git) #2725
Conversation
- Modify pyproject.toml file for poetry build system - Adds build script to generate front end as part of poetry build - Adds renaming script to remove architecture info from wheel file name - Updates Makefile to call the correct build steps with `make build`
For the consideration of @cyberw and community, apologies on the delay, sometimes life happens 😓 |
I'm also unsure if you have squash-on-merge turned on for the repo, just an FYI as I have not lovingly rebased my commits currently |
First run of make install failed for me. Probably the shell doesnt update its list of installed things in the middle of a && (could be enough to move
|
Perhaps it makes more sense to just assume poetry is installed (possibly giving a slightly nicer error message if it is not) |
Make build fails on yarn not being installed too. Bonus points if we can give a nice error message or even provide an action to install that. |
Make install doesnt actually add locust to my path. Do i need to do something more to set up poetry correctly? ( |
I think the versioning is only almost right.
On an un-tagged master it should be something like locust-2.28.1.dev5 (this version is needed to make good versions for pre-releases that are made whenever something is merged to master, see https://pypi.org/project/locust/#history) |
Great shout! I ran this again in a clean |
Yeah good idea, I don't think there's a reliable cross-platform way to do this really - I've added 9c90aec which gives the following behaviour when you don't have the
(I assume we're using classic We could certainly do something like attempting to install it via NPM after checking if NPM is there?
Same goes with |
Would you mind trying again on a clean
|
I'll need to take a look at this behaviour - it could be that I've made a load of tags testing this and something's getting a bit confused, I can try on a clean branch maybe - it should reflect your latest local git tag though right? What is that, in your case?
|
I think this is a reasonable compromise. People may want to install node and yarn in various ways so automating that is out of scope I think. |
.SILENT: | ||
.PHONY: check-poetry | ||
check-poetry: | ||
command -v poetry >/dev/null 2>&1 || { echo >&2 "Locust requires the poetry binary to be available in this shell to build the Python package.\nSee: https://docs.locust.io/en/stable/developing-locust.html#install-locust-for-development"; exit 1; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have found a nicer way to do this:
which poetry > /dev/null || {...
Or maybe that is linux/unix-specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I believe which
is unix-specific - the windows equivalent would be where
Come to think of it, I think command
itself is POSIX compliant and I currently have no idea if this works on Windows either 😓
Cross platform options likely boil down to:
- write a couple more lines (have cases for each platform)
- call out to the system's Python and have that check (we need Python anyway right)
- maybe just call the binary and catch the errors to the pipe anyway
Looking really good now. As for testing twine/publishing, its ok to break the publishing for a little while (maybe time the merge for when you know you have a couple hours to fix any issues that might arise?). I do have a pypy test account where we could publish, but meh. I did have an issue with the docker image. Can you try it and see if the same thing happened to you?
|
Yeah sounds good to me I can set aside some time for this 👍 I'll give the |
Great stuff! I’ll let @andrewbaldwin44 have a whirl when he gets back on Tuesday. Is there something left to do before you feel it is ready to merge? If not, you can take it out of draft and once Andrew has reviewed we’ll find a good time to merge. |
I noticed when running the tests |
@mquinnfd I left a couple of small comments but overall this looks awesome :) Thanks so much for working on this and battling with Windows! 🥇 |
Thanks for the review man 🙇 |
Hey this is a great call! This shaves some time off the Windows and Linux builds 👍
|
Going to open this for Public review and I believe the rest of these comments have been worked through - I'll take the opportunity while people eyeball it, to look at the publish steps next chance I get |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [locust](https://locust.io/) ([source](https://togithub.com/locustio/locust)) | `==2.29.1` -> `==2.30.0` | [![age](https://developer.mend.io/api/mc/badges/age/pypi/locust/2.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/locust/2.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/locust/2.29.1/2.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/locust/2.29.1/2.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>locustio/locust (locust)</summary> ### [`v2.30.0`](https://togithub.com/locustio/locust/releases/tag/2.30.0) [Compare Source](https://togithub.com/locustio/locust/compare/2.29.1...2.30.0) #### What's Changed - FastHttpSession: Enable passing json as a positional argument for post() and stop converting response times to int by [@​tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2772](https://togithub.com/locustio/locust/pull/2772) - Remove Line Chart Default Zoom by [@​andrewbaldwin44](https://togithub.com/andrewbaldwin44) in [https://github.com/locustio/locust/pull/2774](https://togithub.com/locustio/locust/pull/2774) - FastHttpSession requests typing by [@​tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2775](https://togithub.com/locustio/locust/pull/2775) - new events for heartbeat and usage monitor by [@​mgor](https://togithub.com/mgor) in [https://github.com/locustio/locust/pull/2777](https://togithub.com/locustio/locust/pull/2777) - dispatch benchmark test improvements by [@​tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2780](https://togithub.com/locustio/locust/pull/2780) - SequentialTaskSet: Allow weighted tasks and dict in .tasks by [@​bakhtos](https://togithub.com/bakhtos) in [https://github.com/locustio/locust/pull/2742](https://togithub.com/locustio/locust/pull/2742) - Fix StatsEntry docstring by [@​tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2784](https://togithub.com/locustio/locust/pull/2784) - Implement Poetry build system (mainly so we don't have to commit dynamically generated front end bundles to git) by [@​mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2725](https://togithub.com/locustio/locust/pull/2725) - Typing: strict optional in dispatch.py by [@​tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2779](https://togithub.com/locustio/locust/pull/2779) - Correctly set version from Poetry in published builds by [@​mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2791](https://togithub.com/locustio/locust/pull/2791) - Fix Extend Webui Example by [@​andrewbaldwin44](https://togithub.com/andrewbaldwin44) in [https://github.com/locustio/locust/pull/2800](https://togithub.com/locustio/locust/pull/2800) - Provide warning for local installs where yarn is not present by [@​mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2801](https://togithub.com/locustio/locust/pull/2801) - Fix tests on windows by [@​mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2803](https://togithub.com/locustio/locust/pull/2803) - Add example of a bottlenecked server and use that test to make a new graph for the docs by [@​cyberw](https://togithub.com/cyberw) in [https://github.com/locustio/locust/pull/2805](https://togithub.com/locustio/locust/pull/2805) - Replace total avg response time with 50 percentile (avg was broken) by [@​andrewbaldwin44](https://togithub.com/andrewbaldwin44) in [https://github.com/locustio/locust/pull/2806](https://togithub.com/locustio/locust/pull/2806) - Avoid deadlock in gevent/urllib3 connection pool (fixes occasional worker heartbeat timeouts) by [@​tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2813](https://togithub.com/locustio/locust/pull/2813) - Fix Dockerfile style warning by [@​mehrdadbn9](https://togithub.com/mehrdadbn9) in [https://github.com/locustio/locust/pull/2814](https://togithub.com/locustio/locust/pull/2814) - Fix pypy gc.freeze() AttributeError by [@​jimoleary](https://togithub.com/jimoleary) in [https://github.com/locustio/locust/pull/2819](https://togithub.com/locustio/locust/pull/2819) - Update poetry windows tests by [@​mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2821](https://togithub.com/locustio/locust/pull/2821) #### New Contributors - [@​bakhtos](https://togithub.com/bakhtos) made their first contribution in [https://github.com/locustio/locust/pull/2742](https://togithub.com/locustio/locust/pull/2742) - [@​mquinnfd](https://togithub.com/mquinnfd) made their first contribution in [https://github.com/locustio/locust/pull/2725](https://togithub.com/locustio/locust/pull/2725) - [@​mehrdadbn9](https://togithub.com/mehrdadbn9) made their first contribution in [https://github.com/locustio/locust/pull/2814](https://togithub.com/locustio/locust/pull/2814) - [@​jimoleary](https://togithub.com/jimoleary) made their first contribution in [https://github.com/locustio/locust/pull/2819](https://togithub.com/locustio/locust/pull/2819) **Full Changelog**: locustio/locust@2.29.1...2.30.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 10pm every weekday,before 6am every weekday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View the [repository job log](https://developer.mend.io/github/apereo/cas). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIiwibGFiZWxzIjpbIkJvdCIsIlJlbm92YXRlIl19-->
Switching to the Poetry build system
Tip
This PR was worked on iteratively in
Draft
form, so there's a bit of history in the comments here.This is being suggested for integration to make building and bundling the front end a bit easier, comments welcome particularly if you have strong opinions on
poetry
In a Nutshell
This PR changes the build system used to package Locust over to Poetry.
The main use-case for this currently, was to allow the removal of the (snazzy new) dynamically generated front end bundle from the actual source code, and just let the build process create these for the package during the build.
Caveat
Details
I've tried to keep the package output and metadata like-for-like, because I havn't looked behind the curtain at the Locust release process, CI runners, etc. - it's likely this may need to change for that purpose.
There's plenty I am blissfully unaware of when it comes to maintaining this project, this is just a jumping off point for the sake of conversation - I have no particular love of Poetry, though I do use this day-to-day for projects currently. Alternatives noted were PDM (and now Rye by "the Ruff people") but I have no idea if these support the king of things we're doing here currently.
What
Adds the Poetry build system to the Locust package, this changes the build backend to Poetry's PEP-517 compliant build back-end; Poetry Core
Why
Discussed with @cyberw here: #2405 (comment)
Try it Out
Local dev installation
Details
make install
Packaging
Details
make build
TODO
I wanted to put there out there for discussion before doing things like:
README