Skip to content
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

Merged
merged 70 commits into from
Apr 4, 2024

Conversation

yorickvP
Copy link
Contributor

@yorickvP yorickvP commented Jul 14, 2023

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:

  • The C++ ABI is not compatible with many other languages
  • The GC makes it hard to deal with references from other languages
  • It was hard to manipulate C++ stdlib types such as std::string from other languages
  • Ownership was handled by C++ RAII, which is hard to be compatible with
  • The Nix C++ API exposed all of the Nix internals, causing a quickly-moving target that was hard to stay up-to-date with.

A 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
  • Wrap pointers to these objects
    • (+) Hard to use incorrectly
    • (+) Easy to understand
    • (-) Header to interoperate: would not be compatible, for example, the current C++ API.
    • (-) Overhead: any use of these would require an additional dereference
    • (-) Administration cost: creating multiple references on the user side would necessitate some bookkeeping.
  • GC references that you can keep separately
    • (+) Minimal overhead
    • (+) Usage optional if Boehm can see into your heap
    • (-) Exposes implementation details (type of garbage collector)
    • (-) Weird separation between gc-ref and values
    • (-) Hard to understand and use correctly

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 or NULL on errors.

Alternatives considered:

  • More minimal C API

    • (+) Easier to maintain
    • (-) Lessens the benefit from this PR, users will still have to maintain their own wrappers.
    • (-) Would make it harder to do this:
      pkgs["hello"]["overrideAttrs"](lambda o: {
        "pname": str(o["pname"]) + "-test"
      })
  • Stabilizing the C++ API

    • (+) We already have the C++ API
    • (-) The current API surface is unnecessarily big, it would be hard to document and sift through it all.
    • (-) This would make it harder to make changes to the codebase
    • (-) C++ is harder to interoperate from most other languages
  • Creating a new, stable C++ API

    • (+) Doesn't use a language from the 70's
    • (-) As much work as C bindings
    • Something like pimpl could help with long-term compatibility
    • (-) Easier to accidentally expose implementation details such as dataclasses
    • (-) C++ is harder to interoperate from most other languages
  • Keeping the status quo

    • (-) No stability guarantees are made, causing plugin authors and nix lib users to frequently have to update their code, and many projects to stop being maintained or resort to wrapping the CLI.
    • (-) Documentation isn't great, you have to read the source to understand what's going on.

How to read the diff

Checklist for me

  • Low-level documentation
  • High-level documentation
  • Tests
  • libstore bindings

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

Affiliation

This PR was sponsored by Antithesis

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jul 14, 2023
@aakropotkin
Copy link
Contributor

You're my hero.

Please reach out if you'd like any help with similar work. libstore is on my wish list obviously.

Copy link
Member

@roberth roberth left a 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?
  • 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.

*
* Owned by the GC.
*/
typedef void Value; // nix::Value
Copy link
Member

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;?

Copy link
Member

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 Objects.

Suggested change
typedef void Value; // nix::Value
typedef struct Object Object; // nix::Value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "term"?

Value *arg, Value *value);

