Skip to content
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

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 10, 2021

See the tests in https://github.com/obsidiansystems/nix/blob/dynamic-drvs/tests/dyn-drv/ for what is being implemented.

Needs:

  • Make cooler tests
  • RFC draft
  • Unstable feature gate to prevent accidental use
  • Fix Perl bindings

Depends on #4594
Depends on #3959
Depends on #4543
Depends on #5364
Depends on #7601
Depends on #8353
Depends on #8369
Depends on #8813
Depends on #8829
Depends on #8927
Depends on #8938

@Ericson2314 Ericson2314 force-pushed the dynamic-drvs branch 3 times, most recently from c681bac to 4c74f8f Compare April 7, 2021 01:51
@Ericson2314 Ericson2314 force-pushed the dynamic-drvs branch 2 times, most recently from 09f5348 to e85248a Compare October 1, 2021 19:11
@L-as

This comment was marked as resolved.

@Ericson2314

This comment was marked as resolved.

@Ericson2314 Ericson2314 force-pushed the dynamic-drvs branch 3 times, most recently from cfe8286 to 965ad81 Compare October 9, 2021 01:45
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 11, 2022
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.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 11, 2022
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.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 11, 2022
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.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 11, 2022
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.
@stale stale bot added the stale label Sep 21, 2022
@stale stale bot removed the stale label Jan 24, 2023
@Ericson2314

This comment was marked as outdated.

@Ericson2314 Ericson2314 force-pushed the dynamic-drvs branch 3 times, most recently from e57531c to f2dc7dc Compare January 24, 2023 21:35
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 2, 2023
@Ericson2314 Ericson2314 changed the title WIP: Dynamic derivations Dynamic derivations RFC-91 Apr 2, 2023
src/libstore/tests/derivation.cc Outdated Show resolved Hide resolved
doc/manual/src/protocols/derivation-aterm.md Outdated Show resolved Hide resolved
src/libstore/build/derivation-goal.cc Outdated Show resolved Hide resolved
src/libstore/derivations.cc Show resolved Hide resolved
src/libstore/derivations.cc Outdated Show resolved Hide resolved
src/libstore/derivations.cc Outdated Show resolved Hide resolved
src/libstore/tests/derivation.cc Outdated Show resolved Hide resolved
- 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.
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Sep 7, 2023
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions 🚀

src/libcmd/built-path.cc Outdated Show resolved Hide resolved
src/libstore/build/derivation-goal.cc Outdated Show resolved Hide resolved
src/libstore/build/derivation-goal.cc Outdated Show resolved Hide resolved
Comment on lines 502 to 505
if (std::find_if(
inputDrvs.map.begin(),
inputDrvs.map.end(),
[](auto & kv) { return !kv.second.childMap.empty(); })
Copy link
Member

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?

Copy link
Member Author

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?

Comment on lines 363 to 365
// FIXME: Someday have "deep" realizations analogous to
// deep placeholders, rather than just straight-up
// resolving the computed drv to a store path.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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.

src/libstore/remote-store.cc Outdated Show resolved Hide resolved
src/libstore/misc.cc Outdated Show resolved Hide resolved
src/libstore/build/derivation-goal.cc Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the dynamic-drvs branch 3 times, most recently from 7bef7a6 to 95d5ade Compare September 7, 2023 14:37
Ericson2314 and others added 2 commits September 7, 2023 10:39
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>
@dpulls
Copy link

dpulls bot commented Sep 7, 2023

🎉 All dependencies have been resolved !

@roberth roberth merged commit e34493a into NixOS:master Sep 7, 2023
8 checks passed
@roberth
Copy link
Member

roberth commented Sep 7, 2023

That's an MVP for ya!

You'll want to use this with recursive-nix for trying this out with non-trivial builds (writing not a singular drv), but be aware that the interface may change. We want to offer a safer, more limited recursive nix functionality by default, so that a graph of drvs can be constructed, sources can be generated, etc, but that's not been built yet and may or may not use the existing daemon protocol. Again this is experimental. ymmv.

🚀

@Ericson2314 Ericson2314 deleted the dynamic-drvs branch September 7, 2023 16:17
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/rfc-92-status-update/27441/6

@domenkozar
Copy link
Member

Is there any documentation how to use dynamic derivations?

@Ericson2314
Copy link
Member Author

Not enough, that is definitely next on the agenda.

Some parts like the outputOf builtin are documented, but other parts are not (like "text" content addressing derivation outputs) because the original concept was already undocumented! And there is definitely no tutorial yet on putting all the pieces together.

For now, I would look at the tests and imitate what they do. Hopefully that is not too cumbersome.

@domenkozar
Copy link
Member

It seems that dynamic derivations require content addressable store?

@Ericson2314
Copy link
Member Author

Yes. Because derivations themselves are always content addressed, any derivation that produces derivations must be derivation which opts to content-address its outputs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command RFC Related to an accepted RFC store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants