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

Check dependencies on setup in the nicer way. #5989

Merged
merged 10 commits into from
Sep 11, 2019
Merged

Check dependencies on setup in the nicer way. #5989

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f8ca33f
Don't try to import. Use check_dependencies instead.
JorikSchellekens Sep 5, 2019
63bc386
Nicer formatting
JorikSchellekens Sep 5, 2019
6393052
newsfile
JorikSchellekens Sep 5, 2019
eefb810
Missing import
JorikSchellekens Sep 5, 2019
56845ec
lxml
JorikSchellekens Sep 6, 2019
621aa5e
use correct feature
JorikSchellekens Sep 6, 2019
8ed479c
for_feature may be None.
JorikSchellekens Sep 10, 2019
df0b07c
comment on netaddr
JorikSchellekens Sep 10, 2019
1536eb1
Merge remote-tracking branch 'origin/develop' into joriks/config_all_…
JorikSchellekens Sep 10, 2019
a72cced
Clearer conditional.
JorikSchellekens Sep 11, 2019
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/5989.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Clean up dependency checking at setup.
12 changes: 5 additions & 7 deletions synapse/config/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,9 @@

import attr

from ._base import Config, ConfigError
from synapse.python_dependencies import DependencyException, check_requirements

MISSING_SENTRY = """Missing sentry-sdk library. This is required to enable sentry
integration.
"""
from ._base import Config, ConfigError


@attr.s
Expand Down Expand Up @@ -51,9 +49,9 @@ def read_config(self, config, **kwargs):
self.sentry_enabled = "sentry" in config
if self.sentry_enabled:
try:
import sentry_sdk # noqa F401
except ImportError:
raise ConfigError(MISSING_SENTRY)
check_requirements("sentry")
except DependencyException as e:
raise ConfigError(e.message)

self.sentry_dsn = config["sentry"].get("dsn")
if not self.sentry_dsn:
Expand Down
27 changes: 7 additions & 20 deletions synapse/config/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import os
from collections import namedtuple

from synapse.python_dependencies import DependencyException, check_requirements
from synapse.util.module_loader import load_module

from ._base import Config, ConfigError
Expand All @@ -34,17 +35,6 @@
# method: %(method)s
"""

MISSING_NETADDR = "Missing netaddr library. This is required for URL preview API."

MISSING_LXML = """Missing lxml library. This is required for URL preview API.

Install by running:
pip install lxml

Requires libxslt1-dev system package.
"""


ThumbnailRequirement = namedtuple(
"ThumbnailRequirement", ["width", "height", "method", "media_type"]
)
Expand Down Expand Up @@ -171,16 +161,10 @@ def read_config(self, config, **kwargs):
self.url_preview_enabled = config.get("url_preview_enabled", False)
if self.url_preview_enabled:
try:
import lxml

lxml # To stop unused lint.
except ImportError:
raise ConfigError(MISSING_LXML)
check_requirements("url_preview")

try:
from netaddr import IPSet
except ImportError:
raise ConfigError(MISSING_NETADDR)
except DependencyException as e:
raise ConfigError(e.message)

if "url_preview_ip_range_blacklist" not in config:
raise ConfigError(
Expand All @@ -189,6 +173,9 @@ def read_config(self, config, **kwargs):
"to work"
)

# netaddr is a dependency for url_preview
from netaddr import IPSet
richvdh marked this conversation as resolved.
Show resolved Hide resolved

self.url_preview_ip_range_blacklist = IPSet(
config["url_preview_ip_range_blacklist"]
)
Expand Down
7 changes: 6 additions & 1 deletion synapse/python_dependencies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,12 @@ def check_requirements(for_feature=None):
)
except DistributionNotFound:
deps_needed.append(dependency)
errors.append("Needed %s but it was not installed" % (dependency,))
errors.append(
"Needed %s for the '%s' feature but it was not installed"
% (dependency, for_feature)
Copy link
Member

Choose a reason for hiding this comment

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

for_feature can be None, so this error would be nonsense

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've made it predicated on for_feature with the old string as a backup. I still think it's valuable to include the feature name if it's available.

Copy link
Member

Choose a reason for hiding this comment

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

non-blocking suggestion: I think this would be clearer as:

if for_feature:
    errors.append(something)
else:
    errors.append(something_else)

if for_feature
else "Needed %s but it was not installed" % (dependency,)
)

if not for_feature:
# Check the optional dependencies are up to date. We allow them to not be
Expand Down