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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b0b59fd
Merge remote-tracking branch 'obsidian/write-derivation-borrow' into …
Ericson2314 Aug 23, 2020
59979e7
Fix bad debug format string
Ericson2314 Aug 24, 2020
e0b0e18
Add constructor for BasicDerivation -> Derivation
Ericson2314 Aug 23, 2020
8eb73a8
CA derivations that depend on other CA derivations
Ericson2314 Aug 22, 2020
48e8828
Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…
Ericson2314 Aug 28, 2020
421ed52
Update src/libstore/build.cc
Ericson2314 Aug 28, 2020
02e0001
Merge branch 'ca-floating-upstream' of github.com:obsidiansystems/nix…
Ericson2314 Aug 28, 2020
4db0010
Test CA derivation input caching
Ericson2314 Aug 28, 2020
975a47f
Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…
Ericson2314 Sep 3, 2020
b836662
Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…
Ericson2314 Sep 3, 2020
4409530
Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…
Ericson2314 Sep 3, 2020
aad4abc
Fix floating CA tests
Ericson2314 Sep 4, 2020
075d399
Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-f…
Ericson2314 Sep 4, 2020
98dfd75
Fix querying outputs for CA derivations some more
Ericson2314 Sep 4, 2020
c5cceba
Merge remote-tracking branch 'upstream/master' into ca-floating-upstream
Ericson2314 Sep 16, 2020
7fdbb37
Start to fix floating CA + remote building
Ericson2314 Sep 9, 2020
2741fff
Ensure resolved CA derivations are written
Ericson2314 Sep 9, 2020
b7df353
Merge remote-tracking branch 'upstream/master' into ca-floating-upstream
Ericson2314 Sep 17, 2020
3786a80
Merge remote-tracking branch 'upstream/master' into ca-floating-upstream
Ericson2314 Sep 22, 2020
67cc356
Merge remote-tracking branch 'origin/master' into non-ca-depending-on-ca
thufschmitt Sep 22, 2020
10202bb
Merge remote-tracking branch 'upstream/master' into ca-floating-upstream
Ericson2314 Sep 28, 2020
80e335b
Use `drvPath2` and give it a better name
Ericson2314 Sep 28, 2020
00135e1
Clarify comment a bit
Ericson2314 Sep 28, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 46 additions & 5 deletions src/libstore/build.cc
Expand Up @@ -984,6 +984,8 @@ class DerivationGoal : public Goal
void tryLocalBuild();
void buildDone();

void resolvedFinished();

/* Is the build hook willing to perform the build? */
HookReply tryBuildHook();

Expand Down Expand Up @@ -1451,8 +1453,39 @@ void DerivationGoal::inputsRealised()
/* Determine the full set of input paths. */

/* First, the input derivations. */
if (useDerivation)
for (auto & [depDrvPath, wantedDepOutputs] : dynamic_cast<Derivation *>(drv.get())->inputDrvs) {
if (useDerivation) {
auto & fullDrv = *dynamic_cast<Derivation *>(drv.get());

if (!fullDrv.inputDrvs.empty() && fullDrv.type() == DerivationType::CAFloating) {
/* We are be able to resolve this derivation based on the
now-known results of dependencies. If so, we become a stub goal
aliasing that resolved derivation goal */
Derivation drvResolved { fullDrv.resolve(worker.store) };

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

/* Add to memotable to speed up downstream goal's queries with the
original derivation. */
drvPathResolutions.lock()->insert_or_assign(drvPath, pathResolved);

auto msg = fmt("Resolved derivation: '%s' -> '%s'",
worker.store.printStorePath(drvPath),
worker.store.printStorePath(pathResolved));
act = std::make_unique<Activity>(*logger, lvlInfo, actBuildWaiting, msg,
Logger::Fields {
worker.store.printStorePath(drvPath),
worker.store.printStorePath(pathResolved),
});

auto resolvedGoal = worker.makeDerivationGoal(
pathResolved, wantedOutputs,
buildMode == bmRepair ? bmRepair : bmNormal);
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
addWaitee(resolvedGoal);

state = &DerivationGoal::resolvedFinished;
return;
}

for (auto & [depDrvPath, wantedDepOutputs] : fullDrv.inputDrvs) {
/* Add the relevant output closures of the input derivation
`i' as input paths. Only add the closures of output paths
that are specified as inputs. */
Expand All @@ -1472,6 +1505,7 @@ void DerivationGoal::inputsRealised()
worker.store.printStorePath(drvPath), j, worker.store.printStorePath(drvPath));
}
}
}

/* Second, the input sources. */
worker.store.computeFSClosure(drv->inputSrcs, inputPaths);
Expand Down Expand Up @@ -1893,6 +1927,9 @@ void DerivationGoal::buildDone()
done(BuildResult::Built);
}

