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

sympa: init at 6.2.52 + NixOS module #65397

Merged
merged 5 commits into from Feb 10, 2020
Merged

sympa: init at 6.2.52 + NixOS module #65397

merged 5 commits into from Feb 10, 2020

Conversation

@mmilata
Copy link
Member

@mmilata mmilata commented Jul 25, 2019

Motivation for this change

Adds Sympa mailing list manager and its dependencies to nixpkgs, with NixOS module.

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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

(joint effort w/ @sorki)

Copy link
Member

@dasJ dasJ left a comment

Sorry for the high amount of comments, but it's also a huge PR :/

cc @Scriptkiddi maybe you can have a look here?

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
@@ -340,6 +340,7 @@
cockroachdb = 313;
zoneminder = 314;
paperless = 315;
sympa = 316;

This comment has been minimized.

@dasJ

dasJ Jul 28, 2019
Member

I don't think sympa needs to be added here. Just create a user with isSystemUser = true and it will get some free UID. If you want to go wild, you can try systemd's DynamicUser ;)

This comment has been minimized.

@mmilata

mmilata Jul 29, 2019
Author Member

Hmm, DynamicUser looks good though it's not obvious if it works with the kind of setup w/ multiple unit files and multiple daemon processes we have here.

nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
stdenv.mkDerivation rec {
name = "${pname}-${version}";

src = fetchurl {

This comment has been minimized.

@dasJ

dasJ Jul 28, 2019
Member

please use fetchFromGitHub instead

This comment has been minimized.

@mmilata

mmilata Jul 29, 2019
Author Member

Working on this. Is building from git generally preferred to building from release tarballs?

pkgs/servers/mail/sympa/default.nix Outdated Show resolved Hide resolved
for i in chown chgrp chmod; do
echo '#!${stdenv.shell}' >> "$TMP/bin/$i"
chmod +x "$TMP/bin/$i"
PATH="$TMP/bin:$PATH"

This comment has been minimized.

@dasJ

dasJ Jul 28, 2019
Member

Move this one line lower. Also can't you just omit that?

This comment has been minimized.

@mmilata

mmilata Jul 29, 2019
Author Member

Oops. Omitting causes the build to fail in install phase:

make  install-exec-hook
make[4]: Entering directory '/build/sympa-6.2.44/src/cgi'
chown sympa /nix/store/2yv66g71p9bmy7i1gxry4gvp25wl203y-sympa-6.2.44/bin/wwsympa-wrapper.fcgi
chown: invalid user: 'sympa'
make[4]: [Makefile:783: install-exec-hook] Error 1 (ignored)
chgrp sympa /nix/store/2yv66g71p9bmy7i1gxry4gvp25wl203y-sympa-6.2.44/bin/wwsympa-wrapper.fcgi
chgrp: invalid group: 'sympa'
make[4]: [Makefile:784: install-exec-hook] Error 1 (ignored)
chmod 6755 /nix/store/2yv66g71p9bmy7i1gxry4gvp25wl203y-sympa-6.2.44/bin/wwsympa-wrapper.fcgi
chmod: changing permissions of '/nix/store/2yv66g71p9bmy7i1gxry4gvp25wl203y-sympa-6.2.44/bin/wwsympa-wrapper.fcgi': Operation not permitted
Copy link
Contributor

@Scriptkiddi Scriptkiddi left a comment

Hey first of really nice that you are packaging sympa, I know the config is not something that is fun to read.
I would add an option to add files and folders to the dataDir because that covers pretty much all the other options sympa has, like plugins, list templates and list families, but thats something that can be done after this pr

nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
@mmilata mmilata force-pushed the mmilata:sympa branch from 7e21eb7 to ccce783 Jul 29, 2019
Copy link
Member Author

@mmilata mmilata left a comment

@dasJ @Scriptkiddi thank you for your feedback! I tried to address most of the issues and marked them as resolved. Still working on fetchFromGitHub and structural settings.

Sorry for the high amount of comments, but it's also a huge PR :/

Don't worry, that's expected in order to achieve quality;) Would splitting this PR e.g. into one w/ package and another with NixOS module help?

I would add an option to add files and folders to the dataDir because that covers pretty much all the other options sympa has

Something like environment.etc? Is there other module that has option like this for me to copy it study?

@mmilata mmilata force-pushed the mmilata:sympa branch from ccce783 to d377369 Jul 29, 2019
@mmilata
Copy link
Member Author

@mmilata mmilata commented Jul 30, 2019

@Infinisil I've refactored the module to use structural settings, PTAL.

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Jul 30, 2019

Forgot to push?

@mmilata mmilata force-pushed the mmilata:sympa branch from d377369 to 2e7e296 Jul 30, 2019
@mmilata
Copy link
Member Author

@mmilata mmilata commented Jul 30, 2019

Yep ... pushed now.

@Scriptkiddi
Copy link
Contributor

@Scriptkiddi Scriptkiddi commented Jul 30, 2019

something like it, the Gams package is doing it.

@mmilata
Copy link
Member Author

@mmilata mmilata commented Jul 30, 2019

Regarding the rest of the configuration, it doesn't seem feasible to model the structure of all the config files, so we'd have to have something like environment.etc but for /var/lib/sympa. The other problem is that some (most?) of these files are editable from the Sympa web UI.

I'd propose to leave the rest of the configuration (i.e. other than sympa.conf and robot.conf) unmanaged by NixOS as it still allows you to:

  • install an empty configuration and edit it through the web UI
  • manually import the configuration (and archives) from a backup
@Scriptkiddi
Copy link
Contributor

@Scriptkiddi Scriptkiddi commented Jul 31, 2019

Works for me, most of the files are not editable from the web ui but I'm fine with just adding them under /var/lib/sympa

nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/sympa.nix Outdated Show resolved Hide resolved
};

database = {
type = mkOption {

This comment has been minimized.

@aanderse

aanderse Aug 1, 2019
Contributor

A big 👍 for using services.mysql.ensureDatabases, services.mysql.ensureUsers, services.postgresql.ensureDatabases, and services.postgresql.ensureUsers. I highly suggest adding a database.createLocally option which defaults to true so users don't have to do any additional steps to provision a database. A decent example can be seen here:

services.mysql = optionalAttrs mysqlLocal {
enable = true;
package = mkDefault pkgs.mariadb;
ensureDatabases = [ cfg.database.name ];
ensureUsers = [
{ name = cfg.database.user;
ensurePermissions = { "${cfg.database.name}.*" = "ALL PRIVILEGES"; };
}
];
};
services.postgresql = optionalAttrs pgsqlLocal {
enable = true;
ensureDatabases = [ cfg.database.name ];
ensureUsers = [
{ name = cfg.database.user;
ensurePermissions = { "DATABASE ${cfg.database.name}" = "ALL PRIVILEGES"; };
}
];
};

Please let me know if anything isn't clear from the example or you have any questions as I'm always happy to help (especially when it comes to utilizing ensureDatabases and ensureUsers.

pkgs/servers/mail/sympa/default.nix Outdated Show resolved Hide resolved
pkgs/servers/mail/sympa/default.nix Outdated Show resolved Hide resolved
];
nativeBuildInputs = [ autoreconfHook ];
buildInputs = [ perlPackages.perl makeWrapper ];
propagatedBuildInputs = with perlPackages; [

This comment has been minimized.

@aanderse

aanderse Aug 1, 2019
Contributor

Wow! Are all of these direct dependencies of every program? This could probably benefit from using perl.withPackages like so:

prog1Env = perl.withPackages (p: with p; [ ArchiveZip CGI DateTime ... ]);
prog2Env = perl.withPackages (p: with p; [ DBI FGCI ]);

Note you can explicitly list dependencies for each program and a 'better' perl environment will be built. From there you can pass prog1Env and prog2Env into buildInputs and patchShebangs can take care of the perl binary.

This comment has been minimized.

@mmilata

mmilata Aug 6, 2019
Author Member

Don't know ... I'm not familiar with Sympa internals (nor can I read Perl very well) but it seems that the programs mostly depend on Sympa library/package which in turn depends on these packages.

Is there a way to determine which program depends on which package that doesn't involve reviewing the whole source code?

This comment has been minimized.

@aanderse

aanderse Aug 6, 2019
Contributor

Sorry I suppose I shouldn't have bothered mentioning about multiple perl environments.

The point you should takeaway is that you can avoid PERL5LIB if you want to by creating a dedicated (custom) perl environment using perl.withPackages, if you want to.

This comment has been minimized.

@mmilata

mmilata Aug 6, 2019
Author Member

I'm not sure I understand how that works. Do I define a custom perlEnv and then use it instead of perlPackages.perl in buildInputs and use patchShebangs instead of makeWrapper? How do I pass the environment to patchShebangs? Will propagatedBuildInputs be empty? Got an example I can look at?

This comment has been minimized.

@aanderse

aanderse Aug 6, 2019
Contributor

#64646 is a simple example which should help.

This comment has been minimized.

@mmilata

mmilata Sep 4, 2019
Author Member

Modified the package to use perl.withPackages, please take a look if this is what you meant.

@mmilata mmilata force-pushed the mmilata:sympa branch 2 times, most recently from 96398dc to 39e6ad2 Aug 6, 2019
@mmilata
Copy link
Member Author

@mmilata mmilata commented Aug 6, 2019

@Infinisil @aanderse thank you for the comments, I think I addressed most of them, please take a look!

pkgs/servers/mail/sympa/default.nix Outdated Show resolved Hide resolved
];
nativeBuildInputs = [ autoreconfHook ];
buildInputs = [ perlPackages.perl makeWrapper ];
propagatedBuildInputs = with perlPackages; [

This comment has been minimized.

@aanderse

aanderse Aug 6, 2019
Contributor

Sorry I suppose I shouldn't have bothered mentioning about multiple perl environments.

The point you should takeaway is that you can avoid PERL5LIB if you want to by creating a dedicated (custom) perl environment using perl.withPackages, if you want to.

@mmilata mmilata force-pushed the mmilata:sympa branch from 39e6ad2 to 46ee52b Sep 4, 2019
@mmilata
Copy link
Member Author

@mmilata mmilata commented Sep 4, 2019

Changes:

  • remove dataDir parameter from sympa package
  • convert sympa package to using perl.withPackages
  • make unix user and group not configurable
  • bring back forking spawn-fcgi with configurable number of children
  • add settingsFile config option that allows you to customize files under /var/lib/sympa in a way similar to environment.etc

@aanderse @Infinisil @Scriptkiddi PTAL & please advise how to proceed with this pull request.

@sorki
sorki approved these changes Sep 18, 2019
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Sep 28, 2019

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/62

@mmilata mmilata force-pushed the mmilata:sympa branch from 755b01f to 4a75b73 Oct 8, 2019
@mmilata
Copy link
Member Author

@mmilata mmilata commented Oct 8, 2019

Changes:

  • upgrade sympa package to 6.2.48
  • use tmpfiles instead of RuntimeDirectory because /run/sympa/wwsympa.sock needs to be owned by nginx
  • explicitly tell sympa to use /run/mysqld/mysqld.sock for connecting to mysql on localhost because 6.2.48 looks for it under /tmp for some reason
@mmilata mmilata changed the title sympa: init at 6.2.44 + NixOS module sympa: init at 6.2.48 + NixOS module Oct 8, 2019
@ofborg ofborg bot requested a review from sorki Oct 8, 2019
@ajs124
Copy link
Member

@ajs124 ajs124 commented Oct 9, 2019

The mysql.sock thing should be solved by #70010.

@sorki
sorki approved these changes Feb 4, 2020
@mmilata
Copy link
Member Author

@mmilata mmilata commented Feb 4, 2020

FYI I'd still like to get this merged but it needs some more work from my side which will hopefully happen soon.

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Feb 4, 2020

Other than fixing the merge conflicts, what's left?

@mmilata
Copy link
Member Author

@mmilata mmilata commented Feb 7, 2020

I think bumping packages to current versions would be nice to have (e.g. sympa 6.2.52).

@mmilata mmilata force-pushed the mmilata:sympa branch from 4a75b73 to 097ab90 Feb 7, 2020
@mmilata mmilata changed the title sympa: init at 6.2.48 + NixOS module sympa: init at 6.2.52 + NixOS module Feb 7, 2020
@mmilata
Copy link
Member Author

@mmilata mmilata commented Feb 7, 2020

Changes:

  • fix merge conflicts
  • sympa: 6.2.48 -> 6.2.52
  • convert test to python
@ofborg ofborg bot requested a review from sorki Feb 7, 2020
@Infinisil
Copy link
Member

@Infinisil Infinisil commented Feb 10, 2020

Cool! Let's merge this for now so it gets into 20.03 (https://discourse.nixos.org/t/nixos-20-03-feature-freeze/5655/13). We can fix any potential issues later (hopefully before 20.03 is released).

@Infinisil Infinisil merged commit b9d7f1f into NixOS:master Feb 10, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
sympa on aarch64-linux Success
Details
sympa on x86_64-linux Success
Details
@mmilata
Copy link
Member Author

@mmilata mmilata commented Feb 10, 2020

@Infinisil thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.