Skip to content

Comments

Derivations can output "text-hashed" data#3959

Merged
Ericson2314 merged 54 commits intoNixOS:masterfrom
obsidiansystems:ca-drv-exotic
May 10, 2023
Merged

Derivations can output "text-hashed" data#3959
Ericson2314 merged 54 commits intoNixOS:masterfrom
obsidiansystems:ca-drv-exotic

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 25, 2020 •
edited
Loading

Motivation

We're finally getting to RFC 92 features proper!

Derivations are currently serialized to store objects (and also thus given drv paths, even when we just stick with in-memory derivations) using "text hashing" / "text file ingestion method", a type of content addressing. That means, if we want to compute derivations we need to use

  • content-addressed derivations (already have!)
  • with derivations outputs using text ingestion method (don't yet have!)

The latter part is what we are adding.

Context

A few things needed to make this go.

  1. A little cleanup of ContentAddressMethod building on Introduce StoreReferences and ContentAddressWithReferences #3746. The two variants are now TextIngestionMethod and FileIngestionMethod, which together are tantamount to a 3-way enum:

    • flat ingestion
    • Nix archive ingestion ("recursive")
    • text ingestion

    This data type no longer species any hash type, but is just the way of ingestion data. A nice single purpose now.

  2. TextHashMethod -> TextIngestionMethod, since it no longer implies SHA-256 or any other hash type (or hashing at all!), though out of an abundance of caution we disallow trying to use any other hash type with it. (We're separating concerns but not supporting all the options yet!)

  3. FixedOutputHashMethod is deleted. It was a type for a (hash type, flat | recursive) pair, but we simply don't need that anymore. Everything supports text hashing too now.

  4. The new stuff is gated under a new dynamic-derivations feature. I imagine I'll just put all the constituent features that make up dynamic derivations under this, as they are meant to be used together.

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.

In particular, this means that derivations can output derivations. But that ramification isn't (yet!) useful as we would want, since there is no way to have a dependent derivation that is itself a dependent derivation.

depends on #3746

@vikanezrimaya
Copy link
Member

vikanezrimaya commented Aug 28, 2020

I'd like to learn what are the possible use-cases once this feature becomes truly useful and complete (meaning that the drawback you mentioned earlier is solved). This feels very confusing to me as a casual Nix user

@Ericson2314
Copy link
Member Author

Ericson2314 commented Aug 28, 2020 •
edited
Loading

@kisik21 the use-case is instead of using IFD, we can build derivations produced by derivations. For example, we could do nix-instantiate $(cabal2nix ..) > $out in a derivation-producing derivation.

@vikanezrimaya
Copy link
Member

vikanezrimaya commented Aug 28, 2020

Oh, I see! So the extra step of running node2nix or whatever gets used to package up node packages, or something else, is not needed with derivation-ception that's being made possible here.

Or should we call it Nix-ception? 😂

@Ericson2314 Ericson2314 mentioned this pull request Sep 17, 2020
Merged
@Ericson2314 Ericson2314 mentioned this pull request Oct 9, 2020
Merged
@Ericson2314
Derivations can output "text-hashed" data
a4e5de1 
In particular, this means that derivations can output derivations. But
that ramification isn't (yet!) useful as we would want, since there is
no way to have a dependent derivation that is itself a dependent
derivation.
@Ericson2314 Ericson2314 changed the title WIP: Derivations can output "text-hashed" data --- contains many other PRs Derivations can output "text-hashed" data --- contains #3746 Oct 13, 2020
@Ericson2314
Copy link
Member Author

Ericson2314 commented Oct 13, 2020

This should pass, and is rebased to now only contain one other PR.

@Ericson2314 Ericson2314 mentioned this pull request Oct 13, 2020
Merged
7 tasks
@Ericson2314 Ericson2314 mentioned this pull request Dec 14, 2020
Draft
@Ericson2314 Ericson2314 mentioned this pull request Mar 10, 2021
Merged
4 tasks
@Ericson2314 Ericson2314 mentioned this pull request Mar 25, 2022
Open
4 tasks
@Ericson2314
Add a few more content addressing methods
aba8a8a 
Good to round out the library interface.
@Ericson2314
Remove references from fixed output derivation ab syntax
7103c6d 
In other words, use a plain `ContentAddress` not
`ContentAddressWithReferences` for `DerivationOutput::CAFixed`.

Supporting fixed output derivations with (fixed) references would be a
cool feature, but it is out of scope at this moment.
@Ericson2314
Require daemon version for text hashing test
61d3e64
@Ericson2314
Make tests/dyn-drv test dir
f3a31b1
@Ericson2314
Add a more interesting test
e266627 
In this one, we don't just output an existing derivation as is, but
modify it first.
@Ericson2314
Fix typo in tests
969def6
@nixos-discourse
Copy link

nixos-discourse commented Apr 20, 2023

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/1

ghost
ghost reviewed Apr 20, 2023
View reviewed changes
@Ericson2314
Add more docs to TextIngestionMethod
8eeaf59 
Thanks so much!

Co-authored-by: Adam Joseph <[email protected]>
roberth
roberth reviewed May 5, 2023
View reviewed changes
tests/dyn-drv/text-hashed-output.sh
nix path-info $drv --derivation --json | jq
nix path-info $out1 --derivation --json | jq

test $out1 == $drv
Copy link
Member

@roberth roberth May 5, 2023

Choose a reason for hiding this comment

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

I don't suppose we can do nix build $drvProducingDrv^out^out yet?

Copy link
Member Author

@Ericson2314 Ericson2314 May 5, 2023

Choose a reason for hiding this comment

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

:) not yet, but soon!

Ericson2314
Ericson2314 commented May 5, 2023
View reviewed changes
Ericson2314
Ericson2314 commented May 5, 2023
View reviewed changes
@Ericson2314 Ericson2314 force-pushed the ca-drv-exotic branch 3 times, most recently from ead1411 to 2a37fef Compare May 8, 2023 11:10
roberth
roberth reviewed May 9, 2023
View reviewed changes
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.

Hardly anything significant it seems. 🚀

Ericson2314 and others added 6 commits May 9, 2023 12:19
@Ericson2314
Fix spurious change
35dcbe1 
Didn't mean to use the private name that shouldn't be exposed.
@Ericson2314
Fix bug, newInfo -> newInfo0
6513f4f 
It appears we were checking a variable in the process of definining it.
@Ericson2314 @roberth
Apply suggestions from code review
d3c125e 
Thanks!

Co-authored-by: Robert Hensing <[email protected]>
@Ericson2314
Cleanups to content address types
753fc16
@Ericson2314
Add name to some error messages
e514b39
@Ericson2314
Improve error message for self reference with text hashing
6a3a87a 
The `ContentAddressWithReferences` method is made total, with error
handling now squarely the caller's job. This is better.
@Ericson2314 Ericson2314 merged commit 53a1354 into NixOS:master May 10, 2023
@Ericson2314 Ericson2314 deleted the ca-drv-exotic branch May 10, 2023 14:42
@edolstra
Copy link
Member

edolstra commented May 12, 2023 •
edited
Loading

This causes a warning with gcc:

src/libstore/content-address.cc: In static member function ‘static nix::ContentAddressMethod nix::ContentAddressMethod::parsePrefix(std::string_view&)’:
src/libstore/content-address.cc:41:16: warning: ‘method’ may be used uninitialized [-Wmaybe-uninitialized]
   41 |         method = TextIngestionMethod {};
      |         ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
src/libstore/content-address.cc:37:26: note: ‘method’ declared here
   37 |     ContentAddressMethod method = FileIngestionMethod::Flat;
      |                          ^~~~~~

Any idea what causes that?

@Ericson2314 Ericson2314 mentioned this pull request Jul 5, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

2 more reviewers

@roberth roberth roberth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Nix team
Archived in project

Milestone

dynamic-derivations stabilisation

Development

Successfully merging this pull request may close these issues.

7 participants

@Ericson2314 @vikanezrimaya @L-as @anka-213 @nixos-discourse @edolstra @roberth