Parse deriving paths in DerivationOptions#14506
Conversation
github-actions
bot
added
new-cli
Ericson2314
force-pushed
the
derivation-options-parse-paths
branch
from
b126d79 to
00d2bf9
Compare
|
Ah it needs a daemon-version thing for that message. |
| bool ignoreSelfRefs = false; | ||
| std::optional<uint64_t> maxSize, maxClosureSize; | ||
|
|
||
| using DrvRef = nix::DrvRef<Input>; |
There was a problem hiding this comment.
This is already giving me a headache... What is a DrvRef conceptually, and since when do output checks refer to anything other than store paths?
There was a problem hiding this comment.
The underlying DrvRef is defined in another header with lots of Doxygen.
It is OutputName | Input, which is what each element of the alllowed/disallowed references/requisites is already is, semantically. (Input = SingleDerivedPath in general, and StorePath for a resolved derivation.)
| * separately. That would be nice to separate concerns, and not make any | ||
| * environment variable names magical. | ||
| */ | ||
| template<typename Input> |
There was a problem hiding this comment.
Input is a heavily overloaded term. What's the intended meaning of input here?
There was a problem hiding this comment.
An input in the derivation -- a member of the set inputSrcs ∪ inputDrvs.
The manual document inputs to match this. The JSON format also has inputs.srcs and inputs.drvs (nested attributes) to remind the human reader that they are "two sides of the same coin".
|
|
||
| /** | ||
| * A reference is either to a to-be-registered output (by name), | ||
| * or to an already-registered store object (by `Input`). |
There was a problem hiding this comment.
Shouldn't Input be named StorePath then? And why does that need to be a template parameter?
There was a problem hiding this comment.
For an unresolved derivation it is SingleDerivedPath, because of upstream CA derivations we don't know their store paths yet. (And opaque placeholders are a hack we use in on-disk formats but should not use in in-memory formats.)
|
🎉 All dependencies have been resolved ! |
Ericson2314
force-pushed
the
derivation-options-parse-paths
branch
2 times, most recently
from
4611d31 to
81921d7
Compare
| bool shouldWarn = true, | ||
| const ExperimentalFeatureSettings & mockXpSettings = experimentalFeatureSettings); | ||
|
|
||
| std::optional<DerivationOptions<StorePath>> tryResolve( |
There was a problem hiding this comment.
TODO add comment to this
Ericson2314
force-pushed
the
derivation-options-parse-paths
branch
2 times, most recently
from
5f47f81 to
3e0b19a
Compare
This is an example of "Parse, don't validate" principle [1]. Before, we had a number of `StringSet`s in `DerivationOptions` that were not *actually* allowed to be arbitrary sets of strings. Instead, each set member had to be one of: - a store path - a CA "downstream placeholder" - an output name Only later, in the code that checks outputs, would these strings be further parsed to match these cases. (Actually, only 2 by that point, because the placeholders must be rewritten away by then.) Now, we fully parse everything up front, and have an "honest" data type that reflects these invariants: - store paths are parsed, stored as (opaque) deriving paths - CA "downstream placeholders" are rewritten to the output deriving paths they denote - output names are the only arbitrary strings left Since the first two cases both become deriving paths, that leaves us with a `std::variant<SingleDerivedPath, String>` data type, which we use in our sets instead. Getting rid of placeholders is especially nice because we are replacing them with something much more internally-structured / transparent. [1]: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/ Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo> Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Ericson2314
force-pushed
the
derivation-options-parse-paths
branch
from
3f44c20 to
76bd600
Compare
3 participants
Motivation
This is an example of "Parse, don't validate" principle.
Before, we had a number of
StringSets inDerivationOptionsthat were not actually allowed to be arbitrary sets of strings. Instead, each set member had to be one of:a store path
a CA "downstream placeholder"
an output name
Only later, in the code that checks outputs, would these strings be further parsed to match these cases. (Actually, only 2 by that point, because the placeholders must be rewritten away by then.)
Now, we fully parse everything up front, and have an "honest" data type that reflects these invariants:
store paths are parsed, stored as (opaque) deriving paths
CA "downstream placeholders" are rewritten to the output deriving
paths they denote
output names are the only arbitrary strings left
Since the first two cases both become deriving paths, that leaves us with a
std::variant<SingleDerivedPath, String>data type, which we use in our sets instead.Getting rid of placeholders is especially nice because we are replacing them with something much more internally-structured / transparent.
Context
Progress on #13570
Depends on #14505
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.