let derivations expose .inputDrvs and .inputSrcs #5036
Conversation
|
I marked this as stale due to inactivity. → More info |
|
I'm interested in this PR, what can we do to push it forward? |
|
Derivations should by default be treated as block boxes; @roberth has done good work trying to make Nixpkgs hide this stuff more not less. At the same time though, I see that it is easier for Nix code to throw information away than recover it, so we might consider making some primop that exposes the entire derivation. |
|
@Ericson2314 to be fair, I'm not interested in the derivation internals per se, but only want the derivation inputs at eval time. This information is already exposed by This leads for example the report bundler to manually scan the derivation's attributes for derivations, which is error prone (current implementation misses strings with context), despite the info being available in the Nix evaluator. I agree that exposing the internals is unsafe / constraining. What would you think of a primop that only exposes build inputs? |
There was a problem hiding this comment.
I probably shouldn't have reviewed this before asking about the use case, although there's some interesting things in here that I think we should consider regardless of the original (sub)goal.
I suspect that your problem could be better solved by improving the string context
... as usually the dependency information can already be obtained in a derivation using exportReferencesGraph, and so usually the use case for doing it outside the derivation is to access things that aren't just paths.
With the string context, you could add attach the information you might need, such as package meta attrs, or the whole package attrset for that matter, directly to the string context (with a bit of help from an updated Nixpkgs of course; it doesn't want to tell derivation anything yet that doesn't belong in a derivation).
| @@ -4,24 +4,24 @@ | |||
| drvAttrs @ { outputs ? [ "out" ], ... }: | |||
|
|
|||
| let | |||
| strict = derivationStrict2 drvAttrs outputsMap; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| int i=0; | ||
| for (auto & p : drv.inputSrcs) { | ||
| inputSrcsVal->listElems()[i] = state.allocValue(); | ||
| mkString(*(inputSrcsVal->listElems()[i]), p, /*context=*/{"=" + p}); |
There was a problem hiding this comment.
I don't think we need to perpetuate the custom prefixing syntax. inputSrcs already covers the meaning.
There was a problem hiding this comment.
Oh don't worry, this would be a type error once rebased, anyways :)
| @@ -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' | |||
There was a problem hiding this comment.
Let's not get ahead of ourselves ;)
| 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)
There was a problem hiding this comment.
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.
| state.forceFunction(fun, pos); | ||
| mkApp(v, fun, *args[0]); | ||
| state.forceAttrs(v, pos); | ||
| } |
There was a problem hiding this comment.
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)
example on how derivations could expose .inputDrvs and .inputSrcs
partially fixes #5032