Merge remote-tracking branch 'obsidian/single-ca-drv-build' into ca-floating-upstream

6 files changed, 62 insertions, 26 deletions

diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 4e248f979..78dc314fa 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -75,6 +75,18 @@ void EvalState::realiseContext(const PathSet & context)
     }
 }
 
+/* Add and attribute to the given attribute map from the output name to
+   the output path, or a placeholder.
+
+   Where possible the path is used, but for floating CA derivations we
+   may not know it. For sake of determinism we always assume we don't
+   and instead put in a place holder. In either case, however, the
+   string context will contain the drv path and output name, so
+   downstream derivations will have the proper dependency, and in
+   addition, before building, the placeholder will be rewritten to be
+   the actual path.
+
+   The 'drv' and 'drvPath' outputs must correspond. */
 static void mkOutputString(EvalState & state, Value & v,
     const StorePath & drvPath, const BasicDerivation & drv,
     std::pair<string, DerivationOutput> o)
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 1f77b8ea8..e50b23ed6 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -718,7 +718,7 @@ typedef enum {rpAccept, rpDecline, rpPostpone} HookReply;
 
 class SubstitutionGoal;
 
-struct KnownInitialOutputStatus {
+struct InitialOutputStatus {
     StorePath path;
     /* The output optional indicates whether it's already valid; i.e. exists
        and is registered. If we're repairing, inner bool indicates whether the
@@ -731,9 +731,9 @@ struct KnownInitialOutputStatus {
     }
 };
 
-struct InitialOutputStatus {
+struct InitialOutput {
     bool wanted;
-    std::optional<KnownInitialOutputStatus> known;
+    std::optional<InitialOutputStatus> known;
 };
 
 class DerivationGoal : public Goal
@@ -770,7 +770,7 @@ private:
        immediate input paths). */
     StorePathSet inputPaths;
 
-    std::map<std::string, InitialOutputStatus> initialOutputs;
+    std::map<std::string, InitialOutput> initialOutputs;
 
     /* User selected for running the builder. */
     std::unique_ptr<UserLock> buildUser;
@@ -1052,11 +1052,14 @@ private:
     /* Forcibly kill the child process, if any. */
     void killChild();
 
-    /* Map a path to another (reproducably) so we can avoid overwriting outputs
+    /* Create alternative path calculated from but distinct from the
+       input, so we can avoid overwriting outputs (or other store paths)
        that already exist. */
     StorePath makeFallbackPath(const StorePath & path);
-    /* Make a path to another based on the output name alone, if one doesn't
-       want to use a random path for CA builds. */
+    /* Make a path to another based on the output name along with the
+       derivation hash. */
+    /* FIXME add option to randomize, so we can audit whether our
+       rewrites caught everything */
     StorePath makeFallbackPath(std::string_view outputName);
 
     void repairClosure();
@@ -1268,7 +1271,7 @@ void DerivationGoal::haveDerivation()
         for (auto & [_, status] : initialOutputs) {
             if (!status.wanted) continue;
             if (!status.known) {
-                warn("Do not know how to query for unknown floating CA drv output yet");
+                warn("do not know how to query for unknown floating content-addressed derivation output yet");
                 /* Nothing to wait for; tail call */
                 return DerivationGoal::gaveUpOnSubstitution();
             }
@@ -1497,7 +1500,7 @@ void DerivationGoal::inputsRealised()
                     auto optRealizedInput = outputs.at(j);
                     if (!optRealizedInput)
                         throw Error(
-                            "derivation '%s' requires output '%s' from input derivation '%s', which is supposedly realized already, yet we still don't know what path corresponds to that output.",
+                            "derivation '%s' requires output '%s' from input derivation '%s', which is supposedly realized already, yet we still don't know what path corresponds to that output",
                             worker.store.printStorePath(drvPath), j, worker.store.printStorePath(drvPath));
                     worker.store.computeFSClosure(*optRealizedInput, inputPaths);
                 } else
@@ -2070,7 +2073,7 @@ StorePathSet DerivationGoal::exportReferences(const StorePathSet & storePaths)
                        `computeFSClosure` on the output path, rather than
                        derivation itself. That doesn't seem right to me, so I
                        won't try to implemented this for CA derivations. */
-                    throw UnimplementedError("export references including CA derivations (themselves) is not yet implemented");
+                    throw UnimplementedError("exportReferences on CA derivations is not yet implemented");
                 worker.store.computeFSClosure(*k.second.second, paths);
             }
         }
@@ -2179,8 +2182,6 @@ void DerivationGoal::startBuilder()
            with the actual hashes. */
         auto scratchPath =
             !status.known
-                /* FIXME add option to randomize, so we can audit whether our
-                 * rewrites caught everything */
                 ? makeFallbackPath(outputName)
             : !needsHashRewrite()
                 /* Can always use original path in sandbox */
@@ -2213,7 +2214,7 @@ void DerivationGoal::startBuilder()
            differ. */
         if (fixedFinalPath == scratchPath) continue;
 
-        /* Ensure scratch scratch path is ours to use */
+        /* Ensure scratch path is ours to use. */
         deletePath(worker.store.printStorePath(scratchPath));
 
         /* Rewrite and unrewrite paths */
@@ -4139,8 +4140,13 @@ void DerivationGoal::registerOutputs()
                     if (lstat(actualPath.c_str(), &st))
                         throw SysError("getting attributes of path '%1%'", actualPath);
                     mode |= 0200;
-                    if (chmod(actualPath.c_str(), mode) == -1)
-                        throw SysError("changing mode of '%1%' to %2$o", actualPath, mode);
+                    /* Try to change the perms, but only if the file isn't a
+                       symlink as symlinks permissions are mostly ignored and
+                       calling `chmod` on it will just forward the call to the
+                       target of the link. */
+                    if (!S_ISLNK(st.st_mode))
+                        if (chmod(actualPath.c_str(), mode) == -1)
+                            throw SysError("changing mode of '%1%' to %2$o", actualPath, mode);
                 }
                 if (rename(
                         actualPath.c_str(),
@@ -4619,19 +4625,19 @@ void DerivationGoal::checkPathValidity()
 {
     bool checkHash = buildMode == bmRepair;
     for (auto & i : queryPartialDerivationOutputMap()) {
-        InitialOutputStatus status {
+        InitialOutput info {
             .wanted = wantOutput(i.first, wantedOutputs),
         };
         if (i.second) {
             auto outputPath = *i.second;
-            status.known = {
+            info.known = {
                 .path = outputPath,
                 .valid = !worker.store.isValidPath(outputPath)
                     ? std::optional<bool> {}
                     : !checkHash || worker.pathContentsGood(outputPath),
             };
         }
-        initialOutputs.insert_or_assign(i.first, status);
+        initialOutputs.insert_or_assign(i.first, info);
     }
 }
 
diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh
index eaffbf452..8aa496143 100644
--- a/src/libstore/derivations.hh
+++ b/src/libstore/derivations.hh
@@ -154,6 +154,11 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi
 // FIXME: remove
 bool isDerivation(const string & fileName);
 
+/* Calculate the name that will be used for the store path for this
+   output.
+
+   This is usually <drv-name>-<output-name>, but is just <drv-name> when
+   the output name is "out". */
 std::string outputPathName(std::string_view drvName, std::string_view outputName);
 
 // known CA drv's output hashes, current just for fixed-output derivations
@@ -213,8 +218,21 @@ struct Sink;
 Source & readDerivation(Source & in, const Store & store, BasicDerivation & drv, std::string_view name);
 void writeDerivation(Sink & out, const Store & store, const BasicDerivation & drv);
 
+/* This creates an opaque and almost certainly unique string
+   deterministically from the output name.
+
+   It is used as a placeholder to allow derivations to refer to their
+   own outputs without needing to use the hash of a derivation in
+   itself, making the hash near-impossible to calculate. */
 std::string hashPlaceholder(const std::string & outputName);
 
+/* This creates an opaque and almost certainly unique string
+   deterministically from a derivation path and output name.
+
+   It is used as a placeholder to allow derivations to refer to
+   content-addressed paths whose content --- and thus the path
+   themselves --- isn't yet known. This occurs when a derivation has a
+   dependency which is a CA derivation. */
 std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName);
 
 }
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index ea430fa14..d5e6d68ef 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -279,9 +279,8 @@ private:
        specified by the ‘secret-key-files’ option. */
     void signPathInfo(ValidPathInfo & info);
 
-    /* Add a mapping from the deriver of the path info (if specified) to its
-     * out path
-     */
+    /* Register the store path 'output' as the output named 'outputName' of
+       derivation 'deriver'. */
     void linkDeriverToPath(const StorePath & deriver, const string & outputName, const StorePath & output);
     void linkDeriverToPath(State & state, uint64_t deriver, const string & outputName, const StorePath & output);
 
diff --git a/src/nix/command.cc b/src/nix/command.cc
index f697eb84c..37a4bc785 100644
--- a/src/nix/command.cc
+++ b/src/nix/command.cc
@@ -150,8 +150,9 @@ void MixProfile::updateProfile(const Buildables & buildables)
             },
             [&](BuildableFromDrv bfd) {
                 for (auto & output : bfd.outputs) {
-                    if (!output.second)
-                        throw Error("output path should be known because we just tried to build it");
+                    /* Output path should be known because we just tried to
+                       build it. */
+                    assert(!output.second);
                     result.push_back(*output.second);
                 }
             },
diff --git a/tests/content-addressed.nix b/tests/content-addressed.nix
index 5ebc779c1..3dcf916c3 100644
--- a/tests/content-addressed.nix
+++ b/tests/content-addressed.nix
@@ -13,17 +13,17 @@ rec {
       mkdir -p $out
       echo "Hello World" > $out/hello
     '';
-    __contentAddressed = true;
-    outputHashMode = "recursive";
-    outputHashAlgo = "sha256";
   };
   rootCA = mkDerivation {
     name = "dependent";
+    outputs = [ "out" "dev" ];
     buildCommand = ''
       echo "building a CA derivation"
       echo "The seed is ${toString seed}"
       mkdir -p $out
       echo ${rootLegacy}/hello > $out/dep
+      # test symlink at root
+      ln -s $out $dev
     '';
     __contentAddressed = true;
     outputHashMode = "recursive";