Nixpkgs documentation: reword info about version testers #348025

AndersonTorres · 2024-10-12T02:19:42Z

Things done

Add a 👍 reaction to pull requests you find important.

nixos-discourse · 2024-10-17T10:22:51Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4695

Daholli

Sounds very good, I cannot really judge the content besides being able to understand it better

doronbehar

Thanks for the PR. The rephrasings are indeed better, but I'm not sure about a few things.

doc/stdenv/passthru.chapter.md

doc/build-helpers/testers.chapter.md

doc/hooks/versionCheckHook.section.md

doronbehar · 2024-10-19T19:21:14Z

doc/stdenv/passthru.chapter.md

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


Hmm in principal I don't mind much how this got moved and rephrased into versionCheckHook.section.md, but I think there should be a single place in which we compare both of the methods. Perhaps the "Package tests" subtitle in pkgs/README.md is a better? What do you think? If you agree we should concentrate the information about this somewhere, I hope we'd agree the correct thing to do would be to link from here and from versionCheckHook.section.md to that comparison section.

Comparing both methods on a third place is a nice idea.
I will do this asap.

Hmm in principal I don't mind much how this got moved and rephrased into versionCheckHook.section.md, but I think there should be a single place in which we compare both of the methods. Perhaps the "Package tests" subtitle in pkgs/README.md is a better? What do you think? If you agree we should concentrate the information about this somewhere, I hope we'd agree the correct thing to do would be to link from here and from versionCheckHook.section.md to that comparison section.

Comparing both methods on a third place is a nice idea. I will do this asap.

One more thing: I don't like how this section was moved to a different place on a different commit (I'm reviewing this PR commit-wise).

Modifying multiple files at once is confusing and is prone to merge conflicts.

doc/hooks/versionCheckHook.section.md

doc/build-helpers/testers.chapter.md

pkgs/README.md

Aleksanaa · 2024-10-30T03:50:25Z

pkgs/README.md

+- Since [`nixpkgs-review`](https://github.com/Mic92/nixpkgs-review) does not execute `passthru.tests` attributes, it will not execute `passthru.tests.version`.
+
+  On the other hand, `versionCheckHook` will be executed as a regular phase during the build time of the derivation, therefore it will be indirectly catched by `nixpkgs-review`, working around this limitation.
+
+- When a pull request is opened against Nixpkgs repository, [ofborg](https://github.com/NixOS/ofborg)'s CI will automatically run `passthru.tests` attributes for the packages that are [directly changed by your PR (according to your commits' messages)](https://github.com/NixOS/ofborg?tab=readme-ov-file#automatic-building).
+
+  However, ofBorg does not run the `passthru.tests` attributes for _transitive dependencies_ of those packages. To execute them, commands like [`@ofborg build dependency1.tests dependency2.tests ...`](https://github.com/NixOS/ofborg?tab=readme-ov-file#build) are needed.
+
+  On the other hand, `versionCheckHook` will be executed as a regular phase during the build time of the derivation, therefore it will be indirectly catched by ofBorg, working around this limitation.
+
+- Sometimes a package triggers no errors while being built - especially when the upstream provides no unit tests-, however it fails at runtime. When such a package is not used in a regular basis, it leaves room for a silent breakage that rots in the system / profile configuration, not being noticed for long periods of time until the next usage of this package.
+
+  Although `passthru.tests` fills the same purpose, it is more prone to be forgotten by human beings; on the other hand, `versionCheckHook` will be executed as a regular phase during the build time of the derivation, therefore it will not be accidentally ignored.
+
+Despite having an almost identical functioning and a huge overlap, `versionCheckHook` and `testers.testVersion` have complementary roles, and there are no impediments for using both at the same time.


These should be moved back to nixpkgs documentation.

Aleksanaa · 2024-10-30T03:51:25Z

pkgs/README.md

+
+- When a pull request is opened against Nixpkgs repository, [ofborg](https://github.com/NixOS/ofborg)'s CI will automatically run `passthru.tests` attributes for the packages that are [directly changed by your PR (according to your commits' messages)](https://github.com/NixOS/ofborg?tab=readme-ov-file#automatic-building).
+
+  However, ofBorg does not run the `passthru.tests` attributes for _transitive dependencies_ of those packages. To execute them, commands like [`@ofborg build dependency1.tests dependency2.tests ...`](https://github.com/NixOS/ofborg?tab=readme-ov-file#build) are needed.


Ofborg also doesn't run versionCheckHook for those transisive dependencies, unless you add them to passthru?

Ofborg also doesn't run versionCheckHook for those transisive dependencies, unless you add them to passthru?

If passthru.tests is empty, indeed ofborg won't run versionCheckHook for those dependent packages, but nixpkgs-review will.

Aleksanaa · 2024-10-30T03:53:53Z

pkgs/README.md

+
+  However, ofBorg does not run the `passthru.tests` attributes for _transitive dependencies_ of those packages. To execute them, commands like [`@ofborg build dependency1.tests dependency2.tests ...`](https://github.com/NixOS/ofborg?tab=readme-ov-file#build) are needed.
+
+  On the other hand, `versionCheckHook` will be executed as a regular phase during the build time of the derivation, therefore it will be indirectly catched by ofBorg, working around this limitation.


It's worth noting that people are supposed to add one package per commit, so ofborg will still build all passthru.tests.

This is more about future updates. E. g. SDL has many dependencies but an update of SDL does not trigger the already existing transitive dependencies in the current CI setup.

pkgs/README.md

doronbehar

I have a general comment: When such a big system such as Nixpkgs as managed and designed, you want to keep the amount of moving parts to a minimum, especially since you want to attract as much contributors as possible, and to make it easy for them as possible to contribute.

Hence, having 2 perfectly legitimate ways of doing sort of the same thing is a binary moving part that is not justified in my opinion - inexperienced contributors reading these docs won't know what to do and I anticipate their decision will be random in the best case, and they'd stay confused and won't contribute in the worst case. I really think we should reach a decision regarding this.

doronbehar · 2024-10-30T05:11:40Z

doc/build-helpers/testers.chapter.md

- 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.
+- It assures the main program can run.
+- It catches dynamic linking errors, missing or malformed environment variables and other runtime errors.


You know the word "catches" doesn't fit a passthru.tests.version check, because if you won't build that passthru.tests derivation you won't catch anything. I'd use the word "catches" when describing versionCheckHook, but not when describing both.

You approved that wording before.

Further, I don't think this difference matters here.

passthru.tests.version catches errors when executed.
versionCheckHook catches errors when executed.

The difference is passthru.tests.version is executed directly when called e.g. via commandline, while versionCheckHook is executed indirectly when the package containing it is built.

You approved that wording before.

Correct, I inspected your phrasing in more details and changed my mind.

Further, I don't think this difference matters here.

passthru.tests.version catches errors when executed. versionCheckHook catches errors when executed.

The difference is passthru.tests.version is executed directly when called e.g. via commandline, while versionCheckHook is executed indirectly when the package containing it is built.

That's a big difference IMO. Of course we agree that if you'd explicitly build passthru.tests.version you'd catch an issue with the package, but I find it inaccurate to suggest that merely adding a passthru.tests.version will make any such issue be catched, because all the reasons you are already aware of from our discussion on Discourse.

doc/build-helpers/testers.chapter.md

doronbehar · 2024-10-30T05:15:59Z

doc/stdenv/passthru.chapter.md

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


Hmm in principal I don't mind much how this got moved and rephrased into versionCheckHook.section.md, but I think there should be a single place in which we compare both of the methods. Perhaps the "Package tests" subtitle in pkgs/README.md is a better? What do you think? If you agree we should concentrate the information about this somewhere, I hope we'd agree the correct thing to do would be to link from here and from versionCheckHook.section.md to that comparison section.

Comparing both methods on a third place is a nice idea. I will do this asap.

One more thing: I don't like how this section was moved to a different place on a different commit (I'm reviewing this PR commit-wise).

doronbehar · 2024-10-30T05:21:39Z

doc/hooks/versionCheckHook.section.md

@@ -22,14 +24,32 @@ stdenv.mkDerivation (finalAttrs: {
 })
 ```

-Note that for [`buildPythonPackage`](#buildpythonpackage-function) and [`buildPythonApplication`](#buildpythonapplication-function), `doInstallCheck` is enabled by default.
+Note that some builders, e.g. [`buildPythonPackage`](#buildpythonpackage-function) and [`buildPythonApplication`](#buildpythonapplication-function), enable `doInstallCheck` by default.


It's not such a big effort IMO to maintain here a list of all builders that enable installCheck phase by default. Hence, the previous phrasing is a bit more decisive and informative. This phrasing sort of suggests: "We don't keep track of which builders enable installCheck by default and which do, so you should inspect the source code of each if you don't want to enable it when not needed."

Hum, should this paragraph be rewritten each time a new custom builder appears? I don't think so.
This paragraph looks like an example of builder, not a exhaustive listing.

Further, the documentation of these builders should provide this information, and it is the duty of a contributor to read the documentation of that builder at least.
If this info is not found in the docs, a bug report should be opened and the docs team should be pinged.

Further, the documentation of these builders should provide this information, and it is the duty of a contributor to read the documentation of that builder at least.
If this info is not found in the docs, a bug report should be opened and the docs team should be pinged.

I agree with that. But as for:

Hum, should this paragraph be rewritten each time a new custom builder appears? I don't think so.
This paragraph looks like an example of builder, not a exhaustive listing.

Nowadays, the list of builders that enable installCheck is very small, so I don't find it exhaustive to list them. I agree that theoretically if a new builder that enables installCheck will be added in the future to Nixpkgs, probably no one will remember or know to add it to this "list", but I doubt that will happen TBH, from my experience with Nixpkgs.

The previous phrasing also didn't suggest that the builders mentioned is the definitive list of builders that enable installCheck by default. I just say that your phrasing slightly more implies that maybe many more builders are enabling installCheck, and that is not true.

Reverted, in terms. I reworded the example in a more direct sentence.

doc/hooks/versionCheckHook.section.md

doronbehar · 2024-10-30T05:40:22Z

pkgs/README.md

+
+The most notable difference between `versionCheckHook` and `testers.testVersion` is that `versionCheckHook` runs at the build time of the package, while `passthru.tests.versions` executes a whole derivation that depends on the package.
+
+The advantages of `testers.testVersion` over `versionCheckHook` are listed at [Package tests](#var-passthru-tests-packages) already. Below we will list some advantages of `versionCheckHook` over `testers.testVersion`:


Hmm it felt natural how the advantages of testVersion were listed there, but here it doesn't feel natural at all how the advantages of versionCheckHook are listed here and not in it's documentation. If this section is a comparison, it should aggregate advantages and disadvantages of both methods in the same place.

You suggested to put the comparison in pkgs/README before.

Do you think this comparison should be in versionCheckHook.md instead?

You suggested to put the comparison in pkgs/README before.

I know, but I didn't anticipate that it'd come out so inconsistent, because you'd keep the pros and cons of testVersion elsewhere. What I suggested was (this time more accurately):

Put the 2x2 matrix of pros & cons of both testVersion & versionCheckHook in a neutral place like pkgs/README.md, and link to that comparison section from both testVersion's documentation and versionCheckHook.section.md. Mention shortly the motivation to use either tests in testVersion's documentation and versionCheckHook.section.md.

I cannot promise I'd approve such a change never the less, because if we can (and I think we should) reach a consensus that one of the approaches should be the default approach to use, there is no justification for putting such an exhaustive 2x2 matrix of pros and cons in a neutral place.

pkgs/readme.md does not work.
For now I will move this to versioncheckhook docs

doronbehar · 2024-10-30T05:42:10Z

pkgs/README.md

+
+  Although `passthru.tests` fills the same purpose, it is more prone to be forgotten by human beings; on the other hand, `versionCheckHook` will be executed as a regular phase during the build time of the derivation, therefore it will not be accidentally ignored.
+
+Despite having an almost identical functioning and a huge overlap, `versionCheckHook` and `testers.testVersion` have complementary roles, and there are no impediments for using both at the same time.


I don't agree with this sentence. Why would someone make an effort to use both? Why can't we agree that either of them fits most cases?

Why would someone make an effort to use both?

I don't know. Maybe stress-testing.

Why can't we agree that either of them fits most cases?

Because there is no objective metric for this right now.

On the other hand, I can delete this last line.

Why can't we agree that either of them fits most cases?

Because there is no objective metric for this right now.

Objective metric? Metric of what?

Metric of fittingness.

"For packages with such and such characteristics, this approach is well-suited because of such and such premises; otherwise the other approach is better".

(I am redacting this the more neutral possible way, precisely because we have no objective metric.)

- Flip the explanation and buildPythonPackage reference paragraphs - Add a section to emphasize the attributes controlling the hook

…stVersion

- Reword the paragraphs - Remove the comparison with `versionCheckHook` - It will be moved to `versionCheckHook.section.md`

doronbehar · 2024-11-01T11:11:56Z

doc/build-helpers/testers.chapter.md

+Note: see also:
+
+- [`versionCheckHook`](#versioncheckhook), an independent hook with similar functionality.
+  - [A comparison between both](#versioncheckhook-comparison-testversion)


I find it confusing to mention this alternative and not telling the users right away which approach should be used most of the times.

doronbehar · 2024-11-01T11:12:52Z

doc/hooks/versionCheckHook.section.md

 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:
+Note that `doInstallCheck` is enabled by default for [`buildPythonPackage`](#buildpythonpackage-function) and [`buildPythonApplication`](#buildpythonapplication-function).


That's almost the same sentence..

doronbehar

Getting better, but I'd still insist that we should tell users that in most cases one of the approaches is better.

doronbehar · 2024-11-01T11:16:11Z

doc/hooks/versionCheckHook.section.md

+  - E.g. extra tools, customized programs etc.
+- `testers.testVersion` executed in an identical environment of a consumer would, independent of the environment it was built.
+- `testers.testVersion` can be executed without rebuilding the package, saving time especially when the rebuild takes too much time.
+- `testers.testVersion` do not add overhead to each build, since it runs after the build, not during it.


Calling this an overhead is an overstatement IMO.

AndersonTorres marked this pull request as ready for review October 12, 2024 02:19

github-actions bot added the 8.has: documentation label Oct 12, 2024

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Oct 12, 2024

AndersonTorres marked this pull request as draft October 12, 2024 11:51

AndersonTorres force-pushed the documentation branch from c770b3c to 3d6d946 Compare October 12, 2024 13:22

AndersonTorres marked this pull request as ready for review October 12, 2024 14:14

Daholli approved these changes Oct 18, 2024

View reviewed changes

pbsds requested a review from doronbehar October 19, 2024 04:54

wegank added the 12.approvals: 1 label Oct 19, 2024

doronbehar reviewed Oct 19, 2024

View reviewed changes

AndersonTorres force-pushed the documentation branch 2 times, most recently from 851811d to 63d05d4 Compare October 20, 2024 15:59

AndersonTorres marked this pull request as draft October 20, 2024 16:01

github-actions bot added the 6.topic: policy discussion label Oct 20, 2024

nix-owners bot requested a review from infinisil October 20, 2024 16:02

zi3m5f reviewed Oct 20, 2024

View reviewed changes

doc/build-helpers/testers.chapter.md Show resolved Hide resolved

pkgs/README.md Outdated Show resolved Hide resolved

This was referenced Oct 20, 2024

sqlc: adopt #349728

Closed

clang-uml: fix --version output #350575

Merged

AndersonTorres force-pushed the documentation branch 4 times, most recently from 7587fac to 32b264b Compare October 30, 2024 01:32

AndersonTorres marked this pull request as ready for review October 30, 2024 01:39

AndersonTorres force-pushed the documentation branch from 32b264b to 092b9aa Compare October 30, 2024 01:42

AndersonTorres requested a review from doronbehar October 30, 2024 01:42

Aleksanaa reviewed Oct 30, 2024

View reviewed changes

doronbehar requested changes Oct 30, 2024

View reviewed changes

wegank removed the 12.approvals: 1 label Oct 31, 2024

doc/hooks/versionCheckHook.section.md: expand and reword

2e93897

- Flip the explanation and buildPythonPackage reference paragraphs - Add a section to emphasize the attributes controlling the hook

AndersonTorres force-pushed the documentation branch from 092b9aa to 654f87f Compare November 1, 2024 01:29

github-actions bot removed the 6.topic: policy discussion label Nov 1, 2024

doc/hooks/versionCheckHook.section.md: add comparison with testers.te…

895d970

…stVersion

AndersonTorres force-pushed the documentation branch from 654f87f to 49377ae Compare November 1, 2024 01:32

AndersonTorres added 2 commits October 31, 2024 22:44

doc/stdenv/passthru.chapter.md: expand Package tests section

b0c1a96

- Reword the paragraphs - Remove the comparison with `versionCheckHook` - It will be moved to `versionCheckHook.section.md`

doc/build-helpers/testers.chapter.md: expand testVersion

4d4f230

AndersonTorres force-pushed the documentation branch from 49377ae to 4d4f230 Compare November 1, 2024 01:45

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 labels Nov 1, 2024

doronbehar reviewed Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nixpkgs documentation: reword info about version testers #348025

Nixpkgs documentation: reword info about version testers #348025

AndersonTorres commented Oct 12, 2024

nixos-discourse commented Oct 17, 2024

Daholli left a comment

doronbehar left a comment

doronbehar Oct 19, 2024

AndersonTorres Oct 20, 2024

doronbehar Oct 30, 2024

AndersonTorres Nov 1, 2024

Aleksanaa Oct 30, 2024

AndersonTorres Nov 1, 2024

Aleksanaa Oct 30, 2024

doronbehar Oct 30, 2024

Aleksanaa Oct 30, 2024

AndersonTorres Oct 31, 2024

doronbehar left a comment

doronbehar Oct 30, 2024

AndersonTorres Nov 1, 2024 •

edited

Loading

doronbehar Nov 1, 2024

doronbehar Oct 30, 2024

doronbehar Oct 30, 2024

AndersonTorres Oct 31, 2024

doronbehar Oct 31, 2024

AndersonTorres Nov 1, 2024

doronbehar Oct 30, 2024

AndersonTorres Oct 31, 2024

doronbehar Oct 31, 2024

AndersonTorres Nov 1, 2024

doronbehar Oct 30, 2024

AndersonTorres Oct 31, 2024

doronbehar Oct 31, 2024

AndersonTorres Oct 31, 2024 •

edited

Loading

doronbehar Nov 1, 2024

doronbehar Nov 1, 2024

doronbehar left a comment

doronbehar Nov 1, 2024


		- When a pull request is opened against Nixpkgs repository, [ofborg](https://github.com/NixOS/ofborg)'s CI will automatically run `passthru.tests` attributes for the packages that are [directly changed by your PR (according to your commits' messages)](https://github.com/NixOS/ofborg?tab=readme-ov-file#automatic-building).

		However, ofBorg does not run the `passthru.tests` attributes for _transitive dependencies_ of those packages. To execute them, commands like [`@ofborg build dependency1.tests dependency2.tests ...`](https://github.com/NixOS/ofborg?tab=readme-ov-file#build) are needed.


		However, ofBorg does not run the `passthru.tests` attributes for _transitive dependencies_ of those packages. To execute them, commands like [`@ofborg build dependency1.tests dependency2.tests ...`](https://github.com/NixOS/ofborg?tab=readme-ov-file#build) are needed.

		On the other hand, `versionCheckHook` will be executed as a regular phase during the build time of the derivation, therefore it will be indirectly catched by ofBorg, working around this limitation.


		The most notable difference between `versionCheckHook` and `testers.testVersion` is that `versionCheckHook` runs at the build time of the package, while `passthru.tests.versions` executes a whole derivation that depends on the package.

		The advantages of `testers.testVersion` over `versionCheckHook` are listed at [Package tests](#var-passthru-tests-packages) already. Below we will list some advantages of `versionCheckHook` over `testers.testVersion`:


		Although `passthru.tests` fills the same purpose, it is more prone to be forgotten by human beings; on the other hand, `versionCheckHook` will be executed as a regular phase during the build time of the derivation, therefore it will not be accidentally ignored.

		Despite having an almost identical functioning and a huge overlap, `versionCheckHook` and `testers.testVersion` have complementary roles, and there are no impediments for using both at the same time.

Nixpkgs documentation: reword info about version testers #348025

Are you sure you want to change the base?

Nixpkgs documentation: reword info about version testers #348025

Conversation

AndersonTorres commented Oct 12, 2024

Things done

nixos-discourse commented Oct 17, 2024

Daholli left a comment

Choose a reason for hiding this comment

doronbehar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doronbehar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonTorres Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonTorres Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doronbehar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonTorres Nov 1, 2024 •

edited

Loading

AndersonTorres Oct 31, 2024 •

edited

Loading