doc: emphasize trade-off between versionCheckHook and testers.testVersion

[pbsds]

Description of changes

I'm not a fan of the recent push to migrate to versionCheckHook. This is a decision better left to each package maintainer, rather than drive-by fixups or some notion of prescribed "best practice".

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.

[pbsds]

Related case: when i expanded --replace to --replace-{fail,warn,quiet}, i did not prescribe any best practice. I only made the implicit explicit and provided options, letting the community decide.

[doronbehar]

And in contrast to this blocking/non-blocking dilemma, the cases where --replace-fail fits is much more common then --replace-warn but still, IIRC I've seen --replace-quiet legitimate usages.

[doronbehar]

That's great 👍.

[doronbehar]

That's a good start for the sentence, but I don't think that the average contributor knows whether they would like to trigger a build failure or not. Perhaps here, or in the NOTE at the end of the diff we can lay out arguments why would someone want the failure to block and when not?

Personally I don't see a reason why it shouldn't block.

[pbsds]

Some upstreams do not have a bump script but instead do releases by hand. This risks tagging versions where the version provided in e.g. package.json is incorrect.

[doronbehar]

Some upstreams do not have a bump script but instead do releases by hand. This risks tagging versions where the version provided in e.g. package.json is incorrect.

Isn't that a good sign that the package.json should be patched? and that upstream would want to know about that? And perhaps it also means that users reporting upsteram bugs with that incorrect version would confuse them?

[pbsds]

Indeed, but we don't have any large-scale automated reporting on r-ryantm bump failures yet (this may be subject to change), meaning version check failures could make updates stop coming in

EDIT: that argument applies to any sanity-checks not put in passthru.tests

[pbsds]

Here are some arguments for and against. I'm not sure whether including it is beneficial, but I can do it if you prefer so. Please help me flesh out the arguments in that case :)

Cases for versionCheckHook

* Stricter, blocks build on version check failure
* Requires the entrypoint to be hermetic enough to print its version

Cases for testers.testVersion

* Does not block build on version check failure
    * The output of potentially long running builds are thus not discarded
    * Bot updates such as r-ryantm will happen more often, in turn triggering maintainer pings and review.
* Works with non-hermetic entrypoints, dependant on certain environment variables

[doronbehar]

EDIT: that argument applies to any sanity-checks not put in passthru.tests

Sanity checks other then versionCheckHook not in passthru.tests? What do you mean?

• Requires the entrypoint to be hermetic enough to print its version

But that's also a case to not use any version test :). Also, people may not understand what is an entrypoint, or what do you mean by "hermetic".

• Bot updates such as r-ryantm will happen more often, in turn triggering maintainer pings and review.

Isn't this correct only in the rare case where upstream released the new version manually and forgot to update the VERSION file or package.json version or whatever? Also, I wonder whether r-ryantm will notice that and not even open a PR, as you may have noticed that it says every time: "found ${version} with grep in /nix/store/....`.

• Works with non-hermetic entrypoints, dependant on certain environment variables

So if by "hermetic" you mean that an executable may need an environment variable like $HOME to even do the simple version print, that is not explained well with this phrasing. An example of such a case was observed here: https://github.com/NixOS/nixpkgs/pull/339197/files#diff-dc99f6cd821b428eaa2723c4ef39f37283f34ab8c1c430c9b6a8963e64b5a24cR88-R89, and indeed it is worth mentioning, but I'm not sure yet where. Also, in the future I might add support for inheriting environment variables when trying to get version in versionCheckHook, and then this won't be an issue.

• Does not block build on version check failure
  • The output of potentially long running builds are thus not discarded

I would agree upon something like this:

If you have a long and heavy to build package that you are not yet sure how to test it's version print or if it has any, then you may want to postpone the addition of versionCheckHook to a more mature state of the package and it's expression in Nixpkgs.

[pbsds]

rather than enumerate pros and cons with subjective language i amended the PR to only document the current state

[pbsds]

since i goofed the force-push: https://github.com/NixOS/nixpkgs/compare/0b023392177cf9ed310cc2d9fc8d97974b9c92b0..96edc97ca839783772f447c291afd4dfdb761ffc

