Adapt scheduler to work with dynamic derivations by Ericson2314 · Pull Request #8829 · NixOS/nix

Ericson2314 · 2023-08-16T03:52:41Z

Motivation

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 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.

Tracking issue for RFC 92: Dynamic derivations #6316

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
documentation in the internal API docs
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

roberth

A first look.

src/libstore/build/outer-derivation-goal.hh

src/libstore/build/outer-derivation-goal.cc

tomberek · 2023-08-16T13:34:33Z

tests/dyn-drv/build-built-drv.sh

 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)


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`.

@roberth

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.

@roberth

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 <[email protected]>

@roberth

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 <[email protected]>

roberth

Superficial ~~first~~ second review

src/libstore/build/create-derivation-and-realise-goal.cc

src/libstore/build/derivation-goal.cc

src/libstore/build/create-derivation-and-realise-goal.hh

src/libstore/build/worker.cc

src/libstore/build/worker.hh

Ericson2314 · 2023-08-21T15:18:35Z

OK, I think all review items are addressed!

roberth

Looks alright - just minor clarifications.

src/libstore/derived-path-map.cc

src/libstore/derived-path-map.hh

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.

src/libstore/derived-path-map.hh

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 <[email protected]>

Ericson2314 assigned tomberek Aug 16, 2023

Ericson2314 requested a review from thufschmitt as a code owner August 16, 2023 03:52

Ericson2314 assigned roberth Aug 16, 2023

Ericson2314 requested a review from edolstra as a code owner August 16, 2023 03:52

Ericson2314 added the RFC Related to an accepted RFC label Aug 16, 2023

github-actions bot added with-tests Issues related to testing. PRs with tests have some priority store Issues and pull requests concerning the Nix store and removed RFC Related to an accepted RFC labels Aug 16, 2023

Ericson2314 added the RFC Related to an accepted RFC label Aug 16, 2023

Ericson2314 mentioned this pull request Aug 16, 2023

Tracking issue for RFC 92: Dynamic derivations #6316

Open

4 tasks

Ericson2314 changed the title ~~Adapt schedular to work with computed drvs~~ Adapt schedular to work with dynamic derivations Aug 16, 2023

Ericson2314 changed the title ~~Adapt schedular to work with dynamic derivations~~ Adapt scheduler to work with dynamic derivations Aug 16, 2023

Ericson2314 force-pushed the build-dynamic-derivations branch 2 times, most recently from 4e3db84 to a95779d Compare August 16, 2023 04:09

github-actions bot removed the store Issues and pull requests concerning the Nix store label Aug 16, 2023

Ericson2314 force-pushed the build-dynamic-derivations branch from a95779d to f396d8c Compare August 16, 2023 04:14

roberth reviewed Aug 16, 2023

View reviewed changes

src/libstore/build/outer-derivation-goal.hh Outdated Show resolved Hide resolved

src/libstore/build/outer-derivation-goal.hh Outdated Show resolved Hide resolved

tomberek reviewed Aug 16, 2023

View reviewed changes

src/libstore/build/outer-derivation-goal.cc Show resolved Hide resolved

tomberek reviewed Aug 16, 2023

View reviewed changes

Ericson2314 mentioned this pull request Aug 16, 2023

Dynamic derivations RFC 92 #4628

Merged

4 tasks

Ericson2314 force-pushed the build-dynamic-derivations branch 2 times, most recently from c355ead to 060e9c1 Compare August 16, 2023 15:51

Ericson2314 marked this pull request as draft August 16, 2023 19:08

Ericson2314 mentioned this pull request Aug 16, 2023

Refactor Raw pattern, part of #7479 #8839

Merged

8 tasks

Ericson2314 force-pushed the build-dynamic-derivations branch 2 times, most recently from 7b7416c to d77a2c2 Compare August 18, 2023 18:03

roberth reviewed Aug 19, 2023

View reviewed changes

Ericson2314 force-pushed the build-dynamic-derivations branch 3 times, most recently from ccdca62 to b174ef8 Compare August 21, 2023 14:16

Ericson2314 marked this pull request as ready for review August 21, 2023 14:38

Ericson2314 force-pushed the build-dynamic-derivations branch from b174ef8 to 2e0f461 Compare August 21, 2023 14:38

roberth reviewed Aug 25, 2023

View reviewed changes

src/libstore/derived-path-map.cc Outdated Show resolved Hide resolved

src/libstore/derived-path-map.hh Outdated Show resolved Hide resolved

src/libstore/derived-path-map.hh Show resolved Hide resolved

Ericson2314 added 3 commits August 25, 2023 09:55

Introduce OutputName and OutputNameView type aliases

2f5d3da

Hopefully they make the code easier to understand!

Throw MissingRealisation not plain Error in both `resolveDerivedP…

1c4caef

…ath` Now we are consistent with the other `resolveDerivedPath`, and other such functions.

Ericson2314 force-pushed the build-dynamic-derivations branch from bf4361f to 5c5e332 Compare August 25, 2023 13:58

roberth reviewed Aug 25, 2023

View reviewed changes

src/libstore/derived-path-map.hh Outdated Show resolved Hide resolved

Ericson2314 force-pushed the build-dynamic-derivations branch from 6ce18c5 to 5e3986f Compare August 25, 2023 14:01

roberth approved these changes Aug 25, 2023

View reviewed changes

Ericson2314 enabled auto-merge August 25, 2023 14:05

Ericson2314 merged commit 50f40ac into NixOS:master Aug 25, 2023

Ericson2314 deleted the build-dynamic-derivations branch September 25, 2023 14:57

avdv mentioned this pull request Sep 27, 2023

error: path is not valid using nix ==2.18.0 #9052

Closed

roberth mentioned this pull request Nov 20, 2023

Refactors from 5e3986f59cb #9400

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Adapt scheduler to work with dynamic derivations#8829

Adapt scheduler to work with dynamic derivations#8829
Ericson2314 merged 4 commits intoNixOS:masterfrom
obsidiansystems:build-dynamic-derivations

Ericson2314 commented Aug 16, 2023 •

edited by tomberek

Loading

Uh oh!

roberth left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomberek Aug 16, 2023

Uh oh!

roberth left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ericson2314 commented Aug 21, 2023

Uh oh!

roberth left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Comments

Conversation

Ericson2314 commented Aug 16, 2023 • edited by tomberek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Checklist for maintainers

Priorities

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomberek Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

roberth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ericson2314 commented Aug 21, 2023

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ericson2314 commented Aug 16, 2023 •

edited by tomberek

Loading

roberth left a comment •

edited

Loading