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

Create (experimental) outputOf primop. #8813

Merged
merged 3 commits into from
Aug 14, 2023
Merged

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 11, 2023

Motivation

In the Nix language, given a drv path, we should be able to construct another string referencing to one of its output. We can do this today with (import drvPath).output, but this only works for derivations we already have.

With dynamic derivations, however, that doesn't work well because the drvPath isn't yet built: importing it like would need to trigger IFD, when the whole point of this feature is to do "dynamic build graph" without IFD!

Instead, what we want to do is create a placeholder value with the right string context to refer to the output of the as-yet unbuilt derivation. A new primop in the language, analogous to builtins.placeholder can be used to create one. This will achieve all the right properties. The placeholder machinery also will match out the outPath attribute for CA derivations works.

Context

Probably best to review these two commits separately!

In 60b7121 we added that type of placeholder, and the derived path and string holder changes necessary to support it. Then in first commit of this PR we cleaned up the code (inspiration finally hit me!) to deduplicate the code and expose exactly. what we need. In the second commit we can wire up the primop trivally!

Part of RFC 92: dynamic derivations (tracking issue #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.

@Ericson2314 Ericson2314 requested a review from edolstra as a code owner August 11, 2023 14:05
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Aug 11, 2023
@Ericson2314 Ericson2314 marked this pull request as draft August 11, 2023 14:05
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.

Superficial review of draft.

src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
will return a placeholder for the output of the output of `myDrv`,
interpreted as a derivation.

It may help to compare to the `->` operator in C, which can also by
Copy link
Member

Choose a reason for hiding this comment

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

Why not the . operator in Nix?
I guess some distance may help, but it's not so great to assume knowledge of C.

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 removed that and referenced ^ in the CLI syntax for derived paths, which is what it actually corresponds to.

I don't think . is so good because the field in some sense "already exists", attributes in the attribute set being strict and all. but talking about ^ side-steps that issue.

src/libexpr/primops.cc Outdated Show resolved Hide resolved

For instance, `builtins.outputOf (builtins.outputOf myDrv "out) "out"`
will return a placeholder for the output of the output of `myDrv`,
interpreted as a derivation.
Copy link
Member

Choose a reason for hiding this comment

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

Might be helpful to show what the process is for building the placeholder?
ie myDrv produces a .drv, which then produces a .drv, which is then built to get the actual value for the placeholder.

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 expanded upon this, but I didn't really make this clear yet I don't think.

@Ericson2314 Ericson2314 force-pushed the outputOf branch 2 times, most recently from e1c3f03 to 368d213 Compare August 11, 2023 15:56
@Ericson2314 Ericson2314 marked this pull request as ready for review August 11, 2023 15:56
@Ericson2314 Ericson2314 changed the title Create outputOf primop. Create (experimenal) outputOf primop. Aug 11, 2023
@Ericson2314 Ericson2314 added the RFC Related to an accepted RFC label Aug 11, 2023
@Ericson2314 Ericson2314 changed the title Create (experimenal) outputOf primop. Create (experimental) outputOf primop. Aug 11, 2023
@Ericson2314 Ericson2314 mentioned this pull request Aug 11, 2023
4 tasks
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
tests/dyn-drv/text-hashed-output.nix Outdated Show resolved Hide resolved
tests/dyn-drv/text-hashed-output.nix Outdated Show resolved Hide resolved
tests/dyn-drv/eval-outputOf.sh Outdated Show resolved Hide resolved
tests/dyn-drv/eval-outputOf.sh Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the outputOf branch 2 times, most recently from 87c3451 to ecd4e6f Compare August 14, 2023 12:13
Ericson2314 and others added 2 commits August 14, 2023 08:44
This choice of variable name makes it more clear what is going on.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
`EvalState::mkSingleDerivedPathString` previously contained its own
inverse (printing, rather than parsing) in order to validate what was
parsed. Now that is pulled out into its own separate function:
`EvalState::coerceToSingleDerivedPath`.

In additional that pulled out logic is deduplicated with
`EvalState::mkOutputString` via `EvalState::mkOutputStringRaw`, which is
itself deduplicated (and generalized) with
`DownstreamPlaceholder::mkOutputStringRaw`.

All these changes make the unit tests simpler.

(We would ideally write more unit tests for `mkSingleDerivedPathString`
`coerceToSingleDerivedPath` directly, but we cannot yet do that because
the IO in reading the store path won't work when the dummy store cannot
hold anything. Someday we'll have a proper in-memory store which will
work for this.)

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@Ericson2314 Ericson2314 force-pushed the outputOf branch 2 times, most recently from 1deeb74 to 11c9d8b Compare August 14, 2023 13:04
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.

I think I've suggested some broken English before.

tests/dyn-drv/eval-outputOf.sh Outdated Show resolved Hide resolved
tests/dyn-drv/eval-outputOf.sh Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the outputOf branch 2 times, most recently from 7f24e8a to c089400 Compare August 14, 2023 13:15
@Ericson2314 Ericson2314 enabled auto-merge August 14, 2023 13:17
In the Nix language, given a drv path, we should be able to construct
another string referencing to one of its output. We can do this today
with `(import drvPath).output`, but this only works for derivations we
already have.

With dynamic derivations, however, that doesn't work well because the
`drvPath` isn't yet built: importing it like would need to trigger IFD,
when the whole point of this feature is to do "dynamic build graph"
without IFD!

Instead, what we want to do is create a placeholder value with the right
string context to refer to the output of the as-yet unbuilt derivation.
A new primop in the language, analogous to `builtins.placeholder` can be
used to create one. This will achieve all the right properties. The
placeholder machinery also will match out the `outPath` attribute for CA
derivations works.

In 60b7121 we added that type of
placeholder, and the derived path and string holder changes necessary to
support it. Then in the previous commit we cleaned up the code
(inspiration finally hit me!) to deduplicate the code and expose exactly
what we need. Now, we can wire up the primop trivally!

Part of RFC 92: dynamic derivations (tracking issue NixOS#6316)

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@roberth roberth enabled auto-merge August 14, 2023 13:38
@roberth roberth merged commit 5542c1f into NixOS:master Aug 14, 2023
@Ericson2314 Ericson2314 deleted the outputOf branch August 14, 2023 15:45
@aakropotkin
Copy link
Contributor

Jon - you rule, this sounds super useful

@fricklerhandwerk fricklerhandwerk added the feature Feature request or proposal label Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal new-cli Relating to the "nix" command RFC Related to an accepted RFC 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.

5 participants