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

[WIP/Need Help] Redirect stdio to a file with synctl --daemonize #6169

Closed
wants to merge 4 commits into from

Conversation

SohamG
Copy link
Contributor

@SohamG SohamG commented Oct 4, 2019

Attempt to fix #1539

Changes

  • Added synapse/app/_daemonize.py inspired by daemonize but stripped down.
  • Used the file above to redirect stdio/stderr to a file (default "homserver.out")
  • Added config option for "homeserver.out"
  • Ignored --no-redirect-stdio

Need Help!

  • Still need to properly separate the two files by changing twisted's log config. This looks like a pretty huge task, especially cause I'm not familiar with twisted (I'm in the process of learning twisted)
  • Need to find a better/more elegant way of warning --no-redirect-stdio is ignored, ie the warning should be printed only if the option is used etc
  • Need to finish the license header for _daemonize.py. The file is very similar to the original, I don't know how to copyright/license it.
  • Need to know what else is needed to be done to finish this

Disclaimer

I'm pretty unfamiliar with the codebase and twisted. Hence, there's a good chance this PR is a bugfest. I'd love to learn and make changes though 😃

Pull Request Checklist

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

SohamG added 4 commits October 4, 2019 18:37
…ed said file to config. TODO stop twisted sending stderr/out to logs

Signed-off-by: sohamg <[email protected]>
… to change the log config to separate .out and .log files

Signed-off-by: sohamg <[email protected]>
@richvdh
Copy link
Member

richvdh commented Oct 5, 2019

@SohamG: I've only had a chance to skim the changes, so I may be way off-base here, but my main advice to you would be: change fewer things at once. Smaller PRs are much easier for us to review and for you to get bug-free. So for instance: start with a PR which just replaces daemonize with our own version.

@SohamG
Copy link
Contributor Author

SohamG commented Oct 5, 2019

@richvdh Thanks that really seems like a good idea. I'll get to work!

@SohamG SohamG closed this Oct 5, 2019
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.

2 participants