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

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

Merged
merged 3 commits into from Oct 27, 2020

Conversation

@regnat
Copy link
Contributor

@regnat regnat commented Sep 23, 2020

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
Copy link
Member

@Ericson2314 Ericson2314 commented Sep 23, 2020

@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?)

Copy link
Member

@Ericson2314 Ericson2314 left a comment

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 Outdated Show resolved Hide resolved
src/libstore/derivations.cc Outdated Show resolved Hide resolved
case DerivationType::DeferredInputAddressed:
break;
Comment on lines +517 to +516

This comment has been minimized.

@Ericson2314

Ericson2314 Sep 23, 2020
Member

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

This comment has been minimized.

@regnat

regnat Sep 24, 2020
Author Contributor

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

This comment has been minimized.

@Ericson2314

Ericson2314 Sep 24, 2020
Member

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.

This comment has been minimized.

@regnat

regnat Sep 24, 2020
Author Contributor

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.

This comment has been minimized.

@Ericson2314

Ericson2314 Sep 24, 2020
Member

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!)

This comment has been minimized.

@Ericson2314

Ericson2314 Sep 24, 2020
Member

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.

@@ -15,7 +15,7 @@ rec {
'';
};
rootCA = mkDerivation {
name = "dependent";
name = "rootCA";

This comment has been minimized.

@Ericson2314

Ericson2314 Sep 23, 2020
Member

Thanks 😊

src/libstore/derivations.cc Outdated Show resolved Hide resolved
typedef std::variant<
Hash, // regular DRV normalized hash
CaOutputHashes
CaOutputHashes, // Fixed-output derivation hashes
UnknownHashes // Deferred hashes for floating outputs drvs and their dependencies

This comment has been minimized.

@Ericson2314

Ericson2314 Sep 23, 2020
Member

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!

@regnat
Copy link
Contributor Author

@regnat regnat commented Sep 24, 2020

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.

regnat added a commit to tweag/nix that referenced this pull request Sep 24, 2020
@edolstra
Copy link
Member

@edolstra edolstra commented Sep 29, 2020

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

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Sep 29, 2020

(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.)

regnat added a commit to tweag/nix that referenced this pull request Oct 3, 2020
@regnat regnat force-pushed the tweag:non-ca-depending-on-ca branch from c3696f7 to 48b33f3 Oct 3, 2020
@regnat
Copy link
Contributor Author

@regnat regnat commented Oct 3, 2020

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

@regnat
Copy link
Contributor Author

@regnat regnat commented Oct 7, 2020

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)

regnat added 3 commits Sep 23, 2020
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.
@regnat regnat force-pushed the tweag:non-ca-depending-on-ca branch from f57ebd0 to ab21ab6 Oct 27, 2020
@regnat
Copy link
Contributor Author

@regnat regnat commented Oct 27, 2020

Rebased on top of master to fix a conflict.

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

@edolstra edolstra merged commit 02a1fac into NixOS:master Oct 27, 2020
2 checks passed
2 checks passed
@github-actions
tests (ubuntu-latest) tests (ubuntu-latest)
Details
@github-actions
tests (macos-latest) tests (macos-latest)
Details
@edolstra edolstra deleted the tweag:non-ca-depending-on-ca branch Oct 27, 2020
@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Oct 27, 2020

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.

@@ -3166,6 +3167,15 @@ void DerivationGoal::registerOutputs()
[&](DerivationOutputCAFloating dof) {
return newInfoFromCA(dof);
},
[&](DerivationOutputDeferred) {

This comment has been minimized.

@Ericson2314

Ericson2314 Oct 27, 2020
Member

Indentation issue?

regnat added a commit to tweag/nix that referenced this pull request Nov 25, 2020
regnat added a commit to tweag/nix that referenced this pull request Nov 27, 2020
@regnat regnat added this to the ca-derivations-mvp milestone Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants