funkwhale: init at 0.20.0 #53416
funkwhale: init at 0.20.0 #53416
Conversation
mmai
force-pushed
the
funkwhale
branch
2 times, most recently
from
80da105
to
368fb10
Compare
mmai
force-pushed
the
funkwhale
branch
2 times, most recently
from
69c6ba8
to
685ddec
Compare
|
Any progress here? |
|
For the author, reviewers, and committers: this PR was scanned and appears to add a use of the deprecated
|
|
Nothing happening so closing. |
|
@FRidh I'm not sure I understand. Do you expect more actions from me ? I've made all the requested changes for the initial pull request but my fixes didn't received any review. |
|
@mmai I'll see about integrating the python packages later today. |
|
Lol more like today now 🤣 |
|
Thanks @worldofpeace . I've removed the python packages commits. |
ofborg
bot
added
10.rebuild-darwin: 0
10.rebuild-linux: 1-10
and removed
10.rebuild-darwin: 11-100
10.rebuild-linux: 11-100
labels
| installPhase = '' | ||
| mkdir $out | ||
| cp -R ./* $out | ||
| unzip ${srcs.frontend} -d $out |
There was a problem hiding this comment.
In nixpkgs there's a structure for $out you need to follow. And since this is a python package, you should use buildPythonPackage to build, which will do a lot of things automatically. See section 10.15.2.2.2 in the Nixpkgs manual. This way you can also specify the dependencies in the package directly.
There was a problem hiding this comment.
This is a Django application and they usually do not provide any installer. While it would be nice if funkwhale would be usable without the NixOS module, given its a Django app it is typically managed using the manage.py script and various other executables are executed (gunicorn e.g.). Therefore, I think this approach is alright.
Note in another PR (https://github.com/NixOS/nixpkgs/pull/67951/files#diff-5e9f694ce07aa22a80eb0c049c03beaf) I recommended putting the contents of the Django application in a site-packages folder using buildPythonPackage because then the application could be added to the env (that would correspond to adding the funkwhale derivation to the Python env. That would be even nicer, especially if the Python env would be moved here!
| group = cfg.group; | ||
| }); | ||
|
|
||
| users.groups = optionalAttrs (cfg.group == "funkwhale") (singleton { name = "funkwhale"; }); |
There was a problem hiding this comment.
Do users.groups.funkwhale = mkIf (cfg.group == "funkwhale") {} instead, and similarly for users.users
| djangoSecretKey = mkOption { | ||
| type = types.str; | ||
| description = '' | ||
| Django secret key. Generate one using `openssl rand -base64 45` for example. |
There was a problem hiding this comment.
Docs aren't markdown but asciidoc docbook, so here you need <command>the command</command> instead
| type = types.str; | ||
| default = "/srv/funkwhale/static"; | ||
| description = '' | ||
| Where static files (such as API css or icons) should be compiled on your system ? Ensure this directory actually exists. |
There was a problem hiding this comment.
| Where static files (such as API css or icons) should be compiled on your system ? Ensure this directory actually exists. | |
| Where static files (such as API css or icons) should be compiled on your system. Ensure this directory actually exists. |
Same for other descriptions where there's a random question mark.
| type = types.str; | ||
| default = "consolemail://"; | ||
| description = '' | ||
| Configure email sending. By default, it outputs emails to console instead of sending them. See https://docs.funkwhale.audio/configuration.html#email-config for details. |
There was a problem hiding this comment.
Syntax for links is <link xlink:href="https://example.com"/>
| type = types.nullOr types.path; | ||
| default = "/run/postgresql"; | ||
| defaultText = "/run/postgresql"; | ||
| example = "/run/postgresql"; |
There was a problem hiding this comment.
No need to specify defaultText when the default is the same. And no need to specify an example if it's the same as the default
| port = mkOption { | ||
| type = types.int; | ||
| default = 5432; | ||
| defaultText = "5432"; |
There was a problem hiding this comment.
Also here, no need to set defaultText
| let serviceConfig = { | ||
| User = "${cfg.user}"; | ||
| WorkingDirectory = "${pkgs.funkwhale}"; | ||
| EnvironmentFile = "${funkwhaleEnvFile}"; |
There was a problem hiding this comment.
This shouldn't be necessary since you set environment in the services already
| }; | ||
| }; | ||
|
|
||
| musicDirectoryPath = mkOption { |
There was a problem hiding this comment.
Maybe call it musicPath instead
There was a problem hiding this comment.
It is more consistent with the funkwhaleEnvironment variable name (which is MUSIC_DIRECTORY_PATH), so IMO it should not be changed, because of consistency.
| before = [ "funkwhale-init.service" ]; | ||
| serviceConfig = { | ||
| User = "postgres"; | ||
| ExecStart = '' ${config.services.postgresql.package}/bin/psql -d ${cfg.database.name} -c 'CREATE EXTENSION IF NOT EXISTS "unaccent";CREATE EXTENSION IF NOT EXISTS "citext";' ''; |
There was a problem hiding this comment.
Split this into more lines, same for other long lines, try to keep them to ~80 chars wide
There was a problem hiding this comment.
Nice work. It's a very big application. I am worried though how we can keep it stable. I imagine that if the pythonEnv is moved to pkgs.funkwhale, that we can provide overrides there as needed. It might also be good to provide a NixOS test or lighter tests for the pkgs.funkwhale package so we know we don't break it. Maybe we should use the requirements.txt to verify deps as well?
Hook to test against a requirements.txt file. #71908
| Group = "${cfg.group}"; | ||
| }; | ||
| script = '' | ||
| ${pythonEnv}/bin/python ${pkgs.funkwhale}/manage.py migrate |
| installPhase = '' | ||
| mkdir $out | ||
| cp -R ./* $out | ||
| unzip ${srcs.frontend} -d $out |
There was a problem hiding this comment.
This is a Django application and they usually do not provide any installer. While it would be nice if funkwhale would be usable without the NixOS module, given its a Django app it is typically managed using the manage.py script and various other executables are executed (gunicorn e.g.). Therefore, I think this approach is alright.
Note in another PR (https://github.com/NixOS/nixpkgs/pull/67951/files#diff-5e9f694ce07aa22a80eb0c049c03beaf) I recommended putting the contents of the Django application in a site-packages folder using buildPythonPackage because then the application could be added to the env (that would correspond to adding the funkwhale derivation to the Python env. That would be even nicer, especially if the Python env would be moved here!
|
@FRidh @infinisil I didn't managed to set a structure resembling what you describe. Looking at the mailman package code and the documentation didn't help. I'm really sorry to say that at this point I am not able to work on this anymore. I feel that my python and nixos abilities are to shallow and I invested way more time than initially planned, and I see that we are far from having a stable package (the master branch already breaks python dependencies). Feel free to close the pull request. |
|
Hi, I am one of Funkwhale's maintainer, and I wasn't aware of this packaging effort, so I'm a bit late. I don't have any knowledge about NixOS, however, I'd be willing to help if you need some clarification or tweaks on Funkwhale side to make it easier to packagers. I can also answer questions you have regarding our dependencies, for instance. Many thanks to the people who worked on this! |
|
@matthiasbeyer would you be able to finish it? |
|
Holy cow. I doubt it. Mainly because I do not know how to properly write services and also because I have little time. I will try next weekend though, if I do not forget. |
|
I'm not sure how to push directly to this PR, nor am I convinced I should. I have some patches at https://github.com/matthiasbeyer/nixpkgs/tree/funkwhale though. |
|
I'd also be happy to work on this. I've looked at the discussion, but can someone give a list of all the things that need to be changed or fixed to get this PR ready to merge? |
|
@jbaum98 I believe #53416 (review) and maybe https://github.com/NixOS/nixpkgs/pull/53416/files#r338540545? You should re PR and review can begin again. |
|
We also have #53416 (comment) who responsive to questions so this can be transplanted into NixOS as easily as possible. |
|
I'm going to close this since @mmai said they won't be working on it anymore, so it won't be merged. The code should still be available though. |
|
@FRidh I looked at the mailman PR, and I'm not quite clear on what structure we want for the Django app. Also, this project has a pretty complicated architecture:
It seems like we want package the API server separately from the frontend server, and then hook that together with all the other stuff in the NixOS module. |
Motivation for this change
This adds a new package and module which enable the Funkwhale music web service (https://funkwhale.audio/).
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)