void DerivationGoal::resolvedFinished() {
Copy link
Member

Choose a reason for hiding this comment

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

This function seems a bit unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

done(BuildResult::Built);
}

HookReply DerivationGoal::tryBuildHook()
{
Expand Down Expand Up @@ -2065,7 +2102,7 @@ void linkOrCopy(const Path & from, const Path & to)
file (e.g. 32000 of ext3), which is quite possible after a
'nix-store --optimise'. FIXME: actually, why don't we just
bind-mount in this case?

It can also fail with EPERM in BeegFS v7 and earlier versions
which don't allow hard-links to other directories */
if (errno != EMLINK && errno != EPERM)
Expand Down Expand Up @@ -4248,10 +4285,14 @@ void DerivationGoal::registerOutputs()
{
ValidPathInfos infos2;
for (auto & [outputName, newInfo] : infos) {
/* FIXME: we will want to track this mapping in the DB whether or
not we have a drv file. */
if (useDerivation)
worker.store.linkDeriverToPath(drvPath, outputName, newInfo.path);
else {
/* Once a floating CA derivations reaches this point, it must
already be resolved, drvPath the basic derivation path, and
a file existsing at that path for sake of the DB's foreign key. */
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
assert(drv->type() != DerivationType::CAFloating);
}
infos2.push_back(newInfo);
}
worker.store.registerValidPaths(infos2);
Expand Down
53 changes: 53 additions & 0 deletions src/libstore/derivations.cc
Expand Up @@ -672,4 +672,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

Choose a reason for hiding this comment

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

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

Style nitpick.


debug("Rewriting the derivation");

for (auto &rewrite: rewrites) {
debug("rewriting %s as %s", rewrite.first, rewrite.second);
}

drv.builder = rewriteStrings(drv.builder, rewrites);
for (auto & arg: drv.args) {
arg = rewriteStrings(arg, rewrites);
}

StringPairs newEnv;
for (auto & envVar: drv.env) {
auto envName = rewriteStrings(envVar.first, rewrites);
auto envValue = rewriteStrings(envVar.second, rewrites);
newEnv.emplace(envName, envValue);
}
drv.env = newEnv;
}


Sync<DrvPathResolutions> drvPathResolutions;

BasicDerivation Derivation::resolve(Store & store) {
BasicDerivation resolved { *this };

// Input paths that we'll want to rewrite in the derivation
StringMap inputRewrites;

for (auto & input : inputDrvs) {
auto inputDrvOutputs = store.queryPartialDerivationOutputMap(input.first);
StringSet newOutputNames;
for (auto & outputName : input.second) {
auto actualPathOpt = inputDrvOutputs.at(outputName);
if (!actualPathOpt)
throw Error("input drv '%s' wasn't yet built", store.printStorePath(input.first));
auto actualPath = *actualPathOpt;
inputRewrites.emplace(
downstreamPlaceholder(store, input.first, outputName),
store.printStorePath(actualPath));
resolved.inputSrcs.insert(std::move(actualPath));
}
}

rewriteDerivation(store, resolved, inputRewrites);

return resolved;
}

}
18 changes: 18 additions & 0 deletions src/libstore/derivations.hh
Expand Up @@ -4,6 +4,7 @@
#include "types.hh"
#include "hash.hh"
#include "content-address.hh"
#include "sync.hh"

#include <map>
#include <variant>
Expand Down Expand Up @@ -127,6 +128,13 @@ struct Derivation : BasicDerivation
std::string unparse(const Store & store, bool maskOutputs,
std::map<std::string, StringSet> * actualInputs = nullptr) const;

/* Return the underlying basic derivation but with

1. input drv outputs moved to input sources.
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved

2. placeholders replaced with realized input store paths. */
BasicDerivation resolve(Store & store);

Derivation() = default;
Derivation(BasicDerivation && bd) : BasicDerivation(std::move(bd)) { }
};
Expand Down Expand Up @@ -187,6 +195,16 @@ typedef std::map<StorePath, DrvHashModulo> DrvHashes;

extern DrvHashes drvHashes; // FIXME: global, not thread-safe

/* Memoisation of `readDerivation(..).resove()`. */
typedef std::map<
StorePath,
std::optional<StorePath>
> DrvPathResolutions;

// FIXME: global, though at least thread-safe.
// FIXME: arguably overlaps with hashDerivationModulo memo table.
extern Sync<DrvPathResolutions> drvPathResolutions;

bool wantOutput(const string & output, const std::set<string> & wanted);

struct Source;
Expand Down
27 changes: 25 additions & 2 deletions src/libstore/local-store.cc
Expand Up @@ -803,13 +803,36 @@ StorePathSet LocalStore::queryValidDerivers(const StorePath & path)
}


std::map<std::string, std::optional<StorePath>> LocalStore::queryPartialDerivationOutputMap(const StorePath & path)
std::map<std::string, std::optional<StorePath>> LocalStore::queryPartialDerivationOutputMap(const StorePath & path_)
{
auto path = path_;
std::map<std::string, std::optional<StorePath>> outputs;
BasicDerivation drv = readDerivation(path);
Derivation drv = readDerivation(path);
for (auto & [outName, _] : drv.outputs) {
outputs.insert_or_assign(outName, std::nullopt);
}
bool haveCached = false;
{
auto resolutions = drvPathResolutions.lock();
auto resolvedPathOptIter = resolutions->find(path);
if (resolvedPathOptIter != resolutions->end()) {
auto & [_, resolvedPathOpt] = *resolvedPathOptIter;
if (resolvedPathOpt)
path = *resolvedPathOpt;
haveCached = true;
}
}
/* can't just use else-if instead of `!haveCached` because we need to unlock
`drvPathResolutions` before it is locked in `Derivation::resolve`. */
Copy link
Member

Choose a reason for hiding this comment

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

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

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 function and queryPartialDerivationOutputMap are mutually recursive, so it would deadlock.

Copy link
Member

Choose a reason for hiding this comment

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

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

if (!haveCached && drv.type() == DerivationType::CAFloating) {
/* Resolve drv and use that path instead. */
auto pathResolved = writeDerivation(*this, drv.resolve(*this));
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
/* Store in memo table. */
/* FIXME: memo logic should not be local-store specific, should have
wrapper-method instead. */
drvPathResolutions.lock()->insert_or_assign(path, pathResolved);
path = std::move(pathResolved);
}
return retrySQLite<std::map<std::string, std::optional<StorePath>>>([&]() {
auto state(_state.lock());

Expand Down
48 changes: 36 additions & 12 deletions tests/content-addressed.nix
Expand Up @@ -4,16 +4,40 @@ with import ./config.nix;
# A simple content-addressed derivation.
# The derivation can be arbitrarily modified by passing a different `seed`,
# but the output will always be the same
mkDerivation {
name = "simple-content-addressed";
buildCommand = ''
set -x
echo "Building a CA derivation"
echo "The seed is ${toString seed}"
mkdir -p $out
echo "Hello World" > $out/hello
'';
__contentAddressed = true;
outputHashMode = "recursive";
outputHashAlgo = "sha256";
rec {
root = mkDerivation {
name = "simple-content-addressed";
buildCommand = ''
set -x
echo "Building a CA derivation"
echo "The seed is ${toString seed}"
mkdir -p $out
echo "Hello World" > $out/hello
'';
__contentAddressed = true;
outputHashMode = "recursive";
outputHashAlgo = "sha256";
};
dependent = mkDerivation {
name = "dependent";
buildCommand = ''
echo "building a dependent derivation"
mkdir -p $out
echo ${root}/hello > $out/dep
'';
__contentAddressed = true;
outputHashMode = "recursive";
outputHashAlgo = "sha256";
};
transitivelyDependent = mkDerivation {
name = "transitively-dependent";
buildCommand = ''
echo "building transitively-dependent"
cat ${dependent}/dep
echo ${dependent} > $out
'';
__contentAddressed = true;
outputHashMode = "recursive";
outputHashAlgo = "sha256";
};
}
20 changes: 11 additions & 9 deletions tests/content-addressed.sh
Expand Up @@ -2,15 +2,17 @@

source common.sh

clearStore
clearCache

export REMOTE_STORE=file://$cacheDir

drv=$(nix-instantiate --experimental-features ca-derivations ./content-addressed.nix --arg seed 1)
drv=$(nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A root --arg seed 1)
nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drv" --arg seed 1

out1=$(nix-build --experimental-features ca-derivations ./content-addressed.nix --arg seed 1 --no-out-link)
out2=$(nix-build --experimental-features ca-derivations ./content-addressed.nix --arg seed 2 --no-out-link)
testDerivation () {
local derivationPath=$1
local commonArgs=("--experimental-features" "ca-derivations" "./content-addressed.nix" "-A" "$derivationPath" "--no-out-link")
local out1=$(nix-build "${commonArgs[@]}" --arg seed 1)
local out2=$(nix-build "${commonArgs[@]}" --arg seed 2)
test $out1 == $out2
}

test $out1 == $out2
testDerivation root
testDerivation dependent
testDerivation transitivelyDependent
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved