-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Make the Derived Path family of types inductive for dynamic derivations #8369
Conversation
0dd1b78
to
91cb7fd
Compare
I am kinda of nauseated by how big this commit is, but at least it is the last semi-preparatory one changing data structures without adding much by way of new features. There are just a whole bunch of data structures that need to be changed at once. |
src/libexpr/primops.cc
Outdated
// TODO dedup with copy-paste in installables.cc | ||
template<typename DP> | ||
static StorePath getStorePathFrom(const DP & derivedPath) | ||
{ | ||
return std::visit(overloaded { | ||
[&](const typename DP::Built & bfd) { | ||
return getStorePathFrom<SingleDerivedPath>(*bfd.drvPath); | ||
}, | ||
[&](const typename DP::Opaque & bo) { | ||
return bo.path; | ||
}, | ||
}, derivedPath.raw()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was skeptical of making this a method when it just occurred in installables.cc, because it is rarely the right thing and yet awfully tempting to use as the one DerivedPath -> StorePath
(no Store
, no extra IO) function. But now that we are using it twice, I guess it should be...
91cb7fd
to
88cea0e
Compare
88cea0e
to
988beed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed collaboratively in Nix maintainer meeting:
Add a release note that the JSON output format has changed, and possibly that it was never documented to begin with. We could update the release note if we add docs before the next release.
src/libstore/misc.cc
Outdated
return req; | ||
}, | ||
[&](const SingleDerivedPath::Built & bfd0) -> SingleDerivedPath { | ||
SingleDerivedPath::Built bfd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is confusing, what does it mean?
src/libstore/misc.cc
Outdated
auto & evalStore = evalStore_ ? *evalStore_ : store; | ||
|
||
return std::visit(overloaded { | ||
[&](const SingleDerivedPath::Opaque &) -> SingleDerivedPath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberth suggests: We may want to rename this to DerivablePath
as it's not derived yet, and also the constructors to Constant
and Output
.
988beed
to
d736e6d
Compare
This is a part of CA derivations that we forgot to put behind the experimental feature. This was caught by @fricklerhandwerk in NixOS#8369 (comment)
This is a part of CA derivations that we forgot to put behind the experimental feature. This was caught by @fricklerhandwerk in NixOS#8369 (comment)
🎉 All dependencies have been resolved ! |
744a8a0
to
220fcc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First part of first round.
Note to self: resume review at src/libexpr/eval.cc
ccfe59e
to
88ba8d9
Compare
This makes it more useful. In general, the derivation will be in one store, and the realisation info is in another. This also helps us avoid duplication. See how `resolveDerivedPath` is now simpler because it uses `queryPartialDerivationOutputMap`. In NixOS#8369 we get more flavors of derived path, and need more code to resolve them all, and this problem only gets worse. The fact that we need a new method to deal with the multiple dispatch is unfortunate, but this generally relates to the fact that `Store` is a sub-par interface, too bulky/unwieldy and conflating separate concerns. Solving that is out of scope of this PR. This is part of the RFC 92 work. See tracking issue NixOS#6316
88ba8d9
to
bc86ee0
Compare
🎉 All dependencies have been resolved ! |
bc86ee0
to
88d77c4
Compare
bed2acb
to
07c0d6b
Compare
|
||
GENERATE_CMP(DerivedPathOpaque, me->path); | ||
}; | ||
|
||
struct SingleDerivedPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use a high-level description and/or depiction of what the various kinds of "Paths" are, their relationship, and intended meaning.
@@ -532,12 +532,12 @@ public: | |||
StorePath coerceToStorePath(const PosIdx pos, Value & v, NixStringContext & context, std::string_view errorCtx); | |||
|
|||
/** | |||
* Part of `coerceToDerivedPath()` without any store IO which is exposed for unit testing only. | |||
* Part of `coerceToSingleDerivedPath()` without any store IO which is exposed for unit testing only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the check be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be reading the derivation to check that it in fact has the outputs requested. But do we want to improve the error message for this in this already large PR? I think the division of labor between these functions did not change, right? So improving this message seems orthogonal?
Happy to do afterwords, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#8780 made and assigned to self.
07c0d6b
to
8470751
Compare
src/libstore/store-api.hh
Outdated
* much as possible, but where the derivation resolution doesn't exists, leave | ||
* as-is. | ||
*/ | ||
SingleDerivedPath tryResolveDerivedPath(Store &, const SingleDerivedPath &, Store * evalStore = nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to wrap my head around this one, but other than that, I've completed a review of the full diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not even sure if it is needed, tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it is just used by Installables::toDerivations
(virtual method, multiple implementations). The idea is that we want to try to get the "one up" prior derivation, and fail if it is not built.
But maybe this is bad for the same reason --derivation
is bad: be explicit, and pass what you mean instead.
Either way, maybe we should just just unconditionally build the thing, and its up to you to pass some sort of --must-be-already-built
if one doesn't want that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it, and there was problem! It's in obsidiansystems@try-resolve-derived-path if we ever need it later. it is actually needed later, so it is just part of the main PR now.
This comment was marked as resolved.
This comment was marked as resolved.
245a7a1
to
dbc80df
Compare
We want to be able to write down `foo.drv^bar.drv^baz`: `foo.drv^bar.drv` is the dynamic derivation (since it is itself a derivation output, `bar.drv` from `foo.drv`). To that end, we create `Single{Derivation,BuiltPath}` types, that are very similar except instead of having multiple outputs (in a set or map), they have a single one. This is for everything to the left of the rightmost `^`. `NixStringContextElem` has an analogous change, and now can reuse `SingleDerivedPath` at the top level. In fact, if we ever get rid of `DrvDeep`, `NixStringContextElem` could be replaced with `SingleDerivedPath` entirely! Important note: some JSON formats have changed. We already can *produce* dynamic derivations, but we can't refer to them directly. Today, we can merely express building or example at the top imperatively over time by building `foo.drv^bar.drv`, and then with a second nix invocation doing `<result-from-first>^baz`, but this is not declarative. The ethos of Nix of being able to write down the full plan everything you want to do, and then execute than plan with a single command, and for that we need the new inductive form of these types. Co-authored-by: Robert Hensing <roberth@users.noreply.github.com> Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
dbc80df
to
60b7121
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed. Nothing particularly surprising; additional changes will expand the scope too much. Looking forward to the final upcoming steps that bring in the user-facing changes.
Motivation
We want to be able to write down
/nix/store/asdfasdfasdfasf-foo.drv^bar.drv^baz
:/nix/store/asdfasdfasdfasf-foo.drv^bar.drv
is the dynamic derivation (since it is itself a derivation output,bar.drv
fromfoo.drv
).To that end, we create
Single{Derivation,BuiltPath}
types, that are very similar except instead of having multiple outputs (in a set or map), they have a single one. This is for everything to the left of the rightmost^
.NixStringContextElem
has an analogous change, and now can reuseSingleDerivedPath
at the top level. In fact, if we ever get rid ofDrvDeep
,NixStringContextElem
could be replaced withSingleDerivedPath
entirely!Context
Important note: some JSON formats have changed.
We already can produce dynamic derivations, but we can't refer to them directly. Today, we can merely express building or example at the top imperatively over time by building
foo.drv^bar.drv
, and then with a second nix invocation doing<result-from-first>^baz
, but this is not declarative. The ethos of Nix of being able to write down the full plan everything you want to do, and then execute than plan with a single command, and for that we need the new inductive form of these types.Depends on #8691Depends on #8724Not in scope
builtins.outputsOf
^
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*
Priorities
Add 👍 to pull requests you find important.