Add multiple accounts support to himalaya-watch

jalil-salame commented 7 months ago

Description

Currently services.himalaya-watch only supports selecting a single account to watch for changes, I have multiple accounts using himalaya and I would like to watch all of them.

I tried my hand at a patch, but the way I set it up you cannot replicate the behavior of choosing the primary account by default (you need to specify all by name):

himalaya-watch patch

```patch modules/programs/himalaya.nix | 55 ++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/modules/programs/himalaya.nix b/modules/programs/himalaya.nix index 2d216c3d..afad4a18 100644 --- a/modules/programs/himalaya.nix +++ b/modules/programs/himalaya.nix @@ -131,10 +131,10 @@ in { ''; }; - settings.account = lib.mkOption { - type = with lib.types; nullOr str; - default = null; - example = "personal"; + settings.accounts = lib.mkOption { + type = with lib.types; listOf str; + default = [ ]; + example = [ "personal" ]; description = '' Name of the account the watcher should be started for. If no account is given, the default one is used. @@ -171,31 +171,32 @@ in { globalConfig = compactAttrs himalaya.settings; allConfig = globalConfig // { accounts = accountsConfig; }; in tomlFormat.generate "himalaya-config.toml" allConfig; - systemd.user.services = let - inherit (config.services.himalaya-watch) enable environment settings; - optionalArg = key: - if (key ? settings && !isNull settings."${key}") then - [ "--${key} ${settings."${key}"}" ] - else - [ ]; - in { - himalaya-watch = lib.mkIf enable { - Unit = { - Description = "Email client Himalaya CLI envelopes watcher service"; - After = [ "network.target" ]; - }; + systemd.user.services = + let inherit (config.services.himalaya-watch) enable environment settings; + in lib.genAttrs + (builtins.map (account: "himalaya-watch@${account}") settings.accounts) { Install = { WantedBy = [ "default.target" ]; }; - Service = { - ExecStart = lib.concatStringsSep " " - ([ "${himalaya.package}/bin/himalaya" "envelopes" "watch" ] - ++ optionalArg "account"); - ExecSearchPath = "/bin"; - Environment = - lib.mapAttrsToList (key: val: "${key}=${val}") environment; - Restart = "always"; - RestartSec = 10; + } // { + "himalaya-watch@" = lib.mkIf enable { + Unit = { + Description = "Email client Himalaya CLI envelopes watcher service"; + After = [ "network.target" ]; + }; + Service = { + ExecStart = lib.concatStringsSep " " [ + "${himalaya.package}/bin/himalaya" + "envelopes" + "watch" + "--account" + "%i" + ]; + ExecSearchPath = "/bin"; + Environment = + lib.mapAttrsToList (key: val: "${key}=${val}") environment; + Restart = "always"; + RestartSec = 10; + }; }; }; - }; }; } -- 2.43.0 ```

This would remove the need for the enable option too (it would be enabled if accounts is non-empty). Currently it uses template unit, so the enable option does allow for people to dynamically enable more accounts using systemctl --user enable --now himalaya-watch@$account.service, but I am thinking it should be better to directly write the services instead of using a template unit.

Another thing I am considering is how to escape characters; right now my accounts are named after the email address I use (i.e. jalil@example.org) and if I remember correctly, systemd wants those characters (. and @) to be escaped and hex encoded (himalaya-watch@jalil\x40example\x46org.service).

jalil-salame commented 7 months ago

@soywod you probably want to add such a template service to himalaya itself. I would be glad to contribute it c:

soywod commented 7 months ago

Currently services.himalaya-watch only supports selecting a single account to watch for changes, I have multiple accounts using himalaya and I would like to watch all of them.

I thought about this last time, it is a very good point!

I tried my hand at a patch, but the way I set it up you cannot replicate the behavior of choosing the primary account by default (you need to specify all by name)

You can name the default account with the special string "default", so technically you can have a unit himalaya-watch@default. But we can also create a dedicated non-templated service for the default account.

but I am thinking it should be better to directly write the services instead of using a template unit

I like your idea of template unit, it fits well the need in my opinion.

Another thing I am considering is how to escape characters

This may be a pain point. What about letting users mapping their own names? Instead of settings.accounts being a list, we could have a set where the key would be the account name, and the value the template unit name?

@soywod you probably want to add such a template service to himalaya itself

In which form do you see this addition? As a template file in /assets directory? As a documentation entry in the FAQ?

