-
-
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
Dynamic derivations RFC 92 #4628
Conversation
c681bac
to
4c74f8f
Compare
09f5348
to
e85248a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
cfe8286
to
965ad81
Compare
965ad81
to
64fab6e
Compare
These changes were taken from dynamic derivation (NixOS#4628). They somewhat undo the the refactors I first did for floating CA derivations, as the benefit of hindsight + requirements of dynamic derivations made me reconsider some things. They aren't to consequential, but I figured they might be good to land first, before the more profound changes @thufschmitt has in the works.
This changes was taken from dynamic derivation (NixOS#4628). It` somewhat undoes the refactors I first did for floating CA derivations, as the benefit of hindsight + requirements of dynamic derivations made me reconsider some things. They aren't to consequential, but I figured they might be good to land first, before the more profound changes @thufschmitt has in the works.
This changes was taken from dynamic derivation (NixOS#4628). It` somewhat undoes the refactors I first did for floating CA derivations, as the benefit of hindsight + requirements of dynamic derivations made me reconsider some things. They aren't to consequential, but I figured they might be good to land first, before the more profound changes @thufschmitt has in the works.
This changes was taken from dynamic derivation (NixOS#4628). It` somewhat undoes the refactors I first did for floating CA derivations, as the benefit of hindsight + requirements of dynamic derivations made me reconsider some things. They aren't to consequential, but I figured they might be good to land first, before the more profound changes @thufschmitt has in the works.
64fab6e
to
b0b8a91
Compare
This comment was marked as outdated.
This comment was marked as outdated.
e57531c
to
f2dc7dc
Compare
f2dc7dc
to
2bc0116
Compare
2bc0116
to
8cbc86e
Compare
042a74e
to
c6b9809
Compare
c6b9809
to
7376cad
Compare
- Don't assert: Derivation ATerms are not necessarily produced by Nix, and parsers should always throw graceful errors - Improve error message from `static void except(..)`, shows both what we expected and what we actually got. The intention is that we backport it, and then hopefully a few people might get slightly better errors if they try out new experimental drv files (for RFC 92) with an old version of Nix.
7376cad
to
4fa0603
Compare
4fa0603
to
a922a9c
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.
Minor suggestions 🚀
src/libstore/derivations.cc
Outdated
if (std::find_if( | ||
inputDrvs.map.begin(), | ||
inputDrvs.map.end(), | ||
[](auto & kv) { return !kv.second.childMap.empty(); }) |
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.
Could you add a Derivation method for this?
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 made a static function, since nothing else needs it yet. Did you just want it pulled out for legibility, or did you want something publicly in the header in particular?
src/libstore/misc.cc
Outdated
// FIXME: Someday have "deep" realizations analogous to | ||
// deep placeholders, rather than just straight-up | ||
// resolving the computed drv to a store path. |
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.
// FIXME: Someday have "deep" realizations analogous to | |
// deep placeholders, rather than just straight-up | |
// resolving the computed drv to a store path. |
This is not urgent, so best to put this in an issue and reference it from the tracking issue.
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.
// FIXME: Someday have "deep" realizations analogous to | |
// deep placeholders, rather than just straight-up | |
// resolving the computed drv to a store path. | |
// TODO deep resolutions for dynamic derivations, issue #8947, would go here. |
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.
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 left a reference, just to make this all less cryptic :). Let me know if that is OK.
7bef7a6
to
95d5ade
Compare
We use the same nested map representation we used for goals, again in order to save space. We might someday want to combine with `inputDrvs`, by doing `V = bool` instead of `V = std::set<OutputName>`, but we are not doing that yet for sake of a smaller diff. The ATerm format for Derivations also needs to be extended, in addition to the in-memory format. To accomodate this, we added a new basic versioning scheme, so old versions of Nix will get nice errors. (And going forward, if the ATerm format changes again the errors will be even better.) `parsedStrings`, an internal function used as part of parsing derivations in A-Term format, used to consume the final `]` but expect the initial `[` to already be consumed. This made for what looked like unbalanced brackets at callsites, which was confusing. Now it consumes both which is hopefully less confusing. As part of testing, we also created a unit test for the A-Term format for regular non-experimental derivations too. Co-authored-by: Robert Hensing <roberth@users.noreply.github.com> Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io> Apply suggestions from code review Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
The Derivation parser and old ATerm unfortunately leaves few ways to get nice errors when an old version of Nix encounters a new version of the format. The most likely scenario for this to occur is with a new client making a derivation that the old daemon it is communicating with cannot understand. The extensions we just created for dynamic derivation deps will add a version field, solving the problem going forward, but there is still the issue of what to do about old versions of Nix up to now. The solution here is to carefully catch the bad error from the daemon that is likely to indicate this problem, and add some extra context to it. There is another "Ugly backwards compatibility hack" in `remote-store.cc` that also works by transforming an error. Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
e2adb14
to
80d7994
Compare
🎉 All dependencies have been resolved ! |
That's an MVP for ya! You'll want to use this with 🚀 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Is there any documentation how to use dynamic derivations? |
Not enough, that is definitely next on the agenda. Some parts like the For now, I would look at the tests and imitate what they do. Hopefully that is not too cumbersome. |
It seems that dynamic derivations require content addressable store? |
Yes. Because derivations themselves are always content addressed, any derivation that produces derivations must be derivation which opts to content-address its outputs. |
See the tests in https://github.com/obsidiansystems/nix/blob/dynamic-drvs/tests/dyn-drv/ for what is being implemented.
Needs:
Depends on #4594Depends on #3959Depends on #4543Depends on #5364Depends on #7601Depends on #8353Depends on #8369Depends on #8813Depends on #8829Depends on #8927Depends on #8938