-
Notifications
You must be signed in to change notification settings - Fork 8
Add derivationWithMeta builtin #357
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
Changes from all commits
c0a353b
ae725b2
0ee8865
0f69936
e12473d
86b9bcc
5ff902e
a3c549e
7cc42eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1380,16 +1380,15 @@ static void prim_second(EvalState & state, const PosIdx pos, Value ** args, Valu | |
| * Derivations | ||
| *************************************************************/ | ||
|
|
||
| static void derivationStrictInternal(EvalState & state, std::string_view name, const Bindings * attrs, Value & v); | ||
| static void derivationStrictInternal( | ||
| EvalState & state, | ||
| std::string_view name, | ||
| const Bindings * attrs, | ||
| Value & v, | ||
| std::shared_ptr<const Provenance> provenance, | ||
| bool acceptMeta); | ||
|
|
||
| /* 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. */ | ||
| static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value ** args, Value & v) | ||
| static void prim_derivationStrictGeneric(EvalState & state, const PosIdx pos, Value ** args, Value & v, bool acceptMeta) | ||
| { | ||
| state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.derivationStrict"); | ||
|
|
||
|
|
@@ -1409,7 +1408,7 @@ static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value ** | |
| } | ||
|
|
||
| try { | ||
| derivationStrictInternal(state, drvName, attrs, v); | ||
| derivationStrictInternal(state, drvName, attrs, v, state.evalContext.provenance, acceptMeta); | ||
| } catch (Error & e) { | ||
| Pos pos = state.positions[nameAttr->pos]; | ||
| /* | ||
|
|
@@ -1440,6 +1439,18 @@ static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value ** | |
| } | ||
| } | ||
|
|
||
| /* Construct a Nix derivation with metadata provenance */ | ||
| static RegisterPrimOp primop_derivationStrictWithMeta( | ||
| PrimOp{ | ||
| .name = "derivationStrictWithMeta", | ||
| .arity = 1, | ||
| .fun = | ||
| [](EvalState & state, const PosIdx pos, Value ** args, Value & v) { | ||
| prim_derivationStrictGeneric(state, pos, args, v, /*acceptMeta=*/true); | ||
| }, | ||
| .internal = true, | ||
| }); | ||
|
|
||
| /** | ||
| * Early validation for the derivation name, for better error message. | ||
| * It is checked again when constructing store paths. | ||
|
|
@@ -1463,7 +1474,13 @@ static void checkDerivationName(EvalState & state, std::string_view drvName) | |
| } | ||
| } | ||
|
|
||
| static void derivationStrictInternal(EvalState & state, std::string_view drvName, const Bindings * attrs, Value & v) | ||
| static void derivationStrictInternal( | ||
| EvalState & state, | ||
| std::string_view drvName, | ||
| const Bindings * attrs, | ||
| Value & v, | ||
| std::shared_ptr<const Provenance> provenance, | ||
| bool acceptMeta) | ||
| { | ||
| checkDerivationName(state, drvName); | ||
|
|
||
|
|
@@ -1505,8 +1522,6 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName | |
| StringSet outputs; | ||
| outputs.insert("out"); | ||
|
|
||
| auto provenance = state.evalContext.provenance; | ||
|
|
||
| for (auto & i : attrs->lexicographicOrder(state.symbols)) { | ||
| if (i->name == state.s.ignoreNulls) | ||
| continue; | ||
|
|
@@ -1574,29 +1589,6 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName | |
| experimentalFeatureSettings.require(Xp::ImpureDerivations); | ||
| } | ||
| break; | ||
| case EvalState::s.__meta.getId(): | ||
| if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) { | ||
| state.forceAttrs(*i->value, pos, ""); | ||
| 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)); | ||
| } | ||
| break; | ||
| /* The `args' attribute is special: it supplies the | ||
| command-line arguments to the builder. */ | ||
| case EvalState::s.args.getId(): | ||
|
|
@@ -1613,7 +1605,19 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName | |
| the environment. */ | ||
| default: | ||
|
|
||
| if (jsonObject) { | ||
| if (acceptMeta && i->name == EvalState::s.__meta) { | ||
| if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) { | ||
| state.forceAttrs(*i->value, pos, "while evaluating __meta"); | ||
| NixStringContext ctx; | ||
| auto obj = printValueAsJSON(state, true, *i->value, pos, ctx); | ||
|
|
||
| if (!ctx.empty()) | ||
| throw Error("Derivation __meta provenance can't contain string context like store paths."); | ||
|
|
||
| provenance = | ||
| std::make_shared<const DerivationProvenance>(provenance, make_ref<nlohmann::json>(obj)); | ||
| } | ||
| } else if (jsonObject) { | ||
|
Comment on lines
1608
to
1620
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 When processing Use a dedicated 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Metadata never contains store paths There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 I've noted this design constraint for future reference. 🧠 Learnings used
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: and nix path-info showed: 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (i->name == state.s.structuredAttrs) | ||
| continue; | ||
|
|
@@ -1900,11 +1904,21 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName | |
| v.mkAttrs(result); | ||
| } | ||
|
|
||
| /* 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. */ | ||
| static RegisterPrimOp primop_derivationStrict( | ||
| PrimOp{ | ||
| .name = "derivationStrict", | ||
| .arity = 1, | ||
| .fun = prim_derivationStrict, | ||
| .fun = | ||
| [](EvalState & state, const PosIdx pos, Value ** args, Value & v) { | ||
| prim_derivationStrictGeneric(state, pos, args, v, /*acceptMeta=*/false); | ||
| }, | ||
| }); | ||
|
|
||
| /* Return a placeholder string for the specified output that will be | ||
|
|
@@ -5445,14 +5459,7 @@ void EvalState::createBaseEnv(const EvalSettings & evalSettings) | |
| language feature gets added. It's not necessary to increase it | ||
| when primops get added, because you can just use `builtins ? | ||
| primOp' to check. */ | ||
| 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); | ||
| } | ||
| v.mkInt(6); | ||
| addConstant( | ||
| "__langVersion", | ||
| v, | ||
|
|
@@ -5559,6 +5566,16 @@ void EvalState::createBaseEnv(const EvalSettings & evalSettings) | |
| .type = nFunction, | ||
| }); | ||
|
|
||
| auto vDerivationWithMeta = allocValue(); | ||
| if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) { | ||
| addConstant( | ||
| "derivationWithMeta", | ||
| vDerivationWithMeta, | ||
| { | ||
| .type = nFunction, | ||
| }); | ||
| } | ||
|
|
||
| /* Now that we've added all primops, sort the `builtins' set, | ||
| because attribute lookups expect it to be sorted. */ | ||
| const_cast<Bindings *>(getBuiltins().attrs())->sort(); | ||
|
|
@@ -5567,7 +5584,14 @@ void EvalState::createBaseEnv(const EvalSettings & evalSettings) | |
|
|
||
| /* Note: we have to initialize the 'derivation' constant *after* | ||
| building baseEnv/staticBaseEnv because it uses 'builtins'. */ | ||
| evalFile(derivationInternal, *vDerivation); | ||
| auto vDerivationValue = allocValue(); | ||
| evalFile(derivationInternal, *vDerivationValue); | ||
|
|
||
| callFunction(*vDerivationValue, getBuiltin("derivationStrict"), *vDerivation, PosIdx()); | ||
edolstra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) | ||
| callFunction( | ||
| *vDerivationValue, **get(internalPrimOps, "derivationStrictWithMeta"), *vDerivationWithMeta, PosIdx()); | ||
| } | ||
|
|
||
| } // namespace nix | ||
Uh oh!
There was an error while loading. Please reload this page.