/**
* @brief Forces the evaluation of a Nix value.
Copy link
Member

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.

* @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);
Copy link
Member

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).

Copy link
Contributor Author

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.

Comment on lines 13 to 15
struct GCRef {
void *ptr;
};
Copy link
Member

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?

Copy link
Contributor Author

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_expr.h Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests.

*
* @see nix_create_external_value
*/
typedef struct NixCExternalValueDesc {
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 #defined version number; the C++ side can have a copy of the old structs and do the necessary internal adjustments.

src/libexpr/nix_api_external.h Outdated Show resolved Hide resolved
src/libexpr/nix_api_external.h Outdated Show resolved Hide resolved
src/libstore/nix_api_store.h Outdated Show resolved Hide resolved
@fricklerhandwerk
Copy link
Contributor

Reviewed and discussed in Nix team meeting (maintainers will add their own detailed reviews on top):

  • we're open to merge this as an unstable interface, documented such that application developers are encouraged to try it out
    • it could become stable if it sees a lot of use
  • it would be good to have the PR for Python bindings so we can look at it
    • it should be in a separate derivation (as with the Perl bindings) so we don't grow the dependency closure
  • remove thunks and GC (merge GCRef into Value) to make it simpler to use, demote externals (to discuss with @roberth what exactly that would entail)
  • adapt the coding style to the rest of the project
    • use the LLVM formatter
Complete discussion
  • @edolstra: very unenthusiastic about this
    • don't want to maintain a sizeable C library, it's a 1970s language
    • would not be a problem if it were maintained by someone else outside of the repo
    • @thufschmitt: there is no actual logic in C, only wrapper types
    • any change to the C++ API will need work on the C API
      • @thufschmitt: hopefully not, as the C API is supposed to be stable
    • @roberth: working with @yorickvP to better isolate the internals
    • if we were to have a stable API it would be C++
      • @fricklerhandwerk: the rationale is that other languages will have FFI infrastructure
      • @thufschmitt: could provide a C++ interface and add C in a library, but that would make the pipeline more complicated
  • @fricklerhandwerk: can we agree that we want a stable API?
  • @edolstra: practical example of limited maintainability: lazy tree branch changes the path type from string to a tuple.
    • @roberth: could add the path to the store
      • @thufschmitt: this would keep the original performance characteristics
  • @edolstra: does this allow you to add primops?
    • if you can, it would make it extremely tied to interpreter internals
    • @thufschmitt: this is not exposed anywhere in stable interface
    • thinks like nix_set_thunk
  • @fricklerhandwerk: some background:
    • @infinisil is writing Nixpkgs libraries we want to run lots of test cases on, such as for property tests
    • that requires reducing the overhead for evaluation, and originally @infinisil picked up @Mic92's Python bindings
    • those were broadly agreed upon, but deemed too invasive for now, so @yorickvP picked up on a C interface
    • @roberth: another use case is nixos-option which was updated recently
  • @edolstra: this de facto requires the C++ API to be stable, as this is so closely tied to it
    • @roberth: we can stabilise it in stages, as C APIs tend to do
    • @Ericson2314: we could start out of tree to demonstrate that APIs are not moving as fast as feared
    • @thufschmitt: if we want a stable C++ interface, we should design it such that it's a subset of what we use internally, and even then use that internally as much as possible
  • @edolstra: we could have an interface that takes a string and evaluates to an opaque value that can be queried to be a string or int
  • (initial review)
    • thunks should not be part of that interface
    • GCref exposes internals, probably should be integrated into Value
    • we should probably make it a bit simpler and safer overall, and possibly have an additional less-stable layer that allows for more fidelity
    • external should not be part of it, as that would expose a highly unstable interface even more
    • constructing and introspecting arbitrary Nix values is more involved than what we would want for a first version
    • not sure if we want explicit forcing, which relates to the question if we should expose thunks
      • you'd get strictness issues
      • as a consumer of the API you may not ever want to deal with thunks
      • we may want to have two interfaces, one which never returns thunks, it would be a lot easier to work with
    • having the API abstract enough would also allow wrapping it around the eval cache
    • would be good to see the Python bindings first, to clarify what's needed for C and why
      • alternatively, C bindings could live together with Python and be factored out when they are needed
  • @Ericson2314: general observations:
    • we haven't done a lot of the foundational work for these stable interface to be easy to do
    • we want as much as possible out of tree, so Nix can be used more as a library
    • there are different kinds of costs and different strategies to deal with them
    • we don't even know why people are not using C++ bindings
      • no stability guarantees?
      • no documentation?
      • scared of C++?
      • easier to use CLI?
    • we have to move forward somehow, and it seems we're opting for validation rather than testing in this case

@nixos-discourse
Copy link

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

@roberth
Copy link
Member

roberth commented Jul 17, 2023

  • remove thunks

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 nix_get_type. NIX_TYPE_THUNK can then be removed.
The goal is to make the interface simpler safer, while preserving laziness.

@infinisil
Copy link
Member

infinisil commented Jul 17, 2023

we don't even know why people are not using C++ bindings

@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.

@nixos-discourse
Copy link

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

@aakropotkin
Copy link
Contributor

I use the C++ bindings all the time. They work great. Plugins are incredibly useful.

@lf-
Copy link
Member

lf- commented Jul 17, 2023

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.

@puckipedia
Copy link
Contributor

To clarify, we do want to keep the ability to "have" a thunk, as long as you don't try to observe the value [..] The goal is to make the interface simpler safer, while preserving laziness.

I'm unconvinced this actually makes the API much simpler; you still have to handle the same (or more!) amount of errors (what if nix_show_type tries to force a thunk that errors?), while losing an important introspection axis (translating thunks to a Promise object, which is how my use of the CFFI does it already).

In my uses, GCRef is also important for non-Value objects, like primops and external values; as they are how my bindings make native-code lambdas that can be called from Nix code and safely GC'd.

@andreabedini
Copy link
Contributor

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! 💜

@lf-
Copy link
Member

lf- commented Jul 18, 2023

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! purple_heart

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)

