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

Add systemd-python to optional dependencies #4339

Merged
merged 8 commits into from
Apr 16, 2019

Conversation

silkeh
Copy link
Contributor

@silkeh silkeh commented Jan 1, 2019

Using systemd-python allows for logging to the systemd journal, as is documented in: synapse/contrib/systemd/log_config.yaml.

This PR is based on #4325 (will rebase once merged), and adds the package under systemd.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

@silkeh silkeh force-pushed the add-systemd-python branch from d5cdf46 to 0659104 Compare January 1, 2019 15:20
Using systemd-python allows for logging to the systemd journal,
as is documented in: `synapse/contrib/systemd/log_config.yaml`.

Signed-off-by: Silke Hofstra <[email protected]>
@silkeh silkeh force-pushed the add-systemd-python branch from 0659104 to 5c297d5 Compare January 10, 2019 21:21
@richvdh
Copy link
Member

richvdh commented Jan 11, 2019

This seems reasonable, but the CI is failing with a failure to install systemd-python.

It's not needed to run the unit tests, so I'm wondering if we should change the extras = all config in tox.ini. @hawkowl any thoughts on this?

@hawkowl
Copy link
Contributor

hawkowl commented Mar 26, 2019

@richvdh I think excluding systemd-python from all is probably wise, since you need systemd headers to install it.

@richvdh
Copy link
Member

richvdh commented Mar 26, 2019

@richvdh I think excluding systemd-python from all is probably wise, since you need systemd headers to install it.

Which I think means that you think this PR is incorrect?

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #4339 into develop will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop    #4339      +/-   ##
===========================================
+ Coverage    61.56%   61.57%   +0.01%     
===========================================
  Files          332      332              
  Lines        34270    34271       +1     
  Branches      5650     5651       +1     
===========================================
+ Hits         21097    21103       +6     
+ Misses       11656    11653       -3     
+ Partials      1517     1515       -2

@hawkowl
Copy link
Contributor

hawkowl commented Mar 26, 2019

That change there should do -- the branch just needs merging forward.

for opt in CONDITIONAL_REQUIREMENTS.values():
deps = set(opt) | deps
for name, opts in CONDITIONAL_REQUIREMENTS.items():
if name not in ["systemd"]:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the point of including systemd in CONDITIONAL_REQUIREMENTS in the first place is?

Copy link
Member

Choose a reason for hiding this comment

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

oh wait, now I do. But I don't think changing this will change the behaviour for [all], which seems to be done in setup.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

adfghdfh yes of course it won't :/

@anoadragon453
Copy link
Member

@richvdh @hawkowl What's still needed to change here (other than rebasing the PR)?

@hawkowl hawkowl merged commit a137f4e into matrix-org:develop Apr 16, 2019
@silkeh
Copy link
Contributor Author

silkeh commented Apr 17, 2019

@hawkowl: thanks for picking this up!

@silkeh silkeh deleted the add-systemd-python branch May 26, 2019 10:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants