Merge pull request #320266 from doronbehar/pkg/versionInstallHook

versionCheckHook: init

doc/build-helpers/testers.chapter.md

@@ -120,9 +120,10 @@ It has two modes:
 
 Checks that the output from running a command contains the specified version string in it as a whole word.
 
-Although simplistic, this test assures that the main program can run.
-While there's no substitute for a real test case, it does catch dynamic linking errors and such.
-It also provides some protection against accidentally building the wrong version, for example when using an "old" hash in a fixed-output derivation.
+NOTE: In most cases, [`versionCheckHook`](#versioncheckhook) should be preferred, but this function is provided and documented here anyway. The motivation for adding either tests would be:
+
+- Catch dynamic linking errors and such and missing environment variables that should be added by wrapping.
+- Probable protection against accidentally building the wrong version, for example when using an "old" hash in a fixed-output derivation.
 
 By default, the command to be run will be inferred from the given `package` attribute:
 it will check `meta.mainProgram` first, and fall back to `pname` or `name`.

doc/hooks/index.md

@@ -29,6 +29,7 @@ scons.section.md
 tetex-tex-live.section.md
 unzip.section.md
 validatePkgConfig.section.md
+versionCheckHook.section.md
 waf.section.md
 zig.section.md
 xcbuild.section.md

doc/hooks/versionCheckHook.section.md

@@ -0,0 +1,35 @@
+# versionCheckHook {#versioncheckhook}
+
+This hook adds a `versionCheckPhase` to the [`preInstallCheckHooks`](#ssec-installCheck-phase) that runs the main program of the derivation with a `--help` or `--version` argument, and checks that the `${version}` string is found in that output. You use it like this:
+
+```nix
+{
+  lib,
+  stdenv,
+  versionCheckHook,
+  # ...
+}:
+
+stdenv.mkDerivation (finalAttrs: {
+  # ...
+
+  nativeInstallCheckInputs = [
+    versionCheckHook
+  ];
+  doInstallCheck = true;
+
+  # ...
+})
+```
+
+Note that for [`buildPythonPackage`](#buildpythonpackage-function) and [`buildPythonApplication`](#buildpythonapplication-function), `doInstallCheck` is enabled by default.
+
+It does so in a clean environment (using `env --ignore-environment`), and it checks for the `${version}` string in both the `stdout` and the `stderr` of the command. It will report to you in the build log the output it received and it will fail the build if it failed to find `${version}`.
+
+The variables that this phase control are:
+
+- `dontVersionCheck`: Disable adding this hook to the [`preDistPhases`](#var-stdenv-preDist). Useful if you do want to load the bash functions of the hook, but run them differently.
+- `versionCheckProgram`: The full path to the program that should print the `${version}` string. Defaults roughly to `${placeholder "out"}/bin/${pname}`. Using `$out` in the value of this variable won't work, as environment variables from this variable are not expanded by the hook. Hence using `placeholder` is unavoidable.
+- `versionCheckProgramArg`: The argument that needs to be passed to `versionCheckProgram`. If undefined the hook tries first `--help` and then `--version`. Examples: `version`, `-V`, `-v`.
+- `preVersionCheck`: A hook to run before the check is done.
+- `postVersionCheck`: A hook to run after the check is done.

doc/stdenv/passthru.chapter.md

@@ -75,40 +75,17 @@ The Nixpkgs systems for continuous integration [Hydra](https://hydra.nixos.org/)
 #### Package tests {#var-passthru-tests-packages}
 []{#var-meta-tests-packages} <!-- legacy anchor -->
 
-Tests that are part of the source package, if they run quickly, are typically executed in the [`installCheckPhase`](#var-stdenv-phases).
-This phase is also suitable for performing a `--version` test for packages that support such flag.
-Most programs distributed by Nixpkgs support such a `--version` flag, and successfully calling the program with that flag indicates that the package at least got compiled properly.
+Besides tests provided by upstream, that you run in the [`checkPhase`](#ssec-check-phase), you may want to define tests derivations in the `passthru.tests` attribute, which won't change the build. `passthru.tests` have several advantages over running tests during any of the [standard phases](#sec-stdenv-phases):
 
-:::{.example #ex-checking-build-installCheckPhase}
+- They access the package as consumers would, independently from the environment in which it was built
+- They can be run and debugged without rebuilding the package, which is useful if that takes a long time
+- They don't add overhead to each build, as opposed checks added to the [`distPhase`](#ssec-distribution-phase), such as [`versionCheckHook`](#versioncheckhook).
 
-## Checking builds with `installCheckPhase`
+It is also possible to use `passthru.tests` to test the version with [`testVersion`](#tester-testVersion), but since that is pretty trivial and recommended thing to do, we recommend using [`versionCheckHook`](#versioncheckhook) for that, which has the following advantages over `passthru.tests`:
 
-When building `git`, a rudimentary test for successful compilation would be running `git --version`:
-
-```nix
-stdenv.mkDerivation (finalAttrs: {
-  pname = "git";
-  version = "1.2.3";
-  # ...
-  doInstallCheck = true;
-  installCheckPhase = ''
-    runHook preInstallCheck
-    echo checking if 'git --version' mentions ${finalAttrs.version}
-    $out/bin/git --version | grep ${finalAttrs.version}
-    runHook postInstallCheck
-  '';
-  # ...
-})
-```
-:::
-
-However, tests that are non-trivial will better fit into `passthru.tests` because they:
-
-- Access the package as consumers would, independently from the environment in which it was built
-- Can be run and debugged without rebuilding the package, which is useful if that takes a long time
-- Don't add overhad to each build, as opposed to `installCheckPhase`
-
-It is also possible to use `passthru.tests` to test the version with [`testVersion`](#tester-testVersion).
+- If the `versionCheckPhase` (the phase defined by [`versionCheckHook`](#versioncheckhook)) fails, it triggers a failure which can't be ignored if you use the package, or if you find out about it in a [`nixpkgs-review`](https://github.com/Mic92/nixpkgs-review) report.
+- Sometimes packages become silently broken - meaning they fail to launch but their build passes because they don't perform any tests in the `checkPhase`. If you use this tool infrequently, such a silent breakage may rot in your system / profile configuration, and you will not notice the failure until you will want to use this package. Testing such basic functionality ensures you have to deal with the failure when you update your system / profile.
+- When you open a PR, [ofborg](https://github.com/NixOS/ofborg)'s CI _will_ run `passthru.tests` of [packages that are directly changed by your PR (according to your commits' messages)](https://github.com/NixOS/ofborg?tab=readme-ov-file#automatic-building), but if you'd want to use the [`@ofborg build`](https://github.com/NixOS/ofborg?tab=readme-ov-file#build) command for dependent packages, you won't have to specify in addition the `.tests` attribute of the packages you want to build, and no body will be able to avoid these tests.
 
 <!-- NOTE(@fricklerhandwerk): one may argue whether that testing guide should rather be in the user's manual -->
 For more on how to write and run package tests for Nixpkgs, see the [testing section in the package contributor guide](https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#package-tests).

doc/stdenv/stdenv.chapter.md

@@ -762,6 +762,8 @@ Before and after running `make`, the hooks `preBuild` and `postBuild` are called
 
 The check phase checks whether the package was built correctly by running its test suite. The default `checkPhase` calls `make $checkTarget`, but only if the [`doCheck` variable](#var-stdenv-doCheck) is enabled.
 
+It is highly recommended, for packages' sources that are not distributed with any tests, to at least use [`versionCheckHook`](#versioncheckhook) to test that the resulting executable is basically functional.
+
 #### Variables controlling the check phase {#variables-controlling-the-check-phase}
 
 ##### `doCheck` {#var-stdenv-doCheck}

pkgs/applications/misc/syncthingtray/default.nix

@@ -31,6 +31,7 @@ during runtime. Alternatively, one can edit the desktop file themselves after
 it is generated See:
 https://github.com/NixOS/nixpkgs/issues/199596#issuecomment-1310136382 */
 , autostartExecPath ? "syncthingtray"
+, versionCheckHook
 }:
 
 stdenv.mkDerivation (finalAttrs: {
@@ -85,9 +86,10 @@ stdenv.mkDerivation (finalAttrs: {
     # Make binary available in PATH like on other platforms
     ln -s $out/Applications/syncthingtray.app/Contents/MacOS/syncthingtray $out/bin/syncthingtray
   '';
-  installCheckPhase = ''
-    $out/bin/syncthingtray --help | grep ${finalAttrs.version}
-  '';
+  nativeInstallCheckInputs = [
+    versionCheckHook
+  ];
+  doInstallCheck = true;
 
   cmakeFlags = [
     "-DQT_PACKAGE_PREFIX=Qt${lib.versions.major qtbase.version}"

pkgs/by-name/he/hello/package.nix

@@ -4,6 +4,7 @@
 , fetchurl
 , nixos
 , testers
+, versionCheckHook
 , hello
 }:
 
@@ -19,6 +20,9 @@ stdenv.mkDerivation (finalAttrs: {
   doCheck = true;
 
   doInstallCheck = true;
+  nativeInstallCheckInputs = [
+    versionCheckHook
+  ];
 
   # Give hello some install checks for testing purpose.
   postInstallCheck = ''

pkgs/by-name/ve/versionCheckHook/hook.sh

@@ -0,0 +1,60 @@
+_handleCmdOutput(){
+    local versionOutput
+    versionOutput="$(env --chdir=/ --argv0="$(basename "$1")" --ignore-environment "$@" 2>&1 || true)"
+    if [[ "$versionOutput" =~ "$version" ]]; then
+        echoPrefix="Successfully managed to"
+    else
+        echoPrefix="Did not"
+    fi
+    # The return value of this function is this variable:
+    echo "$echoPrefix"
+    # And in anycase we want these to be printed in the build log, useful for
+    # debugging, so we print these to stderr.
+    echo "$echoPrefix" find version "$version" in the output of the command \
+        "$@" >&2
+    echo "$versionOutput" >&2
+}
+versionCheckHook(){
+    runHook preVersionCheck
+    echo Executing versionCheckPhase
+
+    local cmdProgram cmdArg echoPrefix
+    if [[ -z "${versionCheckProgram-}" ]]; then
+        if [[ -z "${pname-}" ]]; then
+            echo "both \$pname and \$versionCheckProgram are empty, so" \
+                "we don't know which program to run the versionCheckPhase" \
+                "upon" >&2
+            exit 2
+        else
+            cmdProgram="${!outputBin}/bin/$pname"
+        fi
+    else
+        cmdProgram="$versionCheckProgram"
+    fi
+    if [[ ! -x "$cmdProgram" ]]; then
+        echo "versionCheckHook: $cmdProgram was not found, or is not an executable" >&2
+        exit 2
+    fi
+    if [[ -z "${versionCheckProgramArg}" ]]; then
+        for cmdArg in "--help" "--version"; do
+            echoPrefix="$(_handleCmdOutput "$cmdProgram" "$cmdArg")"
+            if [[ "$echoPrefix" == "Successfully managed to" ]]; then
+                break
+            fi
+        done
+    else
+        cmdArg="$versionCheckProgramArg"
+        echoPrefix="$(_handleCmdOutput "$cmdProgram" "$cmdArg")"
+    fi
+    if [[ "$echoPrefix" == "Did not" ]]; then
+        exit 2
+    fi
+
+    runHook postVersionCheck
+    echo Finished versionCheckPhase
+}
+
+if [[ -z "${dontVersionCheck-}" ]]; then
+    echo "Using versionCheckHook"
+    preInstallCheckHooks+=(versionCheckHook)
+fi

pkgs/by-name/ve/versionCheckHook/package.nix

@@ -0,0 +1,12 @@
+{
+  lib,
+  makeSetupHook,
+}:
+
+makeSetupHook {
+  name = "version-check-hook";
+  meta = {
+    description = "Lookup for $version in the output of --help and --version";
+    maintainers = with lib.maintainers; [ doronbehar ];
+  };
+} ./hook.sh

pkgs/development/python-modules/ftfy/default.nix

@@ -12,6 +12,7 @@
 
   # tests
   pytestCheckHook,
+  versionCheckHook,
 }:
 
 buildPythonPackage rec {
@@ -30,7 +31,10 @@ buildPythonPackage rec {
 
   propagatedBuildInputs = [ wcwidth ];
 
-  nativeCheckInputs = [ pytestCheckHook ];
+  nativeCheckInputs = [
+    versionCheckHook
+    pytestCheckHook
+  ];
 
   preCheck = ''
     export PATH=$out/bin:$PATH