-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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: Add discovery/relay servers, improve build #34831
Conversation
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 |
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)? |
(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?) |
Here is one where a shared source is used by multiple derivations: #34611
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. |
4a9a7ed
to
d58588d
Compare
@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 |
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 |
Ah, that's helpful to see - thanks! Just pushed another commit that uses a |
}; | ||
stdenv.mkDerivation rec { | ||
version = "0.14.44"; | ||
name = "${args.name}-${version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just use name
instead of args.name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
sha256 = "1gdkx6lbzmdz2hqc9slbq41rwgkxmdisnj0iywx4mppmc2b4v6wh"; | ||
}; | ||
|
||
buildInputs = [ go removeReferencesTo ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeReferencesTo
should be in nativeBuildInputs
.
env GOPATH= go run build.go -no-upgrade -version v${version} build ${target} | ||
''; | ||
|
||
inherit (args) installPhase; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're passing in installPhase
so you can do inherit installPhase
.
homepage = https://www.syncthing.net/; | ||
description = "Open Source Continuous File Synchronization"; | ||
license = licenses.mpl20; | ||
maintainers = with maintainers; [ pshendry joko peterhoeg ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want to add yourself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure what the etiquette here is - if you're okay with it, I've added myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely! We need all the maintainers we can get.
|
||
installPhase = '' | ||
mkdir -p $out/lib/systemd/{system,user} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should move into the linux specific section below.
find $out/bin -type f -exec remove-references-to -t ${go} '{}' '+' | ||
''; | ||
installPhase = '' | ||
mkdir -p $out/lib/systemd/system $out/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also move down.
pkgs/top-level/all-packages.nix
Outdated
inherit (callPackages ../applications/networking/syncthing { }) | ||
syncthing | ||
syncthing-discovery | ||
syncthing-relay; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also create syncthing-cli
with stcli
inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! It needed a small patch since it wasn't a build target by default.
name = "syncthing-${version}"; | ||
let | ||
common = | ||
args@{ name, target, installPhase }: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You don't need
args@
- Please join the 2 lines to become:
common = { name, target, installPhase }:
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've officially gone into nitpick territory now - thanks for your patience!
ln -s $(pwd) src/github.com/syncthing/syncthing | ||
export GOPATH=$(pwd) | ||
installPhase = '' | ||
mkdir -p $out/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need mkdir
here. install -D
will take care of it.
target = "stcli"; | ||
|
||
installPhase = '' | ||
mkdir -p $out/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or here
target = "stdiscosrv"; | ||
|
||
installPhase = '' | ||
mkdir -p $out/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or here
find $out/bin -type f -exec remove-references-to -t ${go} '{}' '+' | ||
''; | ||
installPhase = '' | ||
mkdir -p $out/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or here
version = "0.14.44"; | ||
name = "syncthing-${version}"; | ||
let | ||
common = { stname, target, installPhase, patches ? null}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patches
is a list so it should be []
instead of null
.
@peterhoeg - Changes made! 😀 |
Nicely spotted! I had a look at everything in its entirety and noticed that for every derivation, you essentially call what is:
So that can be standardized in the |
Ah, yeah, that's a good idea - just pushed a commit that does that 👍 |
Awesome! Thank you very much for the reviews 😀 |
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
build-use-sandbox
innix.conf
on non-NixOS)Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)nix-shell -p nox --run "nox-review wip"
./result/bin/
)