nixos/dovecot: More sieve options and precompiling of scripts #35536

griff · 2018-02-25T11:28:14Z

Motivation for this change

This PR does 4 different things in the 3 commit:

Adds test of sieve scripts to nixos/tests/dovecot.nix
Patches dovecot_pigeonhole with this change When binary and script have same mtime they should be seen as up-to-date dovecot/pigeonhole#4
Adds builder to precompile sieve scripts
Adds more options relating to sieve scripts to dovecot2 module

test of sieve scripts

Before making changes to how sieve scripts were handled I thought it best that the dovecot test included some testing of the sieve script functionality.

Patching dovecot_pigeonhole and builder to precompile scripts

One of the changes I wanted to make was to precompile sieve scripts as part of a Nix derivation. But to do that dovecot_pigeonhole needs to be changed since when source and compiled script have same mtime it thinks that the compiled script is out of date. I have tried to upstream that change with dovecot/pigeonhole#4 but it is as of yet not merged.

The builder does its compilation in the install phase because the compiled .svbin files contain a reference to the full path of the source .sieve file and so the source needs to be in $out when doing the compile.

Sieve related options in the dovecot2 module

When precompiling sieve scripts the compiler needs a dovecot configuration that defines sieve_plugins, sieve_extensions and sieve_global_extensions to know which extensions and plugin functionality to allow but you can’t just use the dovecot2 config file directly because that would create a dependency cycle. So to better support precompilation I have added explicit options for these 3 things to the dovecot2 module so that I can reuse their values when precompiling.

The sieveScripts option followed very closely how dovecot configures global scripts and was a direct mapping from attribute name to sieve_${attribute name} which has some problems. E.g. if you defined sieveScripts.after2 without defining sieveScripts.after your script would never be called. It also wasn’t totally clear without reading the dovecot documentation what scripts you could define.

So I have added explicit options sieve.beforeScripts, sieve.defaultScript, sieve.afterScripts and sieve.discardScript for each of the scripts you can define with beforeScripts and afterScripts being lists of paths that are automatically translated to the correct numbered dovecot configuration and with the added bonus that multiple definitions of the option are merged into one list. I have also included functionality so that existing configurations that use the sieveScripts option continue to work as before but now with a warning pointing to the new options.

The other place were you might use global sieve scripts and so would like to precompile the scripts is when using the imapsieve plugin. Before there were no options for this so I have added options for enabling and configuring imapsieve with support for automatic precompilation of scripts.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
Fits CONTRIBUTING.md.

dotlambda · 2018-03-14T15:28:12Z

/cc @andir @fpletz

uwap · 2018-04-16T11:04:06Z

nixos/modules/services/mail/dovecot.nix

+        '';
+      };
+      beforeScript = mkOption {
+        type = types.nullOr types.path;


What is the reason to ask for a path to a sieve script instead of an actual string containing the sieve script here?

Because the sieve script needs to be written to a file no matter what and as a user of this module I can prefer to store my sieve scripts either in files or inline in the configuration.

If I prefer to store my sieve scripts in files and the option is a path I can just provide the path to the sieve script. And if I prefer to store my sieve scripts inline in the configuration I can simply use the writeText builder.

On the other hand if this option was a string and I prefer to store my sieve scripts in files I would have to use fileContents to load the data from the file, only for the module to write that content to a different file which seems a waste.

I will also admit that my personal preference is to store my sieve scripts in files so that things like syntax highlighting work.

On the other hand if this option was a string and I prefer to store my sieve scripts in files I would have to use fileContents to load the data from the file, only for the module to write that content to a different file which seems a waste

Yes, but using a string is standard… and it's not like there's not a lot of copying going on during builds ;)

@wmertens I know. I guess I just like writeText more than fileContents.

I have considered supporting both strings and path by making the value an attrset like environment.etc or services.rspamd.locals but a listOf attrset is not as nice to work with as the named submodules like environment.etc and services.rspamd.locals.

How about having both text and path attributes? Or, if a Sieve script can never be a single line starting with /, autodetect?

@wmertens I looked at the Sieve RFC and from that it seems that while a sieve script can start with / it has to start with /* in that case which is obviously not a valid store path. So I am working on an update for this PR the resolves merge conflicts and supports having both text and path attributes for Sieve scripts.

stale · 2020-06-01T04:24:29Z

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

wmertens · 2020-06-01T10:00:40Z

I don't use Sieve but LGTM. I'm not that bothered by the paths vs text issue.

griff · 2020-10-20T23:03:55Z

I have now resolved all the merge conflicts and removed the commit that was no longer relevant since the functionality had since been implemented in another PR.

I have also done as @wmertens sugested and implemented support for Sieve scripts being either a string with the script or a path and the module will automatically detect the string script and write it to the store.

@uwap I hope this addresses the reservations you had.

nlewo

These changes are really nice!
Here some minor comments and i still would like to play with this PR a bit to see what is generated and better understand it.

nlewo · 2020-10-25T08:23:12Z

nixos/modules/services/mail/dovecot.nix

+        ${concatStringsSep "\n" (imap1 (i: v: ''
+          mkdir -p after/${toString i}
+          if [ -d '${v}' ]; then
+            cp -p '${v}'/*.sieve after/${toString i}


Would it be possible to create symlinks instead of copying files?

Done! And while looking at this I found that when the path was outside the store the user feedback wasn't that great and would result in an unclean build in non-sandboxed builds so I have added logic to use fileContents to write paths outside the store to the store first.

nlewo · 2020-10-25T08:24:57Z

nixos/modules/services/mail/dovecot.nix

+      plugins = cfg.sieve.plugins;
+      extensions = cfg.sieve.extensions;
+      globalExtensions = cfg.sieve.globalExtensions;
+      phases = ["buildPhase" "installPhase" "fixupPhase"];


It is recommended to use dontConfigure instead of defining your own list of phases.

done! But needed to also set dontUnpack

nlewo · 2020-10-25T08:35:01Z

pkgs/servers/mail/dovecot/plugins/pigeonhole/default.nix

@@ -11,6 +11,15 @@ stdenv.mkDerivation rec {

  buildInputs = [ dovecot openssl ];

+  patches = [
+    # Fix mtime comparison so that scripts can be precompiled


Could you link your PR also?

nlewo · 2020-10-25T08:39:47Z

nixos/modules/services/mail/dovecot.nix

-  stateDir = "/var/lib/dovecot";
+
+  sieveToPath = text:
+    if isString text && builtins.substring 0 2 text == "/*"


The lib.hasPrefix would be more explicit.

stale · 2021-07-21T01:12:15Z

I marked this as stale due to inactivity. → More info

poscat0x04 · 2021-10-11T11:45:31Z

What's the current status of this PR? may I ask.

nixos-discourse · 2021-10-15T11:49:54Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/604

SuperSandro2000 · 2021-10-19T15:37:25Z

nixos/modules/services/mail/dovecot.nix

+      then let value = config.services.dovecot2.sieveScripts."${type}${indexName}";
+        in if (substring 0 1 (toString value) == "/")
+          then [value] ++ (listOfScripts type (idx + 1) config)
+          else []


I think we can use lib.optionals here.

Done! And also rewrote the function to use or and hasPrefix so that it is shorter and nicer.

SuperSandro2000 · 2021-11-16T17:59:08Z

pkgs/servers/mail/dovecot/plugins/pigeonhole/compile.nix

+      else
+        echo "Compiling $k -> $(readlink -f "$k")"
+      fi
+      ${dovecot_pigeonhole}/bin/sievec -c "${config}" "$out/sieve/$k"


Suggested change

${dovecot_pigeonhole}/bin/sievec -c "${config}" "$out/sieve/$k"

sievec -c "${config}" "$out/sieve/$k"

This should use nativeBuildInputs where we add dovecot_pigeonhole.

RaitoBezarius · 2022-12-01T15:14:29Z

Can you fix the conflicts please? I think this PR is ready to merge.

griff · 2023-11-02T10:42:58Z

@RaitoBezarius this has been ready to merge for years so I wasn't that motivated to rebase it again and again. In the meantime the patch to pigeonhole has been rejected so I also needed to move the 2 line patch into nixpkgs.

RaitoBezarius · 2024-01-12T19:24:50Z

Apologies for the delay, I missed your message… @2xsaiko became maintainer of that module, can I let you shepherd this PR and give me the go for a merge?

2xsaiko · 2024-01-12T20:10:14Z

Sure, I'll take a look in a bit!

2xsaiko

Please rebase.

2xsaiko · 2024-01-12T21:50:32Z

nixos/modules/services/mail/dovecot.nix

+  sieveToPath = text:
+    # Sieve scripts can start with a block comment
+    if isString text && hasPrefix "/*" text
+      then pkgs.writeText "script.sieve" text
+    # Paths in the store are used as is
+    else if types.path.check text && hasPrefix "${builtins.storeDir}/" (toString text)
+      then text
+    # Paths outside the store get written to the store
+    else if types.path.check text
+      then pkgs.writeText "script.sieve" (fileContents text)
+    # All other is assumed to be file content
+    else pkgs.writeText "script.sieve" text;
+  scriptType = types.coercedTo types.lines sieveToPath types.path;


I would just use the same mechanism as environment.etc where you can set either text, source or target options to explicitly specify what it is, instead of trying to be overly smart trying to figure out what the string means.

2xsaiko · 2024-01-12T21:52:40Z

nixos/modules/services/mail/dovecot.nix

+        mkdir sieve
+        cd sieve
+        ${concatStringsSep "\n" (imap1 (i: v: ''
+          if [ -d '${v}' ]; then


Suggested change

if [ -d '${v}' ]; then

if [[ -d ${lib.escapeShellArg v} ]]; then

generally use escapeShellArg instead of simple quoting especially with user given text, and we have bash, you can use [[

2xsaiko · 2024-01-12T21:59:28Z

pkgs/servers/mail/dovecot/plugins/pigeonhole/default.nix

@@ -1,4 +1,4 @@
-{ lib, stdenv, fetchurl, dovecot, openssl }:
+{ lib, stdenv, fetchpatch, fetchurl, dovecot, openssl }:


I don't think this is used.

2xsaiko · 2024-01-12T22:07:54Z

pkgs/servers/mail/dovecot/plugins/pigeonhole/compile.nix

+    for k in $(find * -name \*.sieve); do
+      mkdir -p "$out/sieve/$(dirname "$k")"
+      cp -a "$k" "$out/sieve/$k"
+      if [[ ! -L "$k" ]]; then
+        echo "Compiling $k"
+        touch -m --date=@0 "$out/sieve/$k"
+      else
+        echo "Compiling $k -> $(readlink -f "$k")"
+      fi
+      sievec -c "${config}" "$out/sieve/$k"
+    done


Suggested change

for k in $(find * -name \*.sieve); do

mkdir -p "$out/sieve/$(dirname "$k")"

cp -a "$k" "$out/sieve/$k"

if [[ ! -L "$k" ]]; then

echo "Compiling $k"

touch -m --date=@0 "$out/sieve/$k"

else

echo "Compiling $k -> $(readlink -f "$k")"

fi

sievec -c "${config}" "$out/sieve/$k"

done

while IFS= read -r k; do

mkdir -p "$out/sieve/$(dirname "$k")"

cp -a "$k" "$out/sieve/$k"

if [[ ! -L "$k" ]]; then

echo "Compiling $k"

touch -m --date=@0 "$out/sieve/$k"

else

echo "Compiling $k -> $(readlink -f "$k")"

fi

sievec -c "${config}" "$out/sieve/$k"

done < <(find -name '*.sieve')

to handle files with spaces

2xsaiko · 2024-01-12T22:13:41Z

nixos/modules/services/mail/dovecot.nix

+    else pkgs.writeText "script.sieve" text;
+  scriptType = types.coercedTo types.lines sieveToPath types.path;
+
+  compileSieveScripts = pkgs.buildSieveScripts {


Suggested change

compileSieveScripts = pkgs.buildSieveScripts {

compiledSieveScripts = pkgs.buildSieveScripts {

Does compiling these in advance break include for non-store paths? Someone may be using that for dynamically generated rules. If it does, would they be doable another way?

dotlambda · 2024-02-07T02:45:13Z

Can this be closed now that #275031 was merged?

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Feb 25, 2018

griff mentioned this pull request Feb 27, 2018

Upstream from my fork r-raymond/nixos-mailserver#61

Open

uwap reviewed Apr 16, 2018

View reviewed changes

ryantm added the 2.status: merge conflict label Feb 26, 2019

griff mentioned this pull request Aug 19, 2019

dovecot-pigeonhole: tries to recompile sieve scripts in /nix/store #66410

Open

stale bot added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md and removed 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Jun 1, 2020

ryantm added 2.status: merge conflict and removed 2.status: merge conflict labels Oct 3, 2020

griff force-pushed the dovecot-precompile branch from aa03ee2 to 957439e Compare October 20, 2020 22:56

ofborg bot removed the 2.status: merge conflict label Oct 20, 2020

ofborg bot requested a review from globin October 20, 2020 23:20

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 labels Oct 20, 2020

nlewo reviewed Oct 25, 2020

View reviewed changes

griff force-pushed the dovecot-precompile branch from 957439e to 1e6aad5 Compare October 29, 2020 22:21

SuperSandro2000 added the 2.status: merge conflict label Jan 18, 2021

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 11, 2021

griff force-pushed the dovecot-precompile branch from 1e6aad5 to ccd41f7 Compare October 11, 2021 19:08

ofborg bot removed the 2.status: merge conflict label Oct 11, 2021

ofborg bot requested a review from ajs124 October 11, 2021 19:23

SuperSandro2000 reviewed Oct 19, 2021

View reviewed changes

griff force-pushed the dovecot-precompile branch from ccd41f7 to cbcff2d Compare October 20, 2021 13:58

SuperSandro2000 reviewed Nov 16, 2021

View reviewed changes

griff force-pushed the dovecot-precompile branch from cbcff2d to e513ff5 Compare April 20, 2022 14:15

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 30, 2022

griff added 3 commits November 1, 2023 18:42

nixos/tests/dovecot: test sieveScripts option

f1781ee

dovecot_pigeonhole: patch and builder for precompiled sieve scripts

d63fe23

nixos/dovecot: more sieve options and precompile sieve scripts

0115539

griff force-pushed the dovecot-precompile branch from e513ff5 to 0115539 Compare November 2, 2023 10:22

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 2, 2023

RaitoBezarius requested a review from 2xsaiko January 12, 2024 19:24

2xsaiko suggested changes Jan 12, 2024

View reviewed changes

SuperSandro2000 closed this Feb 17, 2024

	${dovecot_pigeonhole}/bin/sievec -c "${config}" "$out/sieve/$k"
	sievec -c "${config}" "$out/sieve/$k"

	if [ -d '${v}' ]; then
	if [[ -d ${lib.escapeShellArg v} ]]; then

		@@ -1,4 +1,4 @@
		{ lib, stdenv, fetchurl, dovecot, openssl }:
		{ lib, stdenv, fetchpatch, fetchurl, dovecot, openssl }:

	compileSieveScripts = pkgs.buildSieveScripts {
	compiledSieveScripts = pkgs.buildSieveScripts {

nixos/dovecot: More sieve options and precompiling of scripts #35536

nixos/dovecot: More sieve options and precompiling of scripts #35536

Conversation

griff commented Feb 25, 2018 • edited

Motivation for this change

test of sieve scripts

Patching dovecot_pigeonhole and builder to precompile scripts

Sieve related options in the dovecot2 module

Things done

dotlambda commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 1, 2020

wmertens commented Jun 1, 2020

griff commented Oct 20, 2020

nlewo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jul 21, 2021

poscat0x04 commented Oct 11, 2021

nixos-discourse commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaitoBezarius commented Dec 1, 2022

griff commented Nov 2, 2023

RaitoBezarius commented Jan 12, 2024

2xsaiko commented Jan 12, 2024

2xsaiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotlambda commented Feb 7, 2024

griff commented Feb 25, 2018 •

edited