feat(buildkite): avoid building extraSteps in pipeline construction r/7656

In principle we don't want to build any (later) pipeline target during
pipeline evaluation insofar they appear in extraSteps. For this reason,
we have the needsOutput mechanism which prevents the parent target of an
extraStep from being built in 🦙.

Unfortunately, this mechanism is not general purpose enough, as we use
other (i.e. non parent) targets from depot in extraSteps. As a
consequence, kind of expensive builds need to happen during pipeline
construction at the moment. The solution is to use the fact that the
command script we want to run is exposed via the readTree interface to
depot and build the script proper only when the extra step is executed.

To facilitate this, some prerequisite changes need to be made:

- We need to use a symlink different to result in case needsOutput is
  true which needs support in mkBuildCommand. We also need to avoid this
  symlink being picked up by git, as many extra steps check whether the
  tree is dirty or not. (Is there a way to have it outside the depot
  tree?)

- Since we rely on the build command printing a single store path we
  store in $command_script, we need to avoid it printing two paths
  in cases where nix-store(1) is used (nix-store(1) prints the symlink
  and readlink(1) would print the store path in a separate line).

Future work would be to remove/deprecate the needsOutput mechanism:
After this change the parent target wouldn't be built right away even if
it appeared in the script via string interpolation. Thus we could,
instead of expecting the target being available as `./result`, make our
extra steps nix-ier.

Change-Id: Idd2e88a865eadabe229ce1e05406e8cc4cb63f94
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10850
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI

2 files changed, 31 insertions, 7 deletions

diff --git a/.gitignore b/.gitignore
index 9066757e58e4..8cdaa738f35c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -8,3 +8,6 @@ garbage/
 # Ignore Nix result symlinks
 result
 result-*
+# Nix result symlink used by //nix/buildkite
+/nix-buildkite-extra-step-command-script
+/nix-buildkite-extra-step-command-script-*
diff --git a/nix/buildkite/default.nix b/nix/buildkite/default.nix
index 45af87784c58..6e158ae6c6c0 100644
--- a/nix/buildkite/default.nix
+++ b/nix/buildkite/default.nix
@@ -46,17 +46,21 @@ rec {
     else false;
 
   # Create build command for an attribute path pointing to a derivation.
-  mkBuildCommand = { attrPath, drvPath }: concatStringsSep " " [
+  mkBuildCommand = { attrPath, drvPath, outLink ? "result" }: concatStringsSep " " [
     # First try to realise the drvPath of the target so we don't evaluate twice.
     # Nix has no concept of depending on a derivation file without depending on
     # at least one of its `outPath`s, so we need to discard the string context
     # if we don't want to build everything during pipeline construction.
-    "(nix-store --realise '${drvPath}' --add-root result --indirect && readlink result)"
+    #
+    # To make this more uniform with how nix-build(1) works, we call realpath(1)
+    # on nix-store(1)'s output since it has the habit of printing the path of the
+    # out link, not the store path.
+    "(nix-store --realise '${drvPath}' --add-root '${outLink}' --indirect | xargs realpath)"
 
     # Since we don't gcroot the derivation files, they may be deleted by the
     # garbage collector. In that case we can reevaluate and build the attribute
     # using nix-build.
-    "|| (test ! -f '${drvPath}' && nix-build -E '${mkBuildExpr attrPath}' --show-trace)"
+    "|| (test ! -f '${drvPath}' && nix-build -E '${mkBuildExpr attrPath}' --show-trace --out-link '${outLink}')"
   ];
 
   # Attribute path of a target relative to the depot root. Needs to take into
@@ -212,7 +216,7 @@ rec {
 
           extraSteps = mapAttrs
             (_: steps:
-              map (mkExtraStep buildEnabled) steps)
+              map (mkExtraStep (targetAttrPath target) buildEnabled) steps)
             splitExtraSteps;
         in
         if !buildEnabled then extraSteps
@@ -381,8 +385,11 @@ rec {
 
   # Create the Buildkite configuration for an extra step, optionally
   # wrapping it in a gate group.
-  mkExtraStep = buildEnabled: cfg:
+  mkExtraStep = parentAttrPath: buildEnabled: cfg:
     let
+      # ATTN: needs to match an entry in .gitignore so that the tree won't get dirty
+      commandScriptLink = "nix-buildkite-extra-step-command-script";
+
       step = {
         key = hashString "sha1" "${cfg.label}-${cfg.parentLabel}";
         label = ":gear: ${cfg.label} (from ${cfg.parentLabel})";
@@ -409,8 +416,22 @@ rec {
             "echo '~~~ Preparing build output of ${cfg.parentLabel}'"
           }
           ${lib.optionalString cfg.needsOutput cfg.parent.command}
-          echo '+++ Running extra step command'
-          exec ${cfg.command}
+          echo '--- Building extra step script'
+          command_script="$(${
+            # Using command substitution in this way assumes the script drv only has one output
+            assert builtins.length cfg.command.outputs == 1;
+            mkBuildCommand {
+              # script is exposed at <parent>.meta.ci.extraSteps.<key>.command
+              attrPath =
+                parentAttrPath
+                ++ [ "meta" "ci" "extraSteps" cfg.key "command" ];
+              drvPath = unsafeDiscardStringContext cfg.command.drvPath;
+              # make sure it doesn't conflict with result (from needsOutput)
+              outLink = commandScriptLink;
+            }
+          })"
+          echo '+++ Running extra step script'
+          exec "$command_script"
         '';
 
         soft_fail = cfg.softFail;