testers.shellcheck: refactor, update docs, and simplify tests (NixOS#385940)

Author: ConnorBaker

doc/build-helpers/testers.chapter.md

@@ -118,7 +118,7 @@ It has two modes:
 
 ## `shellcheck` {#tester-shellcheck}
 
-Runs files through `shellcheck`, a static analysis tool for shell scripts.
+Run files through `shellcheck`, a static analysis tool for shell scripts, failing if there are any issues.
 
 :::{.example #ex-shellcheck}
 # Run `testers.shellcheck`
@@ -127,7 +127,7 @@ A single script
 
 ```nix
 testers.shellcheck {
-  name = "shellcheck";
+  name = "script";
   src = ./script.sh;
 }
 ```
@@ -139,7 +139,7 @@ let
   inherit (lib) fileset;
 in
 testers.shellcheck {
-  name = "shellcheck";
+  name = "nixbsd-activate";
   src = fileset.toSource {
     root = ./.;
     fileset = fileset.unions [
@@ -154,15 +154,20 @@ testers.shellcheck {
 
 ### Inputs {#tester-shellcheck-inputs}
 
-[`src` (path or string)]{#tester-shellcheck-param-src}
+`name` (string, optional)
+: The name of the test.
+  `name` will be required at a future point because it massively improves traceability of test failures, but is kept optional for now to avoid breaking existing usages.
+  Defaults to `run-shellcheck`.
+  The name of the derivation produced by the tester is `shellcheck-${name}` when `name` is supplied.
 
+`src` (path-like)
 : The path to the shell script(s) to check.
   This can be a single file or a directory containing shell files.
   All files in `src` will be checked, so you may want to provide `fileset`-based source instead of a whole directory.
 
 ### Return value {#tester-shellcheck-return}
 
-A derivation that runs `shellcheck` on the given script(s).
+A derivation that runs `shellcheck` on the given script(s), producing an empty output if no issues are found.
 The build will fail if `shellcheck` finds any issues.
 
 ## `shfmt` {#tester-shfmt}

doc/redirects.json

@@ -1557,9 +1557,6 @@
   "tester-shellcheck-inputs": [
     "index.html#tester-shellcheck-inputs"
   ],
-  "tester-shellcheck-param-src": [
-    "index.html#tester-shellcheck-param-src"
-  ],
   "tester-shellcheck-return": [
     "index.html#tester-shellcheck-return"
   ],

doc/release-notes/rl-2505.section.md

@@ -18,6 +18,9 @@
 - The hand written `perlPackages.SearchXapian` bindings have been dropped in favor of the (mostly compatible)
   `perlPackages.Xapian`.
 
+- [testers.shellcheck](https://nixos.org/manual/nixpkgs/unstable/#tester-shellcheck) now warns when `name` is not provided.
+  The `name` argument will become mandatory in a future release.
+
 - The `nixLog*` family of functions made available through the standard environment have been rewritten to prefix messages with both the debug level and the function name of the caller.
   The `nixLog` function, which logs unconditionally, was also re-introduced and modified to prefix messages with the function name of the caller.
   For more information, [see this PR](https://github.com/NixOS/nixpkgs/pull/370742).

nixos/modules/config/nix-channel/test.nix

@@ -5,6 +5,7 @@ let
   inherit (lib) fileset;
 
   runShellcheck = testers.shellcheck {
+    name = "activation-check";
     src = fileset.toSource {
       root = ./.;
       fileset = fileset.unions [

nixos/modules/system/activation/lib/test.nix

@@ -31,6 +31,7 @@ let
   };
 
   runShellcheck = testers.shellcheck {
+    name = "activation-lib";
     src = runTests.src;
   };
 

pkgs/build-support/testers/shellcheck/tester.nix

@@ -1,39 +1,35 @@
 # Dependencies (callPackage)
 {
   lib,
-  stdenv,
-  runCommand,
+  stdenvNoCC,
   shellcheck,
 }:
 
 # testers.shellcheck function
 # Docs: doc/build-helpers/testers.chapter.md
 # Tests: ./tests.nix
-{ src }:
-let
-  inherit (lib) pathType isPath;
-in
-stdenv.mkDerivation {
-  name = "run-shellcheck";
-  src =
-    if
-      isPath src && pathType src == "regular" # note that for strings this would have been IFD, which we prefer to avoid
-    then
-      runCommand "testers-shellcheck-src" { } ''
-        mkdir $out
-        cp ${src} $out
-      ''
+{
+  name ? null,
+  src,
+}:
+stdenvNoCC.mkDerivation {
+  __structuredAttrs = true;
+  strictDeps = true;
+  name =
+    if name == null then
+      lib.warn "testers.shellcheck: name will be required in a future release, defaulting to run-shellcheck" "run-shellcheck"
     else
-      src;
+      "shellcheck-${name}";
+  inherit src;
+  dontUnpack = true; # Unpack phase tries to extract an archive, which we don't want to do with source trees
   nativeBuildInputs = [ shellcheck ];
   doCheck = true;
   dontConfigure = true;
   dontBuild = true;
   checkPhase = ''
-    find . -type f -print0 \
-      | xargs -0 shellcheck
+    find "$src" -type f -print0 | xargs -0 shellcheck
   '';
   installPhase = ''
-    touch $out
+    touch "$out"
   '';
 }

pkgs/build-support/testers/shellcheck/tests.nix

@@ -4,39 +4,33 @@
 {
   lib,
   testers,
-  runCommand,
 }:
 lib.recurseIntoAttrs {
-
-  example-dir =
-    runCommand "test-testers-shellcheck-example-dir"
-      {
-        failure = testers.testBuildFailure (
-          testers.shellcheck {
-            src = ./src;
-          }
-        );
-      }
+  example-dir = testers.testBuildFailure' {
+    drv = testers.shellcheck {
+      name = "example-dir";
+      src = ./src;
+    };
+    expectedBuilderExitCode = 123;
+    expectedBuilderLogEntries = [
+      ''
+        echo $@
+             ^-- SC2068 (error): Double quote array expansions to avoid re-splitting elements.
       ''
-        log="$failure/testBuildFailure.log"
-        echo "Checking $log"
-        grep SC2068 "$log"
-        touch $out
-      '';
+    ];
+  };
 
-  example-file =
-    runCommand "test-testers-shellcheck-example-file"
-      {
-        failure = testers.testBuildFailure (
-          testers.shellcheck {
-            src = ./src/example.sh;
-          }
-        );
-      }
+  example-file = testers.testBuildFailure' {
+    drv = testers.shellcheck {
+      name = "example-file";
+      src = ./src/example.sh;
+    };
+    expectedBuilderExitCode = 123;
+    expectedBuilderLogEntries = [
+      ''
+        echo $@
+             ^-- SC2068 (error): Double quote array expansions to avoid re-splitting elements.
       ''
-        log="$failure/testBuildFailure.log"
-        echo "Checking $log"
-        grep SC2068 "$log"
-        touch $out
-      '';
+    ];
+  };
 }