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

CA derivations that depend on other CA derivations #3958

Merged
merged 23 commits into from Sep 29, 2020

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 24, 2020

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

Copy link
Contributor

@regnat regnat left a comment

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?

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

@regnat regnat Aug 25, 2020

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

Copy link
Member Author

@Ericson2314 Ericson2314 Aug 28, 2020

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

Copy link
Member

@edolstra edolstra Sep 28, 2020

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 Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member Author

@Ericson2314 Ericson2314 commented Aug 28, 2020

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 Outdated Show resolved Hide resolved
Ericson2314 added 5 commits Sep 3, 2020
…loating-upstream

Tests also now fail as they should
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.
@Ericson2314 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
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 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 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
@Ericson2314
Copy link
Member Author

@Ericson2314 Ericson2314 commented Sep 16, 2020

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.

@regnat
Copy link
Contributor

@regnat regnat commented Sep 17, 2020

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

@regnat
Copy link
Contributor

@regnat regnat commented Sep 17, 2020

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

@Ericson2314 Ericson2314 commented Sep 17, 2020

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

@regnat
Copy link
Contributor

@regnat regnat commented Sep 18, 2020

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

@Ericson2314 Ericson2314 commented Sep 18, 2020

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

@Ericson2314
Copy link
Member Author

@Ericson2314 Ericson2314 commented Sep 24, 2020

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

@@ -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) {
Copy link
Member

@edolstra edolstra Sep 25, 2020

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

Style nitpick.

assert(attempt);
Derivation drvResolved { *std::move(attempt) };

auto pathResolved = writeDerivation(worker.store, drvResolved);
Copy link
Member Author

@Ericson2314 Ericson2314 Sep 25, 2020

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

Copy link
Member

@edolstra edolstra Sep 28, 2020

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

Copy link
Member Author

@Ericson2314 Ericson2314 Sep 28, 2020

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.

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

void DerivationGoal::resolvedFinished() {
Copy link
Member

@edolstra edolstra Sep 28, 2020

This function seems a bit unnecessary.

Copy link
Member Author

@Ericson2314 Ericson2314 Sep 28, 2020

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 Outdated Show resolved Hide resolved
src/libstore/derivations.hh Outdated Show resolved Hide resolved
@edolstra edolstra merged commit 1e8855a into NixOS:master Sep 29, 2020
2 checks passed
@Ericson2314 Ericson2314 deleted the ca-floating-upstream branch Sep 29, 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