Skip to content

launchd: initial support for LaunchAgents#2497

Merged
rycee merged 1 commit intonix-community:masterfrom
midchildan:feat/launchagents
Feb 26, 2022
Merged

launchd: initial support for LaunchAgents#2497
rycee merged 1 commit intonix-community:masterfrom
midchildan:feat/launchagents

Conversation

@midchildan
Copy link
Copy Markdown
Contributor

Description

Add support for macOS LaunchAgents, the equivalent of systemd user services.

Here's a little example of how to use this module:

launchd.agents.syncthing = {
  # whether to enable the LaunchAgent
  enable = true;

  # LaunchAgent configuration translated to the Nix Expression Language
  config = {
    ProgramArguments = [ "${pkgs.syncthing}/bin/syncthing" "-no-browser" "-no-restart" ];
      KeepAlive = {
        Crashed = true;
        SuccessfulExit = false;
      };
      ProcessType = "Background";
  };
};

Checklist

  • Change is backwards compatible.

  • 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.

@midchildan midchildan requested a review from rycee as a code owner November 21, 2021 17:15

# NOTE: Launch Agent configurations can't be symlinked from the Nix store
# because it needs to be owned by the user running it.
home.activation.setupLaunchAgents =
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The activation script takes care of starting/restarting/stopping the agents automatically.

Comment on lines +111 to +132
# NOTE: Launch Agent configurations can't be symlinked from the Nix store
# because it needs to be owned by the user running it.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LaunchAgent configuration files are copied into the home directory because Launchd refuses to work with files not owned by the user running it. For safety reasons, the activation script would check if any changes are made to configurations files previously deployed by Home Manager.

Comment on lines +1 to +8
# launchd option type from nix-darwin
#
# Original Source:
# https://github.com/LnL7/nix-darwin/blob/a34dea2/modules/launchd/launchd.nix

# Copyright 2017 Daiderd Jordan
#
# Permission is hereby granted, free of charge, to any person obtaining
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This file contains the definition for the Launchd option type, the code responsible for translating Nix expressions into LaunchAgent configurations. Since this file comes from the nix-darwin project, I have included the license (MIT) and copyright headers here.

Other files aren't based on outside projects.

Copy link
Copy Markdown
Member

@berbiche berbiche 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 contribution!

Comment on lines +69 to +86
assertions = [{
assertion = agentPlists != { } -> pkgs.stdenv.isDarwin;
message = let names = lib.concatStringsSep ", " (attrNames agentPlists);
in "Must use Darwin for modules that require Launchd: " + names;
}];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know the systemd module has the opposite logic, but would it be possible to also add an enable option to check against (with a default value of isDarwin)?

Suggested change
assertions = [{
assertion = agentPlists != { } -> pkgs.stdenv.isDarwin;
message = let names = lib.concatStringsSep ", " (attrNames agentPlists);
in "Must use Darwin for modules that require Launchd: " + names;
}];
assertions = [{
assertion = (cfg.enable && agentPlists != { }) -> isDarwin;
message = let names = lib.concatStringsSep ", " (attrNames agentPlists);
in "Must use Darwin for modules that require Launchd: " + names;
}];

This would make it easier to have services defining both systemd services and launchd agents (until a common backend can be created).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wonder if the same should be done for systemd as well, or otherwise each systemd service would need to be hidden behind an mkIf for services using both systemd and launchd.

Copy link
Copy Markdown
Member

@berbiche berbiche Nov 25, 2021

Choose a reason for hiding this comment

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

Yeah, we should do the same for systemd services (in a different PR).

Comment thread modules/launchd/default.nix Outdated
Comment thread modules/launchd/default.nix Outdated
Comment thread modules/launchd/default.nix Outdated
Copy link
Copy Markdown
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

I missed two things in my initial review.

Comment thread modules/launchd/default.nix Outdated
let
inherit (pkgs.stdenv.hostPlatform) isDarwin;
inherit (lib.generators) toPlist;
inherit (hm.dag) entryAfter;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is never used directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've removed this line.

Comment thread modules/launchd/default.nix
@midchildan
Copy link
Copy Markdown
Contributor Author

I've rebased against master to fix merge conflicts.

Comment thread modules/launchd/default.nix Outdated
inherit (lib.generators) toPlist;

cfg = config.launchd;
labelPrefix = "org.nixos.home.";
Copy link
Copy Markdown
Member

@rycee rycee Dec 10, 2021

Choose a reason for hiding this comment

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

Maybe something like

Suggested change
labelPrefix = "org.nixos.home.";
labelPrefix = "org.nix-community.home.";

since we're not an official nixos.org project.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I applied your suggestion and changed the tests accordingly. This made me realize there actually is a https://nix-community.org site.

Comment thread modules/launchd/default.nix Outdated
home.activation.setupLaunchAgents =
hm.dag.entryAfter [ "writeBoundary" ] ''
setupLaunchAgents() {
local oldDir="$(readlink -m "$oldGenPath/LaunchAgents")"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm wondering if this may cause problems on the first activation, when $oldGenPath is unset?

Btw, the general recommendation is to declare the variable and assign it in separate statements. I.e. something like

local oldDir
oldDir="$(readline …)"

See SC2155 for rationale.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added more conditional statements to deal with an empty $oldGenPath and also separated variable declarations and assignments.

Comment thread modules/misc/news.nix
'';
}

{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this change is worth mentioning in the release notes as well!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added an entry in the 22.05 release note.

Comment thread modules/launchd/default.nix Outdated
continue
fi
if [[ -f "$dstPath" ]]; then
while true; do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible to limit this to just a few loops and then, e.g., return with an error message? It makes me a bit nervous to have a potentially infinite loop in an activation script, however unlikely it is to happen 🙂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I set the limit to 10 loops.

continue
fi
if ! cmp --quiet "$srcPath" "$dstPath"; then
warnEcho "Skipping deletion of '$dstPath', since its contents have diverged"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, I like the "diverged" wording 👍

@rycee
Copy link
Copy Markdown
Member

rycee commented Dec 11, 2021

Thanks for the contribution, looks pretty cool! I added a few comments to the best of my ability.

@rycee rycee force-pushed the feat/launchagents branch from b892a57 to c7a13f7 Compare February 26, 2022 09:44
@rycee rycee merged commit c7a13f7 into nix-community:master Feb 26, 2022
@rycee
Copy link
Copy Markdown
Member

rycee commented Feb 26, 2022

Squashed and merged to master now 🙂

@midchildan midchildan deleted the feat/launchagents branch February 26, 2022 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants