Skip to content

Store StructuredAttrs directly in Derivation #13263

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented May 26, 2025

Instead of parsing a structured attrs at some later point, we parsed it right away when parsing the A-Term format, and likewise serialize it to __json = <JSON dump> when serializing a derivation to A-Term.

The JSON format can directly contain the JSON structured attrs without so encoding it, so we just do that.

Motivation

Depends on #13273

Context


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 new-cli Relating to the "nix" command label May 26, 2025
@github-project-automation github-project-automation bot moved this to To triage in Nix team May 26, 2025
@Ericson2314 Ericson2314 force-pushed the structured-attrs-in-drv branch from 6440a9f to e8800da Compare May 26, 2025 17:59
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 26, 2025
@Ericson2314 Ericson2314 marked this pull request as ready for review May 26, 2025 18:05
@Ericson2314
Copy link
Member Author

Thanks to talking to @puckipedia, I became satisfied that the other way of changing the abstract syntax (pair of env and optional structured attrs) wasn't "too lax" because there already was a way within the language to mix arbitrary env vars with structured attrs.

This PR is now updated to work that way, and is passing all the tests, but let's look at #13273 first.

@Ericson2314 Ericson2314 force-pushed the structured-attrs-in-drv branch 2 times, most recently from 62c3124 to 2aa355f Compare May 28, 2025 16:05
Instead of parsing a structured attrs at some later point, we parsed it
right away when parsing the A-Term format, and likewise serialize it to
`__json = <JSON dump>` when serializing a derivation to A-Term.

The JSON format can directly contain the JSON structured attrs without
so encoding it, so we just do that.
@Ericson2314 Ericson2314 force-pushed the structured-attrs-in-drv branch from 2aa355f to d7e3265 Compare May 28, 2025 17:16
Copy link

dpulls bot commented May 28, 2025

🎉 All dependencies have been resolved !

@Ericson2314
Copy link
Member Author

Notes from today's Nix team meeting:

  • What about perf of parsing the structured attrs eagerly?

  • What about derivations that no longer parse during GC?

    This branch in

    nix/src/libstore/gc.cc

    Lines 754 to 760 in 20226c8

    if (gcKeepDerivations && path->isDerivation()) {
    for (auto & [name, maybeOutPath] : queryPartialDerivationOutputMap(*path))
    if (maybeOutPath &&
    isValidPath(*maybeOutPath) &&
    queryPathInfo(*maybeOutPath)->deriver == *path)
    enqueue(*maybeOutPath);
    }
    does parse derivations, to see if they point to outputs we should keep.

    We should recover if we fail to parse a derivation, and have a test for this. We also need to decide what we want to do in that case: delete or keep the derivation that doesn't parse?

  • @edolstra what are the use-cases / ultimate purpose you have for doing this?

    @Ericson2314: Sure

    • Better derivation JSON format (or other new formats):

      That is useful for A very restricted recursive nix socket in the sandbox #8602 and dynamic derivations

    • Maybe better on-disk format too

      For input-addressed outputs, ATerm has to stick around forever at least as part of the path calculation. But for the newer experimental derivations types we have the opportunity to do a clean break.

@tomberek
Copy link
Contributor

tomberek commented Jun 4, 2025

Needs discussion to address potential perf and laziness implications.

@tomberek tomberek moved this from To triage to 🏁 Review in Nix team Jun 4, 2025
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2025-05-28-nix-team-meeting-minutes-229/65205/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2025-05-04-nix-team-meeting-minutes-230/65206/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

3 participants