From eb18dcb0ea9ceab4f341492f4a7d8454d075b26d Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sun, 7 Jul 2024 13:56:28 -0600 Subject: [PATCH 1/5] cleanup and slightly refactor DrvInfo::queryOutputs Change-Id: I00d9ff707fe61995737b86af6d2eaa1e4d8116ff --- src/libexpr/get-drvs.cc | 164 ++++++++++++++++++++++++++++------------ src/libexpr/get-drvs.hh | 2 + 2 files changed, 118 insertions(+), 48 deletions(-) diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 8e3969aac..f5d0f0120 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -101,66 +101,134 @@ StorePath DrvInfo::queryOutPath() return *outPath; } +void DrvInfo::fillOutputs(bool withPaths) +{ + auto fillDefault = [&]() { + std::optional outPath = std::nullopt; + if (withPaths) { + outPath.emplace(this->queryOutPath()); + } + this->outputs.emplace("out", outPath); + }; + + // lol. lmao even. + if (this->attrs == nullptr) { + fillDefault(); + return; + } + + Attr * outputs = this->attrs->get(this->state->sOutputs); + if (outputs == nullptr) { + fillDefault(); + return; + } + + // NOTE(Qyriad): I don't think there is any codepath that can cause this to error. + this->state->forceList( + *outputs->value, + outputs->pos, + "while evaluating the 'outputs' attribute of a derivation" + ); + + for (auto [idx, elem] : enumerate(outputs->value->listItems())) { + // NOTE(Qyriad): This error should be *extremely* rare in practice. + // It is impossible to construct with `stdenv.mkDerivation`, + // `builtins.derivation`, or even `derivationStrict`. As far as we can tell, + // it is only possible by overriding a derivation attrset already created by + // one of those with `//` to introduce the failing `outputs` entry. + auto errMsg = fmt("while evaluating output %d of a derivation", idx); + std::string_view outputName = state->forceStringNoCtx( + *elem, + outputs->pos, + errMsg + ); + + if (withPaths) { + // Find the attr with this output's name... + Attr * out = this->attrs->get(this->state->symbols.create(outputName)); + if (out == nullptr) { + // FIXME: throw error? + continue; + } + + // Meanwhile we couldn't figure out any circumstances + // that cause this to error. + state->forceAttrs(*out->value, outputs->pos, errMsg); + + // ...and evaluate its `outPath` attribute. + Attr * outPath = out->value->attrs->get(this->state->sOutPath); + if (outPath == nullptr) { + continue; + // FIXME: throw error? + } + + NixStringContext context; + // And idk what could possibly cause this one to error + // that wouldn't error before here. + auto storePath = state->coerceToStorePath( + outPath->pos, + *outPath->value, + context, + errMsg + ); + this->outputs.emplace(outputName, storePath); + } else { + this->outputs.emplace(outputName, std::nullopt); + } + } +} DrvInfo::Outputs DrvInfo::queryOutputs(bool withPaths, bool onlyOutputsToInstall) { + // If we haven't already cached the outputs set, then do so now. if (outputs.empty()) { - /* Get the ‘outputs’ list. */ - Bindings::iterator i; - if (attrs && (i = attrs->find(state->sOutputs)) != attrs->end()) { - state->forceList(*i->value, i->pos, "while evaluating the 'outputs' attribute of a derivation"); - - /* For each output... */ - for (auto elem : i->value->listItems()) { - std::string output(state->forceStringNoCtx(*elem, i->pos, "while evaluating the name of an output of a derivation")); - - if (withPaths) { - /* Evaluate the corresponding set. */ - Bindings::iterator out = attrs->find(state->symbols.create(output)); - if (out == attrs->end()) continue; // FIXME: throw error? - state->forceAttrs(*out->value, i->pos, "while evaluating an output of a derivation"); - - /* And evaluate its ‘outPath’ attribute. */ - Bindings::iterator outPath = out->value->attrs->find(state->sOutPath); - if (outPath == out->value->attrs->end()) continue; // FIXME: throw error? - NixStringContext context; - outputs.emplace(output, state->coerceToStorePath(outPath->pos, *outPath->value, context, "while evaluating an output path of a derivation")); - } else - outputs.emplace(output, std::nullopt); - } - } else - outputs.emplace("out", withPaths ? std::optional{queryOutPath()} : std::nullopt); + // FIXME: this behavior seems kind of busted, since whether or not this + // DrvInfo will have paths is forever determined by the *first* call to + // this function?? + fillOutputs(withPaths); } + // Things that operate on derivations like packages, like `nix-env` and `nix build`, + // allow derivations to specify which outputs should be used in those user-facing + // cases if the user didn't specify an output explicitly. + // If the caller just wanted all the outputs for this derivation, though, + // then we're done here. if (!onlyOutputsToInstall || !attrs) return outputs; - Bindings::iterator i; - if (attrs && (i = attrs->find(state->sOutputSpecified)) != attrs->end() && state->forceBool(*i->value, i->pos, "while evaluating the 'outputSpecified' attribute of a derivation")) { - Outputs result; - auto out = outputs.find(queryOutputName()); - if (out == outputs.end()) - throw Error("derivation does not have output '%s'", queryOutputName()); - result.insert(*out); - return result; + // Regardless of `meta.outputsToInstall`, though, you can select into a derivation + // output by its attribute, e.g. `pkgs.lix.dev`, which (lol?) sets the magic + // attribute `outputSpecified = true`, and changes the `outputName` attr to the + // explicitly selected-into output. + if (Attr * outSpecAttr = attrs->get(state->sOutputSpecified)) { + bool outputSpecified = this->state->forceBool( + *outSpecAttr->value, + outSpecAttr->pos, + "while evaluating the 'outputSpecified' attribute of a derivation" + ); + if (outputSpecified) { + auto maybeOut = outputs.find(queryOutputName()); + if (maybeOut == outputs.end()) { + throw Error("derivation does not have output '%s'", queryOutputName()); + } + return Outputs{*maybeOut}; + } } - else { - /* Check for `meta.outputsToInstall` and return `outputs` reduced to that. */ - const Value * outTI = queryMeta("outputsToInstall"); - if (!outTI) return outputs; - auto errMsg = Error("this derivation has bad 'meta.outputsToInstall'"); - /* ^ this shows during `nix-env -i` right under the bad derivation */ - if (!outTI->isList()) throw errMsg; - Outputs result; - for (auto elem : outTI->listItems()) { - if (elem->type() != nString) throw errMsg; - auto out = outputs.find(elem->string.s); - if (out == outputs.end()) throw errMsg; - result.insert(*out); - } - return result; + /* Check for `meta.outputsToInstall` and return `outputs` reduced to that. */ + const Value * outTI = queryMeta("outputsToInstall"); + if (!outTI) return outputs; + auto errMsg = Error("this derivation has bad 'meta.outputsToInstall'"); + /* ^ this shows during `nix-env -i` right under the bad derivation */ + if (!outTI->isList()) throw errMsg; + Outputs result; + for (auto elem : outTI->listItems()) { + if (elem->type() != nString) throw errMsg; + auto out = outputs.find(elem->string.s); + if (out == outputs.end()) throw errMsg; + result.insert(*out); } + return result; } diff --git a/src/libexpr/get-drvs.hh b/src/libexpr/get-drvs.hh index 44aac3015..525ac9803 100644 --- a/src/libexpr/get-drvs.hh +++ b/src/libexpr/get-drvs.hh @@ -37,6 +37,8 @@ private: bool checkMeta(Value & v); + void fillOutputs(bool withPaths = true); + public: /** * path towards the derivation From 6a30ea0cc47845622dd85e4816570a005e6c9e51 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sun, 7 Jul 2024 15:06:37 -0600 Subject: [PATCH 2/5] libexpr: DrvInfo: remove unused bad-citizen constructor DrvInfo's constructor that only takes `EvalState` leaves everything else empty; a DrvInfo which has no iota of information about the derivation it represents is not useful, and was not used anywhere. Change-Id: Ic4d93a08cb2748b8cef9a61e41e70404834b23f9 --- src/libexpr/get-drvs.hh | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libexpr/get-drvs.hh b/src/libexpr/get-drvs.hh index 525ac9803..fd927b9e5 100644 --- a/src/libexpr/get-drvs.hh +++ b/src/libexpr/get-drvs.hh @@ -45,7 +45,6 @@ public: */ std::string attrPath; - DrvInfo(EvalState & state) : state(&state) { }; DrvInfo(EvalState & state, std::string attrPath, Bindings * attrs); DrvInfo(EvalState & state, ref store, const std::string & drvPathWithOutputs); From 5ffed6d06aec09d5ece63fcbf2fa0a28ab73cb80 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sun, 7 Jul 2024 17:56:14 -0600 Subject: [PATCH 3/5] libexpr: mild cleanup to getDerivations Shuffled the logic around a bit so the shorter code paths are early returns, added comments, etc. Should be NFC. Change-Id: Ie3ddb3d0eddd614d6f8c37bf9a4d5a50282084ea --- src/libexpr/get-drvs.cc | 123 ++++++++++++++++++++++++++-------------- 1 file changed, 81 insertions(+), 42 deletions(-) diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index f5d0f0120..fe4d3a7d5 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -1,6 +1,7 @@ #include "get-drvs.hh" #include "eval-inline.hh" #include "derivations.hh" +#include "eval.hh" #include "store-api.hh" #include "path-with-outputs.hh" @@ -418,56 +419,94 @@ static void getDerivations(EvalState & state, Value & vIn, Value v; state.autoCallFunction(autoArgs, vIn, v); - /* Process the expression. */ - if (!getDerivation(state, v, pathPrefix, drvs, ignoreAssertionFailures)) ; - - else if (v.type() == nAttrs) { - - /* Dont consider sets we've already seen, e.g. y in - `rec { x.d = derivation {...}; y = x; }`. */ - if (!done.insert(v.attrs).second) return; - - /* !!! undocumented hackery to support combining channels in - nix-env.cc. */ - bool combineChannels = v.attrs->find(state.symbols.create("_combineChannels")) != v.attrs->end(); - - /* Consider the attributes in sorted order to get more - deterministic behaviour in nix-env operations (e.g. when - there are names clashes between derivations, the derivation - bound to the attribute with the "lower" name should take - precedence). */ - for (auto & i : v.attrs->lexicographicOrder(state.symbols)) { - debug("evaluating attribute '%1%'", state.symbols[i->name]); - if (!std::regex_match(std::string(state.symbols[i->name]), attrRegex)) - continue; - std::string pathPrefix2 = addToPath(pathPrefix, state.symbols[i->name]); - if (combineChannels) - getDerivations(state, *i->value, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures); - else if (getDerivation(state, *i->value, pathPrefix2, drvs, ignoreAssertionFailures)) { - /* If the value of this attribute is itself a set, - should we recurse into it? => Only if it has a - `recurseForDerivations = true' attribute. */ - if (i->value->type() == nAttrs) { - Bindings::iterator j = i->value->attrs->find(state.sRecurseForDerivations); - if (j != i->value->attrs->end() && state.forceBool(*j->value, j->pos, "while evaluating the attribute `recurseForDerivations`")) - getDerivations(state, *i->value, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures); - } - } - } + bool shouldRecurse = getDerivation(state, v, pathPrefix, drvs, ignoreAssertionFailures); + if (!shouldRecurse) { + // We're done here. + return; } - else if (v.type() == nList) { + if (v.type() == nList) { // NOTE we can't really deduplicate here because small lists don't have stable addresses // and can cause spurious duplicate detections due to v being on the stack. for (auto [n, elem] : enumerate(v.listItems())) { - std::string pathPrefix2 = addToPath(pathPrefix, fmt("%d", n)); - if (getDerivation(state, *elem, pathPrefix2, drvs, ignoreAssertionFailures)) - getDerivations(state, *elem, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures); + std::string joinedAttrPath = addToPath(pathPrefix, fmt("%d", n)); + bool shouldRecurse = getDerivation(state, *elem, joinedAttrPath, drvs, ignoreAssertionFailures); + if (shouldRecurse) { + getDerivations( + state, + *elem, + joinedAttrPath, + autoArgs, + drvs, + done, + ignoreAssertionFailures + ); + } } + + return; + } else if (v.type() != nAttrs) { + state.error( + "expression does not evaluate to a derivation (or a list or set of those)" + ).debugThrow(); } - else - state.error("expression does not evaluate to a derivation (or a set or list of those)").debugThrow(); + /* Dont consider sets we've already seen, e.g. y in + `rec { x.d = derivation {...}; y = x; }`. */ + auto const &[_, didInsert] = done.insert(v.attrs); + if (!didInsert) { + return; + } + + // FIXME: what the fuck??? + /* !!! undocumented hackery to support combining channels in + nix-env.cc. */ + bool combineChannels = v.attrs->find(state.symbols.create("_combineChannels")) != v.attrs->end(); + + /* Consider the attributes in sorted order to get more + deterministic behaviour in nix-env operations (e.g. when + there are names clashes between derivations, the derivation + bound to the attribute with the "lower" name should take + precedence). */ + for (auto & attr : v.attrs->lexicographicOrder(state.symbols)) { + debug("evaluating attribute '%1%'", state.symbols[attr->name]); + // FIXME: only consider attrs with identifier-like names?? Why??? + if (!std::regex_match(std::string(state.symbols[attr->name]), attrRegex)) { + continue; + } + std::string joinedAttrPath = addToPath(pathPrefix, state.symbols[attr->name]); + if (combineChannels) { + getDerivations(state, *attr->value, joinedAttrPath, autoArgs, drvs, done, ignoreAssertionFailures); + } else if (getDerivation(state, *attr->value, joinedAttrPath, drvs, ignoreAssertionFailures)) { + /* If the value of this attribute is itself a set, + should we recurse into it? => Only if it has a + `recurseForDerivations = true' attribute. */ + if (attr->value->type() == nAttrs) { + Attr * recurseForDrvs = attr->value->attrs->get(state.sRecurseForDerivations); + if (recurseForDrvs == nullptr) { + continue; + } + bool shouldRecurse = state.forceBool( + *recurseForDrvs->value, + attr->pos, + fmt("while evaluating the '%s' attribute", Magenta("recurseForDerivations")) + ); + if (!shouldRecurse) { + continue; + } + + getDerivations( + state, + *attr->value, + joinedAttrPath, + autoArgs, + drvs, + done, + ignoreAssertionFailures + ); + } + } + } } From 4f6a3d7e9efafdcddedef0245475fdd4314fc54d Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sun, 7 Jul 2024 18:45:06 -0600 Subject: [PATCH 4/5] libexpr: include the type of the non-derivation value in the type error Change-Id: Id4717b5b0df7c09b0dbf17e642d8713a0a3efbae --- src/libexpr/get-drvs.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index fe4d3a7d5..d7869d09b 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -447,7 +447,8 @@ static void getDerivations(EvalState & state, Value & vIn, return; } else if (v.type() != nAttrs) { state.error( - "expression does not evaluate to a derivation (or a list or set of those)" + "expression was expected to be a derivation or collection of derivations, but instead was %s", + showType(v.type(), true) ).debugThrow(); } From 17d7e8870712a2742719424ffd67c64b0a1e8612 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 8 Jul 2024 10:23:38 -0600 Subject: [PATCH 5/5] remove unused headers in installable-attr-path Change-Id: Icc4747aed195e3855b128c73df82e202405af6a8 --- src/libcmd/installable-attr-path.cc | 12 ------------ src/libcmd/installable-attr-path.hh | 14 -------------- 2 files changed, 26 deletions(-) diff --git a/src/libcmd/installable-attr-path.cc b/src/libcmd/installable-attr-path.cc index eb15fecc3..9891b263c 100644 --- a/src/libcmd/installable-attr-path.cc +++ b/src/libcmd/installable-attr-path.cc @@ -1,23 +1,11 @@ -#include "globals.hh" #include "installable-attr-path.hh" #include "outputs-spec.hh" #include "command.hh" #include "attr-path.hh" #include "common-eval-args.hh" -#include "derivations.hh" -#include "eval-inline.hh" #include "eval.hh" #include "get-drvs.hh" -#include "store-api.hh" -#include "shared.hh" #include "flake/flake.hh" -#include "eval-cache.hh" -#include "url.hh" -#include "registry.hh" -#include "build-result.hh" - -#include -#include #include diff --git a/src/libcmd/installable-attr-path.hh b/src/libcmd/installable-attr-path.hh index 86c2f8219..edc9c2906 100644 --- a/src/libcmd/installable-attr-path.hh +++ b/src/libcmd/installable-attr-path.hh @@ -1,25 +1,11 @@ #pragma once ///@file -#include "globals.hh" #include "installable-value.hh" #include "outputs-spec.hh" #include "command.hh" -#include "attr-path.hh" #include "common-eval-args.hh" -#include "derivations.hh" -#include "eval-inline.hh" #include "eval.hh" -#include "get-drvs.hh" -#include "store-api.hh" -#include "shared.hh" -#include "eval-cache.hh" -#include "url.hh" -#include "registry.hh" -#include "build-result.hh" - -#include -#include #include