Allow depending on content-addressed derivations #3776

thufschmitt · 2020-07-02T07:54:02Z

Builds on top of #3740, and extends it to allow depending on ca-derivations, with a proper cutoff in case two different derivations in the build chain produce the same output (see the added test)

Ericson2314 · 2020-07-02T23:44:19Z

Between the first and #3740, This does seem like a bunch code doubling down on the "real and fake outputs" approach, which I do worry will be a source of bugs.

Now that derivation outputs are fully parsed, I'd like to exploit that, going from

struct DerivationOutput
{
    StorePath path;
    std::optional<FixedOutputHash> hash; /* hash used for expected hash computation */
};

to

struct DerivationOutputLegacy {
    // StorePath path;
    // regnat pointed out that when we depend on a ca-derivation,
    // we cannot calculate downstream paths until the upstream one is built.
    std::optional<StorePath> path;
};
struct DerivationOutputCAFixed {
    /* hash used for expected hash computation */
    FixedOutputHash hash;
    // no need to store output store path, as it can be computed on the fly.
};
struct DerivationOutputCAFloating {
    /* hash used for ca path computation */
    HashType hashType;
};
typedef std::variant<
    DerivationOutputLegacy,
    DerivationOutputCAFixed,
    DerivationOutputCAFloating
> DerivationOutput;

/* accessors to make it easier to handle multiple cases at once. */
std::optional<StorePath> outputPath(DerivationOutput) { ... }
std::optional<HashType> outputHashAlgo(DerivationOutput) { ... }

I really do think we can make that change, fix all the type errors "on autopilot', and be quite close to a working version.

As a first, preparatory step, we can do:

struct DerivationOutputLegacy {
    StorePath path;
};
struct DerivationOutputCAFixed {
    /* hash used for expected hash computation */
    FixedOutputHash hash;
    // no need to store output store path, as it can be computed on the fly.
};
typedef std::variant<
    DerivationOutputLegacy,
    DerivationOutputCAFixed
> DerivationOutput;

/* accessors to make it easier to handle multiple cases at once. */
StorePath outputPath(DerivationOutput) { ... }

I think we'll also need to make derivations store their own name.

which is exactly isomorphic to today, to get the churn out of the way, and then add new new variant in a separate PR.

Ericson2314 · 2020-07-02T23:49:34Z

https://github.com/NixOS/nix/pull/3754/files should also help. Every time in that PR I do something like

FixedOutputInfo {
    { .method = ..., .hash = ... },
    {} // empty references
}

the {} is practically a Mad Lib beginning to be filed in with richer reference data.

Move the logic from the `nix make-content-addressed` nix command to a new method in the `Store` class

This is required so that we can call LocalStore::makeContentAddressed on paths that haven't been registered

Very limited atm: - Only `nix-build` works properly (`nix-store`, `nix build`, etc.. will print and create a symlink to a non-existent output path) - It isn't possible to depend on a content-addressed derivation

Otherwise `resolveDerivation` changes the name of the derivation (and as a consequence its hash)

Otherwise substituted or short-circuited paths are not properly registered

edolstra · 2020-07-07T10:47:50Z

src/libstore/build.cc

+        bool isModified = resolveDerivation(worker.store, *(dynamic_cast<Derivation *>(drv.get())));
+        // Define the actual outputs of the build
+        for (auto & i: drv->outputs)
+            actualOutputs.emplace(i.first, i.second.path);


It's confusing to me that here actualOutputs is initialized to what appears to be the non-actual outputs.

I think this is intentional, but this is the sort of foot-gun I want to avoid by making DerivationOutput use variants in my comment above.

edolstra · 2020-07-07T10:50:14Z

src/libstore/build.cc

+            // Replace the output path by the new CA one
+            // FIXME: Why can't this be done in one step?
+            actualOutputs.erase(i.first);
+            actualOutputs.emplace(i.first, info.path);


This can be replaced by insert_or_assign().

edolstra · 2020-07-07T10:53:44Z

src/libstore/build.cc

@@ -4248,6 +4286,7 @@ void DerivationGoal::done(BuildResult::Status status, std::optional<Error> ex)
    mcRunningBuilds.reset();

    if (result.success()) {
+        registerOutputMappings();


Why is this done here rather than in registerOutputs()? To me done() is a function that is only intended to interact with Worker to set error states and trigger dependent goals, so it shouldn't have side-effects like registering stuff in the database.

There might be a better place for this indeed.
I didn't put it in registerOutputs because we want it to be called even when no build took place (either because we substituted the output or because we've short-cutted the build after resolving the derivation).

edolstra · 2020-07-07T10:55:03Z

src/libstore/local-fs-store.cc

@@ -79,8 +79,6 @@ ref<FSAccessor> LocalFSStore::getFSAccessor()

 void LocalFSStore::narFromPath(const StorePath & path, Sink & sink)
 {
-    if (!isValidPath(path))
-        throw Error("path '%s' is not valid", printStorePath(path));


I think this check should stay since narFromPath is supposed to apply to valid paths only. For arbitrary paths you should use dumpPath().

If the issue is that build.cc is calling Store::makeContentAddressed() on an invalid path, then maybe that function should take a separate argument for the path (so it can use dumpPath()).

edolstra · 2020-07-07T10:56:28Z

src/libstore/local-store.cc

@@ -576,6 +576,17 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat
    }
 }

+void LocalStore::linkDeriverToPath(const StorePath deriver, const string outputName, const StorePath output)


This should be const StorePath & deriver etc. Otherwise we're passing by value which is less efficient.

Oh forgot these, thanks

edolstra · 2020-07-07T10:59:03Z

src/libstore/rewrite-derivation.hh

+ * (as given by `queryDerivationOutputMap`)
+ * **if this one differs from the one written in the derivation**
+ */
+bool resolveDerivation(Store & store, Derivation & drv);


What's the boolean return value?

Oh my bad I forgot to document it.

It serves to indicate whether the resolving actually modified the derivation or not (to know whether to retry the substitution). But I'm not very happy with it, so I'd be happy if you have any suggestion on improving this

edolstra · 2020-07-07T11:00:04Z

src/libstore/rewrite-derivation.cc

+        auto outputEnvVar = drv.env.find(i.first);
+        if (outputEnvVar != drv.env.end())
+            outputEnvVar->second = store.printStorePath(outPath);
+        debug(format("Rewrote output %1% to %2%")


format is obsolete, please do debug("rewrite output '%s' to '%s'", arg1, arg2).

edolstra · 2020-07-07T11:02:11Z

src/libstore/rewrite-derivation.cc

+    for (auto & i : drv.outputs) {
+        // XXX: There's certainly a better and less error-prone way
+        // of getting the name than to look it up in the drv environment
+        string name = ParsedDerivation(StorePath::dummy, drv).getStringAttr("name").value_or("");


This can be lifted out of the for loop.

edolstra · 2020-07-07T11:05:11Z

src/libstore/store-api.cc

+        const StringMap & extraRewrites
+    )
+{
+    bool hasSelfReference = info.references.find(info.path) != info.references.end();


=> bool hasSelfReference = info.references.count(info.path).

edolstra · 2020-07-07T11:10:16Z

src/libstore/store-api.hh

+    /* Create a new path similar to the one referred to by `info`, but that's
+     * content-addressed.
+     *
+     * This doesn't alter the original path


This should document whether info needs to denote a valid path.

Ericson2314 · 2020-07-07T13:48:58Z

src/libstore/store-api.hh

    std::optional<StorePath> deriver;
+    std::optional<string> outputname;


Is the intent that we can have outputname whenever we have deriver, except for backwards compat with old infos?

BTW it looks like this field is not actually used anywhere.

Indeed, I've added it for an experiment and forgot to remove it. This field doesn't really make any sense (even the deriver field should probably be deprecated as a store path can now have several derivers)

Yeah once we upload the trust mapping, we'll probably want to key it by the derivation not the output path, which also helps different signatures sign for different derivers.

edolstra · 2020-07-07T14:10:47Z

src/libstore/store-api.hh

@@ -115,6 +115,7 @@ struct ValidPathInfo
 {
    StorePath path;
    std::optional<StorePath> deriver;
+    std::optional<string> outputname;


It seems incorrect to have an outputName field in ValidPathInfo. A CA store path can be produced by differently named output of different derivations (e.g. the "foo" output of derivation A could produce the same contents as the "bar" output of derivation B).

edolstra · 2020-07-07T14:17:06Z

src/libstore/local-store.cc

+        state->stmtAddDerivationOutput.use()
+            (queryValidPathId(*state, deriver))
+            (outputName)
+            (printStorePath(output))


output is an "actual" output here, right? In that case, doesn't this cause confusion wrt the meaning of the DerivationOutputs table?

Replace it by a more straightforward `caOutputs` map that just contains the outputs that are actually overriden by the CA process

This reverts commit 3854aec.

narFromPath expects a valid path as input (and making it more general would be complicated as it's a general method of the `Store` class), so we just inline the bit of `makeContentAddressed` that we need (which is bigger than what I though. Maybe I'm trying to retro-fit functional idioms for which cpp isn't designed)

thufschmitt added 4 commits July 3, 2020 17:27

Internalize the ability to make a store path content-addressed

78f6420

Move the logic from the `nix make-content-addressed` nix command to a new method in the `Store` class

Don't require a valid store path in LocalStore::narFromPath

3854aec

This is required so that we can call LocalStore::makeContentAddressed on paths that haven't been registered

Allow building CA paths

e198893

Very limited atm: - Only `nix-build` works properly (`nix-store`, `nix build`, etc.. will print and create a symlink to a non-existent output path) - It isn't possible to depend on a content-addressed derivation

fixup! Allow building CA paths

56e363c

thufschmitt force-pushed the ca-derivations-dependencies branch from d32357a to 75a06ee Compare July 3, 2020 16:34

thufschmitt added 3 commits July 3, 2020 18:34

Align env["name"] and the drv name in nix develop

c89ab8d

Otherwise `resolveDerivation` changes the name of the derivation (and as a consequence its hash)

Register the output mappings even when nothing is built

1a42494

Otherwise substituted or short-circuited paths are not properly registered

Allow depending on a CA derivation

bab7345

thufschmitt force-pushed the ca-derivations-dependencies branch from 75a06ee to bab7345 Compare July 3, 2020 16:34

edolstra approved these changes Jul 7, 2020

View reviewed changes

Ericson2314 reviewed Jul 7, 2020

View reviewed changes

edolstra reviewed Jul 7, 2020

View reviewed changes

thufschmitt added 6 commits July 8, 2020 08:28

Move rewriteDerivation to the BasicDerivation class

dfa2261

fixup! Register the output mappings even when nothing is built

c5a90cb

fixup! Register the output mappings even when nothing is built

d86a1e4

fixup! Register the output mappings even when nothing is built

c98c210

fixup! Allow building CA paths

726255f

Remove the ambiguous actualOutputs DerivationGoal member

92ea0a7

Replace it by a more straightforward `caOutputs` map that just contains the outputs that are actually overriden by the CA process

Ericson2314 mentioned this pull request Jul 8, 2020

Only store hash in DerivationOutput for fixed output derivations #3795

Merged

thufschmitt added 6 commits July 9, 2020 13:11

fixup! Allow depending on a CA derivation

582e53d

fixup! Allow depending on a CA derivation

c8f8bae

fixup! Allow depending on a CA derivation

17b272a

Revert "Don't require a valid store path in LocalStore::narFromPath"

003b4d1

This reverts commit 3854aec.

Remove the unused outputname ValidPathInfo field

eda7f0f

Ericson2314 mentioned this pull request Aug 24, 2020

CA derivations that depend on other CA derivations #3958

Merged

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

thufschmitt closed this Nov 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow depending on content-addressed derivations #3776

Allow depending on content-addressed derivations #3776

thufschmitt commented Jul 2, 2020

Ericson2314 commented Jul 2, 2020 •

edited

Ericson2314 commented Jul 2, 2020

edolstra Jul 7, 2020

Ericson2314 Jul 7, 2020

edolstra Jul 7, 2020

edolstra Jul 7, 2020

thufschmitt Jul 7, 2020

edolstra Jul 7, 2020

edolstra Jul 7, 2020

edolstra Jul 7, 2020

thufschmitt Jul 7, 2020

edolstra Jul 7, 2020

thufschmitt Jul 7, 2020

edolstra Jul 7, 2020

edolstra Jul 7, 2020

edolstra Jul 7, 2020

edolstra Jul 7, 2020

Ericson2314 Jul 7, 2020

edolstra Jul 7, 2020

thufschmitt Jul 9, 2020

Ericson2314 Jul 9, 2020

edolstra Jul 7, 2020

edolstra Jul 7, 2020

		std::optional<StorePath> deriver;
		std::optional<string> outputname;

Allow depending on content-addressed derivations #3776

Allow depending on content-addressed derivations #3776

Conversation

thufschmitt commented Jul 2, 2020

Ericson2314 commented Jul 2, 2020 • edited

Ericson2314 commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jul 2, 2020 •

edited