Skip to content
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

funkwhale: init at 0.20.0 #53416

Open
wants to merge 2 commits into
base: master
from
Open

funkwhale: init at 0.20.0 #53416

wants to merge 2 commits into from

Conversation

@mmai
Copy link
Contributor

mmai commented Jan 4, 2019

Motivation for this change

This adds a new package and module which enable the Funkwhale music web service (https://funkwhale.audio/).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

Infinisil left a comment

All in all, this PR is far from done and needs a lot of work. Note that a lot of the comments I left also apply to many other places in your changes.

pkgs/servers/web-apps/funkwhale/default.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/funkwhale/0001-changes.patch Outdated Show resolved Hide resolved
@mmai mmai requested a review from FRidh as a code owner Jan 27, 2019
@mmai mmai force-pushed the mmai:funkwhale branch 2 times, most recently from 80da105 to 368fb10 Jan 27, 2019
@mmai mmai changed the title funkwhale: init at 0.17 funkwhale: init at 0.18 Jan 27, 2019
@mmai mmai force-pushed the mmai:funkwhale branch 2 times, most recently from 69c6ba8 to 685ddec Jan 28, 2019
@matthiasbeyer

This comment has been minimized.

Copy link
Contributor

matthiasbeyer commented Jun 29, 2019

Any progress here?

@mmai mmai force-pushed the mmai:funkwhale branch from 8f2a49c to 02cdc32 Jun 29, 2019
@mmai mmai changed the title funkwhale: init at 0.18 funkwhale: init at 0.19 Jun 29, 2019
@ivan

This comment has been minimized.

Copy link
Member

ivan commented Sep 6, 2019

For the author, reviewers, and committers: this PR was scanned and appears to add a use of the deprecated types.string, which emits a warning as of #66346. Before merging, please change this to another type, possibly:

  • types.str for a single string where merging does not make sense, or cannot work
  • types.lines for multi-line configuration or scripts where merging is possible
  • types.listOf types.str for a mergeable list of strings
@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Sep 6, 2019

Nothing happening so closing.

@FRidh FRidh closed this Sep 6, 2019
@mmai

This comment has been minimized.

Copy link
Contributor Author

mmai commented Sep 7, 2019

@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.

@FRidh FRidh reopened this Sep 7, 2019
@mmai

This comment has been minimized.

Copy link
Contributor Author

mmai commented Sep 24, 2019

Hi @worldofpeace I've fixed the descriptions and the hashes of the python packages. There are also new packages extracted from the funkwhale NixOs module. You will see that I've disabled the check phase for half of them, I didn't find obvious ways to make tests pass for those, and my python abilities are limited. If you want to merge the python packages separately, should I prepare distinct pull requests ?

@mmai mmai force-pushed the mmai:funkwhale branch from 982ef53 to eb7fe23 Sep 25, 2019
@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Sep 26, 2019

@mmai I've pushed some fixup commits here for things I knew how to fix right away.
I think I can do one last quick review of the modules, and I can just push them to master from this PR.

Copy link
Member

worldofpeace left a comment

  • pythonPackages.channels-redis

Please add

disabled = pythonOlder "3.6";

for https://github.com/django/channels_redis/blob/c436cca4fccd0d2cf02b4faac7d0927710b3c7f1/setup.py#L32

Possibly also setup tests

Additionally, I'm thinking there might be some confusion on what django is being used in the python modules. django in the python package set will always be the python module, where perhaps there's a requirement for pkgs.django?

Other than that, see my fixup commits for correct whitespace when I've corrected it.

'';

meta = with stdenv.lib; {
homepage = http://github.com/django/channels_redis/;

This comment has been minimized.

Copy link
@worldofpeace
pname = "channels_redis";
sha256 = "1g4izdf8237pwxn85bv5igc2bajrvck1p2a7q448qmjfznrbrk5p";
};
buildInputs = [ redis channels msgpack aioredis hiredis asgiref ];

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Sep 26, 2019

Member

The following install requires should be propagatedBuildInputs

inherit pname version;
sha256 = "1z2dndkpypk4pvb0byh5a771vgp50vn8g1rbk5r3sml6dm4wcn7s";
};
buildInputs = [ six django persisting-theory ];

This comment has been minimized.

buildPythonPackage rec {
pname = "django-oauth-toolkit";
version = "1.2.0";
name = "${pname}-${version}";

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Sep 26, 2019

Member
Suggested change
name = "${pname}-${version}";
rev = version;
sha256 = "1zbksxrcxlqnapmlvx4rgvpqc4plgnq0xnf45cjwzwi1626zs8g6";
};
buildInputs = [ django_2_2 requests oauthlib ];

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Sep 26, 2019

