-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(Towards) stable C bindings for libutil, libexpr #8699
Conversation
You're my hero. Please reach out if you'd like any help with similar work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A first review. I haven't managed to review the entire diff yet, but I think it's good to get the conversation going sooner.
As for the PR as a whole, we'll need
- User documentation, notably
- How to set up the libraries
- Other cross cutting concerns such as error handling and GC
- You've written about this in the PR description, but it needs a permanent place
- Usage examples
- A separate doxygen configuration that only does the C headers. Ideally we integrate this in the manual. Maybe just copy it into a subdirectory during the build and link to it?
- I did find this https://github.com/lowRISC/opentitan/blob/master/util/mdbook_doxygen.py . I wonder how that works. I haven't managed to find an example of how it looks. Maybe it works too well, or it is broken in their docs. I don't know.
- Architectural documentation. How do we go about writing, extending and maintaining these bindings? I have to admit that I only have a mediocre and theoretical understanding of API, let alone ABI compatibility with C; no practical experience. I think that's most of us on the Nix team, and many contributors as well.
- Tests
Furthermore I think we shouldn't call this stable until we have validated that it's possible to build good language bindings with these functions.
src/libexpr/nix_api_expr.h
Outdated
* | ||
* Owned by the GC. | ||
*/ | ||
typedef void Value; // nix::Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not typedef struct Value Value;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO calling this a Value
was a mistake, because a thunk is not a value. I would associate "value" with something in normal form, or at least weak head normal form; the thing returned by forceValue
, which is never a thunk.
I would prefer for values and thunks to be Object
s.
typedef void Value; // nix::Value | |
typedef struct Object Object; // nix::Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "term"?
src/libexpr/nix_api_expr.h
Outdated
Value *arg, Value *value); | ||
|
||
/** | ||
* @brief Forces the evaluation of a Nix value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OT: I think we should define weak head normal form for Nix, because this doesn't really mean anything; something to blame on the Nix authors before you.
src/libexpr/nix_api_expr.h
Outdated
* @param[in] obj The object to create a reference for. | ||
* @return A new garbage collector reference or NULL on failure. | ||
*/ | ||
GCRef *nix_gc_ref(nix_c_context *context, void *obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit precarious as to how it can and should be used. I would expect the C API to do the allocation for its callers, with specific functions to allocate each individual type (mostly just for Value
afaict, certainly for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't really have Values owned on the nix API side, since they can be referenced by other Values. You can also retrieve Values that have been allocated by nix, e.g. from attrsets. That's why we can't have a value_free() call.
However, to simplify the API, I've replaced GCRefs by refcounting. Nix API functions that return Value's call nix_gc_incref, and API consumers are expected to call nix_gc_decref when they're done.
src/libexpr/nix_api_expr_internal.h
Outdated
struct GCRef { | ||
void *ptr; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems overly general. Are we using this only to hold Value
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced this by refcounting. It also holds PrimOps and ExternalValues.
src/libexpr/nix_api_external.cc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs tests.
src/libexpr/nix_api_external.h
Outdated
* | ||
* @see nix_create_external_value | ||
*/ | ||
typedef struct NixCExternalValueDesc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has a lot of exposed detail that I don't think we can keep ABI compatible. For instance the struct size will become a problem. Or should we fork and duplicate the entire struct when we need to add or change something? Not sure what's best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mainly going for API compatibility, meaning you'd have to recompile (which should work well for nix-based systems anyways). However, for ABI compat, we could pad this struct with a bunch of zeroes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally just prefix the struct with a version field that has to be filled in with a #define
d version number; the C++ side can have a copy of the old structs and do the necessary internal adjustments.
Reviewed and discussed in Nix team meeting (maintainers will add their own detailed reviews on top):
Complete discussion
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-07-17-nix-team-meeting-minutes-72/30574/1 |
To clarify, we do want to keep the ability to "have" a thunk, as long as you don't try to observe the value; in other words keep it lazy, but force it in |
@Ericson2314 https://github.com/Mic92/pythonix used them, but iirc it stopped being maintained because Nix frequently changed the C++ bindings, requiring changes in pythonix too. See this commit from my initial Python bindings PR, that updates pythonix with newer C++ API changes. The original idea was to just upstream pythonix so that downstream wouldn't be burdened with this. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nixpkgs-cli-working-group/30517/14 |
I use the C++ bindings all the time. They work great. Plugins are incredibly useful. |
As the author of nix-doc, I have also experienced API breakage on my relatively minimal (a builtin that takes strings and lambdas) C++ Nix plugin on about 5 releases in 2 years. Anyone doing something more complex definitely would be hitting these both more severely and more frequently. Nix's C++ API is definitely not currently very stable. One benefit of a C API that's not used as much internally in Nix is that it is more likely to be decoupled from the faster-changing Nix internals, and I'm in favour of this on this basis alone. |
I'm unconvinced this actually makes the API much simpler; you still have to handle the same (or more!) amount of errors (what if In my uses, |
Just a drive by comment to mention that C++ API can be made stable using patterns like pimpl. Not to say a stable C API would not be appreciated, quite the opposite! 💜 |
I don't think this is a good idea personally. The C++ API being unstable has enabled lots of the recent error messages work and other refactors and it feels like a mistake to try to stabilize the core APIs as they are rather than provide some second intentionally-stable wrapper APIs for other clients. (meta-thought: I don't know how many people writing Nix extensions actually want to be writing C++ if it's avoidable. I don't, and the two extensions I maintain are basically C ABI shims to call Rust where all the actual work is done; I would be happy to stop maintaining this-PR-but-worse) |
I didn't mean the internal API has to be stable (you are right, how could the code base evolve then!?). Instead I meant that one can offer a new external stable C++ API, which wraps the internal code just like the C wrapper would do. In other words: "C vs C++" and "stable vs unstable" are orthogonal issues. |
Title updated to reflect that we won't promise stability immediately after merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small nitpicks rewriting my bindings against this api.
Also, there's no way yet to read/write string context other than calling builtins.getContext
; and the external value's string context format is unspecified (presumably Nix-internal, but..)
It might also be useful to be able to extract expected buffer size for the multiple NIX_ERR_OVERFLOW
cases?
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nix-unit-a-nix-unit-testing-runner-compatible-with-runtests/30765/2 |
This comment was marked as off-topic.
This comment was marked as off-topic.
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-07-27-documentation-team-meeting-notes-67/30998/1 |
Thank you all for the feedback! I've made several updates:
For a motivating example for this API, check out the WIP python bindings: https://github.com/tweag/nix/tree/nix-c-bindings-python/python Changing everything to be forcing could be possible, but would require passing State* pointers around, which I'm not entirely happy with. |
Apologies if I forgot to comment on this before, but I feel the C code ought to be in separate libraries. We don't want other parts of Nix using this stuff --- C++ should only use the C++ and not C wrapper --- and separate libraries enforces that. |
@roberth @jlesquembre : it looks like this broke the cross-compilation to armv7: https://github.com/NixOS/nix/actions/runs/8557577273/job/23451299607 . Any of you care to look at why? (Yeah I know, fun thing to debug) |
We've gotten something like this thing from makefile include order issues before (when the doc stuff wasn't defined after the nix exectable, the dependency got broken). |
|
#10408 fixes the cross build. |
This is causing a Make warning:
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-56/43035/1 |
See NixOS#8699 (comment) Casting a function pointer to `void*` is undefined behavior in the C spec, since there are platforms with different sizes for these two kinds of pointers. A safe alternative might be `void (*callback)()`
This PR mentions that the python bindings should move to nix-community after merging, and given that it has already been merged, what's the current status of it? Apparently the C bindings are not up to date to the current C API and I'm interested in knowing if that is happening or if the plans changed; maybe forking is the way to go? |
This was a mistake. It's not simpler; only slightly easier at the start, but will bite users later. See This should be changed before we call the API stable. |
I have mostly stopped linking to this PR.
|
I see a growing interest in bindings. I've been working on Rust bindings here. They'll be moved to their own repo when they mature. If anyone else is working on bindings, or knows of a project, please share, so we can compile a list together. |
https://github.com/farcaller/gonix is the go bindings, currently broken from the changes that were made lately in this PR, but I started looking into fixing it yesterday. |
I am using the API in a Nim project but I'm not publishing bindings seperately - https://git.syndicate-lang.org/ehmry/nix_actor |
I've made some (kinda incomplete) Zig bindings: https://github.com/water-sucks/zignix that I've somewhat recently separated out from one of my projects and use in it. |
* @struct Value | ||
* @see value_manip | ||
*/ | ||
typedef void Value; // nix::Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a reason to use void *
instead of opaque pointers like Value *
(after struct Value;
)?
I think we should probably just change it: #10561
I'm also working on a Rust binding, with the goal to support clean interop between rust values and nix values, much like pyo3 does with rust and python. I didn't know if that was the goal of @roberth 's lib so I decided to hack away my own. |
Co-authored-by: Yorick van Pelt <[email protected]> Co-authored-by: José Luis Lafuente <[email protected]> Change-Id: I48aa262d880aa75aa5b230553f663c35c283d5f6
This PR introduces a stable C API that can be used to interface with the Nix interpreter externally.
Motivation
In the past, we've wanted to interface Nix with other languages.
This had some problems:
std::string
from other languagesA Nix plugin ecosystem was proposed, but never got off the ground for these reasons.
Perl bindings are maintained upstream, and this was the intention for python bindings, as well.
To resolve these difficulties, a stable C API would be advantageous.
Related: #7271
State
These bindings have been used to develop python bindings, which we intend to move to nix-community after this PR is finished.
Context
This is the second approach to making python bindings, this time we're focusing on creating a stable Nix API, and binding to it via CFFI.
This also make sure we don't have to duplicate the effort to interface with Nix for every language.
Implementation strategy
GC
Objects that are managed by the GC are ref-counted in this API. API consumers should call nix_gc_decref() when they are done with a Value. This makes sure the GC does not have to scan the API consumers heap.
Alternatives
Error handling
To handle C++ exceptions from a C API, a structure called nix_context has been created, which can be allocated by the API consumer and passed to functions. Error state (exception info, strings, error type) can be stored in this context. Many functions also return a
nix_err
code orNULL
on errors.Alternatives considered:
More minimal C API
Stabilizing the C++ API
Creating a new, stable C++ API
Keeping the status quo
How to read the diff
Checklist for me
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*
Priorities
Add 👍 to pull requests you find important.
Affiliation
This PR was sponsored by Antithesis