Skip to content

imapnotify: Add launchd agent#3291

Merged
ncfavier merged 4 commits intonix-community:masterfrom
dbaynard:imapnotify-launchd
Jul 7, 2023
Merged

imapnotify: Add launchd agent#3291
ncfavier merged 4 commits intonix-community:masterfrom
dbaynard:imapnotify-launchd

Conversation

@dbaynard
Copy link
Copy Markdown
Contributor

@dbaynard dbaynard commented Sep 30, 2022

Description

This extends the imapnotify (goimapnotify) configuration with a launchd agent that matches the systemd unit (and shares as much code as possible).

Annoyingly I'd forgotten that goimapnotify is malconfigured in nixpkgs, in that it works (and is supported) on macos, but the metadata platforms are linux only. So while this is blocked on that, I'm making the PR now, and I'll update it when the upstream change is merged.

Edit: NixOS/nixpkgs#193797 (now merged, awaiting and deployed deployment to nixos-unstable)

I added a semi-golden test: it uses the goimapnotify store path, so I've passed that as a parameter. I've seen the pkgs.writeText pattern used in other tests, but I didn't spot this case, so I hope it's ok.

I've seen that #2915 is still open; if this is merged first, there'll be additonal redundant platform checking, from this.

I've a few comments that I'll put in a self-review.

Checklist

  • Change is backwards compatible. (but the version of goimapnotify that lists darwin in platforms is only in master, at the moment)

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

Comment thread modules/services/imapnotify.nix
Comment thread tests/modules/programs/goimapnotify/default.nix
@dbaynard
Copy link
Copy Markdown
Contributor Author

dbaynard commented Oct 11, 2022

goimapnotify at master has the correct platforms for darwin (though I'm not sure this is being backported). All builds in nixpkgs suported by home-manager should work, but require the following override to build (I'm sure there's a more elegant way of doing it with lib.recursiveUpdate but 🤷 ). I’m not sure how to handle these circumstances (specifically, CI failing).

drv: drv.overrideAttrs (old: {
    meta = old.meta or { } // {
      platforms = old.meta.platforms or [ ] ++ lib.platforms.darwin;
    };
  });

@dbaynard dbaynard marked this pull request as ready for review October 11, 2022 18:06
@dbaynard dbaynard requested review from NickHu and rycee as code owners October 11, 2022 18:06
@dbaynard
Copy link
Copy Markdown
Contributor Author

I'm just pushing a commit that should get macos CI showing green.

@dbaynard
Copy link
Copy Markdown
Contributor Author

NixOS 22.11 has the working version of goimapnotify so I'll make the necessary changes to this in the next week.

@dbaynard
Copy link
Copy Markdown
Contributor Author

I'm happy with this, the tests work, and the upstream dependency requires no changes. Ready to go!

@stale
Copy link
Copy Markdown

stale bot commented Mar 17, 2023

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Mar 17, 2023
Comment thread modules/services/imapnotify.nix Outdated
Comment thread modules/services/imapnotify.nix Outdated
Comment thread modules/services/imapnotify.nix Outdated
@dbaynard
Copy link
Copy Markdown
Contributor Author

Thanks for the review — I'll rebase, make the changes and confirm everything is ok, then ping for re-review.

@dbaynard
Copy link
Copy Markdown
Contributor Author

This is still on my roadmap; I'll get to it in the next few months (hopefully soon).

@teto teto added the mail HM email accounts, thunderbird, alot, notmuch, msmtp, meli... label May 15, 2023
@dbaynard dbaynard force-pushed the imapnotify-launchd branch 2 times, most recently from 3611e3e to c322415 Compare June 1, 2023 18:10
@dbaynard
Copy link
Copy Markdown
Contributor Author

dbaynard commented Jun 1, 2023

I've rebased but I haven't been able to figure out the test failure.

Here's my test.

    nmt.script = let
      serviceFileName =
        "org.nix-community.home.imapnotify-hm-example.com.plist";
    in ''
      serviceFile=LaunchAgents/${serviceFileName}
      assertFileExists $serviceFile
      assertFileContent $serviceFile ${./launchd.plist}
    '';
