-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Include extra dependency groups 'systemd' and 'cache_memory' in debian packages #12640
Conversation
--extras all \ | ||
--extras test \ | ||
--extras systemd \ | ||
--extras cache_memory \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point. As it stands, the docker images also don't look to include pympler
:
Line 64 in e5a76ec
RUN /root/.local/bin/poetry export --extras all -o /synapse/requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that dependencies for an experimental config option (cache.track_memory_usage
) are in fact something that we'd like to include in Debian and Docker images, as they would allow people using those distribution methods to actually test the option.
In that case, including them in all
sounds sensible.
I'm not sure if this was clear during the poetry
switch, as they seem to have been purposely omitted during the switch:
Lines 236 to 239 in cc76560
# omitted: | |
# - cache_memory: this is an experimental option | |
# - test: it's useful to have this separate from dev deps in the olddeps job | |
# - systemd: this is a system-based requirement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to push this PR through and get 1.58.1 out as that's preventing some .deb installations from starting, if that sounds sensible? Then we can look at clearing up this situation, and allowing testing of the experimental config option on docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't help but wonder if this is the right fix, or if we should go back to having
all
include, well, all the extras. (Or, most of them.)
By my reading, the first link explicitly excludes systemd
from all
. I can't remember why I didn't include pympler; maybe just an oversight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm othet than the changelog
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Fixes #12631.
The
systemd
andcache_memory
extra package groups contain python dependencies that are necessary for using all available options of the base distribution of Synapse in debian packages.systemd
provides thesystemd-python
package which is required for logging to the systemd journal, whereascache_memory
provides thepympler
package, which is required for thecaches.track_memory_usage
homeserver config option.These dependencies were available before the poetry changeover, where they seem to have gone missing. This PR adds these extras groups back.
Merging to the
release-v1.58
branch with the intention of including this in a v1.58.1 release.I'm not sure if we also need to add the
cache_memory
extras group to the following line?synapse/debian/build_virtualenv
Line 62 in 0d8d595