-
-
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
Adapt scheduler to work with dynamic derivations #8829
Adapt scheduler to work with dynamic derivations #8829
Conversation
4e3db84
to
a95779d
Compare
a95779d
to
f396d8c
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.
A first look.
@@ -18,4 +18,6 @@ clearStore | |||
|
|||
drvDep=$(nix-instantiate ./text-hashed-output.nix -A producingDrv) | |||
|
|||
expectStderr 1 nix build "${drvDep}^out^out" --no-link | grepQuiet "Building dynamic derivations in one shot is not yet implemented" | |||
out2=$(nix build "${drvDep}^out^out" --no-link) |
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.
😍
c355ead
to
060e9c1
Compare
See that issue for details on the general goal. The `DerivationGoal::derivationType` field had a bogus initialization, now caught, so I made it `std::optional`. I think NixOS#8829 can make it non-optional again because it will ensure we always have the derivation when we construct a `DerivationGoal`.
Types converted: - `NixStringContextElem` - `OutputsSpec` - `ExtendedOutputsSpec` - `DerivationOutput` - `DerivationType` The `DerivationGoal::derivationType` field had a bogus initialization, now caught, so I made it `std::optional`. I think NixOS#8829 can make it non-optional again because it will ensure we always have the derivation when we construct a `DerivationGoal`. See that issue (NixOS#7479) for details on the general goal. `git grep 'Raw::Raw'` indicates the two types I didn't yet convert `DerivedPath` and `BuiltPath` (and their `Single` variants) . This is because @roberth and I (can't find issue right now...) plan on reworking them somewhat, so I didn't want to churn them more just yet.
Types converted: - `NixStringContextElem` - `OutputsSpec` - `ExtendedOutputsSpec` - `DerivationOutput` - `DerivationType` The `DerivationGoal::derivationType` field had a bogus initialization, now caught, so I made it `std::optional`. I think NixOS#8829 can make it non-optional again because it will ensure we always have the derivation when we construct a `DerivationGoal`. See that issue (NixOS#7479) for details on the general goal. `git grep 'Raw::Raw'` indicates the two types I didn't yet convert `DerivedPath` and `BuiltPath` (and their `Single` variants) . This is because @roberth and I (can't find issue right now...) plan on reworking them somewhat, so I didn't want to churn them more just yet. Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Types converted: - `NixStringContextElem` - `OutputsSpec` - `ExtendedOutputsSpec` - `DerivationOutput` - `DerivationType` Existing ones mostly conforming the pattern cleaned up: - `ContentAddressMethod` - `ContentAddressWithReferences` The `DerivationGoal::derivationType` field had a bogus initialization, now caught, so I made it `std::optional`. I think NixOS#8829 can make it non-optional again because it will ensure we always have the derivation when we construct a `DerivationGoal`. See that issue (NixOS#7479) for details on the general goal. `git grep 'Raw::Raw'` indicates the two types I didn't yet convert `DerivedPath` and `BuiltPath` (and their `Single` variants) . This is because @roberth and I (can't find issue right now...) plan on reworking them somewhat, so I didn't want to churn them more just yet. Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
7b7416c
to
d77a2c2
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.
Superficial first second review
ccdca62
to
b174ef8
Compare
b174ef8
to
2e0f461
Compare
OK, I think all review items are addressed! |
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.
Looks alright - just minor clarifications.
Hopefully they make the code easier to understand!
…ath` Now we are consistent with the other `resolveDerivedPath`, and other such functions.
We're about to split up `DerivationGoal` a bit. At that point `makeDerivationGoal` will mean something more specific than it does today. (Perhaps a future rename will make this clearer.) On the other hand, the more public `Worker::makeGoal` function will continue to work exactly as before. So by moving some call sites to use that instead, we preemptively avoid issues in the next step.
bf4361f
to
5c5e332
Compare
To avoid dealing with an optional `drvPath` (because we might not know it yet) everywhere, make an `CreateDerivationAndRealiseGoal`. This goal just builds/substitutes the derivation file, and then kicks of a build for that obtained derivation; in other words it does the chaining of goals when the drv file is missing (as can already be the case) or computed (new case). This also means the `getDerivation` state can be removed from `DerivationGoal`, which makes the `BasicDerivation` / in memory case and `Derivation` / drv file file case closer together. The map type is factored out for clarity, and because we will soon hvae a second use for it (`Derivation` itself). Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
6ce18c5
to
5e3986f
Compare
Motivation
To avoid dealing with an optional
drvPath
(because we might not know it yet) everywhere, make anCreateDerivationAndRealiseGoal
. This goal just builds/substitutes the derivation file, and then kicks of a build for that obtained derivation; in other words it does the chaining of goals when the drv file is missing (as can already be the case) or computed (new case).This also means the
getDerivation
state can be removed fromDerivationGoal
, which makes theBasicDerivation
/ in memory case andDerivation
/ drv file case closer together.Context
The description is taken from the final commit. There are two preparatory ones before it.
Part of RFC 92: dynamic derivations.
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.