Expected LaunchAgents/org.nix-community.home.imapnotify-hm-example.com.plist to be same as /nix/store/shpsifrkxyil9gd65295q5kx0ihyqvwx-launchd.plist but were different:
--- actual
+++ expected
@@ -17,7 +17,7 @@
        <string>Background</string>
        <key>ProgramArguments</key>
        <array>
-               <string>/nix/store/ib1yskg8kk1hxzn32yi3sjjpphq45sna-goimapnotify/bin/goimapnotify</string>
+               <string>@goimapnotify@/bin/goimapnotify</string>
                <string>-conf</string>
                <string>/home/hm-user/.config/imapnotify/imapnotify-hm-example.com-config.json</string>
        </array>

I don't see how the call to assertFileContent differs from the helix example.

''
      assertFileContent \
        home-files/.config/helix/languages.toml \
        ${./languages-expected.toml}
''

In both cases the relevant parameter comes from lib.test.mkStubPackage { name = "pkgname" }. I tried with the argument outPath, with name and with both, and in all cases it generated (a variant of) the error here, where the generated code includes a store path, not the path relative to @goimapnotify@.

It's plausible that this behaves differently on launchd units — I haven't looked through the code for the tests. Given the tests did pass, this seems like a regression. (Edit: Perhaps something to do with canonicalizing paths, as that would explain why it prints the nix store path rather than the one relative to outDir.)

Any suggestions?

@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented Jun 1, 2023

The problem is that getExe/getBin does not play well with our stubs, because we just naively override outPath but this does not take care of pkg.out. I'll try a fix later.

@dbaynard
Copy link
Copy Markdown
Contributor Author

dbaynard commented Jun 2, 2023

OK; no rush on my part. Let me know if you'd like me to take a look at it, too. Thanks!

@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented Jun 2, 2023

#4050

This was not the only problem with this PR: you'll also have to pass outPath = "@goimapnotify@"; to mkStubPackage, as the other tests do.

@ncfavier ncfavier mentioned this pull request Jun 2, 2023
6 tasks
@dbaynard dbaynard force-pushed the imapnotify-launchd branch 2 times, most recently from 53ac7b2 to e60bd5d Compare June 3, 2023 21:41
@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented Jun 4, 2023

Rebasing to master should fix the tests now.

dbaynard added 4 commits June 5, 2023 00:17
There are multiple packages that provide an imapnotify interface. Those
packages have differently named executables. This can now be customized.

This change also means test configurations can use stub packages.
Exposing the configuration file makes testing imapnotify configurations much
easier. It also allows for golden tests in home-manager.
Now that home-manager supports launchd agents, the imapnotify service
can be configured (and enabled) for darwin. The configuration matches
that of the linux/systemd version. In particular, by not setting a
`UserName`, this runs as the user whose configuration includes the
module.

Due to the launchd `Program` implementation (it must take an absolute
path) it is not possible to use that for the program and stub the path
in tests. Instead, this uses `ProgramArguments` for the program name.

The `ThrottleInterval` is equivalent to `RestartSec`. `KeepAlive` is
equivalent to `Restart`.

The `ExitTimeOut` default is 20 seconds, but goimapnotify should not
time out — this is achieved by setting the `ExitTimeout` to 0.
This only tests the generated plist (which is new), not the original
systemd implementation, nor the json config file.

(Note the lack of a newline at the end of the plist file.)
@dbaynard dbaynard force-pushed the imapnotify-launchd branch from e60bd5d to 799525f Compare June 4, 2023 23:21
@dbaynard dbaynard requested a review from ncfavier June 5, 2023 01:01
@kciredor
Copy link
Copy Markdown

kciredor commented Jul 7, 2023

Very eager to use this! Thanks @dbaynard and @ncfavier for the upcoming review 😀

@ncfavier ncfavier merged commit 719de87 into nix-community:master Jul 7, 2023
@kciredor
Copy link
Copy Markdown

kciredor commented Jul 7, 2023

That was fast, thanks 🥳

Comment thread modules/services/imapnotify.nix
rycee pushed a commit to dbaynard/home-manager that referenced this pull request Jul 9, 2023
As pointed out in nix-community#3291, using the XDG symlink means the agent/unit
files don’t change when the contents of the config changes, and so the
service will not be restarted.
antholeole pushed a commit to antholeole/home-manager that referenced this pull request Jul 24, 2023
* imapnotify: expose package (and exe) options

There are multiple packages that provide an imapnotify interface. Those
packages have differently named executables. This can now be customized.

This change also means test configurations can use stub packages.

* imapnotify: use/create config in configHome

Exposing the configuration file makes testing imapnotify configurations much
easier. It also allows for golden tests in home-manager.

* imapnotify: extend with launchd agent

Now that home-manager supports launchd agents, the imapnotify service
can be configured (and enabled) for darwin. The configuration matches
that of the linux/systemd version. In particular, by not setting a
`UserName`, this runs as the user whose configuration includes the
module.

Due to the launchd `Program` implementation (it must take an absolute
path) it is not possible to use that for the program and stub the path
in tests. Instead, this uses `ProgramArguments` for the program name.

The `ThrottleInterval` is equivalent to `RestartSec`. `KeepAlive` is
equivalent to `Restart`.

The `ExitTimeOut` default is 20 seconds, but goimapnotify should not
time out — this is achieved by setting the `ExitTimeout` to 0.

* imapnotify: add launchd plist test

This only tests the generated plist (which is new), not the original
systemd implementation, nor the json config file.

