doc/passthru: improve sanctioned use of installCheckPhase #319087
Conversation
github-actions
bot
added
the
8.has: documentation
ofborg
bot
added
10.rebuild-darwin: 0
doc/stdenv/passthru.chapter.md
Outdated
| - Access the package as consumers would, independently from the environment in which it was built | ||
| - Access the package as consumers would, e.g. the environment setup up the way `mkDerivation` or `environment.systemPackages` would. |
There was a problem hiding this comment.
Not to defend the existing text (I see an argument it's leaving too much to the imagination), but the rephrased clause this adds feels incomplete and I'm not sure what it's trying to clarify.
In case it helps, I'm just going to unpack the distinctions I read the existing text as trying to make:
Since they are separate derivations, passthru.tests depend on the package under test just like any other consumer of the package would. This separation ensures these tests depend on actual derivation outputs and are not affected by temporary state in the build sandbox.
There was a problem hiding this comment.
Added a explanation to the example
| @@ -94,7 +94,7 @@ stdenv.mkDerivation (finalAttrs: { | |||
| installCheckPhase = '' | |||
| runHook preInstallCheck | |||
| echo checking if 'git --version' mentions ${finalAttrs.version} | |||
| $out/bin/git --version | grep ${finalAttrs.version} | |||
| env --ignore-environment $out/bin/git --version | grep --silent "${finalAttrs.version}" | |||
There was a problem hiding this comment.
Is this change solving for a specific failure mode in the example, or is it just demonstrating one way to guard against state inside the build environment in order to make a general point about the fact that tests run during the build may be sensitive to temporary state that won't exist after the build completes?
If it's the former, I think it might be better to just lay the cards down here and show the problem we'd have without this workaround (because it will provide a better illustration of the underlying risk of these tests depending on the state inside the sandbox) and also show the fix?
If this just demonstrates how to guard against state without an acute need, I think it's still worth explaining why a "clean" environment is important (and perhaps noting that this only guards against environment variables which are only one kind of state present in the sandbox).
There was a problem hiding this comment.
This is specifically to avoid variables like PYTHONPATH, to guard against build artifacts one could pushd "$(mktemp -d)"
doc/stdenv/passthru.chapter.md
Outdated
| @@ -104,9 +104,9 @@ stdenv.mkDerivation (finalAttrs: { | |||
|
|
|||
| However, tests that are non-trivial will better fit into `passthru.tests` because they: | |||
|
|
|||
| - Access the package as consumers would, independently from the environment in which it was built | |||
| - Access the package as consumers would, e.g. the environment setup up the way `mkDerivation` or `environment.systemPackages` would. | |||
There was a problem hiding this comment.
I'm not sure mkDerivation makes sense to me here
pbsds
force-pushed
the
fix-meta-docs-1718119182
branch
2 times, most recently
from
ed110f5 to
64f1a22
Compare
pbsds
force-pushed
the
fix-meta-docs-1718119182
branch
from
64f1a22 to
cf39d24
Compare
| ::: | ||
|
|
||
| However, tests that are non-trivial will better fit into `passthru.tests` because they: | ||
|
|
||
| - Access the package as consumers would, independently from the environment in which it was built | ||
| - Access the package outputs as any consumer would, with an environment as setup up with either `stdenv` or `environment.systemPackages`, and without temporary build artifacts and state produced in `configurePhase` and `buildPhase`. |
There was a problem hiding this comment.
| - Access the package outputs as any consumer would, with an environment as setup up with either `stdenv` or `environment.systemPackages`, and without temporary build artifacts and state produced in `configurePhase` and `buildPhase`. | |
| - Access the package outputs as any consumer would, with an environment set up with either `stdenv` or `environment.systemPackages`, and without temporary build artifacts and state produced in `configurePhase` and `buildPhase`. |
Just fixes. Generally like this better, though I'm still personally a little unsure about the "with an environment set up with either stdenv or environment.systemPackages" bit:
- Is it clarifying something specific beyond what "as any consumer would" already implies?
- I'm not sure I grok how the
environment.systemPackagesbit fits--is there any testing context in which a passthru test would run with all of the user's systemPackages in scope? (That seems undesirable and a sure source of flakiness, unless maybe it's gesturing at grafting in NixOS VM tests? If the latter, it's worth clarifying.) - It reads like a complete list, but I imagine it's leaving out cases?
There was a problem hiding this comment.
The reason I mention those two means of consuming a package is because of the various hooks that run through the packages and ensure their propagated inputs are also added and set up in various ways. It should read as non-exhausive. environment.systemPackages is a means of adding a package to your environment.
how about
| - Access the package outputs as any consumer would, with an environment as setup up with either `stdenv` or `environment.systemPackages`, and without temporary build artifacts and state produced in `configurePhase` and `buildPhase`. | |
| - Access the package outputs as any consumer would, for example like the environments set up when using `nix-shell`, `stdenv`, or `environment.systemPackages`, and without the temporary build artifacts and state produced in `configurePhase` and `buildPhase`. |
|
Maybe we should create a |
|
closing in favor of #320266 |
Description of changes
I only found #315616 after it was merged. Here are my two cents.
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.