Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a DerivationProvenance type, wires derivation meta JSON into provenance chains during derivation construction, registers/serializes the new provenance type, updates provenance-show rendering to display derivation metadata and licenses, exposes headers/sources in libexpr build, and updates tests/configs to expect meta-wrapped provenance. Changes
Sequence DiagramsequenceDiagram
participant EvalState as EvalState
participant PrimOps as PrimOps
participant JSON as JSONSerializer
participant Provenance as Provenance
EvalState->>PrimOps: derivationStrict(state, args, state.provenance)
PrimOps->>PrimOps: detect meta in args / langVersion check
PrimOps->>JSON: printValueAsJSON(meta)
JSON-->>PrimOps: json_meta
PrimOps->>PrimOps: prune identifiers/licenses from json_meta
PrimOps->>Provenance: create DerivationProvenance(next=state.provenance, meta=json_meta)
Provenance-->>PrimOps: wrapped provenance
PrimOps->>PrimOps: call writeDerivation(..., provenance=wrapped)
PrimOps->>Provenance: provenance->to_json() for storage/display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/functional/flakes/provenance.sh (1)
107-137:⚠️ Potential issue | 🔴 CriticalCopied provenance is missing the
metalayer — test will likely fail.The first test (lines 19–61) asserts that build provenance now follows the chain
build → meta → flake → subpath → tree. However, this "copied" provenance test still expects the old chaincopied → build → flake → subpath → treewithout themetanode. Sincenix copywraps the original provenance with a"copied"envelope, the inner structure should still contain themetalayer.Suggested fix: insert the meta layer into the copied provenance expectation
{ "from": "file://$binaryCache", "next": { "drv": "$(basename "$drvPath")", "next": { - "flakeOutput": "packages.$system.default", + "meta": { + "license": [ + { + "deprecated": true, + "free": true, + "fullName": "GNU Lesser General Public License v2.1", + "redistributable": true, + "shortName": "lgpl21", + "spdxId": "LGPL-2.1", + "url": "https://spdx.org/licenses/LGPL-2.1.html" + } + ] + }, "next": { - "next": { - "attrs": { - "lastModified": $lastModified, - "ref": "refs/heads/master", - "rev": "$rev", - "revCount": 1, - "type": "git", - "url": "file://$flake1Dir" + "flakeOutput": "packages.$system.default", + "next": { + "next": { + "attrs": { + "lastModified": $lastModified, + "ref": "refs/heads/master", + "rev": "$rev", + "revCount": 1, + "type": "git", + "url": "file://$flake1Dir" + }, + "type": "tree" }, - "type": "tree" + "subpath": "/flake.nix", + "type": "subpath" }, - "subpath": "/flake.nix", - "type": "subpath" + "type": "flake" }, - "type": "flake" + "type": "meta" }, "output": "out", "type": "build" }, "type": "copied" }
🤖 Fix all issues with AI agents
In `@src/libexpr/primops.cc`:
- Around line 1577-1594: The new EvalState::s.meta.getId() branch
unconditionally requires experimentalFeatureSettings.require(Xp::Provenance) and
assumes printValueAsJSON returns an object, breaking existing derivations and
causing uncaught exceptions for non-object meta values; change the logic so you
only enforce Provenance and construct MetaProvenance when the feature is
enabled, otherwise fall through to the default handling (so old behavior
remains), and before iterating call printValueAsJSON and verify the returned
nlohmann::json is an object (e.g., is_object()); if it's not an object, either
forward to the default case or raise a clear Nix evaluation error instead of
calling it.key(), then only create MetaProvenance and assign provenance when the
JSON check passes (using symbols: EvalState::s.meta.getId(),
experimentalFeatureSettings.require, printValueAsJSON, MetaProvenance,
provenance).
🧹 Nitpick comments (1)
src/libexpr/primops.cc (1)
5443-5450: Bumping__langVersionbased on an experimental feature conflates language version with feature flags.
__langVersiontraditionally reflects language syntax/semantics evolution. Tying it to whetherXp::Provenanceis enabled means the same Nix binary reports different language versions depending on runtime config, which may confuse downstream consumers that uselangVersionfor feature detection of non-provenance language features. Consider whether a separatebuiltins.provenanceorbuiltins ? provenanceMetacheck would be a cleaner detection mechanism for Nixpkgs.
src/libexpr/primops.cc
Outdated
| case EvalState::s.meta.getId(): | ||
| experimentalFeatureSettings.require(Xp::Provenance); | ||
|
|
||
| { | ||
| auto meta = printValueAsJSON(state, true, *i->value, pos, context); | ||
|
|
||
| for (auto it = meta.begin(); it != meta.end();) { | ||
| if (it.key() == "identifiers" || it.key() == "license" || it.key() == "licenses") { | ||
| it++; | ||
| continue; | ||
| } | ||
|
|
||
| it = meta.erase(it); | ||
| } | ||
|
|
||
| provenance = std::make_shared<const MetaProvenance>(provenance, make_ref<nlohmann::json>(meta)); | ||
| } | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for EvalState::s.meta symbol definitions and usage patterns
echo "=== Searching for Symbol meta definitions in EvalState ==="
rg -n 'Symbol.*meta' --type cpp | head -20
echo ""
echo "=== Searching for s.meta usage patterns ==="
rg -n '\bs\.meta\b' --type cpp -C2 | head -50
echo ""
echo "=== Check derivation function for meta attribute handling ==="
rg -n 'meta.*getId\(\)|s\.meta' --type cpp -B3 -A3 | head -40
echo ""
echo "=== Look for pre-existing meta case in switch statements ==="
rg -n 'case.*meta\b|s\.meta.*case' --type cpp -C1 | head -30
echo ""
echo "=== Search for EvalState symbol initialization ==="
rg -n 'EvalState::s\.' src/libexpr/ --type cpp | grep -E '(Symbol|meta)' | head -20Repository: DeterminateSystems/nix-src
Length of output: 6844
🏁 Script executed:
#!/bin/bash
# Check the derivation function context around line 1577
echo "=== Context around line 1577 in primops.cc ==="
sed -n '1500,1600p' src/libexpr/primops.cc | head -120
echo ""
echo "=== Find the function that contains this switch statement ==="
rg -n 'static.*void.*derivation|derivationInternal\|derivation_impl' src/libexpr/primops.cc -A5 | head -20
echo ""
echo "=== Check what happens with meta before line 1577 (before this new case) ==="
sed -n '1540,1577p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 7275
🏁 Script executed:
#!/bin/bash
# Get the complete switch statement and default case
echo "=== Complete switch statement from line 1558 onwards ==="
sed -n '1558,1750p' src/libexpr/primops.cc
echo ""
echo "=== Check the default case handling ==="
sed -n '1700,1750p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 11637
🏁 Script executed:
#!/bin/bash
# Check test configuration and meta usage in tests
echo "=== Check config.nix.in for meta handling ==="
if [ -f tests/lang/config.nix.in ]; then
cat tests/lang/config.nix.in | head -50
fi
echo ""
echo "=== Search for test files with meta attributes ==="
rg -l 'meta\s*=' tests/lang/ --type nix | head -10
echo ""
echo "=== Check simple.nix mentioned in review ==="
if [ -f tests/lang/simple.nix ]; then
head -40 tests/lang/simple.nix
fi
echo ""
echo "=== Check for Provenance feature guard conditions ==="
rg -n 'Xp::Provenance|require.*Provenance' src/libexpr/ --type cpp -B2 -A2 | head -30Repository: DeterminateSystems/nix-src
Length of output: 1070
🏁 Script executed:
#!/bin/bash
# Check printValueAsJSON return type and JSON handling
echo "=== Search for printValueAsJSON function signature ==="
rg -n 'nlohmann::json.*printValueAsJSON' src/libexpr/ --type cpp -B2 -A5 | head -40
echo ""
echo "=== Check how printValueAsJSON handles different value types ==="
rg -n 'printValueAsJSON' src/libexpr/primops.cc -B2 -A10 | grep -A10 'case.*meta'
echo ""
echo "=== Verify if JSON is guaranteed to be object for meta ==="
rg -n 'printValueAsJSON.*meta' src/libexpr/ --type cpp -C5
echo ""
echo "=== Check if there's type checking before it.key() ==="
sed -n '1580,1595p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 1605
🏁 Script executed:
#!/bin/bash
# Check printValueAsJSON implementation for different value types
echo "=== Check printValueAsJSON implementation ==="
rg -n 'nlohmann::json printValueAsJSON' src/libexpr/value-to-json.cc -A30 | head -50
echo ""
echo "=== Check if JSON type is validated before iteration ==="
rg -n 'is_object\|is_array\|type\(\)' src/libexpr/value-to-json.cc | head -20
echo ""
echo "=== Look for any tests of meta as non-attrset ==="
rg -n 'meta.*string\|meta.*int\|meta.*list' . --type nix | head -10
echo ""
echo "=== Check nlohmann::json begin() behavior on non-objects ==="
python3 << 'EOF'
# Check nlohmann::json documentation/behavior
print("nlohmann::json iterator behavior:")
print("- For objects: iterator over key-value pairs")
print("- For non-objects (arrays, strings, numbers): begin() returns begin iterator")
print("- Calling .key() on non-object iterator is undefined behavior/exception")
print("")
print("The code at lines 1582-1590 does:")
print(" for (auto it = meta.begin(); it != meta.end();)")
print(" if (it.key() == ...)")
print("")
print("This assumes meta is always an object (JSON object type).")
print("If meta = 'foo' (string), printValueAsJSON returns JSON string,")
print("and calling it.key() would cause issues.")
EOFRepository: DeterminateSystems/nix-src
Length of output: 790
Intercepting meta unconditionally breaks existing derivations and lacks type safety.
This new case EvalState::s.meta.getId(): in the derivation builder (line 1577) intercepts all derivation calls containing a meta attribute. Before this change, meta was silently passed through to the derivation environment (via the default case). Now it unconditionally requires experimentalFeatureSettings.require(Xp::Provenance), which means:
-
Any derivation with a
metaattribute will fail if the Provenance feature is not enabled. This is a breaking change for existing code, including any tests or external derivations that includemeta. -
The code assumes
metais always a JSON object and iterates withit.key()without type checking. If a user passes a non-attrsetmeta(e.g.,meta = "foo";ormeta = [1 2];),printValueAsJSONwill return a non-object JSON, causingit.key()to throw an unhelpful exception rather than a clear Nix evaluation error.
🤖 Prompt for AI Agents
In `@src/libexpr/primops.cc` around lines 1577 - 1594, The new
EvalState::s.meta.getId() branch unconditionally requires
experimentalFeatureSettings.require(Xp::Provenance) and assumes printValueAsJSON
returns an object, breaking existing derivations and causing uncaught exceptions
for non-object meta values; change the logic so you only enforce Provenance and
construct MetaProvenance when the feature is enabled, otherwise fall through to
the default handling (so old behavior remains), and before iterating call
printValueAsJSON and verify the returned nlohmann::json is an object (e.g.,
is_object()); if it's not an object, either forward to the default case or raise
a clear Nix evaluation error instead of calling it.key(), then only create
MetaProvenance and assign provenance when the JSON check passes (using symbols:
EvalState::s.meta.getId(), experimentalFeatureSettings.require,
printValueAsJSON, MetaProvenance, provenance).
There was a problem hiding this comment.
metashould be able to be deserialized into a JSON object 99% of the time, if it doesn't then there's a problemmetahas never been passed into a derivation due to structured attrs not being enabled by default- The cool thing with this PR is it only affects how
metais passed into the derivation serialization, nix expressions still see the fullmeta.
There was a problem hiding this comment.
@RossComputerGuy Thank you for the clarifications! Those are important points that address my concerns:
-
Not a breaking change: Your point about
metanever being passed into derivations before (due to the structured attrs and config.nix removal logic) resolves my main concern about breaking existing code. If it wasn't serialized into derivations previously, then requiring the Provenance feature here is indeed a net-new behavior, not a breaking change. -
JSON object assumption: I understand that
metashould be an attrset in practice. Given that this is new functionality gated behind an experimental feature, the assumption is reasonable for the target use case. -
Serialization-only impact: The fact that Nix expressions still see the full
metaand only the internal derivation serialization is affected makes the scope much clearer.
Given this context, my initial characterization was too strong. That said, as a best practice for robustness, you might consider adding a type check before the iteration (e.g., if (!meta.is_object()) throw EvalError(...)) to provide a clearer error message in that 1% edge case where someone might pass a non-attrset meta. But this is a "nice-to-have" rather than critical, especially since it's behind an experimental feature flag.
Thank you for taking the time to explain the background!
🧠 Learnings used
Learnt from: edolstra
Repo: DeterminateSystems/nix-src PR: 321
File: src/libstore/build/derivation-building-goal.cc:443-447
Timestamp: 2026-01-23T12:00:50.308Z
Learning: In the Nix build system, for the `AlreadyValid` case in derivation building (when outputs already exist and are valid), provenance of the current derivation should NOT be returned because the existing outputs may have been built with different provenance than the current derivation, making it misleading to associate them.
RossComputerGuy
force-pushed
the
RossComputerGuy/simple-meta-provenance
branch
3 times, most recently
from
e2a917c to
ee14c7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/libexpr/primops.cc`:
- Around line 1577-1594: The meta branch calls printValueAsJSON which appends
string contexts into the shared context but then breaks, so the meta JSON never
gets added to the derivation (drv) and its string contexts become phantom
inputs; after creating provenance (MetaProvenance) in the EvalState::s.meta
case, explicitly add the filtered meta JSON into the derivation the same way the
default handler does (e.g., push the meta into drv.structuredAttrs and/or set
the appropriate drv.env entries) or invoke the common handling path for
structured values so that meta itself is recorded in drv and its string contexts
are correctly tied to actual drv inputs; reference symbols: EvalState::s.meta,
printValueAsJSON, MetaProvenance, provenance, drv.structuredAttrs, drv.env, and
context when making this change.
In `@tests/functional/flakes/provenance.sh`:
- Around line 167-189: Update the stale comment that currently states "--impure
does not add provenance" to reflect the new behavior: --impure no longer adds
full build/flake provenance but still records a meta provenance node (look for
the assertion expecting a JSON with "type": "meta" and "next": null). Also
normalize the heredoc to match the rest of the file by changing the occurrence
of "<< EOF" to "<<EOF" so the test style is consistent. Ensure the updated
comment and the heredoc token are edited in the test block surrounding the
--impure check and the JSON assertion.
🧹 Nitpick comments (2)
src/nix/provenance.cc (1)
95-97: Consider displaying a summary of the metadata content.The current output
"← with metadata"is opaque — users have no indication of what metadata is attached. Other provenance types display key details (e.g., the derivation path forBuildProvenance, the URL forFlakeProvenance). You could show which metadata fields are present (e.g.,"← with metadata (license, identifiers)") to makenix provenance showmore informative.That said, if the intent is to keep it minimal for now and iterate later, this is fine.
src/libexpr/primops.cc (1)
5443-5450: Coupling__langVersionto an experimental feature flag is unconventional — consider the stabilization path.
__langVersionis typically a monotonically increasing constant that reflects language-level changes available to all users. Tying it toXp::Provenancemeans that the same Nix binary reports different language versions depending on configuration, which could confuse tools or caches that key on__langVersion.When Provenance is stabilized (or if it's removed), this branching needs to be reconciled — either version 7 becomes the permanent value, or a different signaling mechanism is used. The current approach works as a short-term experiment, but consider documenting this caveat or using a different mechanism (e.g., a
builtins.provenanceEnabledflag) if this persists beyond the experimental phase.
RossComputerGuy
force-pushed
the
RossComputerGuy/simple-meta-provenance
branch
from
02837c0 to
bccd6a4
Compare
| break; | ||
| case EvalState::s.__meta.getId(): | ||
| if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) { | ||
| state.forceAttrs(*i->value, pos, ""); |
There was a problem hiding this comment.
missing error message in the force
| if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) { | ||
| /* Provenance allows for meta to be inside of derivations. | ||
| We increment the version to 7 so Nixpkgs will know when | ||
| provenance is available. */ | ||
| v.mkInt(7); | ||
| } else { | ||
| v.mkInt(6); | ||
| } |
There was a problem hiding this comment.
Bumping the language version in an experimental feature seems pretty wrong to me... Especially because it means that if upstream bumps its version for an unrelated reason, we now have two diverging language versions.
Why do we need to do this? What are the alternatives?
There was a problem hiding this comment.
We need some sort of way to communicate that __meta is accepted inside of a derivation. We could split it into its own builtins, call it something like derivationWithMeta, but when I was looking at the code that would complicate how __meta is handled since it doesn't get serialized into the derivation and there would need to be a way to drop it.
There was a problem hiding this comment.
I'm more interested in what happens if upstream happens to bump the language version to v7 as well.
There was a problem hiding this comment.
Then people not using DetNix and pass __meta, things would break. If they are using DetNix, things will be fine.
There was a problem hiding this comment.
Alternatively, we could add a field builtins.derivationSupportsMeta that Nixpkgs could check. Or my original idea, which was to have a builtins.derivationWithMeta, so then Nixpkgs could do
if builtins ? derivationWithMeta then
builtins.derivationWithMeta (attrs // { meta = ... })
else
builtins.derivation attrs
There was a problem hiding this comment.
One of the problems with having it as meta was, I got some weird flakiness with evaluation where in certain cases it would say attributes didn't exist. I'd run nix repl and check that attribute and it would exist. The other problem is figuring out how to make meta behave the same with the regular derivation primop.
| auto update = parts["update"]; | ||
|
|
||
| logger->cout( | ||
| TAB "" ANSI_BOLD "CPE:" ANSI_NORMAL " cpe:2.3:a:%s:%s:%s:%s:*:*:*:*:*:*", |
There was a problem hiding this comment.
For my own knowledge:
https://en.wikipedia.org/wiki/Common_Platform_Enumeration
The a stands for "application"
There was a problem hiding this comment.
Yes because there's no type part.
| provenance = subpath->next; | ||
| } else if (auto drv = std::dynamic_pointer_cast<const DerivationProvenance>(provenance)) { | ||
| logger->cout("← with derivation metadata"); | ||
| #define TAB " " |
There was a problem hiding this comment.
Is there a reason not to define this closer to the top, and not undefine it?
There was a problem hiding this comment.
No, this was just how I did it heh. We could move it to the top, that's fine by me.
3 participants
Motivation
Adds a subset of
metainto the provenance when the provenance experimental feature is used. So far, just recordidentifiers,license, andlicenses. We may want to expand this further but for now, this will allow us to judge how it affects the narinfo size.Context
Summary by CodeRabbit
New Features
Behavior
Tests
Documentation