Member

These should be propagatedBuildInputs.

sha256 = "01xq232h321716r08rari9payas7fsiwwr5q6zgcrlwkckwxxczk";
};

buildInputs = [ django ];

This comment has been minimized.

Copy link
@worldofpeace
checkPhase = "nosetests";

meta = with stdenv.lib; {
homepage = http://code.eliotberriot.com/eliotberriot/persisting-theory;

This comment has been minimized.

Copy link
@worldofpeace
@mmai mmai force-pushed the mmai:funkwhale branch 2 times, most recently from 6303d42 to 701b76b Oct 4, 2019
@mmai

This comment has been minimized.

Copy link
Contributor Author

mmai commented Oct 4, 2019

@worldofpeace I've rebased the fixups.

I've removed the explicit dependencies on django_2_2 on django-oauth-toolkit and django-auth-ldap. django-auth-ldap is in fact compatible with django 1, and for django-oauth-toolkit I took inspiration on django-raster to check the django version.

I tried to setup tests on channels-redis with the github sources, but they eventually fail, I've kept the code in comments.

@mmai mmai force-pushed the mmai:funkwhale branch from 701b76b to 54b51e8 Oct 7, 2019
@mmai mmai changed the title funkwhale: init at 0.19.1 funkwhale: init at 0.20.0 Oct 7, 2019
@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Oct 7, 2019

@mmai I'll see about integrating the python packages later today.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Oct 16, 2019

Lol more like today now 🤣

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Oct 16, 2019

Merged all python modules in #71262.

We need to drop all those commits from here, and the funkwhale commit needs a rewrite to not add you to the maintainer list. I had to do that separately for merging the modules 3895e09.

@mmai mmai force-pushed the mmai:funkwhale branch from 54b51e8 to 35d9ccf Oct 18, 2019
@ofborg ofborg bot removed the 6.topic: python label Oct 18, 2019
@mmai

This comment has been minimized.

Copy link
Contributor Author

mmai commented Oct 18, 2019

Thanks @worldofpeace . I've removed the python packages commits.

Copy link
Member

Infinisil left a comment

Still some important things (and some less important things) to correct, but it's looking much better than initially!

installPhase = ''
mkdir $out
cp -R ./* $out
unzip ${srcs.frontend} -d $out

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

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.

This comment has been minimized.

Copy link
@FRidh

FRidh Oct 24, 2019

Member

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"; });

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

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.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

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.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member
Suggested change
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.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

Syntax for links is <link xlink:href="https://example.com"/>

type = types.nullOr types.path;
default = "/run/postgresql";
defaultText = "/run/postgresql";
example = "/run/postgresql";

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

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";

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

Also here, no need to set defaultText

let serviceConfig = {
User = "${cfg.user}";
WorkingDirectory = "${pkgs.funkwhale}";
EnvironmentFile = "${funkwhaleEnvFile}";

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

This shouldn't be necessary since you set environment in the services already

};
};

musicDirectoryPath = mkOption {

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

Maybe call it musicPath instead

This comment has been minimized.

Copy link
@matthiasbeyer

matthiasbeyer Jan 15, 2020

Contributor

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";' '';

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 24, 2019

Member

Split this into more lines, same for other long lines, try to keep them to ~80 chars wide

Copy link
Member

FRidh left a 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

This comment has been minimized.

Copy link
@FRidh

FRidh Oct 24, 2019

Member

${python.interpreter}

installPhase = ''
mkdir $out
cp -R ./* $out
unzip ${srcs.frontend} -d $out

This comment has been minimized.

Copy link
@FRidh

FRidh Oct 24, 2019

Member

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!

@mmai mmai force-pushed the mmai:funkwhale branch from 35d9ccf to 66e784b Oct 27, 2019
@mmai

This comment has been minimized.

Copy link
Contributor Author

mmai commented Nov 3, 2019

@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.

@EliotBerriot

This comment has been minimized.

Copy link

EliotBerriot commented Jan 13, 2020

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!

@davidak

This comment has been minimized.

Copy link
Member

davidak commented Jan 13, 2020

@matthiasbeyer would you be able to finish it?

@matthiasbeyer

This comment has been minimized.

Copy link
Contributor

matthiasbeyer commented Jan 13, 2020

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.

@matthiasbeyer

This comment has been minimized.

Copy link
Contributor

matthiasbeyer commented Jan 15, 2020

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.

@worldofpeace worldofpeace removed this from the 20.03 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.