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:
📝 WalkthroughWalkthroughThreads provenance metadata through derivation construction by adding an acceptMeta flag and provenance parameter to internal derivation logic, introducing a provenance-aware builtin/primop ( Changes
Sequence Diagram(s)sequenceDiagram
participant Expr as Nix Expr (drvFunc)
participant Builtins as Builtins / EvalState
participant PrimOp as PrimOp (derivationStrictWithMeta)
participant Internal as derivationStrictInternal
participant Prov as Provenance
Expr->>Builtins: call builtins.derivationWithMeta(drvAttrs)
Builtins->>PrimOp: invoke primop (acceptMeta=true, pass args)
PrimOp->>Internal: call derivationStrictInternal(..., provenance, acceptMeta=true)
Internal->>Prov: read/attach provenance metadata
Internal-->>PrimOp: return constructed derivation value
PrimOp-->>Builtins: return value to builtins
Builtins-->>Expr: evaluated derivation value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/primops.cc`:
- Around line 1662-1684: The __meta handling currently calls
printValueAsJSON(..., context) which lets any string context (store-path
references) leak into the shared context and thus into derivation inputs; create
a fresh metaContext (a Context instance with empty string context) and pass
metaContext to printValueAsJSON when processing EvalState::s.__meta fields
(identifiers, license, licenses), then after processing check
metaContext.stringContext (or equivalent) and if it is non-empty reject/raise an
error (or otherwise refuse to accept store-path references in meta) before
creating the DerivationProvenance and assigning provenance via
make_ref<nlohmann::json>(obj).
| if (acceptMeta && i->name == EvalState::s.__meta) { | ||
| if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) { | ||
| state.forceAttrs(*i->value, pos, "while evaluating __meta"); | ||
| auto meta = i->value->attrs(); | ||
| auto obj = nlohmann::json(); | ||
|
|
||
| for (auto & i : meta->lexicographicOrder(state.symbols)) { | ||
| auto key = state.symbols[i->name]; | ||
| switch (i->name.getId()) { | ||
| case EvalState::s.identifiers.getId(): | ||
| case EvalState::s.license.getId(): | ||
| case EvalState::s.licenses.getId(): | ||
| obj.emplace(key, printValueAsJSON(state, true, *i->value, pos, context)); | ||
| break; | ||
| default: | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| provenance = | ||
| std::make_shared<const DerivationProvenance>(provenance, make_ref<nlohmann::json>(obj)); | ||
| } | ||
| } else if (jsonObject) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code at lines 1662-1684 in src/libexpr/primops.cc
sed -n '1662,1684p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 1305
🏁 Script executed:
# Also get some context around those lines to understand the structure better
sed -n '1650,1700p' src/libexpr/primops.cc | head -80Repository: DeterminateSystems/nix-src
Length of output: 2624
🏁 Script executed:
# Find printValueAsJSON definition and signature
rg -n "printValueAsJSON" src/libexpr/ -A 3 -B 1 | head -50Repository: DeterminateSystems/nix-src
Length of output: 3106
🏁 Script executed:
# Check if there are any safeguards or context handling around __meta in primops
rg -n "metaContext\|__meta" src/libexpr/primops.cc -B 2 -A 2Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
# Find the derivationStrictInternal function definition and context variable declaration
rg -n "derivationStrictInternal" src/libexpr/primops.cc -A 100 | head -120Repository: DeterminateSystems/nix-src
Length of output: 5774
🏁 Script executed:
# Search for where context is declared in primops.cc around the derivation function
sed -n '1500,1700p' src/libexpr/primops.cc | grep -n "context\|NixStringContext"Repository: DeterminateSystems/nix-src
Length of output: 1173
🏁 Script executed:
# Check how context is used after the __meta block in the derivation function
sed -n '1680,1750p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 4273
🏁 Script executed:
# Search for where the context collected during printValueAsJSON is actually used
rg -n "drv.env\|structuredAttrs" src/libexpr/primops.cc -B 2 -A 2 | head -80Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
# Let's see how context is used at the end of derivationStrictInternal
sed -n '1700,1800p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 5462
🏁 Script executed:
# Check how drv.env is populated and whether it uses context
rg -n "drv.env" src/libexpr/primops.cc -B 2 -A 2Repository: DeterminateSystems/nix-src
Length of output: 1122
🏁 Script executed:
# Let's also check what happens to drv after it's built
sed -n '1800,1900p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 4423
🏁 Script executed:
# Search for where context is actually used to add dependencies
rg -n "printDerivation\|computeExportReferences" src/libexpr/primops.cc -B 1 -A 3Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
# Let's check if there's any filtering or validation of __meta elsewhere
rg -n "__meta\|metaContext" src/libexpr/primops.cc -B 2 -A 2Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
# Check if there are any validations for meta fields in other files
rg -n "license\|licenses\|identifiers" src/libexpr/ -l | head -20Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
# Let's also check if printValueAsJSON has any logic that clears context
sed -n '45,100p' src/libexpr/value-to-json.ccRepository: DeterminateSystems/nix-src
Length of output: 1847
🏁 Script executed:
# Let's check if there's validation that license/identifiers should not have store paths
rg -n "license\|identifiers" src/libexpr/primops.cc -B 1 -A 1Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
# Check if there's documentation or comments about meta handling
sed -n '1650,1680p' src/libexpr/primops.cc | grep -i "meta\|provenance\|tracking"Repository: DeterminateSystems/nix-src
Length of output: 441
🏁 Script executed:
# Look at the full context flow - see where context is passed and how it affects drv.inputDrvs/inputSrcs
sed -n '1945,1980p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 1222
🏁 Script executed:
# Let's verify the complete flow by checking exactly how context is accumulated
# and whether there's any filtering between __meta processing and the dependency loop
sed -n '1560,1600p' src/libexpr/primops.cc | grep -n "context"Repository: DeterminateSystems/nix-src
Length of output: 101
🏁 Script executed:
# Check the function signature of derivationStrictInternal to understand context scope
sed -n '1560,1650p' src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 3963
🏁 Script executed:
# Verify that context is a single shared variable throughout the function
rg -n "NixStringContext context" src/libexpr/primops.ccRepository: DeterminateSystems/nix-src
Length of output: 984
Isolate __meta context to prevent store-path references from leaking into derivation dependencies.
When processing __meta at line 1674, printValueAsJSON(..., context) accumulates string context into the shared context variable. If any __meta field (identifiers, license, licenses) contains a string with store-path context, that dependency is silently added to drv.inputDrvs and drv.inputSrcs (lines 1945+). This violates the design intent that meta is tracking-only metadata, not build input data. License information should be plain textual without store-path references.
Use a dedicated metaContext variable for __meta processing and reject non-empty context:
Proposed fix
auto obj = nlohmann::json();
+ NixStringContext metaContext;
for (auto & i : meta->lexicographicOrder(state.symbols)) {
auto key = state.symbols[i->name];
switch (i->name.getId()) {
case EvalState::s.identifiers.getId():
case EvalState::s.license.getId():
case EvalState::s.licenses.getId():
- obj.emplace(key, printValueAsJSON(state, true, *i->value, pos, context));
+ obj.emplace(key, printValueAsJSON(state, true, *i->value, pos, metaContext));
break;
default:
continue;
}
}
+ if (!metaContext.empty())
+ state.error<EvalError>("__meta must not reference store paths").atPos(pos).debugThrow();
+
provenance =
std::make_shared<const DerivationProvenance>(provenance, make_ref<nlohmann::json>(obj));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libexpr/primops.cc` around lines 1662 - 1684, The __meta handling
currently calls printValueAsJSON(..., context) which lets any string context
(store-path references) leak into the shared context and thus into derivation
inputs; create a fresh metaContext (a Context instance with empty string
context) and pass metaContext to printValueAsJSON when processing
EvalState::s.__meta fields (identifiers, license, licenses), then after
processing check metaContext.stringContext (or equivalent) and if it is
non-empty reject/raise an error (or otherwise refuse to accept store-path
references in meta) before creating the DerivationProvenance and assigning
provenance via make_ref<nlohmann::json>(obj).
There was a problem hiding this comment.
Metadata never contains store paths
There was a problem hiding this comment.
@RossComputerGuy, thank you for the clarification! I understand that metadata is designed to contain only plain textual data without store-path references, consistent with the documented expectation that meta is for tracking purposes rather than build dependencies.
If you'd like to add defensive validation in the future to catch any accidental violations of this assumption (e.g., if meta fields unexpectedly contain store references), the separate metaContext approach could help surface such issues early. But I understand this may not be a practical concern given current usage patterns.
I've noted this design constraint for future reference.
🧠 Learnings used
Learnt from: RossComputerGuy
Repo: DeterminateSystems/nix-src PR: 342
File: src/libexpr/primops.cc:1577-1594
Timestamp: 2026-02-10T20:44:08.621Z
Learning: In `src/libexpr/primops.cc`, the `EvalState::s.meta` case in `derivationStrictInternal` intentionally records filtered meta information (identifiers, license, licenses fields) into the provenance system via `MetaProvenance` without adding it to the derivation environment (`drv.env` or `drv.structuredAttrs`). This is by design because meta is metadata for tracking purposes, not build inputs. License information is expected to be plain textual data without store path references.
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.
There was a problem hiding this comment.
@RossComputerGuy What if meta does contain a store path, though? Just because it shouldn't or because we don't want it to doesn't mean it never will; there are lots of people doing crazy things out there ;P
Is it actually impossible for meta to include a store path because of some invariant somewhere?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I'm actually not sure entirely what would happen if meta were to contain a store path. Afaik, it would be in the provenance then and nothing special would happen. The store path could be used as normal.
The only invariant is nixpkgs that would affect things because of check-meta.
There was a problem hiding this comment.
We should keep nixpkgs in mind as we design and build, but we need to avoid assuming users are using nixpkgs.
fwiw I tried this:
{ system, seed ? 1 }:
let
meta = derivation {
name = "meta";
inherit system seed;
builder = "/bin/sh";
args = [
"-c"
"echo $dep >$out"
];
};
in
derivationWithMeta {
name = "nested";
inherit system seed;
builder = "/bin/sh";
args = [
"-c"
"echo ${meta} >$out"
];
__meta.license = { meh = meta; };
}
and nix path-info showed:
"provenance": {
"buildHost": "Grahams-MacBook-Pro.local",
"drv": "yid3h2v5bfg1g22zb5yjasy4gxgcl6ng-nested.drv",
"next": {
"meta": {
"license": {
"meh": "/nix/store/71dalf5p05pcfyb6lxqqmz27qjfzvqlx-meta"
}
},
which I think is an interesting thing to think about. I think we should probably prohibit this here to avoid a hard-to-undo mistake. We can always open it up later. Much harder to go backwards.
There was a problem hiding this comment.
It's certainly not great, the problem is meta does need some sort of structure and nixpkgs is really the only standard there is for this stuff. It is certainly nice to have pretty printing of the meta via nix provenance show and nix path-info --json showing the actual raw output. We always could check if there's a type attribute since that attribute is common. If it is set to the magic string then nix provenance show would print and assume the nixpkgs format. That way the caller can tell nix provenance show what that information is like without having to guess. The default could be to print the JSON when not recognizing the format or just not print the metadata at all.
RossComputerGuy
force-pushed
the
RossComputerGuy/meta-provenance-improvements
branch
from
22f83f2 to
738f202
Compare
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)
src/libexpr/primops/derivation.nix (1)
5-29:⚠️ Potential issue | 🟠 Major
:doc derivation(and:doc derivationWithMeta) will silently lose their documentation after this change.The
/** ... */doc comment block (lines 5–28) is now positioned beforedrvFunc:(line 29), so it is attached to the outer curried lambdadrvFunc: drvAttrs@{...}: body. IncreateBaseEnv,callFunction(*vDerivationValue, getBuiltin("derivationStrict"), *vDerivation, …)partially applies the outer lambda, producing the innerdrvAttrs@{...}: bodylambda — which carries no doc comment. The:doc derivationREPL command therefore shows nothing.Line 3 of this file explicitly warns: "the following comment will be shown in
:docin the repl", confirming that doc placement is intentional. That invariant is broken by this change.The fix is to move the
/** ... */block to immediately precededrvAttrs@{…}:, afterdrvFunc::🛠️ Proposed fix
+drvFunc: /** Create a derivation. ... */ -drvFunc: drvAttrs@{ outputs ? [ "out" ], ... }:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/derivation.nix` around lines 5 - 29, The doc comment block was placed before the outer lambda name drvFunc so the inner lambda drvAttrs@{...}: lost its documentation when createBaseEnv partially applies drvFunc (see callFunction(*vDerivationValue, getBuiltin("derivationStrict"), *vDerivation, …)); move the /** ... */ comment so it immediately precedes the inner lambda signature drvAttrs@{...}: (i.e. place the comment after drvFunc: and before drvAttrs@{…}:) so that the doc is attached to the function actually returned/partially-applied and will appear in :doc derivation (and :doc derivationWithMeta).
🧹 Nitpick comments (4)
src/libexpr/primops.cc (3)
1454-1461: Orphaned/misplaced comment block.After the rearrangement, the comment at lines 1454–1460 ("Construct (as a unobservable side effect) a Nix derivation expression…") now floats between
primop_derivationStrictWithMetaandcheckDerivationName, describing neither of them. It was the introductory comment forderivationStrictin the previous layout.✏️ Suggested fix
Move or remove the block; if retained, it belongs immediately before
primop_derivationStrict(around line 1925):-/* Construct (as a unobservable side effect) a Nix derivation - expression that performs the derivation described by the argument - set. Returns the original set extended with the following - attributes: `outPath' containing the primary output path of the - derivation; `drvPath' containing the path of the Nix expression; - and `type' set to `derivation' to indicate that this is a - derivation. */ - /** * Early validation for the derivation name, for better error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops.cc` around lines 1454 - 1461, The orphaned comment block describing construction of a Nix derivation was left between primop_derivationStrictWithMeta and checkDerivationName and no longer sits with the function it documents; move that comment so it immediately precedes primop_derivationStrict (the original target), or remove it if redundant, ensuring the comment appears directly above primop_derivationStrict and not between primop_derivationStrictWithMeta and checkDerivationName so the documentation matches the implementation.
1616-1637: Empty__meta = {}still creates aDerivationProvenanceentry with an empty JSON object.When
__metacontains no recognized fields (identifiers,license,licenses) — which is the common case when callers pass__meta = {}or a meta attrset without those keys —objremains{}butDerivationProvenanceis still constructed. This creates noise in the provenance chain. Consider guarding onobj.empty():✨ Suggested fix
- provenance = - std::make_shared<const DerivationProvenance>(provenance, make_ref<nlohmann::json>(obj)); + if (!obj.empty()) + provenance = + std::make_shared<const DerivationProvenance>(provenance, make_ref<nlohmann::json>(obj));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops.cc` around lines 1616 - 1637, When handling EvalState::s.__meta in the Provenance branch, avoid creating a DerivationProvenance for an empty meta object: after building obj via meta->lexicographicOrder/state.symbols and filtering keys with printValueAsJSON, check if obj.empty() and only call std::make_shared<const DerivationProvenance>(...) when obj is non-empty; leave provenance unchanged when obj is empty. Ensure this guard is placed inside the experimentalFeatureSettings.isEnabled(Xp::Provenance) block right before the assignment to provenance.
5580-5605:vDerivationWithMetais allocated unconditionally even whenXp::Provenanceis disabled.
allocValue()at line 5580 runs regardless of whether Provenance is enabled. When it is disabled, the allocatedValueis never written to (callFunctionon line 5604 is guarded) and the pointer is never stored anywhere useful, so there is no correctness risk — but the allocation is wasted. Moving it inside theifblock is a minor cleanup:✨ Suggested fix
- auto vDerivationWithMeta = allocValue(); if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) { + auto vDerivationWithMeta = allocValue(); addConstant( "derivationWithMeta", vDerivationWithMeta, { .type = nFunction, }); + // ... (move the later callFunction block here, or keep the outer guard) }Note: the second
if (experimentalFeatureSettings.isEnabled(Xp::Provenance))block at line 5603 would also need to reference the moved variable, so both blocks should be merged or the variable kept in shared scope with a conditional guard, per your preference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops.cc` around lines 5580 - 5605, vDerivationWithMeta is being allocated unconditionally via allocValue() even when experimentalFeatureSettings.isEnabled(Xp::Provenance) is false, wasting an unused Value; fix by moving the allocValue() call and related addConstant into the Provenance-enabled branch (or merge the two Provenance checks so the allocValue(), addConstant("derivationWithMeta", vDerivationWithMeta, ...) and the subsequent callFunction(..., getBuiltin("derivationStrictWithMeta"), ...) all live under the same if guarded by experimentalFeatureSettings.isEnabled(Xp::Provenance)), ensuring vDerivationWithMeta is only created when getBuiltin("derivationStrictWithMeta") will be invoked.src/libexpr/primops/derivation.nix (1)
1-2: Stale file-header comment.Line 2 says "It's actually a wrapper around the
'derivationStrict'primop", but now it wraps whicheverdrvFuncis passed (eitherderivationStrictorderivationStrictWithMeta).✏️ Suggested update
-# It's actually a wrapper around the 'derivationStrict' primop. +# It's actually a wrapper around a derivation primop passed as its first argument (drvFunc).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/derivation.nix` around lines 1 - 2, Update the stale header comment for the derivation builtin to reflect that it no longer always wraps derivationStrict; instead it delegates to the provided drvFunc (which may be derivationStrict or derivationStrictWithMeta). Replace the sentence that claims it's "actually a wrapper around the ‘derivationStrict’ primop" with a short description saying it wraps the provided drvFunc and will call either derivationStrict or derivationStrictWithMeta depending on how drvFunc is supplied; mention the drvFunc symbol to make the linkage clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/primops.cc`:
- Around line 1442-1452: Fix the typo in the comment above the primop
registration: change "derivaiton" to "derivation" in the comment that precedes
primop_derivationStrictWithMeta (the block registering PrimOp with .name =
"derivationStrictWithMeta" and calling prim_derivationStrictGeneric).
---
Outside diff comments:
In `@src/libexpr/primops/derivation.nix`:
- Around line 5-29: The doc comment block was placed before the outer lambda
name drvFunc so the inner lambda drvAttrs@{...}: lost its documentation when
createBaseEnv partially applies drvFunc (see callFunction(*vDerivationValue,
getBuiltin("derivationStrict"), *vDerivation, …)); move the /** ... */ comment
so it immediately precedes the inner lambda signature drvAttrs@{...}: (i.e.
place the comment after drvFunc: and before drvAttrs@{…}:) so that the doc is
attached to the function actually returned/partially-applied and will appear in
:doc derivation (and :doc derivationWithMeta).
---
Duplicate comments:
In `@src/libexpr/primops.cc`:
- Around line 1616-1637: The handling of EvalState::s.__meta that calls
printValueAsJSON for meta fields (EvalState::s.identifiers,
EvalState::s.license, EvalState::s.licenses) is intentional and should not be
changed; leave the provenance construction in DerivationProvenance as-is, but
add a short inline comment near the block (around the loop that inspects
meta->lexicographicOrder and the call to printValueAsJSON) stating that meta
fields are expected to be plain text and that leaking context via
printValueAsJSON is by-design to avoid future accidental changes.
---
Nitpick comments:
In `@src/libexpr/primops.cc`:
- Around line 1454-1461: The orphaned comment block describing construction of a
Nix derivation was left between primop_derivationStrictWithMeta and
checkDerivationName and no longer sits with the function it documents; move that
comment so it immediately precedes primop_derivationStrict (the original
target), or remove it if redundant, ensuring the comment appears directly above
primop_derivationStrict and not between primop_derivationStrictWithMeta and
checkDerivationName so the documentation matches the implementation.
- Around line 1616-1637: When handling EvalState::s.__meta in the Provenance
branch, avoid creating a DerivationProvenance for an empty meta object: after
building obj via meta->lexicographicOrder/state.symbols and filtering keys with
printValueAsJSON, check if obj.empty() and only call std::make_shared<const
DerivationProvenance>(...) when obj is non-empty; leave provenance unchanged
when obj is empty. Ensure this guard is placed inside the
experimentalFeatureSettings.isEnabled(Xp::Provenance) block right before the
assignment to provenance.
- Around line 5580-5605: vDerivationWithMeta is being allocated unconditionally
via allocValue() even when experimentalFeatureSettings.isEnabled(Xp::Provenance)
is false, wasting an unused Value; fix by moving the allocValue() call and
related addConstant into the Provenance-enabled branch (or merge the two
Provenance checks so the allocValue(), addConstant("derivationWithMeta",
vDerivationWithMeta, ...) and the subsequent callFunction(...,
getBuiltin("derivationStrictWithMeta"), ...) all live under the same if guarded
by experimentalFeatureSettings.isEnabled(Xp::Provenance)), ensuring
vDerivationWithMeta is only created when getBuiltin("derivationStrictWithMeta")
will be invoked.
In `@src/libexpr/primops/derivation.nix`:
- Around line 1-2: Update the stale header comment for the derivation builtin to
reflect that it no longer always wraps derivationStrict; instead it delegates to
the provided drvFunc (which may be derivationStrict or
derivationStrictWithMeta). Replace the sentence that claims it's "actually a
wrapper around the ‘derivationStrict’ primop" with a short description saying it
wraps the provided drvFunc and will call either derivationStrict or
derivationStrictWithMeta depending on how drvFunc is supplied; mention the
drvFunc symbol to make the linkage clear.
738f202 to
078c39c
Compare
RossComputerGuy
force-pushed
the
RossComputerGuy/meta-provenance-improvements
branch
from
078c39c to
ae725b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/libexpr/primops/derivation.nix (1)
1-3: Update stale comment to reflect the parameterizeddrvFuncdesign.Line 2 still says
"It's actually a wrapper around the 'derivationStrict' primop."but the file is now a higher-order function that wraps whateverdrvFuncis injected (eitherderivationStrictorderivationStrictWithMeta).✏️ Suggested fix
-# It's actually a wrapper around the 'derivationStrict' primop. +# It's actually a wrapper around a strict derivation primop (`derivationStrict` or +# `derivationStrictWithMeta`) which is passed as the `drvFunc` argument.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/derivation.nix` around lines 1 - 3, Update the stale top-of-file comment for the derivation builtin to reflect that this module is a higher-order wrapper which delegates to an injected drvFunc (not always derivationStrict); mention that drvFunc can be derivationStrict or derivationStrictWithMeta and that this file implements the public-facing derivation wrapper around the provided drvFunc rather than hardcoding derivationStrict.src/libexpr/primops.cc (1)
1622-1623: Inner loop variablesiandkeyshadow outer loop variables.The inner loop at line 1622 (
for (auto & i : meta->lexicographicOrder(...))) and theauto keyon line 1623 shadow the outer loop'si(line 1533) andkey(line 1536). While the scoping is correct, renaming these reduces confusion and maintenance risk.♻️ Suggested rename
- for (auto & i : meta->lexicographicOrder(state.symbols)) { - auto key = state.symbols[i->name]; - switch (i->name.getId()) { + for (auto & metaAttr : meta->lexicographicOrder(state.symbols)) { + auto metaKey = state.symbols[metaAttr->name]; + switch (metaAttr->name.getId()) { case EvalState::s.identifiers.getId(): case EvalState::s.license.getId(): case EvalState::s.licenses.getId(): - obj.emplace(key, printValueAsJSON(state, true, *i->value, pos, context)); + obj.emplace(metaKey, printValueAsJSON(state, true, *metaAttr->value, pos, context)); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops.cc` around lines 1622 - 1623, The inner loop variables shadow outer-scope names: rename the loop variable in the for (auto & i : meta->lexicographicOrder(state.symbols)) and the subsequent auto key = state.symbols[i->name]; to non-conflicting names (e.g., orderEntry or symRef for the iterator and innerKey or symKey for the looked-up key) so they don't shadow the outer i and key; update all uses within that loop body accordingly to reference the new names and keep meta->lexicographicOrder(state.symbols) and state.symbols access unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/primops.cc`:
- Around line 1616-1638: The code currently allows __meta to fall through into
the jsonObject/structuredAttrs path when acceptMeta is false, causing __meta to
be included in structuredAttrs; change the logic so __meta is explicitly
excluded unless acceptMeta is true: in the branch handling
jsonObject/structuredAttrs (the else if (jsonObject) path and the
structuredAttrs handling around EvalState::s.__meta), add a guard that
skips/continues when i->name == EvalState::s.__meta and acceptMeta is false
(mirroring the earlier acceptMeta check), so __meta is never added to
structuredAttrs or jsonObject unless acceptMeta is true.
---
Duplicate comments:
In `@src/libexpr/primops.cc`:
- Around line 1616-1638: The __meta handling passes the full evaluation context
(store path) into printValueAsJSON causing a sensitive store-path leak; update
the loop that builds obj so calls to printValueAsJSON for keys (inside the
EvalState::s.__meta block where provenance is constructed and
DerivationProvenance is created) do not forward the original context—either pass
a sanitized/empty context or a flag that prevents including store path, or use
an alternate helper that serializes values without the store-path info; ensure
you change the printValueAsJSON invocations in this branch (the switch inside
the for over meta->lexicographicOrder) and keep provenance construction via
make_ref<nlohmann::json>(obj) intact.
---
Nitpick comments:
In `@src/libexpr/primops.cc`:
- Around line 1622-1623: The inner loop variables shadow outer-scope names:
rename the loop variable in the for (auto & i :
meta->lexicographicOrder(state.symbols)) and the subsequent auto key =
state.symbols[i->name]; to non-conflicting names (e.g., orderEntry or symRef for
the iterator and innerKey or symKey for the looked-up key) so they don't shadow
the outer i and key; update all uses within that loop body accordingly to
reference the new names and keep meta->lexicographicOrder(state.symbols) and
state.symbols access unchanged.
In `@src/libexpr/primops/derivation.nix`:
- Around line 1-3: Update the stale top-of-file comment for the derivation
builtin to reflect that this module is a higher-order wrapper which delegates to
an injected drvFunc (not always derivationStrict); mention that drvFunc can be
derivationStrict or derivationStrictWithMeta and that this file implements the
public-facing derivation wrapper around the provided drvFunc rather than
hardcoding derivationStrict.
|
triggering s3 upload |
grahamc
left a comment
grahamc
left a comment
There was a problem hiding this comment.
(prophylactic request changes, I'm testing something out and think there might be a bug, I'll follow up shortly)
grahamc
left a comment
grahamc
left a comment
There was a problem hiding this comment.
I tried it with this derivation:
{ system, seed ? 2 }:
derivationWithMeta {
name = "nested-no-derivation";
inherit system seed;
builder = "/bin/sh";
args = [
"-c"
"echo hi >$out"
];
__meta.example = "hi";
}
and it builds but the CLI tool throws an error:
% nix provenance show .\#packages.aarch64-darwin.packageWithMeta
warning: Git tree '/Users/grahamc/src/github.com/DeterminateSystems/samples' has uncommitted changes
/nix/store/c4c2axl0cyr66xw5d9m34xqj8h9913rh-nested-no-derivation
← built from derivation /nix/store/k2kcd1fd0symg50r8rg6zni3i8bhzlp5-nested-no-derivation.drv (output out) on Grahams-MacBook-Pro.local for aarch64-darwin
← with derivation metadata
error: Expected JSON value to be of type 'object' but it is of type 'null': null
Then I tried changing it to __meta.license = "hi"; and that also puked:
grahamc@Grahams-MacBook-Pro samples % nix provenance show .\#packages.aarch64-darwin.packageWithMeta
warning: Git tree '/Users/grahamc/src/github.com/DeterminateSystems/samples' has uncommitted changes
/nix/store/i64frhgi70rfxi946c9dn5bdw5gn9ds2-nested-no-derivation
← built from derivation /nix/store/3982x23pid0k96cjpnj4hj9hnsz9ndq9-nested-no-derivation.drv (output out) on Grahams-MacBook-Pro.local for aarch64-darwin
← with derivation metadata
error: Expected JSON value to be of type 'object' but it is of type 'string': "hi"
so I changed it to __meta.license = { meh = "hi";}; and got:
warning: Git tree '/Users/grahamc/src/github.com/DeterminateSystems/samples' has uncommitted changes
/nix/store/b1101z0mmqk8m8a31cpkb9s62g26ziky-nested-no-derivation
← built from derivation /nix/store/fmg06ppkrpq266ca4k1fc4z4b64xc0ad-nested-no-derivation.drv (output out) on Grahams-MacBook-Pro.local for aarch64-darwin
← with derivation metadata
error: [json.exception.type_error.302] type must be string, but is null
and I'm not really sure how to use it yet.
Hmm not sure what it got hung up about.
The |
|
Where is that in the code? I don't think it is a good idea to design this to fit-like-a-glove the structure defined in Nixpkgs today. |
|
The problem is quite a bit of this metadata isn't simple so fitting it to nixpkgs makes things easier. If we don't use the nixpkgs format, we'd have to define our own format and map between the nixpkgs format and our own to maintain compatibility. Licenses in particular are defined in different ways and contains various amount of information such as the SPDX ID, unfree, etc. |
|
Very interesting, I tried it again but not from the |
|
Ok, it seems like |
|
So, remaining issues: { system, seed ? 12 }:
derivationWithMeta {
name = "nested-no-derivation";
inherit system seed;
builder = "/bin/sh";
args = [
"-c"
"echo hi >$out"
];
__meta = {
dizzle = "dazzle";
license = { meh = "hi";};
};
}this shouldn't cause this error: |
|
Done, example: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nix/provenance.cc (1)
14-15:TABmacro is defined but never used — remove it.After the CPE/license printing logic was removed,
TABhas no references in this file. The only indentation in the new code (line 109) uses a hardcoded" "(two spaces), notTAB.🧹 Proposed cleanup
-#define TAB " " - using namespace nix;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nix/provenance.cc` around lines 14 - 15, The TAB macro (`#define` TAB " ") is unused in this file; remove the TAB macro definition to clean up dead code and ensure no other references exist (search for TAB and the literal " " indentation near the provenance printing logic) and, if any indentation constant is needed, replace uses with the explicit string currently used in the printing logic (the hardcoded " ") or a newly named constant; specifically delete the TAB macro definition and run a quick grep for TAB to confirm no remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/nix/provenance.cc`:
- Around line 14-15: The TAB macro (`#define` TAB " ") is unused in this file;
remove the TAB macro definition to clean up dead code and ensure no other
references exist (search for TAB and the literal " " indentation near the
provenance printing logic) and, if any indentation constant is needed, replace
uses with the explicit string currently used in the printing logic (the
hardcoded " ") or a newly named constant; specifically delete the TAB macro
definition and run a quick grep for TAB to confirm no remaining references.
|
Looking great. I do think we should prohibit strings with context in the meta, though, at least for the initial releases. With this: { system, seed ? 3 }:
let
meta = derivation {
name = "meta";
inherit system seed;
builder = "/bin/sh";
args = [
"-c"
"echo $dep >$out"
];
};
in
derivationWithMeta {
name = "nested";
inherit system seed;
builder = "/bin/sh";
args = [
"-c"
"echo ${meta} >$out"
];
__meta.fizzle = { meh = meta; };
}I'm seeing that succeed: when I don't think it should. I think it should be an evaluation time error. |
Hmm, is there a function for that or would we have to recurse, force, and check the value? |
|
|
Thanks, that is added. |
4 participants
Motivation
Resolves some of the problems from #342
Context
Summary by CodeRabbit
New Features
Changes
Tests