(Note the lack of a newline at the end of the plist file.)
antholeole pushed a commit to antholeole/home-manager that referenced this pull request Jul 24, 2023
As pointed out in nix-community#3291, using the XDG symlink means the agent/unit
files don’t change when the contents of the config changes, and so the
service will not be restarted.
SimSaladin added a commit to SimSaladin/home-manager that referenced this pull request Aug 26, 2023
* upstream/master: (352 commits)
  flake.lock: Update
  qcal: add module
  bat: generate cache file in XDG cache home
  nixos: only refer to `users.users` if needed
  home-cursor: remove IFD when linking icon directories
  flake.lock: Update
  home-cursor: improve icon compatibility
  fix(qt): allow theming for apps started by systemd (nix-community#4349)
  flake.lock: Update
  fcitx5: add `fcitx5-config-qt` to test stub
  pqiv: only run tests on Linux platforms
  pqiv: add module
  Translate using Weblate (German)
  xplr: add module
  browserpass: support librewolf
  Translate using Weblate (Romanian)
  Translate using Weblate (Portuguese (Brazil))
  hyprland: remove xwayland.hidpi (nix-community#4302)
  home-manager: handle profile list in Nix >2.17
  lf: simplify option validation (nix-community#4334)
  programs.khal: uncomment locale config (nix-community#4319)
  kanshi: support adaptive sync (nix-community#4328)
  mu: add package option (nix-community#4325)
  modules: types.string throws error now (nix-community#4324)
  taffybar: Avoid restarting too quickly (nix-community#4316)
  aerc: do not use smtp-starttls (nix-community#4272)
  zsh: Add `zsh.history.ignoreAllDups` config option (nix-community#4248)
  jujutsu: fix zsh completion (nix-community#4305)
  Translate using Weblate (Korean)
  Translate using Weblate (Romanian)
  Translate using Weblate (German)
  flake.lock: Update
  network-manager-applet: remove `--sm-disable` flag
  himalaya: update a link to ticket
  nushell: deprecation of let-env (nix-community#4292)
  man: fix caches generation in cross-compiled system (nix-community#4294)
  fish: fix session vars build in cross-compiled system (nix-community#4293)
  Update translation files
  Translate using Weblate (Chinese (Simplified))
  gh: option to enable helper for additional hosts (nix-community#4288)
  home-manager: rework news command
  home-manager: skip manual in uninstall configuration
  gnome-terminal: add assertion on profile names
  hyprland: use `toKeyValue`'s `indent` argument (nix-community#4274)
  gpg: fix typo (nix-community#4277)
  firefox: make package nullable (nix-community#4113)
  git-sync: add news entry for darwin
  tmate: don't generate empty config file (nix-community#4271)
  gh-dash: add module
  git-sync: add darwin support
  flake.lock: Update
  hyprland: prioritize variables and beziers (nix-community#4263)
  hyprland: add module
  script-directory: fix documentation link (nix-community#4258)
  Translate using Weblate (Indonesian)
  Translate using Weblate (Ukrainian)
  Translate using Weblate (Spanish)
  Translate using Weblate (Swedish)
  docs: hide `_module.*` from NixOS/nix-darwin docs
  jujutsu: update for Jujutsu 0.8.0 (nix-community#4250)
  ripgrep: don't set env. variable if no config (nix-community#4254)
  docs: update for Markdown migration
  treewide: remove now-redundant `lib.mdDoc` calls
  docs: clean up after Markdown migration
  flake: remove temporary nixpkgs pin
  treewide: convert all option docs to Markdown
  treewide: adjust some DocBook for conversion
  treewide: `mkAliasOption` -> `mkAliasOptionMD`
  treewide: `mkPackageOption` -> `mkPackageOptionMD`
  treewide: convert options with tables to Markdown
  treewide: convert options with lists to Markdown
  treewide: convert custom `enable` docs to Markdown
  treewide: convert parameterized docs to Markdown
  treewide: manually convert some docs to Markdown
  treewide: fix `mkEnableOption` arguments
  treewide: add missing option descriptions
  docs: use `nixosOptionsDoc`
  flake: temporarily pin nixpkgs version
  version: add `isReleaseBranch`
  manual: add test
  Update translation files
  flake.lock: Update
  home-manager: remove Home Manager default paths
  imapnotify: Use JSON type for extraConfig (nix-community#4238)
  Translate using Weblate (French)
  zsh: fix custom syntax highlighting styles (nix-community#4236)
  i3-sway: allow arbitrary floating modifier (nix-community#4229)
  i3-sway: multiple outputs (nix-community#4223)
  home-cursor.nix: enable gtk module when enabling gtk config generation (nix-community#4144)
  aerc: add assertion to limit per-account extraConfig to UI config (nix-community#4196)
  flake.lock: Update
  ssh-agent: add assertion and fix news entry (nix-community#4210)
  imapnotify: move test
  imapnotify: use direct nix store path for config
  flake.lock: Update
  nushell: add login.nu configuration option
  himalaya: fix notmuch backend
  swayosd: add module
  pyenv: add module
  darcs: add module
  tests: some minor cleanups
  goimapnotify: remove test dependency on notmuch
  unison: Allow using same option multiple times (nix-community#4208)
  imapnotify: Add launchd agent (nix-community#3291)
  i3: remove deprecated example in i3.config.startup (nix-community#4201)
  antidote: static file move to /tmp
  Translate using Weblate (Indonesian)
  PULL_REQUEST_TEMPLATE.md: add maintainer cc section (nix-community#4193)
  programs.khal
  vdirsyncer: synchronize metadata as well (nix-community#4167)
  i18n: Use glibcLocales from NixOS if possible (nix-community#2333) (nix-community#4177)
  xfconf: remove properties with null values (nix-community#4192)
  home-manager: Use `path:` URI type for flake default (nix-community#3646)
  ci: autolabel calendars PRs (nix-community#4165)
  flake.lock: Update (nix-community#4161)
  news: fix typo in programs.zsh.antidote entry
  ssh-agent: init module (nix-community#4178)
  chromium: fix `commandLineArgs` to use the user specified package (nix-community#4175)
  sxhkd: allow usage of derivations as keybind commands (nix-community#4169)
  README: Remove the pills (nix-community#4181)
  lsd: use -A instead of -a in aliases (nix-community#4173)
  kakoune: add defaultEditor option
  docs: update link to allowed users setting (nix-community#4176)
  zsh: allow setting custom syntax highlighting styles (nix-community#4122)
  flake: add formatter (nix-community#3620)
  broot: fix test (nix-community#4170)
  antidote: fix .dot path
  ci: build manual and push to home-manager.dev
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mail HM email accounts, thunderbird, alot, notmuch, msmtp, meli...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants