Nixpkgs documentation: reword info about version testers #348025
Conversation
github-actions
bot
added
the
8.has: documentation
ofborg
bot
added
10.rebuild-darwin: 0
AndersonTorres
force-pushed
the
documentation
branch
from
c770b3c to
3d6d946
Compare
|
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 |
wegank
added
the
12.approvals: 1
AndersonTorres
force-pushed
the
documentation
branch
2 times, most recently
from
851811d to
63d05d4
Compare
AndersonTorres
force-pushed
the
documentation
branch
4 times, most recently
from
7587fac to
32b264b
Compare
AndersonTorres
force-pushed
the
documentation
branch
from
32b264b to
092b9aa
Compare
There was a problem hiding this comment.
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.
AndersonTorres
force-pushed
the
documentation
branch
2 times, most recently
from
20cf91c to
be53783
Compare
There was a problem hiding this comment.
OK now I reviewed the PR with the intention of actually perhaps merging it, so I noticed a few more details with which I'm not satisfied. I must say that most of the rephrasing is pretty good, but I feel like this PR tries under the hood to give the testers.testVersion and versionCheckHook a similar position, and I don't agree with this direction.
Earlier I suggested putting a 2x2 comparison matrix of pros and cons of testers.testVersion and versionCheckHook. I don't think it is required, because it is more important to give readers the bottom line that (agreeably) states that versionCheckHook fits most typical cases, but never the less I'll write here the 2x2 comparison matrix in order to perhaps convince slightly more the author of the PR:
| **** | versionCheckHook |
testers.testVersion |
|---|---|---|
| Special customization | The hook doesn't provide many ways to control it, other then with attributes document to control it. | Can access all features of Nix and Nixpkgs, like generating multiple version test derivations from a list of strings, and accessing programs / tools not part of the derivation's inputs. |
| Environment | Uses env --ignore-environment --chdir=/ to run the executables in a pure environment as possible, but there is no way to change this behavior. |
Executed in an identical environment of a consumer, with independently of the environment the executable was built at. |
| Requires rebuild? | Yes | No |
| Overhead? | Negligible | Zero |
| content-addressed derivations compatible | Can be problematic (TODO: Explain why - I don't understand this part) | TODO: Should be OK, explain why. |
| Being run by @ofborg? | Yes, as it is part of building a package. | Yes, for directly changed packages, but for dependent packages you'd @ofborg build, you'd have to remember to also build the dependent package's .tests derivations. |
Being run by nixpkgs-review? |
Yes, as it is part of building a package. | No, but see this nixpkgs-review PR. |
What happens when package A depending on B is all of a sudden builds but does not work due to a change in B? |
You will notice a breakage as soon you attempt to build the package - will happen at your next system upgrade etc, and when someone runs a nixpkgs-review when changing B. |
No one will notice the breakage until you will attempt to use the package on your system, because practically no one runs passthru.tests regularly. |
| How likely will you notice a breakage? | Impossible not to notice. | Very possible, especially if the package is not well maintained and not often used. |
P.S:
I'd change the commit title of the last commit to:
versionCheckHook.section.md: compare with testers.testVersion
So it will not span over 2 lines..
| - 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. |
There was a problem hiding this comment.
Objectively the overhead of testVersion is zero.
I will add an extra bullet.
Your insistence on being so accurate is a bit tiring, not only for me as a reviewer, but mostly to the readers. If a reader really wishes to get into all the details, they can read the source code. 99% percent of users won't complain about the docs not mentioning the (lack of) "overhead" of (testVersion) versionCheckHook.
AndersonTorres
force-pushed
the
documentation
branch
4 times, most recently
from
dad44a8 to
79a38da
Compare
|
And the prize for the best error message goes to
|
| @@ -169,15 +169,24 @@ The build will fail if `shellcheck` finds any issues. | |||
|
|
|||
| Checks that the output from running a command contains the specified version string in it as a whole word. | |||
|
|
|||
| NOTE: In most cases, [`versionCheckHook`](#versioncheckhook) should be preferred, but this function is provided and documented here anyway. The motivation for adding either tests would be: | |||
| Note: Given the current limitations of Nixpkgs's tooling and CI, `versionCheckHook` fits better than `testers.testVersion` in most of typical situations. | |||
There was a problem hiding this comment.
Hmm this suggests a sort of dissatisfaction that might be opinionated, but I don't have a strong opinion regarding it, so I won't make this comment of mine a blocker.
There was a problem hiding this comment.
You recognized the limitations of the tooling, at least indirectly:
My opinion is that no one should be blamed for not doing something that our tooling doesn't allow them to easily do.
There was a problem hiding this comment.
You recognized the limitations of the tooling, at least indirectly:
My opinion is that no one should be blamed for not doing something that our tooling doesn't allow them to easily do.
I agree that these are "limitations", that also don't allow us to easily run passthru.tests of packages automatically etc., but I don't necessarily think that these limitations should be solved at some point. I'd say that calling this a limitation might even be an overstatement, at least for some tools. It'd be very nice if nixpkgs-review would have built passthru.tests automatically, but I wouldn't say that ofborg is not that well designed. Not only that, even if nixpkgs-review would have run it, I'd still claim that versionCheckHook is better, because running nixpkgs-review is not mandatory, and silent breakages can still rot in your system.
| | Execution environment | The hook uses `env --ignore-environment --chdir=/` to run the executables in an environment as clean as possible, but there is no way to change its behavior. | The tester executes in an identical environment of a consumer, independent from the building environment. | | ||
| | Rebuild after modification | The hook rebuilds the package when the hook is modified, since it is a phase running during the build time of the package. | The tester does not require rebuilding the package, since it is a derivation. | | ||
| | Overhead during package building | Negligible. Although running during the build time of the package, the hook is lean and executes few commands. | Zero, since the tester is a derivation that runs after package building. | | ||
| | Content-addressed-derivation awareness | Since it runs during the building, the hook does not deal with failures happening after building, like rewritings that happen post installation. | Since it runs after the building, the tester detects failures at this time. | |
There was a problem hiding this comment.
I must say that I don't understand this line of comparison at all. But maybe it is just me, who is not very well familiar with content addressed derivations...
There was a problem hiding this comment.
The citation of CA will change to something more general.
| | Execution by OfBorg CI tool | Yes, since the hook is a phase running during the build time of the package. | OfBorg does run the tester for _directly affected packages only_; transitive dependencies are ignored by default, requiring extra commands like `@ofborg build dependency1.tests dependency2.tests ...`. | | ||
| | Execution by `nixpkgs-review` tool | Yes, since the hook is a phase running during the build time of the package. | No, since the tool has no support for executing `passthru.tests`. [^1] | | ||
| | Package breakage awareness | Loud and clear as soon as the hook is reached, since it is a phase running during the build time of the package. | Requires specific command to be noticed, e.g. `nix-build -A pkg.tests.version`, since it is a derivation dependent on the package | | ||
| | Transitive package breakage | Never ever goes unnoticed by `nixpkgs-review`, since the tool executes the transitive dependencies. | Since human beings are prone to forget the duty of running passthru tests and `nixpkgs-review` has no support for running it, it certainly goes unnoticed. | |
There was a problem hiding this comment.
Haha this:
human beings are prone to forget the duty of running passthru tests
Is a very blaming phrasing.. I don't think there is a consensus that people should run passthru.tests of all packages they use on their system regularly or whatever. It is not exactly clear in which context it would be correct to blame someone about such a breakage. My opinion is that no one should be blamed for not doing something that our tooling doesn't allow them to easily do.
There was a problem hiding this comment.
I will remove references to less-than-omniscient things in the next iteration.
|
|
||
|
|
||
| | Item | `versionCheckHook` | `testers.testVersion` | | ||
| |:---------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------------------:|:-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------:| |
There was a problem hiding this comment.
Nit: Looks better IMO when lines are aligned to the left.
There was a problem hiding this comment.
I will remove these lines, at least until I discover why the manual was not built.
| | Package breakage awareness | Loud and clear as soon as the hook is reached, since it is a phase running during the build time of the package. | Requires specific command to be noticed, e.g. `nix-build -A pkg.tests.version`, since it is a derivation dependent on the package | | ||
| | Transitive package breakage | Never ever goes unnoticed by `nixpkgs-review`, since the tool executes the transitive dependencies. | Since human beings are prone to forget the duty of running passthru tests and `nixpkgs-review` has no support for running it, it certainly goes unnoticed. | |
There was a problem hiding this comment.
In general, it is not very clear from reading the 1st column of these 2 lines, what exactly is being compared. I understand how you wanted to make it more concise then in the table I wrote in my previous review, but it is not exactly clear what does "breakage" mean, in which context? In Nixpkgs PRs? Or in a user's NixOS system? Or in Hydra?
I admit that even in my table the last line could have been a bit clearer, (I hope you will forgive me for demanding better clarity when this is something that everyone will read v.s something only you read when I suggest something for your PR).
There was a problem hiding this comment.
but it is not exactly clear what does "breakage" mean
Breakage is any event that triggers the respective tool.
, in which context? In Nixpkgs PRs? Or in a user's NixOS system? Or in Hydra?
It does not matter who is the one that triggers the event.
versionCheckHook is triggered by anyone that tries to build the package containing it, whether it is a nixos-rebuild switch executed by a GitHub Action or a user running commands in a Powershell instance.
The same can be said about passthru.tests.version mutatis mutandis.
There was a problem hiding this comment.
but it is not exactly clear what does "breakage" mean
Breakage is any event that triggers the respective tool.
The respective tool is nixpkgs-review or ofborg? Or the package itself?
, in which context? In Nixpkgs PRs? Or in a user's NixOS system? Or in Hydra?
It does not matter who is the one that triggers the event.
I disagree, because in any of the above contexts there are different chances of a package with versionCheckHook or a package's passthru.tests.version derivation to be built. This distinction is a very important point for comparison, and the first column of these 2 lines don't make it clear to which context you are referring to.
versionCheckHookis triggered by anyone that tries to build the package containing it, whether it is anixos-rebuild switchexecuted by a GitHub Action or a user running commands in a Powershell instance.
Thanks for adding to the list of possible contexts :). To summarize, the possible contexts I see are:
- A user builds locally packages in a modified Nixpkgs checkout.
nixpkgs-reviewin a PR.- Someone running
@ofborg buildin a PR. - A user running
nixos-rebuild. - A GitHub action running
nixos-rebuild, and possibly other checks if configured to do so.
AndersonTorres
force-pushed
the
documentation
branch
5 times, most recently
from
22f35a5 to
5b3ba95
Compare
| doInstallCheck = true; | ||
|
|
||
| # ... | ||
| }) | ||
| ``` | ||
|
|
||
| Note that for [`buildPythonPackage`](#buildpythonpackage-function) and [`buildPythonApplication`](#buildpythonapplication-function), `doInstallCheck` is enabled by default. | ||
| ::: |
There was a problem hiding this comment.
Just a question: What is the purpose of this?
There was a problem hiding this comment.
The triple dot?
It closes the ::: {.example . . .} block.
I don't remember if this is mandatory or merely recommended, but example code is usually enclosed in blocks like these.
| | Item | `versionCheckHook` | `testers.testVersion` via `passthru.tests.version` | | ||
| |-------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | Customization | Besides the attributes described above, `versionCheckHook` provides no other methods for controlling it. | `tests.version` has access to the whole feature set of Nixpkgs, like multiple version test derivations and tools outside the package inputs. | | ||
| | Execution time | `versionCheckHook` is executed automatically whenever the package is executed, e.g. `nix-build -A pkg`, since it is a phase executed during the package build time. | `tests.version` is executed only when explicitly called, e.g. `nix-build -A pkg.tests.version`; it is executed after package build time. | |
There was a problem hiding this comment.
"Executing a package" seems incorrect / misleading to me.
| | Execution time | `versionCheckHook` is executed automatically whenever the package is executed, e.g. `nix-build -A pkg`, since it is a phase executed during the package build time. | `tests.version` is executed only when explicitly called, e.g. `nix-build -A pkg.tests.version`; it is executed after package build time. | | |
| | Execution time | `versionCheckHook` is executed automatically whenever the package is built, e.g. `nix-build -A pkg`, since it is a phase executed during the package build time. | `tests.version` is executed only when explicitly called, e.g. `nix-build -A pkg.tests.version`; it is executed after package build time. | |
OTH in the right column it is correct to say that test is executed, although the test is a package.
| |-------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | Customization | Besides the attributes described above, `versionCheckHook` provides no other methods for controlling it. | `tests.version` has access to the whole feature set of Nixpkgs, like multiple version test derivations and tools outside the package inputs. | | ||
| | Execution time | `versionCheckHook` is executed automatically whenever the package is executed, e.g. `nix-build -A pkg`, since it is a phase executed during the package build time. | `tests.version` is executed only when explicitly called, e.g. `nix-build -A pkg.tests.version`; it is executed after package build time. | | ||
| | Execution environment | `versionCheckHook` uses `env --ignore-environment --chdir=/` to run the executables in an environment as clean as possible, but there is no way to change its behavior. | `tests.version` executes in an environment identical to that of a consumer, independent from the package build environment. | |
There was a problem hiding this comment.
I know I wrote that sentence before myself, so I hope you'd forgive me for these nitpicks:
| | Execution environment | `versionCheckHook` uses `env --ignore-environment --chdir=/` to run the executables in an environment as clean as possible, but there is no way to change its behavior. | `tests.version` executes in an environment identical to that of a consumer, independent from the package build environment. | | |
| | Execution environment | `versionCheckHook` uses `env --ignore-environment --chdir=/` to run the executables in an environment as clean as possible, and there is no way to change this behavior. | `tests.version` executes in an environment identical to that of a consumer, independent from the package's build environment, and can be fully customized without changing the environment of the package's build. | |
| | Customization | Besides the attributes described above, `versionCheckHook` provides no other methods for controlling it. | `tests.version` has access to the whole feature set of Nixpkgs, like multiple version test derivations and tools outside the package inputs. | | ||
| | Execution time | `versionCheckHook` is executed automatically whenever the package is executed, e.g. `nix-build -A pkg`, since it is a phase executed during the package build time. | `tests.version` is executed only when explicitly called, e.g. `nix-build -A pkg.tests.version`; it is executed after package build time. | | ||
| | Execution environment | `versionCheckHook` uses `env --ignore-environment --chdir=/` to run the executables in an environment as clean as possible, but there is no way to change its behavior. | `tests.version` executes in an environment identical to that of a consumer, independent from the package build environment. | | ||
| | Rebuild after modification | Modifying `versionCheckHook` triggers the package rebuild. | `tests.version` does not rebuild the package, since it runs after package build time. | |
There was a problem hiding this comment.
Another accuracy nit:
| | Rebuild after modification | Modifying `versionCheckHook` triggers the package rebuild. | `tests.version` does not rebuild the package, since it runs after package build time. | | |
| | Rebuild after modification of hook/test derivation | Modifying `versionCheckHook` triggers rebuilds for all packages using it. | Doesn't require rebuilding the package, as the test derivation is independent of it. | |
| | Execution time | `versionCheckHook` is executed automatically whenever the package is executed, e.g. `nix-build -A pkg`, since it is a phase executed during the package build time. | `tests.version` is executed only when explicitly called, e.g. `nix-build -A pkg.tests.version`; it is executed after package build time. | | ||
| | Execution environment | `versionCheckHook` uses `env --ignore-environment --chdir=/` to run the executables in an environment as clean as possible, but there is no way to change its behavior. | `tests.version` executes in an environment identical to that of a consumer, independent from the package build environment. | | ||
| | Rebuild after modification | Modifying `versionCheckHook` triggers the package rebuild. | `tests.version` does not rebuild the package, since it runs after package build time. | | ||
| | Overhead during package build time | Negligible. Although executed during the the package build time, `versionCheckHook` is lean and executes few commands. | Zero, since `tests.version` is a derivation that runs after package build time. | |
There was a problem hiding this comment.
Can be more accurate:
| | Overhead during package build time | Negligible. Although executed during the the package build time, `versionCheckHook` is lean and executes few commands. | Zero, since `tests.version` is a derivation that runs after package build time. | | |
| | Overhead during package build time | Negligible - executes the main package's command 1-2 times, during the package's build time. | Zero, since `tests.version` is an independent derivation. | |
It's the 2nd time you write "a derivation that runs after a package build time" or something roughly the same - I hope you are OK with the fact I emphasize that not necessarily people will build (and not run) that derivation, because people might forget to do it by themselves or look at ofborg's attempts (which are sometimes inaccurate).
| | Execution environment | `versionCheckHook` uses `env --ignore-environment --chdir=/` to run the executables in an environment as clean as possible, but there is no way to change its behavior. | `tests.version` executes in an environment identical to that of a consumer, independent from the package build environment. | | ||
| | Rebuild after modification | Modifying `versionCheckHook` triggers the package rebuild. | `tests.version` does not rebuild the package, since it runs after package build time. | | ||
| | Overhead during package build time | Negligible. Although executed during the the package build time, `versionCheckHook` is lean and executes few commands. | Zero, since `tests.version` is a derivation that runs after package build time. | | ||
| | Failure detection time | `versionCheckHook` runs during the package build time, therefore it does not deal with failures happening after that. | `tests.version` detects failures after package build time, like rewritings and relocations. | |
There was a problem hiding this comment.
I don't understand this line of comparison. Do you mean "when will versionCheckHook/testVersion tell us a package is failing? Is this supposed to be the content-addressed derivations related line? What do you mean by failures happening after builds? Again perhaps my lack of CA derivations knowledge is holding me back from understanding.
| | Rebuild after modification | Modifying `versionCheckHook` triggers the package rebuild. | `tests.version` does not rebuild the package, since it runs after package build time. | | ||
| | Overhead during package build time | Negligible. Although executed during the the package build time, `versionCheckHook` is lean and executes few commands. | Zero, since `tests.version` is a derivation that runs after package build time. | | ||
| | Failure detection time | `versionCheckHook` runs during the package build time, therefore it does not deal with failures happening after that. | `tests.version` detects failures after package build time, like rewritings and relocations. | | ||
| | Package breakage awareness | Loud and clear as soon as `versionCheckHook` is reached during the package build time. | Requires specific commands to be noticed, e.g. `nix-build -A pkg.tests.version`. | |
There was a problem hiding this comment.
Small nit now:
| | Package breakage awareness | Loud and clear as soon as `versionCheckHook` is reached during the package build time. | Requires specific commands to be noticed, e.g. `nix-build -A pkg.tests.version`. | | |
| | Package breakage awareness | Loud and clear as soon as `versionCheckHook` is reached during the package build time. | Requires running specific commands to be noticed, e.g. `nix-build -A pkg.tests.version`. | |
| | OfBorg CI | `versionCheckHook` will be executed by OfBorg, since ofBorg automatically executes the packages affected by the pull request (PR). | OfBorg supports automatic execution of `passthru.tests` for _packages directly affected_ by the PR. | | ||
| | OfBorg CI, transitive dependencies | `versionCheckHook` will executed by OfBorg, since it builds transitive dependencies of a package affected by the PR. (????) | OfBorg does not support automatic execution of `passthru.tests` for transitive dependencies, requiring extra commands for this. | |
There was a problem hiding this comment.
Hmm maybe each line should explain better to which ofborg behavior are we referring to. Here's my suggestion:
| | OfBorg CI | `versionCheckHook` will be executed by OfBorg, since ofBorg automatically executes the packages affected by the pull request (PR). | OfBorg supports automatic execution of `passthru.tests` for _packages directly affected_ by the PR. | | |
| | OfBorg CI, transitive dependencies | `versionCheckHook` will executed by OfBorg, since it builds transitive dependencies of a package affected by the PR. (????) | OfBorg does not support automatic execution of `passthru.tests` for transitive dependencies, requiring extra commands for this. | | |
| | OfBorg's [automatic building](https://github.com/NixOS/ofborg?tab=readme-ov-file#automatic-building) treatment | Will be executed as it is part of the package's build phase | Will be executed too as part of the _directly_ changed packages' `passthru.tests` | | |
| | [`@ofborg build`](https://github.com/NixOS/ofborg?tab=readme-ov-file#commands) treatment | Will be executed too as it is part of the package's build phase | Will be executed only if you'd remember to specify the test derivation with e.g `@ofborg build pkg.passthru.tests.version` | |
| | `nixpkgs-review` | `versionCheckHook` will be executed by `nixpkgs-review`. | `nixpkgs-review` has no support for executing `passthru.tests` at all. [^1] | | ||
| | `nixpkgs-review`, transitive dependencies | `versionCheckHook` will be executed by `nixpkgs-review`, since the tool reaches transitive dependencies automatically. | `nixpkgs-review` has no support for executing `passthru.tests` at all, even for transitive dependencies. [^1] | |
There was a problem hiding this comment.
Here it is weird to split these to 2 lines, as nixpkgs-review always acts upon all affected packages.
I am trying to find better words, since
|
I stopped reading this commit by commit long ago TBH :), but I'll take that into consideration next time.
True! I used https://tableconvert.com/ to post the initial table.
I agree. Seems like a bug. Let's ask for help from the @NixOS/documentation-team . |
Any member of @NixOS/documentation-team can help with the CI error we see here? |
|
Hm, could be something related to the custom extensions. I recommend bisecting by removing the footnote and table, and check if it goes away. Maybe it's the footnote in the table. |
AndersonTorres
force-pushed
the
documentation
branch
from
5b3ba95 to
162f40d
Compare
|
@AndersonTorres see also https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/69 - it might be related. |
- Reword the paragraphs - Remove the comparison with `versionCheckHook` - It will be moved to `versionCheckHook.section.md`
- Flip the explanation and buildPythonPackage reference paragraphs - Add a section to emphasize the attributes controlling the hook
AndersonTorres
force-pushed
the
documentation
branch
from
162f40d to
c2d89a1
Compare
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.