Derivations can output "text-hashed" data #3959

Ericson2314 · 2020-08-25T14:19:10Z

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.

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.
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!)
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.
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 · 2020-08-28T13:55:57Z

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 · 2020-08-28T17:29:24Z

@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 · 2020-08-28T18:53:33Z

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? 😂

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 · 2020-10-13T03:59:37Z

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

…xotic

Need to get experiment features to daemon like with the other tests.

Good to round out the library interface.

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.

In this one, we don't just output an existing derivation as is, but modify it first.

nixos-discourse · 2023-04-20T01:55:19Z

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

src/libstore/content-address.hh

Thanks so much! Co-authored-by: Adam Joseph <54836058+amjoseph-nixpkgs@users.noreply.github.com>

src/libstore/misc.cc

tests/dyn-drv/text-hashed-output.nix

tests/dyn-drv/text-hashed-output.sh

roberth · 2023-05-05T13:19:54Z

tests/dyn-drv/text-hashed-output.sh

+nix path-info $drv --derivation --json | jq
+nix path-info $out1 --derivation --json | jq
+
+test $out1 == $drv


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

:) not yet, but soon!

tests/dyn-drv/text-hashed-output.nix

tests/dyn-drv/text-hashed-output.sh

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

roberth

Hardly anything significant it seems. 🚀

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

src/libstore/content-address.cc

src/libstore/content-address.hh

src/libstore/daemon.cc

src/libstore/remote-store.cc

Didn't mean to use the private name that shouldn't be exposed.

It appears we were checking a variable in the process of definining it.

Thanks! Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

The `ContentAddressWithReferences` method is made total, with error handling now squarely the caller's job. This is better.

edolstra · 2023-05-12T10:32:47Z

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 mentioned this pull request Sep 17, 2020

Overhaul wopAddToStore #4030

Merged

Ericson2314 mentioned this pull request Oct 9, 2020

Split build.cc -- new version of #3098 #4114

Merged

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 force-pushed the ca-drv-exotic branch from 2c3be0b to a4e5de1 Compare October 13, 2020 02:15

Merge remote-tracking branch 'obsidian/path-info' into ca-drv-exotic

d8d7f50

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 mentioned this pull request Oct 13, 2020

Introduce StoreReferences and ContentAddressWithReferences #3746

Merged

7 tasks

Ericson2314 added 5 commits October 13, 2020 04:11

Work around clang destructing + capturing bug again

00c607b

Work around clang destructing + capturing bug yet again

b6b383d

Merge remote-tracking branch 'obsidian/path-info' into ca-drv-exotic

9c5de06

Cleanup tabs

47f0d7b

Merge remote-tracking branch 'obsidian/path-info' into ca-drv-exotic

4636cc9

Ericson2314 mentioned this pull request Dec 14, 2020

Trustful IPFS Store #3727

Draft

Merge remote-tracking branch 'obsidian/path-info' into ca-drv-exotic

90d76fa

Ericson2314 force-pushed the ca-drv-exotic branch from c17f78e to 90d76fa Compare February 25, 2021 21:58

Merge remote-tracking branch 'obsidian/path-info' into ca-drv-exotic

7863036

Ericson2314 mentioned this pull request Mar 10, 2021

Dynamic derivations RFC 92 #4628

Merged

4 tasks

Ericson2314 added 8 commits April 5, 2021 19:06

Merge commit 'e12308dd63f0ad27b22dcdb3da89c411eebcad2b' into ca-drv-e…

cdc9f34

…xotic

Merge commit 'd5cef6c33a051dfc672cb1e5f4739948b167315b' into ca-drv-e…

386765e

…xotic

Merge commit '1b6cf0d5f56e166a1cbbf38142375b7a92fc88f2' into ca-drv-e…

d0ed11c

…xotic

Merge branch 'path-info' into ca-drv-exotic

9af9ab4

Fix texted hash output test to work when testing daemon

d6e0c51

Need to get experiment features to daemon like with the other tests.

Merge branch 'path-info' into ca-drv-exotic

