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

Build ca derivations remotely #4477

Merged
merged 12 commits into from Feb 26, 2021
Merged

Build ca derivations remotely #4477

merged 12 commits into from Feb 26, 2021

Conversation

thufschmitt
Copy link
Member

Allow using the build hook to build ca derivations remotely.

Required extending the remote build protocol(s) a bit to send back the newly built outputs.

@thufschmitt thufschmitt added this to the ca-derivations-mvp milestone Jan 26, 2021
@thufschmitt thufschmitt added ca-derivations Derivations with content addressed outputs feature Feature request or proposal labels Jan 26, 2021
@thufschmitt
Copy link
Member Author

Rebased on top of master to fix some conflicts

Comment on lines 61 to 63
// XXX: Should use something like
// `queryPartialDerivationOutputMap`, but we can't do that
// as it assumes a registered derivation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a DerivationGoal::queryPartialDerivationOutputMap for this purpose that you might be able to somehow use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh I might indeed. Will need to update it a bit to handle non-static output paths, but I think it's a good thing anyways. Thanks :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a second though it can't, because queryPartialDerivationOutputMap only returns the output paths and not the full realisations. So we'll have to update it first.

I've updated the comment to reflect this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK sounds good.

@@ -1,5 +0,0 @@
source common.sh
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be deleted (but fixed in a separate PR)

@thufschmitt thufschmitt force-pushed the ca/build-remote branch 2 times, most recently from 86701db to c9d3276 Compare February 22, 2021 18:07
@thufschmitt
Copy link
Member Author

Rebased on top of latest master

- Pass it the name of the outputs rather than their output paths (as
  these don't exist for ca derivations)
- Get the built output paths from the remote builder
- Register the new received realisations
To allow it to build ca derivations remotely
To allow it to build ca derivations remotely
To allow it to build ca derivations remotely
Otherwise they don't get registered, triggering an assertion failure
at some point later
It's possible that all the paths are already there, but just not
associated to the current drv output
There was already some logic for that, but it didn't handle the case of
content-addressed outputs, so extend it a bit for that
if (!store->isValidPath(store->parseStorePath(path))) missing.insert(store->parseStorePath(path));
std::set<Realisation> missingRealisations;
StorePathSet missingPaths;
if (settings.isExperimentalFeatureEnabled("ca-derivations")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should check whether drv is a CA derivation? Since we don't have to do the CA-related stuff (missingRealisations etc.) if it isn't.

src/build-remote/build-remote.cc Outdated Show resolved Hide resolved
src/build-remote/build-remote.cc Outdated Show resolved Hide resolved
src/libstore/build/derivation-goal.cc Outdated Show resolved Hide resolved
outputId,
Realisation{ outputId, *staticOutput.second}
);
if (settings.isExperimentalFeatureEnabled("ca-derivations")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise this should probably be conditional on drv being a CA derivation instead.

@edolstra edolstra merged commit 94637cd into master Feb 26, 2021
@edolstra edolstra deleted the ca/build-remote branch February 26, 2021 15:54
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-8/11758/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ca-derivations Derivations with content addressed outputs feature Feature request or proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants