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

mail module additions #318

Closed
wants to merge 5 commits into from
Closed

mail module additions #318

wants to merge 5 commits into from

Conversation

teto
Copy link
Collaborator

@teto teto commented Jul 22, 2018

Adds msmtp, offlineimap, alot configurations.

I updated my previous code to fit the new system and removed some coupling between modules to make reviewing easier.

@@ -0,0 +1,8 @@
{ config, lib }:
{
isGmail = account:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ config, lib, pkgs, ... } @ top:

with lib;
with import ../lib/dag.nix { inherit lib; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better remove this and add

let

  cfg = config.programs.alot;

  dag = config.lib.dag;

  …

instead.

thread_indent_replies = 4;
'';
description = ''
Extra configuration lines to add to ~/.config/alot/config.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either need some markup here, e.g.,

Extra configuration lines to add to <filename>~/.config/alot/config</filename>.

or rewording it, e.g.,

Extra lines to add to the alot configuration file.

default = ''
auto_remove_unread = True;
ask_subject = False;
handle_mouse = "True;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is some syntax error? I guess a quote has to be terminated by another quote…

extraConfig = mkOption {
type = types.lines;
default = ''
auto_remove_unread = True;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does alot configuration lines really end with a semicolon?


notmuchConfig = account: "$XDG_CONFIG_HOME/notmuch/notmuchrc";

alot_accounts = config.accounts.email.accounts;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe need to filter out those accounts for which alot.enable == true? Something like

filter (a: a.alot.enable) (attrValues config.accounts.email.accounts)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing alot requires notmuch to be enabled for the account? If so, then it's probably best to add an assertion along the lines of

assertions = [
  (
    let
      badAccounts = filter (a: !a.notmuch.enable) alotAccounts;
    in
      {
        assertion = badAccounts == [];
        message = "alot: Notmuch not enabled for accounts: "
          + concatMapStringsSep ", " (a: a.name) badAccounts;
      }
  )

  {
    assertion = config.notmuch.enable;
    message = "Notmuch not enabled";
  }
];

Note, not tested and the message phrasing could be improved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather have alot enable notmuch automatically ?! (yes notmuch is mandatory)

@@ -0,0 +1,145 @@
# alot config loader is sensitive to leading space !
{ config, lib, pkgs, ... } @ top:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove @ top since top doesn't seem to be used.

Copy link
Member

@rycee rycee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I've begun adding comments but don't have time to go through everything right now. I'll continue at some later time.

Also it would be nice to have the commit split into one commit per program. That would make the PR easier to review and the Git history would be cleaner.


notmuchConfig = account: "$XDG_CONFIG_HOME/notmuch/notmuchrc";

alot_accounts = config.accounts.email.accounts;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use camel case: alotAccounts.


cfg = config.programs.alot;

notmuchConfig = account: "$XDG_CONFIG_HOME/notmuch/notmuchrc";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The account argument is not used so I guess it could be removed? Also, $XDG_CONFIG_HOME might not be set, in which case this path would be incorrect. I'd suggest to use

"${config.xdg.configHome}/notmuch/notmuchrc"

instead. Or even notmuchConfig = "$NOTMUCH_CONFIG" since the Notmuch module sets that session variable.


# contact completion
${contactCompletionStr account}
${if showSignature == "attach" then "gpg_key = {account." else ""}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "gpg_key = {account." intentional? Looks like a mistake 🙂

B.t.w, could also use optionalString, e.g.,

${optionalString (showSignature == "attach") "gpg_key = {account."}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the gpg aspect. This can be added a posteriori

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've left gpg support undefined in the accounts module for now. I believe it should be included as some point but I'd prefer to think about it first so that the design is sufficiently good.

pkgs.writeText "alot.conf" (''
# Generated by home-manager
# see http://alot.readthedocs.io/en/latest/configuration/config_options.html
hooksfile = "$XDG_CONFIG_HOME/alot/hm_hooks"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this would result in the wrong path if $XDG_CONFIG_HOME is not set. I would suggest using

hooksfile = "${config.xdg.configHome}/alot/hm_hooks"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact I would prefer to rely on the environment variable so I've added a fallback ${XDG_CONFIG_HOME:-$HOME/.config}. Hardcoding the path kinda break the purpose of environment variables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I'm happy as long as it is able to cope with the variable being missing.

description = "Can override what is decided in contactCompletion";
};

bindings = mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at the alot manual and perhaps this could be a submodule? Something like

bindings = {
  global = {
    "mouse press 4" = "move up";
    # …
  };

  bufferlist = {
    x = "close";
    # …
  };

  search = {
    enter = "select";
    # …
  };

  envelope = {
    a = "prompt 'attach ~/'";
    # …
  };

  taglist = {
    enter = "select";
    # …
  };

  thread = {
    enter = "select";
    # …
  };
};

I.e., "global", "envelope", "search", "thread", "taglist", and "bufferlist" are options inside the submodule and each of them has a type such as types.attrsOf types.str.


contactCompletionCommand = mkOption {
default = contactCompletionStr;
description = "Can override what is decided in contactCompletion";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some better description that doesn't assume knowledge of the module implementation.

if cfg.offlineSendMethod == "native" then
"${pkgs.msmtp}/bin/msmtpq -C $XDG_CONFIG_HOME/msmtp/config --account=${account.name} -t"
else
"${pkgs.msmtp}/bin/msmtp $XDG_CONFIG_HOME/msmtp/config --account=${account.name} -t";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't assume that $XDG_CONFIG_HOME is set, better use ${config.xdg.configHome}.

accountStr = {userName, address, realName, ...} @ account: ''
defaults
tls on
logfile ~/.msmtp.log
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps config.xdg.dataHome is a better location for the log file?

else "";

accountStr = {userName, address, realName, ...} @ account: ''
defaults
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, won't this produce a defaults section for each account? I would assume it's best to only have one defaults section in the configuration.

''passwordeval bash -c "echo $(${toString account.passwordCommand})"''
else "";

accountStr = {userName, address, realName, ...} @ account: ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps simpler to do

accountStr = account: with account; ''

'';

?

@teto
Copy link
Collaborator Author

teto commented Jul 28, 2018

I would like to automatically enable notmuch for every account using alot. This

  config = mkIf cfg.enable {


    accounts.email = let
        enableNotmuch = account: 
          { account.notmuch.enable = true; };
      in
        mkMerge ( map enableNotmuch alotAccounts);
}

but this generates an infinite loop. any idea ?

# generates a script to fetch only a specific account
genOfflineImapScript = {name, ...} @ account:
pkgs.writeShellScriptBin "offlineimap-${name}" ''
${pkgs.offlineimap}/bin/offlineimap -a${account.name} -c "$XDG_CONFIG_HOME/offlineimap/config" $@
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the -c flag here since, from what I can tell, offlineimap will look for a config in $XDG_CONFIG_HOME/offlineimap/config by default: OfflineIMAP/offlineimap@5150de5

'';

localTypeStr = account:
if isGmail account then "GmailMaildir" else "Maildir";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use the account flavor here.

else "";

sslStr = account:
if traceVal (account.imap.tls.enable) then ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove the trace here 🙂


postSyncHookStr = account:
if (builtins.stringLength account.offlineimap.postSyncHookCommand > 0) then
"postsynchook = ${account.offlineimap.postSyncHookCommand}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if the command is multiple lines will this not confuse offlineimap? The config file would contain something like

postsynchook = cmd1
cmd2
cmd3

Perhaps better to write the command to a shell script (writeScript) and then put the path to this script here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have the command in the config file rather than having another level of indirection.
The user can deal with it himself or we can revisit if there are problems ?

''
else "ssl = no";

accountStr = {name, userName, realName, ...} @ account:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can simplify using simply accountStr = account: with account; …

maxage=30
'';
description = ''
Extra configuration lines added to alot config.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alot?

return res
'';
description = ''
Python code that can then be used in alot extraConfig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alot?

options = [
{
offlineimap = {
enable = mkEnableOption "notmuch indexing";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mkEnableOption "OfflineIMAP"


options = {
programs.offlineimap = {
enable = mkEnableOption "Offlineimap";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The website says "OfflineIMAP" so might be nice to use that here as well.


xdg.configFile."offlineimap/get_settings.py".text = cfg.pythonFile;

xdg.configFile."offlineimap/config".source = configFile accounts;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simpler to just use .text here as well.

msmtp is a simple mail transfer agent (MTA).
... is a Mail Retreival Agent (MRA) like mbsync but in python.
alot is a python mail user agent (MUA) built around a notmuch database.
to help with some autoconfiguration
so that MUAs can more easily look for contacts.
@teto
Copy link
Collaborator Author

teto commented Aug 6, 2018

Enabling alot will now enable notmuch.
I splitted the commits into module chunks but there is still some interaction between the commits so most likely you can't revert each of them individually. I managed to get a working setup again with this code and thanks to your reviews I would consider this RFC/RDY.

This can be improved for sure (signature settings/gpg support) but this can be done at later date.

@teto
Copy link
Collaborator Author

teto commented Aug 21, 2018

I will try to open one PR per module since that will help I think. I wonder how you want to proceed with the lingering PRs: (procmail/getmail etc), e.g., push into master or push everything into a mail_related branch to not break master ? We could tag a release on master before potentially "breaking" it.

@teto teto mentioned this pull request Aug 21, 2018
@teto
Copy link
Collaborator Author

teto commented Aug 30, 2018

among the different modules, there is only alot left which I already opened a separate PR for so let's close this.

@teto teto closed this Aug 30, 2018
@teto teto added mail and removed mail labels Nov 20, 2018
@teto teto deleted the alot_clean branch August 15, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants