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

Fix dependency resolving #8367

Closed
wants to merge 2 commits into from
Closed

Fix dependency resolving #8367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3187d2f
Fix dependency resolving
ShadowJonathan Sep 21, 2020
4c9b543
Add new snippet, make linter happy
ShadowJonathan Sep 21, 2020
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/8367.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes the way synapse distinguishes between runtime and developer dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

We should use the same newsfragment as the PR that regressed this so that they'll show up together in the changelog.

29 changes: 20 additions & 9 deletions synapse/python_dependencies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# limitations under the License.

import logging
from typing import List, Set
from typing import Set

from pkg_resources import (
DistributionNotFound,
Expand All @@ -25,9 +25,6 @@
get_provider,
)

logger = logging.getLogger(__name__)


# REQUIREMENTS is a simple list of requirement specifiers[1], and must be
# installed. It is passed to setup() as install_requires in setup.py.
#
Expand Down Expand Up @@ -116,6 +113,12 @@
if name not in ["systemd", "lint"]:
ALL_OPTIONAL_REQUIREMENTS = set(optional_deps) | ALL_OPTIONAL_REQUIREMENTS

# We exclude lint and test as those are developer requirements
ALL_RUNTIME_REQUIREMENT_COMPONENTS = set(CONDITIONAL_REQUIREMENTS.keys()) - {
"lint",
"test",
}

Comment on lines +116 to +121
Copy link
Contributor Author

@ShadowJonathan ShadowJonathan Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@richvdh This change filters out all developer dependencies from all conditional dependencies, by only excluding lint and test, and then flattening the resulting dependencies into a single set.

I could change this to be only a set with {"lint", "test"}, as this piece is only used in one part later on.


def list_requirements():
return list(set(REQUIREMENTS) | ALL_OPTIONAL_REQUIREMENTS)
Expand All @@ -139,12 +142,12 @@ def dependencies(self):
yield "'" + i + "'"


def check_requirements(for_feature=None):
def check_requirements(for_feature=None, dev=False):
deps_needed = []
errors = []

if for_feature:
reqs = CONDITIONAL_REQUIREMENTS[for_feature]
reqs = CONDITIONAL_REQUIREMENTS.get(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.

Why this change? It seems unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if for_feature is not a key in CONDITIONAL_REQUIREMENTS, it'll raise a KeyError, i think the right logic then is to return a default empty list

This is so that when check_requirements is called with a dependency list that does not exist anymore, any time in the future, it should not return a cryptic error, should i revert this?

else:
reqs = REQUIREMENTS

Expand All @@ -171,10 +174,16 @@ def check_requirements(for_feature=None):
else:
errors.append("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
if for_feature is None:
# Check if the optional runtime dependencies are up to date. We allow them to not be
# installed.
OPTS = sum(CONDITIONAL_REQUIREMENTS.values(), []) # type: List[str]
OPTS = set().union(
*(
set(reqs)
for key, reqs in CONDITIONAL_REQUIREMENTS.items()
if key in ALL_RUNTIME_REQUIREMENT_COMPONENTS or dev
)
) # type: Set[str]
Comment on lines +177 to +186
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richvdh this is two compounded changes;

  • Change OPTS to be a flattened set, to deduplicate any elements, and have potential faster iteration.
  • if dev == False, only check default (runtime) dependencies, if dev == True, also check development dependencies, this is useful as an addition in __main__ down below, which does dev=True, making an invocation of the file (python -m synapse.python_dependencies) automatically also check those development dependencies.


for dependency in OPTS:
try:
Expand Down Expand Up @@ -221,3 +230,5 @@ def _check_requirement(dependency_string):
import sys

sys.stdout.writelines(req + "\n" for req in list_requirements())

check_requirements(dev=True)
Copy link
Member

Choose a reason for hiding this comment

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

Does something run this module at some point? When is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, currently nothing runs the module at this point, but this way, python -m synapse.python_dependencies can be suggested by someone from the synapse team to be ran by developers to check if their dependencies are alright, or if one of them is the wrong version.