edf67e1

Merge remote-tracking branch 'upstream/master' into ca-drv-exotic

195daa8

Merge branch 'path-info' into ca-drv-exotic

9386507

Ericson2314 mentioned this pull request Mar 25, 2022

Tracking issue for RFC 92: Dynamic derivations #6316

Open

4 tasks

Ericson2314 marked this pull request as ready for review April 19, 2023 16:55

Ericson2314 added 6 commits April 19, 2023 14:13

Add a few more content addressing methods

aba8a8a

Good to round out the library interface.

Require daemon version for text hashing test

61d3e64

Make tests/dyn-drv test dir

f3a31b1

Add a more interesting test

e266627

In this one, we don't just output an existing derivation as is, but modify it first.

Fix typo in tests

969def6

ghost reviewed Apr 20, 2023

View reviewed changes

src/libstore/content-address.hh Outdated Show resolved Hide resolved

Add more docs to TextIngestionMethod

8eeaf59

Thanks so much! Co-authored-by: Adam Joseph <54836058+amjoseph-nixpkgs@users.noreply.github.com>

roberth reviewed May 5, 2023

View reviewed changes

Ericson2314 commented May 5, 2023

View reviewed changes

tests/dyn-drv/text-hashed-output.nix Outdated Show resolved Hide resolved

Ericson2314 commented May 5, 2023

View reviewed changes

tests/dyn-drv/text-hashed-output.sh Outdated Show resolved Hide resolved

Ericson2314 force-pushed the ca-drv-exotic branch 3 times, most recently from ead1411 to 2a37fef Compare May 8, 2023 11:10

Rename a few things in new tests

278c94d

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

Ericson2314 force-pushed the ca-drv-exotic branch from 2a37fef to 278c94d Compare May 8, 2023 12:02

roberth reviewed May 9, 2023

View reviewed changes

Ericson2314 and others added 6 commits May 9, 2023 12:19

Fix spurious change

35dcbe1

Didn't mean to use the private name that shouldn't be exposed.

Fix bug, newInfo -> newInfo0

6513f4f

It appears we were checking a variable in the process of definining it.

Apply suggestions from code review

d3c125e

Thanks! Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

Cleanups to content address types

753fc16

Add name to some error messages

e514b39

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.

roberth approved these changes May 10, 2023

View reviewed changes

Ericson2314 merged commit 53a1354 into NixOS:master May 10, 2023
8 checks passed

Ericson2314 deleted the ca-drv-exotic branch May 10, 2023 14:42

Ericson2314 mentioned this pull request Jul 5, 2023

Simplify ContentAddress #8650

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derivations can output "text-hashed" data #3959

Derivations can output "text-hashed" data #3959

Ericson2314 commented Aug 25, 2020 •

edited

vikanezrimaya commented Aug 28, 2020

Ericson2314 commented Aug 28, 2020 •

edited

vikanezrimaya commented Aug 28, 2020

Ericson2314 commented Oct 13, 2020

nixos-discourse commented Apr 20, 2023

roberth May 5, 2023

Ericson2314 May 5, 2023

roberth left a comment

edolstra commented May 12, 2023 •

edited

Derivations can output "text-hashed" data #3959

Derivations can output "text-hashed" data #3959

Conversation

Ericson2314 commented Aug 25, 2020 • edited

Motivation

Context

Checklist for maintainers

Priorities

vikanezrimaya commented Aug 28, 2020

Ericson2314 commented Aug 28, 2020 • edited

vikanezrimaya commented Aug 28, 2020

Ericson2314 commented Oct 13, 2020

nixos-discourse commented Apr 20, 2023

roberth May 5, 2023

Choose a reason for hiding this comment

Ericson2314 May 5, 2023

Choose a reason for hiding this comment

roberth left a comment

Choose a reason for hiding this comment

edolstra commented May 12, 2023 • edited

Ericson2314 commented Aug 25, 2020 •

edited

Ericson2314 commented Aug 28, 2020 •

edited

edolstra commented May 12, 2023 •

edited