-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
nixos/peertube: init service #119110
nixos/peertube: init service #119110
Conversation
port = lib.mkOption { | ||
type = lib.types.port; | ||
default = 6379; | ||
description = "Redis port."; |
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.
Why not socket?
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.
The socket is specified by the parameter redis.enableUnixSocket
. Used by default.
5ce998e
to
248f8d4
Compare
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.
Great work but I'm a perfectionist :D
redisActuallyCreateLocally = cfg.redis.createLocally && cfg.redis.host == "127.0.0.1"; | ||
databaseActuallyCreateLocally = cfg.database.createLocally && cfg.database.host == "/run/postgresql"; |
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.
Is there a reason to verify this? Is there a case where enabling createLocally is useful without actually using the database? Also this wouldn't work if you would like to use unix socket for redis locally but still want to get the database created.
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 tested with different combinations of parameters - it works correctly in all cases.
settingsFormat = pkgs.formats.yaml {}; | ||
configFile = pkgs.writeText "production.yaml" '' | ||
listen: | ||
port: ${toString cfg.listenHttp} |
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 don't know if you have finished this PR yet but do you want to add a settings
option (with settingsFormat
)? Then these options may be set in there. But I think this is not easy if you also want to access options from there so maybe do this after everything else.
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've tried creating a complete config using settingsFormat = pkgs.formats.yaml {};
As a result, peertube gave a configuration error.
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.
Do you need any help with this? Using settings
instead of extraConfig
would be nice.
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.
With settingsFormat = pkgs.formats.yaml {};
files generated with this format:
{
"database": {
"hostname": "localhost",
"port": "5432",
"name": "peertube_test",
"username": "peertube_test"
},
"listen": {
"hostname": "127.0.0.1",
"port": "9000"
},
Needed this format:
database:
hostname: 'localhost'
port: 5432
name: 'peertube_test'
username: 'peertube_test'
listen:
hostname: 'localhost'
port: 9000
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.
YAML is a superset of JSON so this should be fine although it looks pretty odd. If it doesn't work then there is probably a bug in peertube
58c5f19
to
bf7fcd7
Compare
bf7fcd7
to
0ea09c8
Compare
I need to go to bed now but master...mohe2015:my-add-peertube-service contains some changes that you may want to incorporate. |
0ea09c8
to
9475184
Compare
@Izorkin I added some more commits to master...mohe2015:my-add-peertube-service maybe you find some of them useful. |
@mohe2015 With
For example, if someone changed the permissions on a directory or file. Are these options to be disabled by default?:
|
@mohe2015 with this variant configuration:
Not working connections to redis with socket. Needed clean hostname. |
What about running ExecStartPre as root (with the install --directory --mode=0700 --owner=${cfg.user} --group=${cfg.group} /var/lib/peertube/config
I think this may be useful so it doesn't create databases / starts services unexpectedly but I don't know. |
Better not to give root rights.
And needed return:
|
Ok fine, keep using tmpfiles.
I can clean this up but I think I have the part you mean in |
@mohe2015 update PR without add assertions. |
9f75a1e
to
73d8f30
Compare
@mohe2015 it seems that this function is not working.
|
73d8f30
to
9fb6dc2
Compare
I added another assertion I didn't test yet: Izorkin@d4dcbb7 |
Then I think it could be simplified to Izorkin@15fcc67 but I'm not sure. |
If somebody sets createLocally and a foreign host I don't think that should be checked. |
9fb6dc2
to
a9b2817
Compare
I think it's better to leave the current variant |
Ok, I wanted to test running the service once, but there is something on staging-next that needs attention. |
Thanks! |
The package won't build, apparently. (on Hydra and locally)
Note that as-merged the package specifies only x86_64-linux in |
That's super weird because I built from the latest pushed version I'm pretty sure. Unfortunately I can not look into it right now - hopefully later today. I hope it doesn't block the channel (just build failure not evaluation failure) so we don't need to revert if will be likely fixed today |
This certainly won't block channels, we have many packages failing all the time. Actually I have 2/2 successful builds on master locally (EDIT: + 1/1 on Hydra), so perhaps something in |
Yeah, master builds peertube without a problem |
If one of the maintainers has the bandwidth, it would be good to test building peertube with the next staging-next |
The minimum should be {
services.peertube = {
enable = true;
localDomain = "peertube.localhost";
serviceEnvironmentFile = "/etc/nixos/secrets/peertube-root"; # don't do this
database = {
createLocally = true;
};
redis = {
createLocally = true;
};
};
networking.extraHosts =
''
127.0.0.1 peertube.localhost
'';
} (at least it works for me |
|
||
systemd.services.peertube = { | ||
description = "PeerTube daemon"; | ||
after = [ "network.target" ] |
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.
According to https://www.freedesktop.org/software/systemd/man/systemd.unit.html and my testing we need to add wants
here in a followup PR.
Edit: I mean the lines below not the network.target
Can reproduce... Will try to investigate. wait I got the following - wasn't there something about this with Rust? (I think I saw what I mean on the rust repo and not here)
|
Okay now I get the error that's on hydra. |
I was getting (those?) EPIPE errors locally. |
The error on staging has to do with the coreutils upgrade. |
Also if I'm on linux? Edit: seems like yes Only 389 more to go... |
I only have a few dozen to go, on a 32-threaded box. |
It truly seems that the issue disappears with reverting the coreutils update #139541. (EDIT: 2/2 attempts succeeded) |
Still building ([4/366/389]) - do you have an idea what tool / what is the problem? |
I don't have a clue. |
Can somebody confirm /nix/store/1danz1kww3bg64439bx7p7sx63am8na0-peertube-3.4.1 is the derivation? Because that one built for me. from 3db3126 |
Yes, I built that one a few times. The content hash is always different, though.
|
Okay, so reverting works. Interesting. |
Just for the lulz I will try to bisect (will update this - tell me if I could improve something): On top of https://github.com/mohe2015/nixpkgs/tree/staging-coreutils-regression:
Edit: I'm stupid I need to wrap dependencies not peertube itself. |
Well it didn't work... Edit: Maybe because replaceDependency is not recursive? Or I missed a dependency? I should not have trusted that v9.0 does actually come out bad with that approach because then I would've spottet this way earlier. |
Anyway, it seems very likely that the issue is in coreutils and not in anything around peertube. |
@happysalada hey, i just wanted to say thank you (and all participating maintainers) for your effort in packaging peertube and even putting it into a service! |
And in turn I give all my ❤️ to @Izorkin @mohe2015 @stevenroose @matthiasbeyer @immae for doing all the heavy lifting! This was a lot of work! Thank you again! |
Motivation for this change
Add only peertube service without package.
Updating service configuration from this PR - #106492
cc @stevenroose @mohe2015 @matthiasbeyer @aanderse @Mic92
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)