versionCheckHook: add versionCheckScript parameter

[Defelo]

Currently the versionCheckHook only allows executing commands with a single argument (usually something like --version). However in some cases it is necessary to run more complex commands, e.g. with multiple arguments (see #434081 (comment) for example).

This PR introduces a versionCheckScript parameter which can be used instead of versionCheckProgram and versionCheckProgramArg to run an arbitrary bash command, similar to the command parameter in testers.testVersion.

An alternative I considered was to add a versionCheckExtraProgramArgs parameter instead which would contain a list of arguments in addition to versionCheckProgramArg. However it's not clear whether these additional arguments should appear before or after the versionCheckProgramArg and they could also not contain any spaces.

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[keegancsmith]

Changes to src-cli LGTM

[doronbehar]

A few general comments:

It should target staging.

Easier to digest diff

In such cases where the changes include a new conditional branch, it'd be easier to review this if the versionCheckHook/hook.sh changes would be split to 2 commits, where the diff of the 1st of which is something like this:

diff --git c/pkgs/by-name/ve/versionCheckHook/hook.sh w/pkgs/by-name/ve/versionCheckHook/hook.sh
index 4a977fe7a888..72d1744cda66 100644
--- c/pkgs/by-name/ve/versionCheckHook/hook.sh
+++ w/pkgs/by-name/ve/versionCheckHook/hook.sh
@@ -1,15 +1,17 @@
 _handleCmdOutput(){
-    local command=("$1" "$2")
     local versionOutput
 
     local envArgs=()
-    if [[ "$3" != "*" ]]; then
+    if [[ "$1" != "*" ]]; then
       envArgs+=("--ignore-environment")
-      for var in $3; do
+      for var in $1; do
         envArgs+=("$var=${!var}")
       done
     fi
 
+    shift 1
+    local command=("$@")
+
     versionOutput="$(env \
         --chdir=/ \
         --argv0="$(basename "${command[0]}")" \
@@ -38,6 +40,10 @@ versionCheckHook(){
     : "${versionCheckKeepEnvironment:=}"
 
     local cmdProgram cmdArg echoPrefix
+
+    if [[ -n "${versionCheckScript-}" ]]; then
+      echoPrefix="$(_handleCmdOutput "$versionCheckKeepEnvironment" "@bash@" "-c" "$versionCheckScript")"
+    else
     if [[ ! -z "${versionCheckProgram-}" ]]; then
         cmdProgram="$versionCheckProgram"
     elif [[ ! -z "${NIX_MAIN_PROGRAM-}" ]]; then
@@ -62,14 +68,15 @@ versionCheckHook(){
     fi
     if [[ -z "${versionCheckProgramArg}" ]]; then
         for cmdArg in "--help" "--version"; do
-            echoPrefix="$(_handleCmdOutput "$cmdProgram" "$cmdArg" "$versionCheckKeepEnvironment")"
+            echoPrefix="$(_handleCmdOutput "$versionCheckKeepEnvironment" "$cmdProgram" "$cmdArg")"
             if [[ "$echoPrefix" == "Successfully managed to" ]]; then
                 break
             fi
         done
     else
         cmdArg="$versionCheckProgramArg"
-        echoPrefix="$(_handleCmdOutput "$cmdProgram" "$cmdArg" "$versionCheckKeepEnvironment")"
+        echoPrefix="$(_handleCmdOutput "$versionCheckKeepEnvironment" "$cmdProgram" "$cmdArg")"
+    fi
     fi
     if [[ "$echoPrefix" == "Did not" ]]; then
         exit 2

The next commit can be a pure bash formatting commit.

A different API suggestion

Do you think it'd be possible to support a simpler API for multiple arguments? I was thinking of supporting versionCheckProgramArgsArray which will take precedence over versionCheckProgramArg. I don't like how Bash is pulled there - it is too complex IMO for something which is already almost supported. BTW this would make the diff easier to digest, as you'd simply set differently the command Bash array.

[Defelo]

Hey, thanks for the quick feedback!

It should target staging.

Yes, but I think it already does?

Easier to digest diff

In such cases where the changes include a new conditional branch, it'd be easier to review this if the versionCheckHook/hook.sh changes would be split to 2 commits, where the diff of the 1st of which is something like this: [...]
The next commit can be a pure bash formatting commit.

On GitHub you can enable the "Hide whitespace" option (hidden behind the small gear icon) to make the diff more readable. Also tools like difftastic exist, but those of course require the PR to be checked out locally. I personally don't really see much value in splitting the commit this way, but if it helps reviewing this PR I will do that of course.

A different API suggestion

Do you think it'd be possible to support a simpler API for multiple arguments? I was thinking of supporting versionCheckProgramArgsArray which will take precedence over versionCheckProgramArg.

The problem here is that these arguments could not contain any spaces. For example versionCheckProgramArgsArray = [ "--version" "--foo=some arg with spaces" ] couldn't be distinguished from versionCheckProgramArgsArray = [ "--version" "--foo=some" "arg" "with" "spaces" ] (see wgpu-py for a concrete example). This wouldn't be a problem with versionCheckScript. However I do agree that your suggestion would make it simpler for consumers as they wouldn't have to use the placeholder "out" trick to get the program path. But maybe we could simply forward the versionCheckProgram variable (which already defaults to the mainProgram path) to the bash script, so you would write something like versionCheckScript = "$versionCheckProgram version --foo --bar";?

Besides supporting more than one argument, another advantage of versionCheckScript is that it would allow for more complex commands, for example if you need to write something to stdin like in mini-calc or if you need to chain multiple commands to extract the version like in menulibre or lib25519.

I don't like how Bash is pulled there - it is too complex IMO for something which is already almost supported. BTW this would make the diff easier to digest, as you'd simply set differently the command Bash array.

Hm, doesn't mkDerivation itself already pull in bash? Is there any other application for setup hooks than to use them in other derivations? Maybe we could use stdenvNoCC.shell instead? It looks like this would also use bashNonInteractive instead of bashInteractive (which bash is an alias of).

[doronbehar]

On GitHub you can enable the "Hide whitespace" option (hidden behind the small gear icon) to make the diff more readable. Also tools like difftastic exist, but those of course require the PR to be checked out locally. I personally don't really see much value in splitting the commit this way, but if it helps reviewing this PR I will do that of course.

Indeed that hide whitespace option makes it easier, thanks for the info 👍. I will not insist on this detail, although I personally still try to split commits like that when I think it is needed.

The problem here is that these arguments could not contain any spaces. For example versionCheckProgramArgsArray = [ "--version" "--foo=some arg with spaces" ] couldn't be distinguished from versionCheckProgramArgsArray = [ "--version" "--foo=some" "arg" "with" "spaces" ]

Are you sure it is impossible? If you could prove it for me, it'd be great. Try to create a separate setup hook that prints each element in the array, just for testing.

(see wgpu-py for a concrete example).

That's an extreme example! When I created versionCheckHook, I never expected it to be used for such complex cases :). If you ask me, that example should be even more common, and it should be transformed to a generic pythonVersionCheckHook, that can be used in many Python packages!

(which already defaults to the mainProgram path)

I couldn't help but mention that this behavior is new to me :) (#412768)

Besides supporting more than one argument, another advantage of versionCheckScript is that it would allow for more complex commands, for example if you need to write something to stdin like in mini-calc

Damn what an outstanding example! I failed to reproduce that behavior even in a breakpointHook shell. That's so weird - I'd try to discuss this with upstream, if we are so eager to make it use versionCheckHook.

if you need to chain multiple commands to extract the version like in menulibre

versionCheckHook doesn't need to see the version clearly separated from other text. I successfully managed to make menulibre use versionCheckHook:

diff --git a/pkgs/by-name/me/menulibre/package.nix b/pkgs/by-name/me/menulibre/package.nix
index 1a5c1d1743c3..6785cad5b6c1 100644
--- a/pkgs/by-name/me/menulibre/package.nix
+++ b/pkgs/by-name/me/menulibre/package.nix
@@ -7,9 +7,7 @@
   intltool,
   gobject-introspection,
   wrapGAppsHook3,
-  nix-update-script,
-  testers,
-  menulibre,
+  versionCheckHook,
   writableTmpDirAsHomeHook,
 }:

(1/2) Discard this hunk from worktree [y,n,q,a,d,j,J,g,/,e,?]? y
@@ -46,13 +44,8 @@ python3Packages.buildPythonApplication rec {
       --replace-fail 'update_desktop_file(desktop_file, script_path)' ""
   '';

-  passthru = {
-    updateScript = nix-update-script { };
-    tests.version = testers.testVersion {
-      package = menulibre;
-      command = "HOME=$TMPDIR menulibre --version | cut -d' ' -f2";
-    };
-  };
+  nativeCheckInputs = [ versionCheckHook ];
+  doCheck = true;

   meta = with lib; {
     description = "Advanced menu editor with an easy-to-use interface";

or lib25519

Same:

diff --git i/pkgs/by-name/li/lib25519/package.nix w/pkgs/by-name/li/lib25519/package.nix
index 0cd67b7b9451..68f4de0db233 100644
--- i/pkgs/by-name/li/lib25519/package.nix
+++ w/pkgs/by-name/li/lib25519/package.nix
@@ -3,7 +3,7 @@
   lib,
   python3,
   fetchzip,
-  testers,
+  versionCheckHook,
   valgrind,
   librandombytes,
   libcpucycles,
@@ -51,19 +51,11 @@ stdenv.mkDerivation (finalAttrs: {
 
   # failure: crypto_pow does not handle p=q overlap
   doInstallCheck = !stdenv.hostPlatform.isDarwin;
-  installCheckPhase = ''
-    runHook preInstallCheck
-    $out/bin/lib25519-test
-    runHook postInstallCheck
-  '';
+  nativeInstallCheckInputs = [ versionCheckHook ];
+  versionCheckProgram = "${placeholder "out"}/bin/lib25519-test";
 
   passthru = {
     updateScript = ./update.sh;
-    tests.version = testers.testVersion {
-      package = finalAttrs.finalPackage;
-      command = "lib25519-test | head -n 2 | grep version";
-      version = "lib25519 version ${finalAttrs.version}";
-    };
   };
 
   meta = {

Though here, it is a bit more dirty. Running the lib25519-test file both runs the tests themselves, and looks for the version there too.

Hm, doesn't mkDerivation itself already pull in bash? Is there any other application for setup hooks than to use them in other derivations?

Well I'm sorry but I wasn't clear. Technically you are right - Bash is already very abundant and it wouldn't make any difference. I mostly didn't like the way the hook is executing an external script, but let's put this discussion aside for now.

---

To summarize, you have given a fair amount of edge cases where seemingly versionCheckHook doesn't fit. I wish to keep it simple as it is now. The only actually valid example that demonstrates a (currently mysterious) limit of versionCheckHook is that of mini-calc, and I think it should be investigated, because we might learn something profound for Nixpkgs in general out of it.

Although I don't think it should be needed for any program in the world, I believe that in principal our Nix - Bash interface should support arguments with spaces, and that should solve even the python -c 'import .. print(..__version__)' usecase, which I think should be generalized to a separate pythonVersionCheckHook either way.

[Defelo]

The problem here is that these arguments could not contain any spaces. For example versionCheckProgramArgsArray = [ "--version" "--foo=some arg with spaces" ] couldn't be distinguished from versionCheckProgramArgsArray = [ "--version" "--foo=some" "arg" "with" "spaces" ]

Are you sure it is impossible? If you could prove it for me, it'd be great.

Iiuc all attributes in a mkDerivation call are passed as environment variables to the builder and setup hooks, after converting their values to strings using toString. For a list toString returns the elements separated by spaces, so for example toString [ "foo" "bar baz" ] == "foo bar baz" == toString [ "foo" "bar" "baz" ] (i.e. from just the string representation we can't tell whether bar and baz are two separate elements or one element which contains a space). In a __structuredAttrs = true; derivation this wouldn't be a problem though.

(see wgpu-py for a concrete example).

That's an extreme example! When I created versionCheckHook, I never expected it to be used for such complex cases :). If you ask me, that example should be even more common, and it should be transformed to a generic pythonVersionCheckHook, that can be used in many Python packages!

Yeah I agree, in this specific case a dedicated pythonVersionCheckHook would probably be a better fit.

if you need to chain multiple commands to extract the version like in menulibre

versionCheckHook doesn't need to see the version clearly separated from other text.

That's true, however some programs include a lot more information in the --version output than just the version and maybe the program name. I think in these cases it can make sense to extract the version first, so in case the version does not match, the versionCheckHook doesn't accidentally find the expected version in a different part of the output and thus succeeds.

To summarize, you have given a fair amount of edge cases where seemingly versionCheckHook doesn't fit. I wish to keep it simple as it is now. The only actually valid example that demonstrates a (currently mysterious) limit of versionCheckHook is that of mini-calc, and I think it should be investigated, because we might learn something profound for Nixpkgs in general out of it.

Although I don't think it should be needed for any program in the world, I believe that in principal our Nix - Bash interface should support arguments with spaces, and that should solve even the python -c 'import .. print(..__version__)' usecase, which I think should be generalized to a separate pythonVersionCheckHook either way.

I agree that most examples I mentioned here are edge cases and that the versionCheckHook as is already works very well for many "normal" packages. However I really would like to have a good solution for those cases that versionCheckHook doesn't support right now. Currently you either have to use testers.testVersion (which in my opinion is suboptimal because you can't integrate it into your main derivation, so you always have to remember to also build my-pkg.tests.version separately), or you write a custom installCheckPhase.

Personally, I don't think this PR would add too much complexity to versionCheckHook and it would resolve many different limitations of the current implementation, so in my opinion a generic solution like this which can resolve all of these limitations and make versionCheckHook as useful as testers.testVersion would make sense. Of course I do understand that you would like to keep the implementation as simple as possible, I just don't think that this PR would make it much more complex than it already is.

[RossSmyth]

I've been working on some bash -> python conversion for hooks and here's the unsolicited WIP for this hook
master...RossSmyth:nixpkgs:versionPython