doc/meta: Mention --version as a good usecase for installCheckPhase

[doronbehar]

Description of changes

The first 3 commits of this PR are rather trivial, hopefully will be easily accepted. The last commit is a minor change of policy - recommending using installCheckPhase over passthru.tests.version.

This is in response to a discussion that wasn't fully resolved in the original PR that added these lines and at discourse here

The arguments for using installCheckPhase over passthru.tests.version, for the simple case of a --version | grep ${version} test, should be fully explained in the content added. Review is welcome.

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.

[raboof]

The general sentiment makes sense to me: this test is useful and not resource-intensive, so it seems helpful to execute it as part of the 'main' build.

This begs the question whether we should remove testVersion, as that now seems duplicate. I think it still makes sense to keep it, to catch issues where the program incorrectly relies on something that is only available in the build environment.

Added a couple of smaller suggestions.

[raboof]

this style seems less common than mkDerivation rec {, is there a reason we'd want to show finalAttrs here?

[doronbehar]

this style seems less common than mkDerivation rec {, is there a reason we'd want to show finalAttrs here?

using finalAttrs is the modern way to mkDerivation nowadays, as it makes overriding the package much easier when the package references it self, and especially if you want to make it easy for the user reading this to maybe add a passthru.tests.comprehensive that will use finalAttrs.finalPackage which is not possible with the old mkDerivation rec { pattern.

[raboof]

using finalAttrs is the modern way to mkDerivation nowadays

Do you have a reference for that? https://nixos.org/manual/nixpkgs/unstable/#sec-using-stdenv is still showing rec

it makes overriding the package much easier when the package references it self, and especially if you want to make it easy for the user reading this to maybe add a passthru.tests.comprehensive that will use finalAttrs.finalPackage which is not possible with the old mkDerivation rec { pattern

TIL, very cool

[doronbehar]

Do you have a reference for that? nixos.org/manual/nixpkgs/unstable/#sec-using-stdenv is still showing rec

I assumed that TBH :) The current docs for that don't mention it as a superior way to use mkDerivation, but personally I think it is. As for the docs at hand though, I still think it is better to suggest using a function, as it is especially relevant for passthru.tests when you want to use finalAttrs.finalPackage.

[raboof]

Some more ongoing discussion on this in #315337

[doronbehar]

The general sentiment makes sense to me: this test is useful and not resource-intensive, so it seems helpful to execute it as part of the 'main' build.

Great I'm glad you agree.

This begs the question whether we should remove testVersion, as that now seems duplicate. I think it still makes sense to keep it, to catch issues where the program incorrectly relies on something that is only available in the build environment.

Definitely I don't think the right thing to do now is to iterate all usages of testVersion and replace them with installCheckPhase - that would create too many rebuilds and it needs to be tested manually. Perhaps though, we can only remove the documentation? Which is here BTW:

nixpkgs/doc/build-helpers/testers.chapter.md

Lines 119 to 166 in 340d46c

	## `testVersion` {#tester-testVersion}
	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.
	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`.
	The default argument to the command is `--version`, and the version to be checked will be inferred from the given `package` attribute as well.
	:::{.example #ex-testversion-hello}
	# Check a program version using all the default values
	This example will run the command `hello --version`, and then check that the version of the `hello` package is in the output of the command.
	```nix
	{
	passthru.tests.version = testers.testVersion { package = hello; };
	}
	```
	:::
	:::{.example #ex-testversion-different-commandversion}
	# Check the program version using a specified command and expected version string
	This example will run the command `leetcode -V`, and then check that `leetcode 0.4.2` is in the output of the command as a whole word (separated by whitespaces).
	This means that an output like "leetcode 0.4.21" would fail the tests, and an output like "You're running leetcode 0.4.2" would pass the tests.
	A common usage of the `version` attribute is to specify `version = "v${version}"`.
	```nix
	{
	version = "0.4.2";
	passthru.tests.version = testers.testVersion {
	package = leetcode-cli;
	command = "leetcode -V";
	version = "leetcode ${version}";
	};
	}
	```
	:::

[doronbehar]

this style seems less common than mkDerivation rec {, is there a reason we'd want to show finalAttrs here?

using finalAttrs is the modern way to mkDerivation nowadays, as it makes overriding the package much easier when the package references it self, and especially if you want to make it easy for the user reading this to maybe add a passthru.tests.comprehensive that will use finalAttrs.finalPackage which is not possible with the old mkDerivation rec { pattern.

[raboof]

Perhaps though, we can only remove the documentation?

I think this test still has value, so I don't think we should remove it or the documentation.

[doronbehar]

Perhaps though, we can only remove the documentation?

I think this test still has value, so I don't think we should remove it or the documentation.

OK, another option is to slightly modify the introduction to it, such that it will mention installCheckPhase, and how that is preferred for most cases?

[raboof]

Perhaps though, we can only remove the documentation?

I think this test still has value, so I don't think we should remove it or the documentation.

OK, another option is to slightly modify the introduction to it, such that it will mention installCheckPhase, and how that is preferred for most cases?

I don't think I agree installCheckPhase should replace testVersion, having both makes sense to me. I appreciate you might feel differently - perhaps let's just keep that part as it is for now?

[doronbehar]

having both makes sense to me.

It makes sense to me too. However I felt we lack some consistency here if this part of the docs doesn't mention the other and vise versa. I added a sentence here that mentions testVersion, with a link.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/95

[raboof]

Some more ongoing discussion on this in #315337

[Atemu]

It'd be great if this could simply be a hook which would be configured using a similar interface as the tester.

[Artturin]

This doesn't show the whole output on failure, nor is it separated from the build environment, therefore it is inferior to testVersion, a hook which contains the testVersion code and clears the env and switches the dir to an empty directory will be needed.