Skip to content
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

let derivations expose .inputDrvs and .inputSrcs #5036

Draft
wants to merge 4 commits into
base: 2.3-maintenance
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions corepkgs/derivation.nix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing builtins.derivation is not ok. It is so widely used at this point that I would consider the addition of an attribute a breaking change, unless it is guarded by a flag attribute, but that doesn't seem necessary if we're introducing a new builtin derivationStrict2.
Here's how we could use derivationStrict or derivationStrict2 in Nixpkgs, taking advantage of the cleaner return attrset as well, optionally.

Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,24 @@
drvAttrs @ { outputs ? [ "out" ], ... }:

let
strict = derivationStrict2 drvAttrs outputsMap;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a derivationStrict2 is ok, but we might want to use the opportunity to make a few other improvements while we introduce it. What comes to mind is pre-applying unsafeDiscardOutputDependency to drvPath, but there could be others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, good idea!


strict = derivationStrict drvAttrs;

commonAttrs = drvAttrs // (builtins.listToAttrs outputsList) //
{ all = map (x: x.value) outputsList;
commonAttrs = drvAttrs // outputsMap //
{ all = builtins.attrValues outputsMap;
inherit drvAttrs;
};

outputToAttrListElement = outputName:
{ name = outputName;
value = commonAttrs // {
outPath = builtins.getAttr outputName strict;
drvPath = strict.drvPath;
type = "derivation";
inherit outputName;
};
};
outputsMap = builtins.genAttrs outputs (outputName:
commonAttrs // {
outPath = strict.${outputName};
drvPath = strict.drvPath;
inputSrcs = strict.inputSrcs;
inputDrvs = strict.inputDrvs;
type = "derivation";
inherit outputName;
}
);

outputsList = map outputToAttrListElement outputs;
in
outputsMap.${builtins.head outputs}

in (builtins.head outputsList).value
1 change: 1 addition & 0 deletions src/libexpr/eval.hh
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ private:
std::unordered_map<Path, Path> resolvedPaths;

public:
FileEvalCache derivationCache;

EvalState(const Strings & _searchPath, ref<Store> store);
~EvalState();
Expand Down
88 changes: 80 additions & 8 deletions src/libexpr/primops.cc
Original file line number Diff line number Diff line change
Expand Up @@ -519,14 +519,27 @@ void prim_valueSize(EvalState & state, const Pos & pos, Value * * args, Value &
*************************************************************/


/* Add a wrapper around the derivation primop that computes the
`drvPath', `outPath', `inputSrcs' and `inputDrvs' attributes lazily. */
static void prim_derivation(EvalState & state, const Pos & pos, Value * * args, Value & v)
{
Value fun;
state.evalFile(state.sDerivationNix, fun);
state.forceFunction(fun, pos);
mkApp(v, fun, *args[0]);
state.forceAttrs(v, pos);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this wrapper is a little silly from a mechanical perspective, if we register it with a docstring that would solve a little UX problem that we have. (Could be an independent PR as well)



/* 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 Pos & pos, Value * * args, Value & v)
derivation; `inputSrcs' and `inputDrvs' containing the build-time
dependencies. */
static void prim_derivationStrict2(EvalState & state, const Pos & pos, Value * * args, Value & v)
{
state.forceAttrs(*args[0], pos);

Expand Down Expand Up @@ -764,13 +777,49 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * *
case we don't actually write store derivations, so we can't
read them later. */
drvHashes[drvPath] = hashDerivationModulo(*state.store, drv);
state.derivationCache[drvPath] = *args[1];

state.mkAttrs(v, 1 + drv.outputs.size());
state.mkAttrs(v, 1 + drv.outputs.size() + 2);
mkString(*state.allocAttr(v, state.sDrvPath), drvPath, {"=" + drvPath});
for (auto & i : drv.outputs) {
mkString(*state.allocAttr(v, state.symbols.create(i.first)),
i.second.path, {"!" + i.first + "!" + drvPath});
}

{
Value * inputSrcsVal = state.allocAttr(v, state.symbols.create("inputSrcs"));
state.mkList(*inputSrcsVal, drv.inputSrcs.size());
int i=0;
for (auto & p : drv.inputSrcs) {
inputSrcsVal->listElems()[i] = state.allocValue();
mkString(*(inputSrcsVal->listElems()[i]), p, /*context=*/{"=" + p});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to perpetuate the custom prefixing syntax. inputSrcs already covers the meaning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh don't worry, this would be a type error once rebased, anyways :)

i++;
}
}
{
Value * inputDrvsVal = state.allocAttr(v, state.symbols.create("inputDrvs"));

int size=0;
for (auto & d : drv.inputDrvs)
for (auto & outputName : d.second)
size++;
state.mkList(*inputDrvsVal, size);

int i=0;
for (auto & d : drv.inputDrvs) {
auto j = state.derivationCache.find(d.first);
if (j == state.derivationCache.end()) {
throw TypeError(format("derivation '%1%' is not cached, at %2%") % d.first % pos);

for (auto & outputName : d.second) {
Bindings::iterator k = j->second.attrs->find(state.symbols.create(outputName));
if (k == v.attrs->end())
throw TypeError(format("derivation '%1%' is missing output '.%2%', at %3%") % d.first % outputName % pos);
inputDrvsVal->listElems()[i] = k->value;
i++;
}
}
}
v.attrs->sort();
}

Expand Down Expand Up @@ -1387,6 +1436,31 @@ static void prim_mapAttrs(EvalState & state, const Pos & pos, Value * * args, Va
}


/* Given a set of attribute names, return the set of the corresponding
attributes from the given set. Former `lib.genAttrs'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not get ahead of ourselves ;)

Suggested change
attributes from the given set. Former `lib.genAttrs'
attributes from the given set. Implements Nixpkgs `lib.genAttrs'

genAttrs might be a good candidate for a primop regardless of your original goal. As currently implemented it allocates an unnecessary list, and 2-attr attrsets for each attribute name. I don't know what would be the impact of this optimization though. (TODO: register it with a documentation string like most of the other primops, add tests)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also argue that builtins is explicitly not a replacement for lib, because the builtins can't be changed. The analogy between kernel syscalls and a standard library (like libc or go) is fitting.

*/
static void prim_genAttrs(EvalState & state, const Pos & pos, Value * * args, Value & v)
{
state.forceList(*args[0], pos);

state.mkAttrs(v, args[0]->listSize());

std::set<Symbol> seen;

for (unsigned int i = 0; i < args[0]->listSize(); ++i) {
Value & vName(*args[0]->listElems()[i]);
string name = state.forceStringNoCtx(vName, pos);

Symbol sym = state.symbols.create(name);
if (seen.find(sym) == seen.end()) {
seen.insert(sym);
mkApp(*state.allocAttr(v, sym), *args[1], vName);
}
}

v.attrs->sort();
}


/*************************************************************
* Lists
Expand Down Expand Up @@ -2244,6 +2318,7 @@ void EvalState::createBaseEnv()
addPrimOp("__hasAttr", 2, prim_hasAttr);
addPrimOp("__isAttrs", 1, prim_isAttrs);
addPrimOp("removeAttrs", 2, prim_removeAttrs);
addPrimOp("__genAttrs", 2, prim_genAttrs);
addPrimOp("__listToAttrs", 1, prim_listToAttrs);
addPrimOp("__intersectAttrs", 2, prim_intersectAttrs);
addPrimOp("__catAttrs", 2, prim_catAttrs);
Expand Down Expand Up @@ -2294,19 +2369,16 @@ void EvalState::createBaseEnv()
addPrimOp("__splitVersion", 1, prim_splitVersion);

// Derivations
addPrimOp("derivationStrict", 1, prim_derivationStrict);
addPrimOp("derivationStrict2", 2, prim_derivationStrict2);
addPrimOp("placeholder", 1, prim_placeholder);
addPrimOp("derivation", 1, prim_derivation);

// Networking
addPrimOp("__fetchurl", 1, prim_fetchurl);
addPrimOp("fetchTarball", 1, prim_fetchTarball);

/* Add a wrapper around the derivation primop that computes the
`drvPath' and `outPath' attributes lazily. */
string path = canonPath(settings.nixDataDir + "/nix/corepkgs/derivation.nix", true);
sDerivationNix = symbols.create(path);
evalFile(path, v);
addConstant("derivation", v);

/* Add a value containing the current Nix expression search path. */
mkList(v, searchPath.size());
Expand Down