pkgs/by-name: testVersion -> versionCheckHook where applicable

[getchoo]

Description of changes

Follow up on #320266

This should help

• Simplify expressions
• Reduce (some) repition/workarounds
• Improve automated testing by making these checks a requirement to
  build
• Maintain consistency between which of these version check
  implementations are used across pkgs/by-name
• Provide more real-world examples of this newly added hook
• Avoid Lint on broken recursion structure #312345 in many places

CC @doronbehar

---

I believe all occurrences of testVersion have been either replaced or given a TODO comment, with the exception of

clang-tidy-sarif
clippy-sarif
hadolint-sarif
hello
sarif-fmt
shellcheck-sarif

The sarif packages have been taken care of in #327451, #327452, #327454, #327455, and #327456 earlier today. hello will retain it's use of testVersion to serve as an example (as I believe this was the intention, given it was already there)

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[eclairevoyant]

Two issues I see:

• This gets executed in the build environment rather than a separate test eevironment.
• This basically breaks cross-compilation unless you also have binfmt set up, which in turn further restricts the platforms these packages can be built on (and you have to deal with emulation, which presumably you wanted to avoid in the first place if you were cross-compiling...)

And to respond to the "pro" list:

• Simplify expressions

Simplification shouldn't introduce incorrect designs

• Reduce (some) repition/workarounds

Unclear

• Improve automated testing by making these checks a requirement to build

Probably the only benefit, except r-ryantm and ofborg already build the tests anyway.

• Maintain consistency between which of these version check
  implementations are used across pkgs/by-name

We should consistently use testVersion imo

• Provide more real-world examples of this newly added hook

Not really a "pro"

• Avoid Lint on broken recursion structure #312345 in many places

Those builders should be fixed instead

[getchoo]

This gets executed in the build environment rather than a separate test eevironment.

This is an "issue" with the hook itself and should have been discussed in #320266. The changes here simply follow the docs recommendation of preferring checkVersionHook. If you would like to discuss this further, feel free to open a separate issue

This basically breaks cross-compilation unless you also have binfmt set up, which in turn further restricts the platforms these packages can be built on (and you have to deal with emulation, which presumably you wanted to avoid in the first place if you were cross-compiling...)

As already mentioned in the original PR, the installCheck phase is disabled for cross compiled builds. This is not an issue

Simplification shouldn't introduce incorrect designs

Aside from what you mentioned above...what is incorrect here?

Unclear

Many packages require workarounds in filtering their output to use testVersion -- two examples are fastfetch and menulibre. Other packages also did things similar to the new hook, such as using --help to test and/or find a version -- see dependabot-cli and flite for examples of this

except r-ryantm and ofborg already build the tests anyway

Many contributors look over this though, and it commonly results in these tests failing in PRs when they could have otherwise been caught earlier. This obviously isn't a knock against testVersion or passthru.tests in general (making sure contributors are correct is ofborgs job after all), but an advantage here nonetheless

We should consistently use testVersion imo

Once again, feel free to open another issue to discuss this further. For now, the docs explicitly state checkVersionHook should be preferred

Those builders should be fixed instead

I 100% agree! Until then though, this does avoid the issue

[eclairevoyant]

If we're talking about docs, they do say:

passthru.tests have several advantages over running tests during any of the standard phases:

• 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

Build overhead is not as relevant but the independent env is relevant and the point I made above.

[getchoo]

the independent env is relevant

The hook does run in a clean environment with a different working directory -- which was actually added for similar reasons here. Once again though, this feels more like you taking issue with the existence and efficacy of the hook itself rather than this PR specifically. I would highly encourage opening a separate issue to discuss this

[eclairevoyant]

Of course I am discussing the hook, hence I object to a treewide mass rebuild PR that uses it if it's not a net benefit.

[doronbehar]

I agree that if our consensus is not strong regarding the hook and the recommendation for it by the docs, perhaps these disagreements should block this PR. Unfortunately the PR that added the hook hasn't received wide attention. @eclairevoyant please open a new discussion in an new issue where you address specific sentences in the docs with which you don't agree, and explain why thoroughly.

[doronbehar]

I object to a treewide mass rebuild PR that uses it if it's not a net benefit.

I partially agree with this argument, but not exactly with how it is phrased: We have a staging branch specifically to not really trigger mass rebuilds, because the staging receives all the time changes that trigger such mass rebuilds, and sometimes even basic stuff like compilers and zlib don't build on it, and eventually in a staging-next cycle things stabilize.

I do agree that in terms of potential master -> staging merge conflicts issues, and also because this PR is so big, it'd be better to only touch here leaf packages that don't trigger mass rebuilds. Also we should remember the main motivation for creating the hook was especially significant for leaf packages:

nixpkgs/doc/stdenv/passthru.chapter.md

Lines 86 to 88 in 716331f

	- 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.

Additionally, running a nixpkgs-review on this PR would be easier if only leaf packages would be touched. Perhaps we should limit this to less then 200 rebuilds? The hard part would be to filter out from the current PR the mass rebuild triggering packages...

See also discussion that started here (which was the seed for the idea of this hook):

https://discourse.nixos.org/t/improve-the-jdk-infrastructure-on-nixpkgs/45678/7

[pbsds]

Looking for reviewers for #344321 i now discover this discussion. I believe the trade-off between the two is worth considering, but should be at the package maintainers discretion.