Skip to content

Remove platform check from the systemd module#2915

Merged
rycee merged 2 commits intonix-community:masterfrom
midchildan:refactor/platform-assertions
Feb 7, 2023
Merged

Remove platform check from the systemd module#2915
rycee merged 2 commits intonix-community:masterfrom
midchildan:refactor/platform-assertions

Conversation

@midchildan
Copy link
Copy Markdown
Contributor

@midchildan midchildan commented Apr 24, 2022

Description

This is related to #2497 (comment).

The systemd module currently contains checks to make sure user services aren't defined on Darwin hosts. But with launchd support now in HM, this can result in feature gating boilerplate when trying to create modules that supports both systemd and launchd.

After this change, modules would be able to define both systemd services and launchd agents without the boilerplate. The systemd module would just ignore them on macOS and vice versa for launchd. I'm hoping this wouldn't be a big issue because most consumers of the systemd module already checks if it supports the target OS. I've also added checks for those that didn't.

For a more concrete example, here's how a module supporting both systemd and launchd might look before this change:

{ lib, pkgs, ... }:

let
  cfg = config.services.example;
  inherit (lib) mkIf mkMerge;
  inherit (pkgs.stdenv.hostPlatforms) isLinux isDarwin;
in {
  options.services.example.enable = mkEnableOption "example service";

  config = mkIf cfg.enable (mkMerge [
    (lib.mkIf isLinux {
      systemd.user.services.example = {
        Unit.Description = "example service";
        Service.ExecStart = "${pkgs.example}/bin/example";
        Install.WantedBy = [ "default.target" ];
      };
    })

    (lib.mkIf isDarwin {
      launchd.agents.example = {
        enable = true;
        config = {
          ProgramArguments = [ "${pkgs.example}/bin/example" ];
          ProcessType = "Background";
        };
      };
    })
  ]);
}

And here's how it'd look after this change:

{ lib, pkgs, ... }:

let
  cfg = config.services.example;
  inherit (lib) mkIf mkMerge;
in {
  options.services.example.enable = mkEnableOption "example service";

  config = mkIf cfg.enable {
    systemd.user.services.example = {
      Unit.Description = "example service";
      Service.ExecStart = "${pkgs.example}/bin/example";
      Install.WantedBy = [ "default.target" ];
    };

    launchd.agents.example = {
      enable = true;
      config = {
        ProgramArguments = [ "${pkgs.example}/bin/example" ];
        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.

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.

LGTM!

@rycee friendly ping

@stale
Copy link
Copy Markdown

stale bot commented Aug 4, 2022

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 Aug 4, 2022
@midchildan midchildan force-pushed the refactor/platform-assertions branch from 76faac1 to 8ac174c Compare August 9, 2022 16:55
@dbaynard dbaynard mentioned this pull request Sep 30, 2022
7 tasks
@berbiche
Copy link
Copy Markdown
Member

berbiche commented Feb 2, 2023

@midchildan would you be interested in rebasing the PR?

@ncfavier @rycee I believe this PR is a decent QOL improvement

@stale stale bot removed the status: stale label Feb 2, 2023
@midchildan midchildan force-pushed the refactor/platform-assertions branch from 8ac174c to 1f1e1e7 Compare February 7, 2023 17:11
@midchildan
Copy link
Copy Markdown
Contributor Author

I rebased this PR and added an additional platform check to the services.picom module.

Allow modules to define systemd services on macOS. It won't actually
have any effect, but it would allow modules to define both systemd
services and launchd agents without boilerplate conditionals.

As a consequence of this change, each module would have to check for
compatibility with the OS target instead.
@rycee rycee force-pushed the refactor/platform-assertions branch from 1f1e1e7 to edb3645 Compare February 7, 2023 20:54
@rycee rycee merged commit edb3645 into nix-community:master Feb 7, 2023
@rycee
Copy link
Copy Markdown
Member

rycee commented Feb 7, 2023

Looks quite nice! 🙂 Merged to master now.

@midchildan midchildan deleted the refactor/platform-assertions branch February 8, 2023 15:59
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