Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syncthing: expand declarative config to Darwin #6104

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

pitkling
Copy link
Contributor

Description

This PRs extends the recently merged PR #5616, which expanded the Synching config to allow declarative settings under Linux, such that it also works under Darwin.

To simplify the review process, I left split the PR into several commits for now:

  • The first five commits perform some minor refactoring tasks of the module code such that parts of it can be reused in the Darwin specific changes.
  • Commit six implements the actual Darwin specific changes. It does the following:
    • Update the module's syncthing launchd agent to copy the synching key/certificate before starting syncthing, analogously to the systemd service from the above mentioned PR syncthing: expand declarative config #5616.
    • Adds an syncthing-initlaunchd agent (analogously to the systemd service synching-init from the above mentioned PR syncthing: expand declarative config #5616) that updates the configuration files. Since this must be run after the syncthing service started, we use a WatchPath to coordinate both launchd agents.
  • The last commit just fixes the formatting according to HM's format tool.

There might be better ways to coordinate the two launchd agents I'm not aware of. But I have had this running flawlessly for over two months now (on four Darwin and two Linux machines sharing various folders).

Fixes #4049 for Darwin systems.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted (see commit 6; I'll squash the other commits into it after the review process)

Maintainer CC

@karaolidis (pinging as author of the original (linux-specific) PR #5616)
@rycee (maintainer)

Copy link
Member

@rycee rycee left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, looks OK to me! Especially thanks for splitting the change in multiple commits with explanations 🙂

Just added one comment.

Comment on lines +15 to +18
syncthing_dir = if pkgs.stdenv.isDarwin then
"$HOME/Library/Application Support/Syncthing"
else
"\${XDG_STATE_HOME:-$HOME/.local/state}/syncthing";
Copy link
Member

Choose a reason for hiding this comment

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

Quick question. Will the Darwin directory be used even if the XDG_STATE_HOME variable is set?

Copy link
Contributor Author

@pitkling pitkling Dec 1, 2024

Choose a reason for hiding this comment

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

I think so, that's at least how I understood the documentation.

Anyway, I remember that I was also a bit surprised by this when I wrote the module. So just to be sure, this time I took a quick look at the source code (release tag v1.28.0, as used in NixOS 24.11). Indeed, XDG_STATE_HOME is only considered when not on Windows/Darwin and for Darwin the location is hardcoded as $HOME/Library/Application Support/Syncthing.

This makes it possible to reuse the script in launchd setup.
Allows us to reuse the function in different scripts.
Make launchd.agents an explicit attribute set, since we're going to add another agent.
This extends the recently merged PR nix-community#5616, which expanded the Synching config to allow declarative settings under Linux, such that it also works under Darwin.

Changes:
* Update the module's `syncthing` launchd agent to copy the synching key/certificate before starting syncthing, analogously to the systemd service from the above mentioned PR nix-community#5616.
* Adds an `syncthing-init`launchd agent (analogously to the systemd service `synching-init` from the above mentioned PR nix-community#5616) that updates the configuration files. Since this must be run after the syncthing service started, we use a `WatchPath` to coordinate both launchd agents.
@pitkling pitkling force-pushed the syncthing-declarative-macos branch from 2d676db to eaee6ea Compare December 5, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add declarative config options for Syncthing
3 participants