@andreabedini
Copy link
Contributor

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

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.

@roberth roberth changed the title Stable C bindings for libutil, libexpr (Towards) stable C bindings for libutil, libexpr Jul 18, 2023
@roberth
Copy link
Member

roberth commented Jul 18, 2023

Title updated to reflect that we won't promise stability immediately after merge.

Copy link
Contributor

@puckipedia puckipedia left a 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?

src/libexpr/nix_api_external.h Outdated Show resolved Hide resolved
src/libexpr/nix_api_value.h Outdated Show resolved Hide resolved
src/libexpr/nix_api_value.h Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

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

@DieracDelta

This comment was marked as off-topic.

@nixos-discourse
Copy link

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

@yorickvP
Copy link
Contributor Author

yorickvP commented Jul 31, 2023

Thank you all for the feedback!

I've made several updates:

  • Considered several alternatives in the PR description
  • Incorporated some review comments: Exprs are now gone
  • Improved documentation a bit: https://pub.yori.cc/nix-c-api-docs/
  • Moved from the separate GC references to an internal reference counter (using std::unordered_map on object pointers).

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.

@Ericson2314
Copy link
Member

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 roberth self-assigned this Aug 19, 2023
@roberth roberth added the c api Nix as a C library with a stable interface label Apr 4, 2024
@thufschmitt
Copy link
Member

@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)

@Ericson2314
Copy link
Member

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).

@yorickvP
Copy link
Contributor Author

yorickvP commented Apr 5, 2024

local.mk:5: warning: overriding recipe for target '/nix/store/8dbs2d7g0lb8wv83322lb6g5z1859745-nix-armv6l-unknown-linux-gnueabihf-2.22.0pre20240404_12ec315-dev/include/nix/nix_api_expr.h' looks suspicious

@roberth
Copy link
Member

roberth commented Apr 5, 2024

#10408 fixes the cross build.

@edolstra
Copy link
Member

edolstra commented Apr 5, 2024

This is causing a Make warning:

local.mk:5: warning: overriding recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_expr.h'
local.mk:5: warning: ignoring old recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_expr.h'
local.mk:5: warning: overriding recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_external.h'
local.mk:5: warning: ignoring old recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_external.h'
local.mk:5: warning: overriding recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_value.h'
local.mk:5: warning: ignoring old recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_value.h'
local.mk:5: warning: overriding recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_store.h'
local.mk:5: warning: ignoring old recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_store.h'
local.mk:5: warning: overriding recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_util.h'
local.mk:5: warning: ignoring old recipe for target '/home/eelco/Dev/nix-master/outputs/dev/include/nix/nix_api_util.h'

@nixos-discourse
Copy link

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

jlesquembre added a commit to tweag/nix that referenced this pull request Apr 12, 2024
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)()`
@o-santi
Copy link

o-santi commented Apr 12, 2024

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?

@roberth
Copy link
Member

roberth commented Apr 14, 2024

  • remove thunks

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.

@roberth
Copy link
Member

roberth commented Apr 21, 2024

I have mostly stopped linking to this PR.
To find recent related work, look at

@roberth
Copy link
Member

roberth commented Apr 23, 2024

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.

@farcaller
Copy link

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.

@ehmry
Copy link
Contributor

ehmry commented Apr 24, 2024

I am using the API in a Nim project but I'm not publishing bindings seperately - https://git.syndicate-lang.org/ehmry/nix_actor

@water-sucks
Copy link
Member

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.

@prednaz prednaz mentioned this pull request May 2, 2024
* @struct Value
* @see value_manip
*/
typedef void Value; // nix::Value
Copy link
Member

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

@o-santi
Copy link

o-santi commented May 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c api Nix as a C library with a stable interface idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.