[getchoo]

I think this is a much better way to position both of these options. While versionCheckHook is in theory a safer solution, as we've had some time to let it simmer, I've personally noticed a couple things:

1. This is incredibly painful for longer builds like most Rust packages, both when making changes to Nixpkgs and as a user overriding packages. Waiting for one to complete, only for the whole thing to be thrown away because upstream may have forgotten to bump a version or you're using a development revision isn't very fun

2. Explaining and sometimes using the API isn't great. testVersion is quite comfortable with it using meta.mainProgram, while versionCheckHook requires you to not only duplicate that program name, but also provide it's full path. Of course versionCheckHook is also nice in that you don't need to rely on broken recursion or finalAttrs.finalPackage -- plus it's a bit better at detecting the actual version -- but this still a trade-off either way at the end of the day

Considering that these serve the same exact purpose and should always be run prior to any merge, it's hard to say one is better for even most cases. The wording here is a lot better

[getchoo]

Suggested change

	NOTE: This is a check you add to `passthru.tests` which is mainly run by ofborg but not hydra. If you want a version check failure to block the build altogether, then [`versionCheckHook`](#versioncheckhook) is the tool you're looking for. The motivation for adding either of these checks would be:
	NOTE: This is a check you add to `passthru.tests`, which is mainly run by OfBorg, but not in Hydra. If you want a version check failure to block the build altogether, then [`versionCheckHook`](#versioncheckhook) is the tool you're looking for. The motivation for adding either of these checks would be:

Nit

[doronbehar]

The whole point of the long debate that was present in previous revisions of this PR and also in other places, was whether do we want by default, to recommend people to make a failing --version test block the build. The new phrasing you are suggesting indeed encapsulates the whole essence of the debate and the advantage of versionCheckHook, but you removed the recommendation to make a failing --version test block the build - which slightly defeats the original motivation for creating versionCheckHook.

I'm afraid that readers won't figure out by themselves that it is recommended for almost all packages to do block the build in case they are modified in a way that makes a --version check fail. Hence I'd suggest (on top of @getchoo's suggestion):

Suggested change

	NOTE: This is a check you add to `passthru.tests` which is mainly run by ofborg but not hydra. If you want a version check failure to block the build altogether, then [`versionCheckHook`](#versioncheckhook) is the tool you're looking for. The motivation for adding either of these checks would be:
	NOTE: This is a check you add to `passthru.tests`, which is mainly run by OfBorg, but not in Hydra. If you want a version check failure to block the build altogether (recommended in most cases), then [`versionCheckHook`](#versioncheckhook) is the tool you're looking for. The motivation for adding either of these checks would be:

[getchoo]

I'm afraid that readers won't figure out by themselves that it is recommended for almost all packages to do block the build in case they are modified in a way that makes a --version check fail.

Why should this be "almost all packages"? (This is not a literal question, as there has been plenty of debate on it already. Please see my comment above)

I feel like this suggestion is just a sneaky way of going back to the status quo of plain recommending versionCheckHook over testers.testVersion. It completely misses the main goal of this PR, which is to encourage package maintainers to instead use their best judgement in what would be important for each package

[pbsds]

Suggested change

	NOTE: This is a check you add to `passthru.tests` which is mainly run by ofborg but not hydra. If you want a version check failure to block the build altogether, then [`versionCheckHook`](#versioncheckhook) is the tool you're looking for. The motivation for adding either of these checks would be:
	NOTE: This is a check you add to `passthru.tests` which is mainly run by OfBorg, but not in Hydra. If you want a version check failure to block the build altogether, then [`versionCheckHook`](#versioncheckhook) is the tool you're looking for (and reccomended for quick builds). The motivation for adding either of these checks would be:

[doronbehar]

That sounds better to me :) Though it is recommended (not reccomended).

[nixpkgs-ci]

Successfully created backport PR for release-24.11:

• [Backport release-24.11] doc: emphasize trade-off between versionCheckHook and testers.testVersion #377213