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: Fix systemd service #52852

Merged
merged 3 commits into from Feb 6, 2019
Merged

Conversation

@jokogr
Copy link
Contributor

@jokogr jokogr commented Dec 25, 2018

Motivation for this change

On some systems I tend to run syncthing only as a user, e.g.:

  services.syncthing = {
    enable = true;
    systemService = false;
  };

I have noticed recently that the syncthing service was missing. It appears that due to a refactoring, the syncthing package has now multiple outputs (bin and out). This PR:

  1. Fixes the executable paths in the systemd service files to the bin output.
  2. Includes both outputs in systemd.packages (bin to have the executables in /nix/store and out to include properly the systemd files - the latter was the reason that the syncthing service was missing)
  3. Stops creating service users if syncthing is not a system service.

I am still unsure whether to include syncthing.bin in systemd.packages, let me know if we can do it in another way, e.g. include it in system.extraDependencies.

Pinging @Mic92 since he refactored the syncthing package to use buildGoPackage in 643fabf.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@@ -120,9 +120,9 @@ in {
allowedUDPPorts = [ 21027 ];
};

systemd.packages = [ pkgs.syncthing ];
systemd.packages = [ pkgs.syncthing.out pkgs.syncthing.bin ];
Copy link
Member

@Infinisil Infinisil Dec 27, 2018

Choose a reason for hiding this comment

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

What's the problem if pkgs.syncthing.bin isn't included here?

Copy link
Contributor Author

@jokogr jokogr Dec 27, 2018

Choose a reason for hiding this comment

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

Fair question, initially it was pkgs.syncthing which meant pkgs.syncthing.bin. This was a problem, since the bin output does not have the systemd service files.

By using pkgs.syncthing.out the reference to the binaries got lost, so I thought to include bin as a systemd.package.

Copy link
Member

@Infinisil Infinisil Dec 27, 2018

Choose a reason for hiding this comment

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

Looked it up, systemd.packages (as I thought) only uses $out/etc/systemd of the packages, it only adds systemd units defined in them:

# Symlink all units provided listed in systemd.packages.
for i in ${toString cfg.packages}; do
for fn in $i/etc/systemd/${type}/* $i/lib/systemd/${type}/*; do

So there won't be any problems with removing that (and pkgs.syncthing.out = pkgs.syncthing)

Copy link
Contributor Author

@jokogr jokogr Dec 27, 2018

Choose a reason for hiding this comment

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

So there won't be any problems with removing that (and pkgs.syncthing.out = pkgs.syncthing)

I am afraid this is not correct, from my tests it is more like pkgs.syncthing.bin = pkgs.syncthing.

My impression is that buildGoPackage in pkgs/development/go-modules/generic/default.nix sets

 outputs = args.outputs or [ "bin" "out" ];

so the first output ("bin") is picked when output is not defined (a.k.a. pkgs.syncthing).

So, we have to explicitly declare the "out" output in the systemd.packages and then again we are missing a reference to the other output, which contains binaries.

Copy link
Member

@Infinisil Infinisil Dec 27, 2018

Choose a reason for hiding this comment

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

Ohh, I didn't check, you're right. pkgs.syncthing.out in systemd.packages should be good then. The binaries aren't needed in systemd.packages though, so that can be removed.

Copy link
Contributor Author

@jokogr jokogr Dec 27, 2018

Choose a reason for hiding this comment

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

ok, good thing I understood out well.

Now, I agree that binaries are not needed in systemd.packages, but if I leave syncthing.bin out, then it is not referred anywhere else, so it is not installed. This is why I kept it.

Other approaches would be:

  1. To include bin in environment.systemPackages.
  2. To include bin in system.extraDependencies as mentioned in motivation.
  3. To move the systemd files to $bin as suggested by @Mic92.

Personally I find 2 the most elegant, but it is mentioned that this option is primarily used by the installation tests. In the end, I will follow @Mic92's suggestion.

Copy link
Member

@Infinisil Infinisil Dec 27, 2018

Choose a reason for hiding this comment

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

In what way does putting it in systemd.packages make it get installed? It shouldn't. If by "install" you mean having it accessible in $PATH, then environment.systemPackages is what you want (and I suspect you do). In NixOS it doesn't make sense to "install" something just to have it be built. Either it's needed (and then you refer to it where you need it and it gets available like this), or it's not needed (so you don't refer to it and it won't be available). There's no notion of installing it just to have it installed.

Copy link
Member

@Mic92 Mic92 Dec 29, 2018

Choose a reason for hiding this comment

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

syncthing provides systemd user units not system-wide ones.

Copy link
Member

@Mic92 Mic92 Dec 29, 2018

Choose a reason for hiding this comment

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

Now you can use pkgs.syncthing again. In future we should get rid of the pointless out output of go packages. We had go source code there before, which also didn't made any sense. However migrating from $bin back to $out is a bigger refactoring challenge.

@Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 27, 2018

Can you move all outputs to $bin instead?

@Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 27, 2018

@GrahamcOfBorg build syncthing

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Feb 3, 2019

status?

@jokogr jokogr force-pushed the f/syncthing-service branch from f6a42b1 to a142eb8 Feb 3, 2019
@jokogr
Copy link
Contributor Author

@jokogr jokogr commented Feb 3, 2019

  • Followed @Mic92's piece of advice and moved the systemd unit files to the bin output.
  • Rebased to current master.
  • Retested the service on my machine, it still works as intended.

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Feb 5, 2019

Oh, can you format your last commit message with the nixos/syncthing: prefix?

@jokogr jokogr force-pushed the f/syncthing-service branch from a142eb8 to 243062a Feb 6, 2019
@jokogr jokogr force-pushed the f/syncthing-service branch from 243062a to 6642f3f Feb 6, 2019
@jokogr
Copy link
Contributor Author

@jokogr jokogr commented Feb 6, 2019

Amended and rebased once again :)

@Infinisil Infinisil merged commit 08870ee into NixOS:master Feb 6, 2019
1 check was pending
@jokogr jokogr deleted the f/syncthing-service branch Feb 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants