-
Notifications
You must be signed in to change notification settings - Fork 425
Switch the build backend from poetry-core to maturin
#19234
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
Conversation
Keeping `packages` and the rust build script allow `poetry install` to continue to automatically build the rust modules for ease of development.
`maturin` will build an abi3 compatible wheel by default.
4d31c9f to
772614c
Compare
| # Hold off on migrating these to `dev-dependencies` (PEP 735) for now until | ||
| # Poetry 2.2.0+, pip 25.1+ are more widely available. | ||
| [tool.poetry.group.dev.dependencies] |
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.
Great comment!
| @@ -0,0 +1 @@ | |||
| Switch the build backend from `poetry-core` to `maturin`. No newline at end of file | |||
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.
The PR description makes this sound good and nice that we can eliminate a few more pieces. And the less Poetry, the better.
Overall, the changes themselves are kinda like 🤷 sure - looks equivalent.
There is bound to be some intricacy that isn't quite right but only way to find that out is people complaining. poetry install --extras all works for me ⏩
| [tool.poetry] | ||
| packages = [{ include = "synapse" }] | ||
|
|
||
| [tool.poetry.build] | ||
| script = "build_rust.py" | ||
| generate-setup-file = true |
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.
This is just moved from below.
These make me go "why?" but prior art ⏩
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 went on a little investigation to figure out why, and have documented the reasoning in b885d1e.
|
Merging manually due to Complement test flakes. |
This PR switches the build backend from
poetry-coretomaturin. This was prompted by the switch to poetry 2.x in #19137 being incompatible with Linux distributions that still only ship poetry 1.x.maturinshould be supported on said distributions.maturinit comes with the benefit of better PyO3 support. For instance, we can now remove the.ci/scripts/auditwheel_wrapper.pyscript which was needed to make the wheels produced bypoetry-coreincludeabi3in the tags, instead ofcpX.Importantly,
poetry install ...will still work and still build thesynapserust module. Development workflows do not need to change.I've held off on migrating the
[tool.poetry.group.dev.dependencies]section to[dependency-groups].dev(which was deliberately excluded in #19137). These dependencies are only for development of Synapse (running the release script, checking types, etc.). Switching to[dependency-groups]section would end up requiringpoetry 2.2.0+(andpip 25.1+). Despite this, as of #19137,pip install .[all]will install all other dependencies, which is enough to build packages.You can then use
maturin build(orcibuildwheel, as we do) to build packaged wheels undertarget/wheels/; no need forpoetrynorpoetry-core2.x to be directly involved in the build process.maturinincludes its ownauditwheelimplementation.cibuildwheelwill by default runauditwheel repairwhen building on Linux. In the interest of not breaking our wheels, we've opted to leave both implementations in place. We can look into disabling one of them separately.Dev notes
Discussed in the internal backend team lobby room
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.