Conversation
|
I did changes recently (like, yesterday :D ) to this package to make it work in 19.09 (still in progress for further), but I did not push it yet, it might be good to wait a little so that I can push the changes? |
pkgs/servers/peertube/default.nix
Outdated
| }; | ||
|
|
||
| patchedPackages = stdenv.mkDerivation (fetchedGithub ./peertube.json // rec { | ||
| patches = if ldap then [ ./ldap.patch ././yarn_fix_bluebird_ldap.patch ] else [ ./yarn_fix_bluebird.patch ]; |
There was a problem hiding this comment.
This hack is a personnal one (I added ldap support to peertube because I needed it), it may not have its place here in a distribution package? (the patch was refused upstream)
|
I forgot: thanks a lot for taking the steps to include it in the nixpkgs, it’s a task I delayed for quite some time to avoid overloading the already-full list of PRs... |
|
Actually the diff for peertube was quite simple: The first chunk you already noticed it in your PR description, it corresponds to the fact that in 19.03 the default node version was 8.X while it is 10.X in later nixpkgs channels. There might be a better way to handle it, but at least it will fail during build. The second chunk about bcrypt is important and will fail only at runtime: the node api version (57 -> 64) changed between 8 and 10, but it won’t appear at build time. We should definitely add an assert somewhere to make sure we don’t miss it... NB: you may add me as a maintainer if you wish, I’m okay with keeping the maintenance |
|
Hm, any ideas why ofborg fails here? It builds fine on my machine! |
I’m not in the maintainer list yet :) |
|
@immae you're okay with me adding this to the PR: diff --git a/maintainers/maintainer-list.nix b/maintainers/maintainer-list.nix
index f0a00b4e39e..667819252db 100644
--- a/maintainers/maintainer-list.nix
+++ b/maintainers/maintainer-list.nix
@@ -3184,6 +3184,12 @@
githubId = 4085046;
name = "Imuli";
};
+ immae = {
+ email = REDACTED;
+ github = "immae";
+ githubId = 510202;
+ name = "Immae";
+ };
infinisil = {
email = REDACTED;
github = "infinisil";(I redacted the mails to not leak stuff here) |
|
It’s fine for me, thanks ! Please use |
|
Note: I will finish my upgrade of my whole system to nixos-unstable tonight, including the peertube service. That will permit me to make sure that it runs smoothly in this version (even though I see no reason why not), and I can then confirm that everything is in order |
|
Awesome 🎉 |
0a2552c to
5062e79
Compare
|
So I had to make a minor change to follow "recent" standards (namely deprecation of types.loaOf): - users.users = lib.optionalAttrs (cfg.user == name) (lib.singleton {
- inherit name;
- inherit uid;
- group = cfg.group;
- description = "Peertube user";
- home = cfg.dataDir;
- useDefaultShell = true;
- });
- users.groups = lib.optionalAttrs (cfg.group == name) (lib.singleton {
- inherit name;
- inherit gid;
- });
+ users.users = lib.optionalAttrs (cfg.user == name) {
+ "${name}" = {
+ inherit uid;
+ group = cfg.group;
+ description = "Peertube user";
+ home = cfg.dataDir;
+ useDefaultShell = true;
+ };
+ };
+ users.groups = lib.optionalAttrs (cfg.group == name) {
+ "${name}" = {
+ inherit gid;
+ };
+ };Apart from that, this PR should provide a working peertube instance 😄 |
pkgs/servers/peertube/default.nix
Outdated
| src = fetchFromGitHub json.github; | ||
| }; | ||
|
|
||
| yarn2nixPackage = let |
There was a problem hiding this comment.
I don't think this is good. Instead the tool should be part of nixpkgs.
Another problem is: If the source is required to evaluate the derivation (not build it, just evaluate) then that means that nix needs to download the source even to just build the .drv file
An idea would be to add the yarn.lock to nixpkgs aswell as the tool and then pull the yarn.lock from nixpkgs, so evaluation does not depend on external dependecies
There was a problem hiding this comment.
Inspiration can be taken from https://github.com/mkg20001/nix-node-package/blob/master/nix/default.nix which solves that problem and also the one where the nodeHeaders are pulled separately
There was a problem hiding this comment.
@immae care to contribute this? I'm new to the node packaging infra and thus do not entirely understand how to approach this.
There was a problem hiding this comment.
There is the yarn2nix-moretea which contains exactly what we need and didn’t exist at the time when I wrote this package, I’ll try to make a use of it and give feedback (there seem to be some kind of issues with yarn packages coming from github)
There was a problem hiding this comment.
So, this cannot work, because one of the dependencies ( https://github.com/Chocobozzz/jsonld-signatures/ ) has been removed by his author. To deal with that, I will need to upgrad epeertube to latest version, which may take a little longer than expected (but it will arrive at some point). In my case the dependency is already in my store, but there is no way that it is the case in the hydra yet.
I’ll keep this thread updated but it may take a few more days.
There was a problem hiding this comment.
Shouldn't you be able to use mkYarnPackage here?
|
Would we benefit from something like this: matthiasbeyer@7e72019 ??? |
The idea is good, but I think it’s too much. You should have a look at this nice RFC: https://github.com/NixOS/rfcs/blob/e719102ace17c2b4a8b98efe0f08e344a7713dec/rfcs/0042-config-option.md to find a balance between "too much options" and "nothing configurable" |
|
So, as promised @matthiasbeyer , I finished to repackage peertube to latest version (v2.1.1). You can find it there: I addressed the issue of the yarn2nix package as pointed by @mkg20001 . I also greatly improved (in my opinion) the packaging (also thanks to upstream/yarn2nix improvements I admit) If I am remaining as maintainer of the package, I would like to keep the ldap / sendmail flags for the package (depending on wether it is in accordance with nixpkgs guidelines of course). The default version will build vanillia peertube, while ldap or sendmail flag would add the patches implementing the named feature. Note that they will be supported upstream at some point, so it’s just a way to have the feature before they’re implemented upstream. About the "light" flag I don’t really know what to do. Peertube comes with a bunch of translations, which will be all built by default, but it will take hours to build them. The light=true version will build only English, and the light="fr-FR" will build English + the given language (The peertube build script only support French, but I patched it to accept anything). Same here, I don’t really know what is the best option to build / according to the guidelines. |
|
Thanks. We'll see how I can update this PR with your changes. It might get complicated because I don't know what changed (because I don't know the git commit when I started my efforts...) |
Basically it’s a completely different layout, so you should take the whole directory as is. The only thing you’ll need to re-add is the mylibs.fetchedGitHub function, the rest is now "standard" nixpkgs. (I don’t know if github permits to do it, but I may also push to your branch if you give me the rights) |
|
If you replace the content of pkgs/servers/peertube/ in your PR with my whole pkgs/webapps/peertube folder, and replace Then it should work as intended :) |
1cfeebe to
7d40d98
Compare
|
Thanks for the update @matthiasbeyer I made one more small change recently: can you add a line just after the similar one in the |
|
Also, can you specify how the "light" fix fails? for me it works correctly, I may have missed something... |
the |
Signed-off-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
f1124d5 to
d96ed86
Compare
|
Reverting to v2.1.1 and changing to nodejs version 12 fixes the SCSS issue for me. This also requires changing Also, is the server-yarn-packages.nix generated from the yarn.lock in the root of PeerTube? I'm having difficulty regenerating it to update PeerTube. I think yarn-packages.nix can be removed. The only other changes are to enable Postgres and Redis and create the Postgres database. It runs, but I haven't tested much more than that. |
|
Can you send me patches for what you did? |
|
I also regenerated client-yarn-packages.nix, but it was only whitespace changes. Edit: added back Details |
|
Please give me an url to pull from or send patchmails. |
|
https://github.com/samlich/nixpkgs/tree/peertube And it's not really organized as I was going to work on updating to 2.4.0 and maybe add declarative plugins. If the plugins work fine imperatively though, I'll probably hold off on that part. |
Signed-off-by: Matthias Beyer <[email protected]> Suggested-by: samlich <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]> Suggested-by: samlich <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]> Suggested-by: samlich <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]> Suggested-by: samlich <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]> Suggested-by: samlich <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]> Suggested-by: samlich <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]> Suggested-by: samlich <[email protected]>
|
Thanks, @samlich for the patches. Please commit properly next time, so I can simply cherry-pick your patches 😉 ! |
stevenroose
left a comment
There was a problem hiding this comment.
I don't really like that you patched PeerTube itself (with ldap etc). Wouldn't it be a better idea to release this without and PR them to PeerTube itself and wait for it to get accepted there and then update here to have support for it?
| }; | ||
|
|
||
| # Output variables | ||
| systemdStateDirectory = lib.mkOption { |
There was a problem hiding this comment.
What is this for? I'm no Nix expert but the assert down seems to make this break if the dataDir is set to something outside of /var/lib.
There was a problem hiding this comment.
Can this not be hardcoded to /var/run/${name}?
| }; | ||
|
|
||
| config = lib.mkIf cfg.enable { | ||
| users.users = lib.optionalAttrs (cfg.user == name) { |
There was a problem hiding this comment.
Hmm is this a common pattern? To only create the user if it's the same name as the package?
| description = "Peertube user"; | ||
| home = cfg.dataDir; | ||
| useDefaultShell = true; | ||
| # todo: fix this. needed for postgres authentication |
There was a problem hiding this comment.
Any idea no how to fix this?
| set -e | ||
|
|
||
| if ! [ -e "${cfg.dataDir}/.first_run" ]; then | ||
| set -v |
There was a problem hiding this comment.
Could you indent those lines inside the if block?
|
Any updates on this? I had a simpler pkg based on yarn2nix, but it's definitely not working due to some broken yarn dependencies. Also, I read this was for nixos 19.09 and I see it's PeerTube 2.1.1 while 2.4.0 is already out. I can try update a bit of this, but I'm totally lost in the |
|
Feel free to take any patches from this, but I am no longer interested in this javascript hell. Sorry. |
|
If you’re interested in reviving this PR, I packaged the last version of peertube (3.0.1, with "live streaming"). Notes:
|
This is a starting point for closing #46987. It is a complete rip-off of the nur package from @immae !
I still get an error though:
but its better than nothing, innit? Ideas and especially patches are more than welcome!