Repository navigation
libexpr: let the embedder realise an import from derivation - #16683
DerDennisOP wants to merge 4 commits into
Conversation
github-actions
Bot
added
documentation
c api
cac60da to
ec50b51
Compare
github-actions
Bot
added
the
store
| its build attempt once per process rather than once per importer. */ | ||
| for (auto & d : drvs) | ||
| if (auto failed = getConcurrent(*failedRealisations, d.drvPath->getBaseStorePath())) | ||
| std::rethrow_exception(*failed); |
There was a problem hiding this comment.
This is very wrong. Exceptions pointers are refcounted and we mutate them. That's why we have throwClone CRTP trick for rethrowing exceptions.
I guess I really should add that clang-tidy rule banning std::rethrow_exception and having a helper which handles that for you...
There was a problem hiding this comment.
It's usually good to have some codebase rules for things like that. I'll fix the Exeception.
| /** | ||
| * Realises the derivations of an import from derivation instead of | ||
| * `buildPaths`. On return, the outputs and their closure must be valid in | ||
| * the evaluation store. Throws `Error` on failure. | ||
| */ | ||
| using RealiseHook = std::function<void(const std::vector<DerivedPath::Built> &)>; | ||
|
|
||
| RealiseHook realiseHook; |
There was a problem hiding this comment.
I'm not a fan of having hooks that inject into the regular code flow. At the very least the regular mechanism should be expressable with such hooks. But also I think this ties too deeply to the current architecture of the evaluator which blocks on IFD.
There was a problem hiding this comment.
Hmm, maybe you have a better idea how to implement this? Generally speaking I think this is feature really important for CIs out there.
There was a problem hiding this comment.
I'm not sure what the best shape would be.
I suppose for the implementation that @edolstra cooked up with coroutines this API should be completion-callback driven instead of being blocking-shaped. But our underlying Store interface is currently blocking so not much to be done about it.
So I'd suggest for now to have the C API receive a completion callback that it invokes when the build completes. That will make us a bit more future proof for non-blocking IFD.
But also at the C++ API level it might make sense to instead have an eval-specific Store class that would instead just intercept the build request. I've had some luck doing store adapters in https://github.com/xokdvium/nix/tree/writeback-store (that didn't yet see the light of day, but I want to get back to that).
73a7749 to
a0b6276
Compare
A cached string's context paths each required one `isValidPath` call from `getStringWithContext()`. Each path cost a daemon round-trip. The function switched to `addTempRoot` for each path. Those calls deduplicate per connection in `RemoteStore`. A single `queryValidPaths` call can check all paths.
a7c7774 to
bd17d9a
Compare
Attributes that import the same failing derivation ask the store to build that derivation again. Many attributes import the same broken derivation. These importers cause one attempt each. The output lock of the store serialized concurrent evaluator attempts. Build with `buildPathsWithResults`. Save a copy of the build error for each derivation that failed alone. Reasons include failure of the builder, rejection of inputs or outputs, hash mismatch and log limit. Same for a timeout. A second attempt can wait for the full timeout again. Rethrow that copy rather than rebuild. Rebuild transient failures such as network errors from a substituter, dependency failure and loss of the daemon connection. The `:reload` command in the REPL can call `resetFileCache()`. Stored failures disappear after `resetFileCache()`.
An embedder with a build system of its own, such as a CI, has no say in how an import from derivation is built: realiseContext() always builds through the store. EvalSettings::realiseHook replaces that build when set, and nix_eval_state_builder_set_realise_hook() installs a C callback as the hook. The embedder only has to make the outputs valid in the evaluation store before it returns. With a hook set, the failed-realisation memo stays off: the embedder decides how long a failure holds, since its evaluator can outlive one evaluation.
bd17d9a to
4b9d047
Compare
|
Agreed. A setting that can bypass the normal path is inappropriate. I propose to remove |
Motivation
I maintain a small CI system with a build scheduler of its own. I nned to control how an import from derivation is built:
realiseContext()always builds through the store.Context
EvalSettings::realiseHookreplaces that build when set andnix_eval_state_builder_set_realise_hook()installs a C callback as the hook. The embedder only has to make the outputs valid in the evaluation store before returning. Unit test and release note included.Stacked on the failed-realisation memo PR (#16682).
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.