Allow non-CA derivations to depend on CA ones by thufschmitt · Pull Request #4056 · NixOS/nix

thufschmitt · 2020-09-23T14:38:53Z

Builds on top of #3958

There's a few things that I'm not really happy with wrt. the clarity of the code, but it would require a bit of refactoring so I'll probably address that in a subsequent PR in order not to delay this one

Ericson2314 · 2020-09-23T14:59:18Z

@regnat So everything is the same but the commit at the end? Are you opposed to merging #3958 so it's easier to just review those? (Correct me if I am wrong, but this is mainly about adding the missing feature and doesn't change the way #3958 worked for the CA -> CA case too much?)

Ericson2314

Nice to see this implemented! :) This all looks actually pretty clean to me; I'm OK with saving whatever you had in mind for a future PR too.

src/libstore/derivations.cc

Ericson2314 · 2020-09-23T15:00:30Z

src/libstore/derivations.cc

+    case DerivationType::DeferredInputAddressed:
+        break;


I think we need to set hasUnknownHash = true here or something so that DeferredInputAddressed cascades to downstream input-addressed derivations?

I'm not sure. The reasoning behind the current code is that that way we can call hashDerivationModulo on a derivation that's advertising as DeferredIA but actually has a statically known hash. It's used in rewriteDerivation to convert the DeferredIA into an IA.
And if the derivation is indeed Deferred, then at least one recursive call to hashDerivationModulo will return UnknownHash{} so we'll get the same result

Ah OK. That makes total sense. Then I'd say just one thing we might do is add a flag to hashDerivationModulo about whether it should try to un-defer DeferredIA derivations: in build.cc it definitely should (just like it does now), but I think in primops.cc it shouldn't because that be impure, leaking what derivations we've already built and which CAs we got from them, into eval itself.

that be impure, leaking what derivations we've already built and which CAs we got from them, into eval itself.

I don't think it does, hashDerivationModulo only looks at the derivations themselves. So if we depend on a floating-ca derivation it will always return UnknownHash{}, regardless of whether it has been built or not. The reason why we have a different return in build.cc is that we call it on the resolved derivation which is still marked as Deferred (because we haven't rebuilt its inputs yet) but doesn't depend on any floating-ca derivation anymore.

Ah right, and it's only drvPathResolutions that we insert into by hand in build.cc, whereas drvHashes is just manually inserted into in primops.cc, where as you say there is nothing intrinsically impure going on. Sorry for the noise then! It's tricky but ends up working out great :).

(I'm not sure why, but it's not showing me the button to resolve the thread I started. I would if I could!)

Wait, one thing is that input-addressed derivations conventionally keep their their input-addressed input drvs. But if we rewrite then hashDerivationModulo, they won't have any input drvs, even if those input drvs were input-addressed.

This isn't too bad, in fact this is how we "wish" input-addressed derivations worked all along, but it does mean that if one replaced a fixed output derivation with a floating CA derivation that produces the same outputs, one will get a different input-addressed path.

Ericson2314 · 2020-09-23T15:02:01Z

tests/content-addressed.nix

  };
  rootCA = mkDerivation {
-    name = "dependent";
+    name = "rootCA";


Thanks 😊

src/libstore/derivations.cc

Ericson2314 · 2020-09-23T15:06:02Z

src/libstore/derivations.hh

    Hash, // regular DRV normalized hash
-    CaOutputHashes
+    CaOutputHashes, // Fixed-output derivation hashes
+    UnknownHashes // Deferred hashes for floating outputs drvs and their dependencies


Is it safe to put UnknownHashes in the memo table? I long though it wasn't but I think since deferred derivations look different it, it is!

thufschmitt · 2020-09-24T08:09:39Z

Are you opposed to merging #3958 so it's easier to just review those?

Certainly not! This builds on top of #3958 but doesn't change anything to the way it works, so it definitely makes sense not to make it a blocker.

See NixOS#4056 (comment)

edolstra · 2020-09-29T09:47:28Z

Please rebase now that #3958 is merged, it will make it a bit more convenient to review.

Ericson2314 · 2020-09-29T15:42:59Z

(rebasing is fine, but it should be enough merely to do something that will get GitHub to recompute the diff, such as pushing any new commit or even just setting the PR target branch to something else and then setting it back.)

See NixOS#4056 (comment)

thufschmitt · 2020-10-03T18:06:49Z

I've just rebased on top of master, should make the diff smaller

thufschmitt · 2020-10-07T10:29:34Z

I've just added a test case to ensure that non-ca derivations that depend on a CA one are properly cached (this is kind-of obvious given the flow of the code, but I think it's worth testing if only to prove that it actually works)