jalil-salame commented 7 months ago

You can name the default account with the special string "default", so technically you can have a unit himalaya-watch@default. But we can also create a dedicated non-templated service for the default account.

O.O Then I'll add that to the module's documentation.

This may be a pain point. What about letting users mapping their own names? Instead of settings.accounts being a list, we could have a set where the key would be the account name, and the value the template unit name?

That would work if we are not using a systemd template, the template passes everthing after the @ to the unit (i.e. himalaya-watch@personal.service will replace %i with personal. I am using --account %i so it is required for it to be the account name (which is why I thought about not using the template service). Another option is to just warn against using special characters in the account names c:

In which form do you see this addition? As a template file in /assets directory? As a documentation entry in the FAQ?

In assets so that other packages can make use of it benefiting people not using NixOS.

soywod commented 7 months ago

I am using --account %i so it is required for it to be the account name (which is why I thought about not using the template service).

I understand better. How do you imagine the version without template?

In assets so that other packages can make use of it benefiting people not using NixOS.

It makes sense, feel free to send a git patch whenever you want :)

jalil-salame commented 7 months ago

I am using --account %i so it is required for it to be the account name (which is why I thought about not using the template service).

I understand better. How do you imagine the version without template?

I imagine enumerating the accounts and creating a unit for each one of them.

In assets so that other packages can make use of it benefiting people not using NixOS.

It makes sense, feel free to send a git patch whenever you want :)

Already did c:

soywod commented 7 months ago

I imagine enumerating the accounts and creating a unit for each one of them.

But you would not have the same issue with the unit names?

Already did c:

How fast :D thanks a lot!

jalil-salame commented 7 months ago

I imagine enumerating the accounts and creating a unit for each one of them.

But you would not have the same issue with the unit names?

I was planning to assing them sequential numbers:

himalaya-watch-1.service, etc...

It's not very nice, but it fixes the issue...

jalil-salame commented 7 months ago

Just did a quick search, and there is systemd-escape.

You can do systemd-escape --template=himalaya-watch@.service $account and it will return the escaped unit name.

I don't know how to hook it into nix, but it should probably be added to the himalaya docs if you ship the template service.

soywod commented 7 months ago

Just did a quick search, and there is systemd-escape.

So great!

I don't know how to hook it into nix, […]

Good point, I don't see how to call this program from within Nix code. I tried to search for modules in nixpkgs solving this issue but I did not find anything relevant.

I have 2 ideas coming to my mind:

We let the user setting up the unit name in a set "{ account-name = unit-name; }", this way we can build multiple services "himalaya-watch..service",

or,

The "unit name prefix" must consist of one or more valid characters (ASCII letters, digits, ":", "-", "_", ".", and "\")

we develop our own Nix function to escape unit names, for example replacing all invalid chars by "-". This way we can use systemd templates. We can just explain in the doc that unit names are kebab-case-like version of account names.

but it should probably be added to the himalaya docs if you ship the template service.

Totally agree, we can even add an example with systemd-escape.

soywod commented 5 months ago

The new v1.0.0-beta.4 is out, so it's the right moment to update the home manager package. I open a PR with your patch modified (https://github.com/nix-community/home-manager/pull/5297). I found the missing pieces:

%I: Same as "%i", but with escaping undone. [doc]

If you use the service with a non-escaped name, systemd do it for you with a warning:

$ systemctl --user status himalaya-watch@bad~name
Invalid unit name "himalaya-watch@bad~name" escaped as "himalaya-watch@bad\x7ename" (maybe you should use systemd-escape?).
○ himalaya-watch@bad\x7ename.service - Email client Himalaya CLI envelopes watcher service
   Loaded: loaded (/home/soywod/.config/systemd/user/himalaya-watch@.service; disabled; preset: enabled)
   Active: inactive (dead)

I would love your feedback.

I open the issue as draft, as I also plan to integrate xdg.desktopEntries.

jalil-salame commented 5 months ago

I would love your feedback.

I open the issue as draft, as I also plan to integrate xdg.desktopEntries.

I'm struggling to find time, but I'll take a look when I can!

stale[bot] commented 2 months ago

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

jalil-salame commented 2 months ago

I fully forgot about this; himalaya 1.0.0-beta4 has an open bug with some email accounts which affects me so I wasn't able to test it. Sorry about the radio silence.

nix-community / home-manager

Add multiple accounts support to himalaya-watch #5069

Description