syncthing: Add discovery/relay servers, improve build #34831

andrew-d · 2018-02-11T08:58:27Z

Motivation for this change

I wanted to be able to run the Syncthing discovery / relay servers on my own infrastructure. In the process, I also fixed the silly $GOPATH check that the Syncthing build does (see here), and included man pages in the package.

cc @pshendry @jokogr @peterhoeg (existing maintainers of this package)

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
Fits CONTRIBUTING.md.

peterhoeg · 2018-02-12T01:38:34Z

I'm not sure if the use of multiple outputs to do this is the right way. The "proper" way IMHO would be to create separate derivations (and an additional "cli" derivation for the stcli tool).

andrew-d · 2018-02-12T01:46:06Z

Sure, happy to switch to separate derivations if you'd prefer - mind pointing me to an example of what this would look like (e.g. a package that does this)?

andrew-d · 2018-02-12T01:47:43Z

(I was also considering just not bothering splitting things up and building everything in the main package, since the build is only a couple seconds slower here - thoughts on that approach?)

peterhoeg · 2018-02-12T14:34:20Z

mind pointing me to an example of what this would look like (e.g. a package that does this)?

Here is one where a shared source is used by multiple derivations: #34611

building everything in the main package

From my point of view - "it depends". The go binaries are pretty big so the nice thing to do is to split it up and people can pull in just what they need.

andrew-d · 2018-02-15T02:28:34Z

@peterhoeg - Apologies for the wait on this one, but I just pushed a commit that splits up things into a common core, and different packages for syncthing-relay and syncthing-discovery. Thoughts on this approach?

peterhoeg · 2018-02-15T02:55:05Z

Nothing to apologize for - we all get busy with other things.

It makes sense to split them up if they are likely to be used independently.

As for the approach using common.nix - there is technically nothing wrong with it, it just isn't how most of the other packages are done. Have a look at https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/compilers/cudatoolkit/default.nix as an example of using a function.

andrew-d · 2018-02-15T04:13:50Z

Ah, that's helpful to see - thanks! Just pushed another commit that uses a common function in a single file. The diff without whitespace might be helpful.

peterhoeg · 2018-02-15T06:21:02Z

pkgs/applications/networking/syncthing/default.nix

-  };
+    stdenv.mkDerivation rec {
+      version = "0.14.44";
+      name = "${args.name}-${version}";


You can just use name instead of args.name.

I wasn't able to do this, since I got an infinite recursion error (presumably with the value of the name attribute itself); instead, I renamed the parameter.

peterhoeg · 2018-02-15T06:21:26Z

pkgs/applications/networking/syncthing/default.nix

+        sha256 = "1gdkx6lbzmdz2hqc9slbq41rwgkxmdisnj0iywx4mppmc2b4v6wh";
+      };
+
+      buildInputs = [ go removeReferencesTo ];


removeReferencesTo should be in nativeBuildInputs.

peterhoeg · 2018-02-15T06:22:08Z

pkgs/applications/networking/syncthing/default.nix

+        env GOPATH= go run build.go -no-upgrade -version v${version} build ${target}
+      '';
+
+      inherit (args) installPhase;


You're passing in installPhase so you can do inherit installPhase.

peterhoeg · 2018-02-15T06:22:28Z

pkgs/applications/networking/syncthing/default.nix

+        homepage = https://www.syncthing.net/;
+        description = "Open Source Continuous File Synchronization";
+        license = licenses.mpl20;
+        maintainers = with maintainers; [ pshendry joko peterhoeg ];


You don't want to add yourself?

I wasn't sure what the etiquette here is - if you're okay with it, I've added myself.

Absolutely! We need all the maintainers we can get.

peterhoeg · 2018-02-15T06:22:56Z

pkgs/applications/networking/syncthing/default.nix


-  installPhase = ''
-    mkdir -p $out/lib/systemd/{system,user}


This should move into the linux specific section below.

peterhoeg · 2018-02-15T06:23:23Z

pkgs/applications/networking/syncthing/default.nix

-    find $out/bin -type f -exec remove-references-to -t ${go} '{}' '+'
-  '';
+    installPhase = ''
+      mkdir -p $out/lib/systemd/system $out/bin


This should also move down.

peterhoeg · 2018-02-15T06:23:50Z

pkgs/top-level/all-packages.nix

+  inherit (callPackages ../applications/networking/syncthing { })
+    syncthing
+    syncthing-discovery
+    syncthing-relay;


Please also create syncthing-cli with stcli inside.

Done! It needed a small patch since it wasn't a build target by default.

peterhoeg · 2018-02-15T06:25:36Z

pkgs/applications/networking/syncthing/default.nix

-  name = "syncthing-${version}";
+let
+  common =
+    args@{ name, target, installPhase }:


You don't need args@

Please join the 2 lines to become:

common = { name, target, installPhase }:

…ackage

andrew-d · 2018-02-15T07:30:21Z

@peterhoeg - Okay, I believe that I've addressed all of your feedback - let me know if I missed anything. During the process, I also noticed a bit of an impurity in the build - the build script sets linker flags based on the current username and host name, so I've added a commit to override those to both be nix.

peterhoeg

We've officially gone into nitpick territory now - thanks for your patience!

peterhoeg · 2018-02-15T07:30:44Z

pkgs/applications/networking/syncthing/default.nix

-    ln -s $(pwd) src/github.com/syncthing/syncthing
-    export GOPATH=$(pwd)
+    installPhase = ''
+      mkdir -p $out/bin


You don't need mkdir here. install -D will take care of it.

peterhoeg · 2018-02-15T07:31:03Z

pkgs/applications/networking/syncthing/default.nix

+    target = "stcli";
+
+    installPhase = ''
+      mkdir -p $out/bin


peterhoeg · 2018-02-15T07:31:12Z

pkgs/applications/networking/syncthing/default.nix

+    target = "stdiscosrv";
+
+    installPhase = ''
+      mkdir -p $out/bin


peterhoeg · 2018-02-15T07:31:20Z

pkgs/applications/networking/syncthing/default.nix

-    find $out/bin -type f -exec remove-references-to -t ${go} '{}' '+'
-  '';
+    installPhase = ''
+      mkdir -p $out/bin


peterhoeg · 2018-02-15T07:32:56Z

pkgs/applications/networking/syncthing/default.nix

-  version = "0.14.44";
-  name = "syncthing-${version}";
+let
+  common = { stname, target, installPhase, patches ? null}:


patches is a list so it should be [] instead of null.

andrew-d · 2018-02-15T07:38:48Z

@peterhoeg - Changes made! 😀

peterhoeg · 2018-02-15T07:45:03Z

current username and host name, so I've added a commit to override those to both be nix.

Nicely spotted!

I had a look at everything in its entirety and noticed that for every derivation, you essentially call what is:

install -Dm755 ${target} $out/bin/${target}

So that can be standardized in the common function and the 2 derivations that do more can instead pass in a postInstall for the other bits. That would rip out a few lines and reduce duplication.

andrew-d · 2018-02-15T07:55:48Z

Ah, yeah, that's a good idea - just pushed a commit that does that 👍

andrew-d · 2018-02-15T08:22:04Z

Awesome! Thank you very much for the reviews 😀

andrew-d added 3 commits February 11, 2018 00:48

syncthing: fix GOPATH checks

45f7f8b

syncthing: Install manpages

4e9114d

syncthing: Also build and install discovery and relay servers

941acb4

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Feb 11, 2018

Review feedback: split into different derivations

d58588d

andrew-d force-pushed the adunham/syncthing branch from 4a9a7ed to d58588d Compare February 15, 2018 02:28

GrahamcOfBorg added the 8.has: package (new) label Feb 15, 2018

Review feedback: keep common config in a single file

c79dd9b

peterhoeg requested changes Feb 15, 2018

View reviewed changes

andrew-d added 2 commits February 14, 2018 23:24

Review feedback: simplify and move things around, add syncthing-cli p…

ee5bab9

…ackage

Make the build more pure by explicitly setting the build username/host

ca5678f

peterhoeg requested changes Feb 15, 2018

View reviewed changes

Review feedback: remove spurious 'mkdir's and fix default argument

5988319

Review feedback: reduce duplication with a postInstall hook

7eaf44e

peterhoeg merged commit 394a881 into NixOS:master Feb 15, 2018

andrew-d deleted the adunham/syncthing branch February 15, 2018 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

syncthing: Add discovery/relay servers, improve build #34831

syncthing: Add discovery/relay servers, improve build #34831

andrew-d commented Feb 11, 2018 •

edited

peterhoeg commented Feb 12, 2018

andrew-d commented Feb 12, 2018

andrew-d commented Feb 12, 2018

peterhoeg commented Feb 12, 2018 •

edited

andrew-d commented Feb 15, 2018

peterhoeg commented Feb 15, 2018

andrew-d commented Feb 15, 2018

peterhoeg Feb 15, 2018

andrew-d Feb 15, 2018

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

andrew-d Feb 15, 2018

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

andrew-d Feb 15, 2018

peterhoeg Feb 15, 2018

andrew-d commented Feb 15, 2018

peterhoeg left a comment •

edited

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

peterhoeg Feb 15, 2018

andrew-d commented Feb 15, 2018

peterhoeg commented Feb 15, 2018

andrew-d commented Feb 15, 2018

andrew-d commented Feb 15, 2018

syncthing: Add discovery/relay servers, improve build #34831

syncthing: Add discovery/relay servers, improve build #34831

Conversation

andrew-d commented Feb 11, 2018 • edited

Motivation for this change

Things done

peterhoeg commented Feb 12, 2018

andrew-d commented Feb 12, 2018

andrew-d commented Feb 12, 2018

peterhoeg commented Feb 12, 2018 • edited

andrew-d commented Feb 15, 2018

peterhoeg commented Feb 15, 2018

andrew-d commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-d commented Feb 15, 2018

peterhoeg left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-d commented Feb 15, 2018

peterhoeg commented Feb 15, 2018

andrew-d commented Feb 15, 2018

andrew-d commented Feb 15, 2018

andrew-d commented Feb 11, 2018 •

edited

peterhoeg commented Feb 12, 2018 •

edited

peterhoeg left a comment •

edited