CA derivations that depend on other CA derivations #3958

Ericson2314 · 2020-08-24T20:05:35Z

This is tested and works, but some things should be cleaned wrt the new TODOs.

Feature-wise, the good news is that we no longer rebuild resolution-equivalent drvs for CA derivations. We resolve the derivation if it isn't already resolved, and then build that instead.

The bad news is input-addressed derivations still cannot depend on CA derivations, that will wait for later.

Finally, thanks to @regnat, as I stole a few things of stuff from #3776 :).

…HEAD

Co-authored-by: Théophane Hufschmitt <regnat@users.noreply.github.com>

thufschmitt

The bad news is input-addressed derivations still cannot depend on CA derivations, that will wait for later.

What's preventing this except the need to recompute the output paths?

thufschmitt · 2020-08-25T08:35:38Z

src/libstore/local-store.cc

+        }
+    }
+    /* can't just use else-if instead of `!haveCached` because we need to unlock
+       `drvPathResolutions` before it is locked in `Derivation::resolve`. */


Maybe a silly question, but what is preventing us from reusing resolutions rather than re-locking drvPathResolutions?

This function and queryPartialDerivationOutputMap are mutually recursive, so it would deadlock.

Maybe there should be internal variants that take a reference to the locked drvPathResolutions? Then you only need to lock it once.

tests/content-addressed.sh

src/libstore/build.cc

…loating-upstream

src/libstore/build.cc

@regnat

Thanks for catching, @regnat.

… into ca-floating-upstream

Ericson2314 · 2020-08-28T21:59:46Z

What's preventing this except the need to recompute the output paths?

Nothing major. I'd need to change the types and drv file format at least somewhat to accommodate the lack of a known output path, and I just didn't want to get into that yet until this was all good.

src/libstore/local-store.cc

…loating-upstream

…loating-upstream Tests also now fail as they should

…loating-upstream

We will sometimes try to query the outputs of derivations we can't resolve. That's fine; it just means we don't know what those outputs are yet.

…loating-upstream

If we resolve using the known path of a derivation whose output we didn't have, we previously blew up. Now we just fail gracefully, returning the map of all outputs unknown.

so we can link outputs to deriver and thus properly cache.

Ericson2314 · 2020-09-16T18:57:25Z

OK, I think this is ready. Everything should work except for remote building, which will require changes to the build hook protocol to fix. I think that's best dealt with in another PR.

The CI failure is that spurious impure fixed output derviation one that keeps on cropping up from time to time, ugh.

thufschmitt · 2020-09-17T11:51:46Z

I tried bootsraping firefox with this branch (as I had done for #3883), but I ran into a lot of (apparently random) failures.
These seem to appear randomly (and non-deterministically) so I haven't been able to put a proper repro yet (nor confirm that it's specifically due to this branch and not something occuring on master).

The errors are

opening file '/proc/24534/uid_map': Permission denied (I didn't double-check, but I guess that the pid changes between two errors)
creating pipe: Too many open files
opening directory '/tmp/nix-build-foo-0.0.drv-0': Bad file descriptor followed by opening directory '/home/regnat/test-store/nix/store/xxx-foo-0.0.drv.chroot/bin': Bad file descriptor followed by creating pipe: Too many open files

thufschmitt · 2020-09-17T13:52:37Z

nor confirm that it's specifically due to this branch and not something occuring on master

It's still building the world, but nothing crashed so far on master, so it's likely to be specific to this branch

Ericson2314 · 2020-09-17T16:13:53Z

@regnat huh, weird. Are those happening when building input-addressed derivations too?

thufschmitt · 2020-09-18T07:53:54Z

I tried again tonight and this morning, and I actually hit twice the same kind of error on master.

error (ignored): error: --- SysError --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- nix-build
opening directory '/home/regnat/test-store-master/nix/store/iyfs2mp14qq1rzpcm5xvn101af5ih17p-libcanberra-0.30.tar.xz.drv.chroot/var/run': Bad file descriptor
error: --- SysError --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- nix-build
creating pipe: Too many open files

It seems to be less frequent than what I had yesterday, but might just be an accident − I'm using a shared machine for that, maybe it had a higher load yesterday or something like that which caused it to trigger this more often.

Ericson2314 · 2020-09-18T19:19:49Z

@regnat Just looking at the code, I had no idea what could go wrong/differently in the non-floating-CA case, so I'm relieved you could reproduce without this branch!

Anything else need changing here then? With the latest pushes I have Nix writing down resolved trees for sake of caching (resolving means no super-linear storage overhead, and no new network overhead, thankfully) but we can always revisit these things later. I think we're ready!

src/libstore/derivations.hh

src/libstore/local-store.cc

Ericson2314 · 2020-09-24T14:04:08Z

#4056 (comment) I take it this means we agree this is ready.

edolstra · 2020-09-25T15:58:39Z

src/libstore/derivations.cc

@@ -644,4 +644,57 @@ std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath
    return "/" + hashString(htSHA256, clearText).to_string(Base32, false);
 }

