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

Support both import names of PyPI package python-multipart. #17932

Merged
merged 5 commits into from
Nov 20, 2024

Conversation

renaudallard
Copy link
Contributor

This allows synapse to run with python multipart >=0.0.13 without any tricky fallback. There is a conflict between 2 packages called multipart, so this should be modified.

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.
  • Code style is correct
    (run the linters)

This allows synapse to run with python multipart >=0.0.13 without any tricky fallback.
There is a conflict between 2 packages called multipart, so this should be modified.
@renaudallard renaudallard requested a review from a team as a code owner November 14, 2024 14:59
@CLAassistant
Copy link

CLAassistant commented Nov 14, 2024

CLA assistant check
All committers have signed the CLA.

@reivilibre
Copy link
Contributor

Requires Kludex/python-multipart#166 (0.0.13)

We support >=0.0.9:

synapse/pyproject.toml

Lines 228 to 229 in e80dad5

# This is used for parsing multipart responses
python-multipart = ">=0.0.9"

We either need to bump this version up (if it's OK with distro packagers) or support both versions for a while.

Given 0.0.13 was released on Oct 20, it seems a bit hard to ask distro packagers to update this dependency already, so I think supporting both import names would be prudent for now

@renaudallard
Copy link
Contributor Author

Yes, perhaps it's safer to make a conditional import for the time being.

@arkamar
Copy link
Contributor

arkamar commented Nov 15, 2024

Also, please, be aware of this twisted/treq#399, which basically ignited the python-multipart import name change. We already have to patch it locally in gentoo https://github.com/gentoo/gentoo/blob/cf6eec4024656661b7bf28da9576a50b0b579ff6/net-im/synapse/synapse-1.117.0-r1.ebuild#L182-L184 to deal with import collision for direct and indirect dependencies.

@renaudallard
Copy link
Contributor Author

Also, please, be aware of this twisted/treq#399, which basically ignited the python-multipart import name change. We already have to patch it locally in gentoo https://github.com/gentoo/gentoo/blob/cf6eec4024656661b7bf28da9576a50b0b579ff6/net-im/synapse/synapse-1.117.0-r1.ebuild#L182-L184 to deal with import collision for direct and indirect dependencies.

This is exactly because of that conflict that I pushed the change in the first place. I am not completely sure how to solve the issue the cleanest possible way, but this clearly needs to be done upstream and not in package/ports systems.
On OpenBSD, www/py-multipart is already at 0.0.17 (in -current), but net/synapse package has not been patched in ports and still uses the old import name, but this is going to give issues at some point.

@erikjohnston erikjohnston linked an issue Nov 18, 2024 that may be closed by this pull request
@reivilibre reivilibre changed the title Update client.py Support both import names of PyPI package python-multipart. Nov 19, 2024
@reivilibre
Copy link
Contributor

I added some changes to support both import names, supporting both <0.0.13 using the old name (for distro packagers who are still stuck on the old version, if any), and >=0.0.13 with the new name (to help avoid the name conflict).

@reivilibre reivilibre merged commit 8291aa8 into element-hq:develop Nov 20, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure when downloading any remote media
5 participants