See NixOS#4056 (comment)

Although the non-resolved derivation will never get a cache-hit (it doesn't have an output path to query the cache for anyways), we might get one on the resolved derivation.

thufschmitt · 2020-10-27T16:26:57Z

Rebased on top of master to fix a conflict.

@edolstra is this good to go? Should be mostly uncontroversial after #3958

Ericson2314 · 2020-10-27T18:17:34Z

Well there was the thread up to #4056 (comment) with a smidgen of controversy, but that's just something for you to be aware of @edolstra. I'm with this for now; and we can either change that later or just not care.

Ericson2314 · 2020-10-27T18:18:56Z

src/libstore/build/derivation-goal.cc

            [&](DerivationOutputCAFloating dof) {
                return newInfoFromCA(dof);
            },
+                [&](DerivationOutputDeferred) {


Indentation issue?

…sed ones Fix an overlook of NixOS#4056

Ericson2314 approved these changes Sep 23, 2020

View reviewed changes

thufschmitt added a commit to tweag/nix that referenced this pull request Sep 24, 2020

Inline unkownHashes

3c723d3

See NixOS#4056 (comment)

This was referenced Sep 24, 2020

CA derivations that depend on other CA derivations #3958

Merged

Floating content-addressed derivations #4087

Open

Ericson2314 mentioned this pull request Sep 29, 2020

Impure derivations #520

Open

thufschmitt added a commit to tweag/nix that referenced this pull request Oct 3, 2020

Inline unkownHashes

48b33f3

See NixOS#4056 (comment)

thufschmitt force-pushed the non-ca-depending-on-ca branch from c3696f7 to 48b33f3 Compare October 3, 2020 16:42

thufschmitt mentioned this pull request Oct 6, 2020

Don't explicitely pattern-match on the various outputs #4112

Closed

thufschmitt added 3 commits October 27, 2020 07:29

Allow non-CA derivations to depend on CA derivations

c092fa4

Inline unkownHashes

bc081bc

See NixOS#4056 (comment)

Test the remote caching of non-ca-depending-on-ca derivations

ab21ab6

Although the non-resolved derivation will never get a cache-hit (it doesn't have an output path to query the cache for anyways), we might get one on the resolved derivation.

thufschmitt force-pushed the non-ca-depending-on-ca branch from f57ebd0 to ab21ab6 Compare October 27, 2020 06:32

edolstra merged commit 02a1fac into NixOS:master Oct 27, 2020

edolstra deleted the non-ca-depending-on-ca branch October 27, 2020 16:38

Ericson2314 reviewed Oct 27, 2020

View reviewed changes

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

thufschmitt mentioned this pull request Nov 25, 2020

Allow fixed-output derivations to depend on (floating) content-addressed ones #4284

Merged

thufschmitt added a commit to tweag/nix that referenced this pull request Nov 25, 2020

Allow fixed-output derivations to depend on (floating) content-addres…

39cfe5c

…sed ones Fix an overlook of NixOS#4056

thufschmitt added a commit to tweag/nix that referenced this pull request Nov 27, 2020

Allow fixed-output derivations to depend on (floating) content-addres…

9bd8184

…sed ones Fix an overlook of NixOS#4056

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

Uh oh!

Comments

Conversation

thufschmitt commented Sep 23, 2020

Uh oh!

Ericson2314 commented Sep 23, 2020

Uh oh!

Ericson2314 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ericson2314 Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

thufschmitt Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thufschmitt Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ericson2314 Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thufschmitt commented Sep 24, 2020

Uh oh!

edolstra commented Sep 29, 2020

Uh oh!

Ericson2314 commented Sep 29, 2020

Uh oh!

thufschmitt commented Oct 3, 2020

Uh oh!

thufschmitt commented Oct 7, 2020

Uh oh!

thufschmitt commented Oct 27, 2020

Uh oh!

Ericson2314 commented Oct 27, 2020

Uh oh!

Ericson2314 Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ericson2314 left a comment •

edited

Loading

Ericson2314 Sep 24, 2020 •

edited

Loading

Ericson2314 Sep 24, 2020 •

edited

Loading

Ericson2314 Sep 23, 2020 •

edited

Loading