+
+// N.B. Outputs are left unchanged
+static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites) {


Suggested change

static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites) {

static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites)

{

Style nitpick.

Ericson2314 · 2020-09-25T15:59:26Z

src/libstore/build.cc

+            assert(attempt);
+            Derivation drvResolved { *std::move(attempt) };
+
+            auto pathResolved = writeDerivation(worker.store, drvResolved);


Note we are writing down the resolved derivation so we can link the deriver to the CA output in the sqlite db.

Do we need that? It doubles the number of .drv files on disk, while we'd like to get rid of them eventually...

Well it only doubles them for floating CA derivations. Not doing this means changing the SQL schema, and I think there's a few such change's will end up wanting to make, so I rather save that for later to think about them all at once.

edolstra · 2020-09-28T13:47:14Z

src/libstore/build.cc

@@ -1906,6 +1941,9 @@ void DerivationGoal::buildDone()
    done(BuildResult::Built);
 }

+void DerivationGoal::resolvedFinished() {


This function seems a bit unnecessary.

Well many derivation goals could resolve to the same derivation, Trying to make the resolve derivations track all those to call done on seems wrong to me.

src/libstore/build.cc

src/libstore/derivations.hh

Ericson2314 and others added 4 commits August 23, 2020 15:11

Merge remote-tracking branch 'obsidian/write-derivation-borrow' into …

b0b59fd

…HEAD

Fix bad debug format string

59979e7

Add constructor for BasicDerivation -> Derivation

e0b0e18

CA derivations that depend on other CA derivations

8eb73a8

Co-authored-by: Théophane Hufschmitt <regnat@users.noreply.github.com>

thufschmitt reviewed Aug 25, 2020

View reviewed changes

Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…

48e8828

…loating-upstream

Ericson2314 commented Aug 28, 2020

View reviewed changes

src/libstore/build.cc Outdated Show resolved Hide resolved

Ericson2314 added 2 commits August 28, 2020 17:22

Update src/libstore/build.cc

421ed52

Thanks for catching, @regnat.

Merge branch 'ca-floating-upstream' of github.com:obsidiansystems/nix…

02e0001

… into ca-floating-upstream

Test CA derivation input caching

4db0010

Ericson2314 mentioned this pull request Sep 1, 2020

Can build single CA derivations #3883

Merged

Ericson2314 commented Sep 2, 2020

View reviewed changes

src/libstore/local-store.cc Outdated Show resolved Hide resolved

Ericson2314 added 5 commits September 3, 2020 22:09

Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…

975a47f

…loating-upstream

Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…

b836662

…loating-upstream Tests also now fail as they should

Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…

4409530

…loating-upstream

Fix floating CA tests

aad4abc

We will sometimes try to query the outputs of derivations we can't resolve. That's fine; it just means we don't know what those outputs are yet.

Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…

075d399

…loating-upstream

Ericson2314 changed the title ~~Semi-WIP: CA derivations that depend on other CA derivations --- contains #3883~~ WIP: CA derivations that depend on other CA derivations --- contains #3883 Sep 4, 2020

Fix querying outputs for CA derivations some more

98dfd75

If we resolve using the known path of a derivation whose output we didn't have, we previously blew up. Now we just fail gracefully, returning the map of all outputs unknown.

Ericson2314 changed the title ~~WIP: CA derivations that depend on other CA derivations --- contains #3883~~ CA derivations that depend on other CA derivations --- contains #3883 Sep 4, 2020

Ericson2314 and others added 3 commits September 16, 2020 17:50

Merge remote-tracking branch 'upstream/master' into ca-floating-upstream

c5cceba

Start to fix floating CA + remote building

7fdbb37

Ensure resolved CA derivations are written

2741fff

so we can link outputs to deriver and thus properly cache.

Ericson2314 changed the title ~~CA derivations that depend on other CA derivations --- contains #3883~~ CA derivations that depend on other CA derivations Sep 16, 2020

Merge remote-tracking branch 'upstream/master' into ca-floating-upstream

b7df353

Merge remote-tracking branch 'upstream/master' into ca-floating-upstream

3786a80

thufschmitt reviewed Sep 22, 2020

View reviewed changes

src/libstore/derivations.hh Show resolved Hide resolved

Ericson2314 commented Sep 22, 2020

View reviewed changes

src/libstore/local-store.cc Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into non-ca-depending-on-ca

67cc356

thufschmitt mentioned this pull request Sep 23, 2020

Allow non-CA derivations to depend on CA ones #4056

Merged

edolstra reviewed Sep 25, 2020

View reviewed changes

Ericson2314 commented Sep 25, 2020

View reviewed changes

edolstra reviewed Sep 28, 2020

View reviewed changes

src/libstore/build.cc Outdated Show resolved Hide resolved

edolstra reviewed Sep 28, 2020

View reviewed changes

src/libstore/derivations.hh Outdated Show resolved Hide resolved

edolstra approved these changes Sep 28, 2020

View reviewed changes

Ericson2314 added 3 commits September 28, 2020 15:39

Merge remote-tracking branch 'upstream/master' into ca-floating-upstream

10202bb

Use drvPath2 and give it a better name

80e335b

Clarify comment a bit

00135e1

edolstra merged commit 1e8855a into NixOS:master Sep 29, 2020

Ericson2314 deleted the ca-floating-upstream branch September 29, 2020 15:21

Ericson2314 mentioned this pull request Sep 30, 2020

Floating content-addressed derivations #4087

Open

9 tasks

thufschmitt added the ca-derivations Derivations with content addressed outputs label Nov 16, 2020

thufschmitt added this to the ca-derivations-mvp milestone Dec 11, 2020

ldesgoui mentioned this pull request Aug 1, 2021

Write Nix 2.4 release notes #4521

Closed

aciceri mentioned this pull request May 12, 2022

Bad file descriptor when building content addressed derivation #6516

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CA derivations that depend on other CA derivations #3958

CA derivations that depend on other CA derivations #3958

Ericson2314 commented Aug 24, 2020

thufschmitt left a comment

thufschmitt Aug 25, 2020

Ericson2314 Aug 28, 2020

edolstra Sep 28, 2020

Ericson2314 commented Aug 28, 2020

Ericson2314 commented Sep 16, 2020 •

edited

thufschmitt commented Sep 17, 2020

thufschmitt commented Sep 17, 2020

Ericson2314 commented Sep 17, 2020

thufschmitt commented Sep 18, 2020

Ericson2314 commented Sep 18, 2020 •

edited

Ericson2314 commented Sep 24, 2020

edolstra Sep 25, 2020

Ericson2314 Sep 25, 2020

edolstra Sep 28, 2020

Ericson2314 Sep 28, 2020

edolstra Sep 28, 2020

Ericson2314 Sep 28, 2020

	static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites) {
	static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites)
	{

CA derivations that depend on other CA derivations #3958

CA derivations that depend on other CA derivations #3958

Conversation

Ericson2314 commented Aug 24, 2020

thufschmitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Aug 28, 2020

Ericson2314 commented Sep 16, 2020 • edited

thufschmitt commented Sep 17, 2020

thufschmitt commented Sep 17, 2020

Ericson2314 commented Sep 17, 2020

thufschmitt commented Sep 18, 2020

Ericson2314 commented Sep 18, 2020 • edited

Ericson2314 commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Sep 16, 2020 •

edited

Ericson2314 commented Sep 18, 2020 •

edited