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

Don't require setuptools_rust at runtime #13952

Merged
merged 3 commits into from
Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13952.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug introduced in v1.68.0 where Synapse would require `setuptools_rust` at runtime, even though the package is only required at build time.
13 changes: 12 additions & 1 deletion synapse/util/check_dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,17 @@ def _is_dev_dependency(req: Requirement) -> bool:
)


def _should_ignore_runtime_requirement(req: Requirement) -> bool:
# This is a build-time dependency. Irritatingly, `poetry build` ignores the
# requirements listed in the [build-system] section of pyproject.toml, so in order
Comment on lines +70 to +71
Copy link
Member

Choose a reason for hiding this comment

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

Should we link to a poetry bug here?

Copy link
Member

Choose a reason for hiding this comment

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

python-poetry/poetry#6154 I guess would be this and is linked to someplace else, so probably OK not to.

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 wasn't 100% sure if this is considered a bug by poetry's authors, but the issue is a good point of reference---I'll gladly add it

# to support `poetry install --no-dev` we have to mark it as a runtime dependency.
# Workaround this by ignoring it here. (It might be slightly cleaner to put
# `setuptools_rust` in a `build` extra or similar, but . But for now I'
Copy link
Contributor

Choose a reason for hiding this comment

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

some words have gotten lost here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Guh, I think this is a sign I need to eat. Thanks, will amend

if req.name == "setuptools_rust":
return True
return False


class Dependency(NamedTuple):
requirement: Requirement
must_be_installed: bool
Expand All @@ -77,7 +88,7 @@ def _generic_dependencies() -> Iterable[Dependency]:
assert requirements is not None
for raw_requirement in requirements:
req = Requirement(raw_requirement)
if _is_dev_dependency(req):
if _is_dev_dependency(req) or _should_ignore_runtime_requirement(req):
continue

# https://packaging.pypa.io/en/latest/markers.html#usage notes that
Expand Down
20 changes: 18 additions & 2 deletions tests/util/test_check_dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ class TestDependencyChecker(TestCase):
def mock_installed_package(
self, distribution: Optional[DummyDistribution]
) -> Generator[None, None, None]:
"""Pretend that looking up any distribution yields the given `distribution`."""
"""Pretend that looking up any package yields the given `distribution`.

If `distribution = None`, we pretend that the package is not installed.
"""

def mock_distribution(name: str):
if distribution is None:
Expand Down Expand Up @@ -81,7 +84,7 @@ def test_version_reported_as_none(self) -> None:
self.assertRaises(DependencyException, check_requirements)

def test_checks_ignore_dev_dependencies(self) -> None:
"""Bot generic and per-extra checks should ignore dev dependencies."""
"""Both generic and per-extra checks should ignore dev dependencies."""
with patch(
"synapse.util.check_dependencies.metadata.requires",
return_value=["dummypkg >= 1; extra == 'mypy'"],
Expand Down Expand Up @@ -142,3 +145,16 @@ def test_release_candidates_satisfy_dependency(self) -> None:
with self.mock_installed_package(new_release_candidate):
# should not raise
check_requirements()

def test_setuptools_rust_ignored(self) -> None:
"""Test a workaround for a `poetry build` problem. Reproduces #13926."""
with patch(
"synapse.util.check_dependencies.metadata.requires",
return_value=["setuptools_rust >= 1.3"],
):
with self.mock_installed_package(None):
# should not raise, even if setuptools_rust is not installed
check_requirements()
with self.mock_installed_package(old):
# We also ignore old versions of setuptools_rust
check_requirements()