nixos/activation: improve preSwitchChecks

A couple of improvements: 1. Avoid the generally discouraged apply argument to options, as it has quite weird semantics 2. Avoid issues when a user calls a preSwitchCheck `script`, which would've been silently overridden by the existing implementation. Reliance on a special attribute name like that is bound to lead to a very-hard-to-debug problem for someone at some point 3. Use writeShellApplication so that the preSwitchChecks are checked by shellcheck and and so that they run with basic bash guardrails 4. Fix shellcheck issue (testing the value of $?) 5. Add a positive preSwitchCheck to the nixos test, to make sure that that works as intended
NixOS · Jan 23, 2025 · c120205 · c120205
1 parent 7db6d6c
commit c120205
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 25 deletions.
diff --git a/nixos/modules/system/activation/pre-switch-check.nix b/nixos/modules/system/activation/pre-switch-check.nix
@@ -1,21 +1,34 @@
-{ lib, pkgs, ... }:
+{
+  lib,
+  config,
+  pkgs,
+  ...
+}:
 let
-  preSwitchCheckScript =
-    set:
-    lib.concatLines (
-      lib.mapAttrsToList (name: text: ''
-        # pre-switch check ${name}
-        (
-          ${text}
-        )
-        if [[ $? != 0 ]]; then
-          echo "Pre-switch check '${name}' failed"
-          exit 1
-        fi
-      '') set
-    );
+  preSwitchCheckScript = lib.concatLines (
+    lib.mapAttrsToList (name: text: ''
+      # pre-switch check ${name}
+      if ! (
+        ${text}
+      ); then
+        echo "Pre-switch check '${name}' failed"
+        exit 1
+      fi
+    '') config.system.preSwitchChecks
+  );
 in
 {
+  options.system.preSwitchChecksScript = lib.mkOption {
+    internal = true;
+    readOnly = true;
+    default = lib.getExe (
+      pkgs.writeShellApplication {
+        name = "pre-switch-checks";
+        text = preSwitchCheckScript;
+      }
+    );
+  };
+
   options.system.preSwitchChecks = lib.mkOption {
     default = { };
     example = lib.literalExpression ''
@@ -33,12 +46,5 @@ in
     '';
 
     type = lib.types.attrsOf lib.types.str;
-
-    apply =
-      set:
-      set
-      // {
-        script = pkgs.writeShellScript "pre-switch-checks" (preSwitchCheckScript set);
-      };
   };
 }
diff --git a/nixos/modules/system/activation/switchable-system.nix b/nixos/modules/system/activation/switchable-system.nix
@@ -61,7 +61,7 @@ in
           --subst-var-by coreutils "${pkgs.coreutils}" \
           --subst-var-by distroId ${lib.escapeShellArg config.system.nixos.distroId} \
           --subst-var-by installBootLoader ${lib.escapeShellArg config.system.build.installBootLoader} \
-          --subst-var-by preSwitchCheck ${lib.escapeShellArg config.system.preSwitchChecks.script} \
+          --subst-var-by preSwitchCheck ${lib.escapeShellArg config.system.preSwitchChecksScript} \
           --subst-var-by localeArchive "${config.i18n.glibcLocales}/lib/locale/locale-archive" \
           --subst-var-by perl "${perlWrapped}" \
           --subst-var-by shell "${pkgs.bash}/bin/sh" \
@@ -94,7 +94,7 @@ in
             --set TOPLEVEL ''${!toplevelVar} \
             --set DISTRO_ID ${lib.escapeShellArg config.system.nixos.distroId} \
             --set INSTALL_BOOTLOADER ${lib.escapeShellArg config.system.build.installBootLoader} \
-            --set PRE_SWITCH_CHECK ${lib.escapeShellArg config.system.preSwitchChecks.script} \
+            --set PRE_SWITCH_CHECK ${lib.escapeShellArg config.system.preSwitchChecksScript} \
             --set LOCALE_ARCHIVE ${config.i18n.glibcLocales}/lib/locale/locale-archive \
             --set SYSTEMD ${config.systemd.package}
         )

diff --git a/nixos/modules/system/activation/top-level.nix b/nixos/modules/system/activation/top-level.nix
@@ -343,7 +343,7 @@ in
       perl = pkgs.perl.withPackages (p: with p; [ ConfigIniFiles FileSlurp ]);
       # End if legacy environment variables
 
-      preSwitchCheck = config.system.preSwitchChecks.script;
+      preSwitchCheck = config.system.preSwitchChecksScript;
 
       # Not actually used in the builder. `passedChecks` is just here to create
       # the build dependencies. Checks are similar to build dependencies in the

diff --git a/nixos/tests/switch-test.nix b/nixos/tests/switch-test.nix
@@ -611,6 +611,10 @@ in {
     other = {
       system.switch.enable = true;
       users.mutableUsers = true;
+      system.preSwitchChecks.succeeds = ''
+        echo this will succeed
+        true
+      '';
       specialisation.failingCheck.configuration.system.preSwitchChecks.failEveryTime = ''
         echo this will fail
         false