Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Upgrade poetry-core range to fix #16147 #16702

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

thebalaa
Copy link
Contributor

@thebalaa thebalaa commented Nov 29, 2023

This fixes #16147 but I'm not sure this is precisely the right version bound. Looking through Poetry related issues directly or indirectly related to #16147 I tend to argue Poetry has created more problems than benefit and would prefer removing it altogether.

For example, we are using an unsupported Poetry mechanism for building the Rust synapse.synapse_rust modules, see python-poetry/poetry#2740 (comment)

Happy to make the necessary changes in order to improve the new contributor onboarding experience, whatever they may be.

Pull Request Checklist

  • Pull request is based on the develop branch
  • [] Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Signed-off-by: Mo Balaa mo@fractalnetworks.co

Copy link
Contributor

@michaelkaye michaelkaye left a comment

Choose a reason for hiding this comment

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

This seems to be a) required and b) sufficient to get setup-matrix-synapse installing via poetry again. See https://github.com/michaelkaye/setup-matrix-synapse/actions/runs/7036170569/job/19148201740

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thank you for persisting with this. LGTM per below.


Reproduced a broken installation as follows:

docker run -it debian:12-slim bash
apt update
apt install python3 python3-pip pipx git pkg-config libsystemd-dev libicu-dev libpq-dev rustc
pipx install poetry==1.6
pipx ensurepath
exec $SHELL
cd ~
git clone https://github.com/matrix-org/synapse
cd synapse
git checkout v1.97.0
poetry install --all-extras -vv

And then:

$ poetry --version; poetry run python -c "import synapse"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/root/synapse/synapse/__init__.py", line 26, in <module>
    from synapse.util.rust import check_rust_lib_up_to_date
  File "/root/synapse/synapse/util/rust.py", line 20, in <module>
    from synapse.synapse_rust import get_rust_file_digest
ModuleNotFoundError: No module named 'synapse.synapse_rust'

Trying again with newer poetry and this patch:

docker run -it debian:12-slim bash
apt update
apt install python3 python3-pip pipx git pkg-config libsystemd-dev libicu-dev libpq-dev rustc
pipx install poetry==1.7
pipx ensurepath
exec $SHELL
cd ~
git clone https://github.com/matrix-org/synapse
cd synapse
git fetch origin pull/16702/head
git checkout FETCH_HEAD 
poetry install --all-extras -vv
# poetry --version; poetry run python -c "import synapse"
Poetry (version 1.7.0)

@DMRobertson
Copy link
Contributor

Can you add a changelog entry please, so we can get CI churning? I tried to add one to this PR myself, but I don't think I have write access to your fork's branch. Here's my suggestion:

From 161330f3383ee04d8f4d6249e939b3c883540c10 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Wed, 29 Nov 2023 18:06:22 +0000
Subject: [PATCH] Changelog

---
 changelog.d/16702.misc | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/16702.misc

diff --git a/changelog.d/16702.misc b/changelog.d/16702.misc
new file mode 100644
index 0000000000..be5a508208
--- /dev/null
+++ b/changelog.d/16702.misc
@@ -0,0 +1 @@
+Raise poetry-core upper bound to <=1.8.1. This allows contributors to import Synapse after `poetry install`ing with Poetry 1.6 and above.
-- 
2.43.0

@clokep clokep merged commit 3a09269 into matrix-org:develop Nov 29, 2023
38 checks passed
@thebalaa thebalaa deleted the rev-poetry-core branch November 29, 2023 21:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

synapse fails to install with poetry 1.6.0 - "No module named 'synapse.synapse_rust'" at runtime
4 participants