From 113dbd79de4d1825a6cae69088c4f8db13744dbe Mon Sep 17 00:00:00 2001 From: r-vdp Date: Thu, 23 Jan 2025 01:42:42 +0100 Subject: [PATCH 1/2] 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 --- .../system/activation/pre-switch-check.nix | 51 +++++++++++-------- .../system/activation/switchable-system.nix | 4 +- nixos/modules/system/activation/top-level.nix | 2 +- nixos/tests/switch-test.nix | 4 ++ 4 files changed, 36 insertions(+), 25 deletions(-) diff --git a/nixos/modules/system/activation/pre-switch-check.nix b/nixos/modules/system/activation/pre-switch-check.nix index 2cbd539a74c8..60ccd448d155 100644 --- a/nixos/modules/system/activation/pre-switch-check.nix +++ b/nixos/modules/system/activation/pre-switch-check.nix @@ -1,21 +1,35 @@ -{ 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 { + type = lib.types.pathInStore; + 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 +47,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 index b4f153f7755e..4783a0704740 100644 --- 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 index 3c42e6c04a48..6e12b63054ef 100644 --- 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 index 1f5574cfd704..37a808c3c471 100644 --- 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 From 73f3fe4839a09cb95c0b27ecaa12030e37b96a07 Mon Sep 17 00:00:00 2001 From: r-vdp Date: Thu, 23 Jan 2025 15:50:31 +0100 Subject: [PATCH 2/2] nixos/activation: log output from pre-switch checks on stderr --- nixos/modules/system/activation/pre-switch-check.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nixos/modules/system/activation/pre-switch-check.nix b/nixos/modules/system/activation/pre-switch-check.nix index 60ccd448d155..e8640a0db3f2 100644 --- a/nixos/modules/system/activation/pre-switch-check.nix +++ b/nixos/modules/system/activation/pre-switch-check.nix @@ -10,8 +10,8 @@ let # pre-switch check ${name} if ! ( ${text} - ); then - echo "Pre-switch check '${name}' failed" + ) >&2 ; then + echo "Pre-switch check '${name}' failed" >&2 exit 1 fi '') config.system.preSwitchChecks