Skip to content

Conversation

RossComputerGuy
Copy link
Member

Motivation

Upstreaming more C API exposure, specifically more derivation related things.

Context

Comes from DeterminateSystems#210


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the c api Nix as a C library with a stable interface label Sep 19, 2025
@RossComputerGuy RossComputerGuy requested review from Ericson2314 and removed request for edolstra September 19, 2025 22:53
/** @brief Nix Derivation */
typedef struct nix_derivation nix_derivation;
/** @brief Nix Derivation Output */
typedef struct nix_derivation_output nix_derivatio_noutput;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
typedef struct nix_derivation_output nix_derivatio_noutput;
typedef struct nix_derivation_output nix_derivation_output;

Copy link
Member

Choose a reason for hiding this comment

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

This one and associated functions is not needed if the consumer uses the JSON

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@Ericson2314
Copy link
Member

Ericson2314 commented Sep 19, 2025

I have a preference that consumers should achieve these goals by deserializing the JSON themselves. Then we can make do with a smaller interface which will be easier to maintain and more stable.

NIXC_CATCH_ERRS
}

nix_err nix_derivation_get_outputs_and_optpaths(
Copy link
Member

Choose a reason for hiding this comment

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

This should just be the opt paths, which is useful for the FOD case. The outputs themselves can be gotten from the JSON.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

This is me elaborating what I wrote in #14031 (comment)

@RossComputerGuy RossComputerGuy force-pushed the upstream-RossComputerGuy/feat/expose-drvfrompath branch from 521f2c9 to cd12d13 Compare September 19, 2025 23:17
@RossComputerGuy
Copy link
Member Author

I have a preference that consumers should achieve these goals by deserializing the JSON themselves. Then we can make do with a smaller interface which will be easier to maintain and more stable.

How do I get the JSON myself? All I have is a store path.

@Ericson2314
Copy link
Member

  1. Use your new function to read into memory derivation in store at given path
  2. Use my function (recently added) to dump JSON from in memory drv

@RossComputerGuy
Copy link
Member Author

Isn't there increased overhead if everything is being accessed through JSON? Wouldn't some operations make sense to do without deserializing?

@Ericson2314
Copy link
Member

Ericson2314 commented Sep 19, 2025

I would say optimization should come later. Let's see evidence of there being a bottleneck in practice before we make the interface bigger.

@RossComputerGuy RossComputerGuy force-pushed the upstream-RossComputerGuy/feat/expose-drvfrompath branch 4 times, most recently from df99081 to d0c6698 Compare September 22, 2025 18:25
@RossComputerGuy RossComputerGuy force-pushed the upstream-RossComputerGuy/feat/expose-drvfrompath branch from d0c6698 to 4ffac98 Compare September 22, 2025 23:24
@Ericson2314
Copy link
Member

  1. Good to put from and to JSON next to each other
  2. I still think we can get rid of nix_derivation entirely.
  3. I want to make the output path setting easier in C++ too. I might try to land a patch for that.

@RossComputerGuy RossComputerGuy force-pushed the upstream-RossComputerGuy/feat/expose-drvfrompath branch 2 times, most recently from c1e3593 to bdf7104 Compare September 23, 2025 17:00
@Ericson2314
Copy link
Member

Ah does anything use the BuildResult JSON today? Hopefully that is one I can fix as part of my big JSON rework next :)

@RossComputerGuy
Copy link
Member Author

I'm looking at our usage and it looks like there's not much usage of it.

@RossComputerGuy RossComputerGuy force-pushed the upstream-RossComputerGuy/feat/expose-drvfrompath branch from 517c686 to bec7506 Compare September 30, 2025 22:35
@RossComputerGuy RossComputerGuy force-pushed the upstream-RossComputerGuy/feat/expose-drvfrompath branch from bec7506 to 96b3f09 Compare September 30, 2025 22:42
nix_err nix_derivation_make_outputs(
nix_c_context * context,
Store * store,
const char * json,
Copy link
Member

Choose a reason for hiding this comment

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

this should take a nix_derivation, not parse the JSON itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we'll have to remove the verify function to even be able to get a nix_derivation.

Copy link
Member

@Ericson2314 Ericson2314 Oct 1, 2025

Choose a reason for hiding this comment

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

hmm? I am not sure what you mean. You mean to make an input-addressed derivation?

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd have to remove this:

drv.checkInvariants(*store->ptr, drvPath);

The checkInvariants function failing with a mismatch outpath hash error can prevent a nix_derivation from returning. If nix_derivation_make_outputs needs a nix_derivation and you can't get one without getting the right outpath hashes, now we have a catch-22 and it'll be impossible to add derivations.

Copy link
Member

Choose a reason for hiding this comment

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

I am going to bed, but we can look into this soon, I think we should be able to untangle it perhaps simpler than you think

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants