libexpr-c: expose essential evaluator functions for derivation tree inspection#15842
libexpr-c: expose essential evaluator functions for derivation tree inspection#15842NotAShelf wants to merge 8 commits into
Conversation
|
Uh, for some reason the PR form discarded all of my text... Edit: fixed, but I have no idea why that happened. |
|
Tbh I'm not too sure why this would be needed - i.e. afaict all of this can be implemented on the other side of the bindings in a more flexible manner? There's some argument to be made to keeping to bindings minimal while providing all the necessary facilities - the larger the interface surface - the larger the maintenance burden and less flexibility we have to change the C++ code. Is it particularly problematic / a lot of code to implement via existing bindings? |
roberth
left a comment
There was a problem hiding this comment.
I'm doing some work on generalizing our evaluator interfaces, and interestingly, these functions...
nix::getDerivation,nix::findAlongAttrPath,
EvalState::autoCallFunction,
I wouldn't classify them as part of the evaluator. They're helpers for users of the evaluator that are not called by the evaluator itself.
Now that's currently "just semantics", but not in the context of my refactor. I guess this does reinforce that the C API should connect to the new evaluator interface, not just nix::EvalState.
Anyway, don't worry about it, I'll get back with an actual review.
|
Thank you both for the comments. I might have overstepped a little bit. The additions were a direct mapping of what nix-eval-jobs actually calls today through internal headers, which I guess was the wrong instinct for a stable API. Asking myself the correct question ("which of those calls cannot be reproduced from the other side.") and testing a bit further with my patch applied, I'm dropping the redundant helpers. @roberth re: the evaluator interface refactor - Good to know, and I agree these sit in the "user of the evaluator" layer rather than the evaluator proper. Whichever interface they land behind after the refactor, the C bindings just need to delegate to it; the shape of the public function shouldn't change. This is, of course, under your jurisdiction. I'll make the changes as necessary. |
…ion` These encapsulate internal evaluator machinery that cannot be replicated through the existing attrset/value access API, which makes them the bare minimum necessary surface for consuemers wanting to to traverse and inspect derivation trees. Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I1f6aa9222083068300de22e3f6aad3ac6a6a6964
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/gradient-call-for-testers/77549/10 |
|
Is there anything else to do on my side? I'd like to get this in before there are more merge conflicts if possible, and provide the necessary APIs for downstream consumers :') |
|
Is there any reason this can't be merged? I would really like to use this improvement. |
|
How is this suppose to work? @xokdvium the work has been done, it's ready to be merged / reviewed once more. I can see that there are a lot other PRs open, but I would really appreciate if we could get this done sooner or later. |
xokdvium
left a comment
There was a problem hiding this comment.
Ideally we'd also have accompanying unit tests for every single function (and hopefully all code paths/error handling).
Also, we shouldn't really refer to the C++ API and document the semantic of this function separate from the C++ counterpart, since the latter is more of an internal thing.
| * @brief Auto-call a function with auto-args (the --arg / --argstr pattern). | ||
| * | ||
| * This corresponds to nix::EvalState::autoCallFunction. |
There was a problem hiding this comment.
This doesn't quite describe what the function does, the C++ implementation details can change while this functionality is supposed to be self sufficient.
Motivation
I maintain a set of Rust bindings for Nix that interact with the evaluator
through FFI. While looking to create a
nix-eval-jobsreplacement I've noticedthat it depends heavily on C++ APIs that are not a part of stable C bindings.
This effectively blocks efforts to build external tooling (such as but not
limited to a Rust-based nix-eval-jobs replacement) that interacts with the
evaluator through FFI. Furthermore, other external tools also call internal C++
APIs like
nix::getDerivation,nix::findAlongAttrPath,EvalState::autoCallFunction, andnix::printValueAsJSONbecause the stable Cbindings lack equivalents. This forces consumers to link against unstable
headers that shift between Nix versions, which I find to be making maintenance
brittle. In the light of recent work on the C API, expanding the C API surface
to cover these four operations allows downstream tooling to migrate to stable
bindings and, in turn, enables pure-FFI consumers without any new internal
linkage. This is not all, of course, but I think it's a good start.
Context
nix-eval-jobs (
src/worker.cc,src/drv.cc) is the canonical consumer. Itevaluates Nix attribute sets in parallel, inspecting each value to determine
whether it is a derivation (recurse vs. emit JSON) and serializing the result.
Conversely, every other piece of derivation metadata nix-eval-jobs extracts
(name, system, output names, meta) is a plain attribute lookup on the forced
attrset, already covered by
nix_get_attr_byname,nix_get_string,nix_get_attrs_size, andnix_get_attr_name_byidx.This is a non-trivial change (obviously, new public API surface) but not
invasive as no existing behavior is altered. 250~ lines added across one new .cc
file and one small addition to the public header. My implementation strategy is
relevatively simple: four new functions now live in a new
nix_api_eval.cctranslation unit that was also added to meson.build. Each follows the
established C API conventions; error reporting through
nix_c_contextwithlast_err_code,EvalState*wrapping the internalnix::EvalState&, andNIXC_CATCH_ERRS/NIXC_CATCH_ERRS_NULLmacros for exception-to-error-codetranslation.
Initially I've wrapped
nix::PackageInfoin an opaquenix_package_infohandleand exposed typed query functions (queryName, querySystem, queryOutputs,
queryMeta, etc.). I've since dropped this approach because every one of those
accessors reduces to an attrset key lookup on the forced derivation value. Those
are operations the existing C API (
nix_get_attr_byname,nix_get_string,nix_get_attrs_size) already supports. PullingPackageInfointo the stableABI would also couple the C API to a C++ type whose signature can drift between
Nix versions, which I think would be defeating the purpose of the stable
bindings. The latest design I've settled on
keepsnix_get_derivationreturning a plainStorePath*`, and leaves derivationintrospection to the caller via the attrset API.
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.