NixOS / nix

Nix, the purely functional package manager
https://nixos.org/
GNU Lesser General Public License v2.1
12.09k stars 1.47k forks source link

Track test derivations and parallelize building and testing #7662

Open roberth opened 1 year ago

roberth commented 1 year ago

Is your feature request related to a problem? Please describe.

Tests can be run as part of a package's main derivation (that produces the binaries, etc.), or the tests can be run separately. Currently this presents a trade-off between build latency and reliability.

Describe the solution you'd like

Break the trade-off by tracking test dependencies in the string context.

This has the effect of making all the test derivations fairly independently schedulable tasks without the risk of skipping any.

Describe alternatives considered

Track it in a package ("passthru") attribute instead. This isn't airtight, because dependencies added through string interpolations won't be captured. Example:

''
  ${hello}/bin/hello >$out
''

In the above string, the test dependencies of hello aren't necessary for the string with context to be realized. Without adding them as regular dependencies (and therefore linearizing the build, which we don't want) it's not possible to extract test dependencies from the string. It's not feasible to do this manually with a helper function involving builtins.getContext and unsafeDiscardStringContext, because this helper would have to be used in far too many places. Furthermore, it has two return values, necessitating a let binding at every call site, making the thing far too cumbersome as well.

Additional context Add any other context or screenshots about the feature request here.

Priorities

Add :+1: to issues you find important.

ghost commented 7 months ago

Both the upside and downside that you hypothesize have been confirmed "in the wild".

When the gcc bootstrap was split up into two separate derivations, the checkPhase had to be moved into yet a third derivation.

I'm :+1: on making these kinds of "separate derivation" tests more automatic, but :-1: on string contexts in general. They are an icky hack that we recently discovered isn't even necessary (tvix and parnix don't have them) and can be omitted. I think this would be a bad time to go looking around for reasons to make them unremovable.

I don't really think there needs to be a nixlang/libexpr-level solution. The goal is to express the following:

Anytime you realize the build-derivation, you must also realize the test-derivation concurrently with any referrers of the build-derivation.

The word "concurrently" is what distinguishes this from a simple wrapper, which would cause the test-derivation to be realized in series with the build-derivations' referrers. So this is really a build-scheduling problem, and it seems like it calls for adding a new store derivation field, rather than a new behavior for derivation expressions.

Here's the litmus test for me: I could easily imagine utilizing this feature from some non-libexpr-using, raw-derivation-writing system (e.g. Guix or this example in RFC92). When that's true, I think it's a sign that the feature in question is a libstore thing rather than a libexpr thing.

roberth commented 7 months ago

String context in general

tl;dr agreed, may not be necessary!

but 👎 on string contexts in general. They are an icky hack

I haven't considered it to be an icky hack, as it removes a reliance on a form of global state, but that's interesting, to try and avoid it. I've previously gathered input for extra use cases that could be built on top of string context - mostly:

However, alternative implementations also seem feasible, as follows

Async test derivation relation in derivation

It would be unnecessary to invalidate output hashes based on such a new store derivation field. This is another reason to improve the output hashing algorithm along with

An open question is who's responsible for running which tests. When tests are dependencies or phases, the existence of an output implies that the tests have completed successfully. Hence they are abstracted away. In the new scheduling model with async test dependencies, this is not the case anymore. Running all tests in the entire derivation closure does not seem feasible. Open questions:

Ericson2314 commented 7 months ago

Here is another one:

I am also skeptical about not having a string context keeping the same semantics. I bet I can find some bugs around spoofing store paths. Similar to discussion in-band vs out-of-band capabilities (not sure what terms are used for that).

nixos-discourse commented 1 month ago

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

https://discourse.nixos.org/t/why-does-the-nixos-infrastructure-have-to-be-hosted-in-a-centralized-way/46789/41

nixos-discourse commented 1 month ago

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

https://discourse.nixos.org/t/discourse-via-email/29/13

nixos-discourse commented 1 month ago

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

https://discourse.nixos.org/t/review-of-333575/50496/1

nixos-discourse commented 4 weeks ago

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

https://discourse.nixos.org/t/review-of-333575/50496/3