Merge pull request #32 from ipetkov/fix-dummy-invalidation

mkDummySrc: fix cache invalidation if src is flake root

.github/workflows/test.yml

@@ -32,6 +32,8 @@ jobs:
         authToken: '${{ secrets.CACHIX_AUTH_TOKEN }}'
     - name: flake checks
       run: nix flake check --print-build-logs ${{ matrix.nixpkgs-override }}
+    - name: extra tests
+      run: nix develop ${{ matrix.nixpkgs-override }} --command ./extra-tests/test.sh
     - name: validate examples
       run: |
         for f in $(find examples -maxdepth 1 -mindepth 1 -type d); do

.gitignore

@@ -1,3 +1,4 @@
 /examples/*/flake.lock
 /result*
 target/
+/extra-tests/*/flake.lock

CHANGELOG.md

@@ -6,6 +6,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## Unreleased
 
+### Fixed
+* Dummy source derivations go to greater lengths to only depend on the files
+  they consume. Specifying the entire flake source as an input (e.g. via
+  `buildPackage { src = self; }`) now avoids rebuilding everything from scratch
+  whenever _any_ file is changed. #28
+
 ## [0.4.0] - 2022-05-10
 
 ### Changed

extra-tests/dummy-does-not-depend-on-flake-source-via-path/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "simple"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+byteorder = "*"

extra-tests/dummy-does-not-depend-on-flake-source-via-path/flake.nix

@@ -0,0 +1,14 @@
+{
+  inputs = {
+    crane.url = "github:ipetkov/crane";
+    nixpkgs.follows = "crane/nixpkgs";
+    flake-utils.follows = "crane/flake-utils";
+  };
+
+  outputs = { self, nixpkgs, flake-utils, crane }: flake-utils.lib.eachDefaultSystem
+    (system: {
+      packages.dummy = crane.lib.${system}.mkDummySrc {
+        src = ./.;
+      };
+    });
+}

extra-tests/dummy-does-not-depend-on-flake-source-via-path/test.sh

@@ -0,0 +1,22 @@
+#!/bin/sh
+
+set -eu
+
+scriptDir=$(dirname "$0")
+cd "${scriptDir}"
+
+craneOverride="--override-input crane ../.."
+flakeSrc=$(nix flake metadata ${craneOverride} --json 2>/dev/null | jq -r '.path')
+
+# Get information about the default derivation
+# Then pull out any input sources
+drvSrcs=$(nix show-derivation ${craneOverride} '.#dummy' 2>/dev/null |
+  jq -r 'to_entries[].value.inputSrcs[]')
+
+# And lastly make sure we DO NOT find the flake root source listed
+# or else the dummy derivation will depend on _too much_ (and get
+# invalidated with irrelevant changes)
+if echo "${drvSrcs}" | grep -q -F "${flakeSrc}"; then
+  echo "error: dummy derivation depends on flake source"
+  exit 1
+fi

extra-tests/dummy-does-not-depend-on-flake-source-via-self/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "simple"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+byteorder = "*"

extra-tests/dummy-does-not-depend-on-flake-source-via-self/flake.nix

@@ -0,0 +1,14 @@
+{
+  inputs = {
+    crane.url = "github:ipetkov/crane";
+    nixpkgs.follows = "crane/nixpkgs";
+    flake-utils.follows = "crane/flake-utils";
+  };
+
+  outputs = { self, nixpkgs, flake-utils, crane }: flake-utils.lib.eachDefaultSystem
+    (system: {
+      packages.dummy = crane.lib.${system}.mkDummySrc {
+        src = self;
+      };
+    });
+}

extra-tests/dummy-does-not-depend-on-flake-source-via-self/test.sh

@@ -0,0 +1,22 @@
+#!/bin/sh
+
+set -eu
+
+scriptDir=$(dirname "$0")
+cd "${scriptDir}"
+
+craneOverride="--override-input crane ../.."
+flakeSrc=$(nix flake metadata ${craneOverride} --json 2>/dev/null | jq -r '.path')
+
+# Get information about the default derivation
+# Then pull out any input sources
+drvSrcs=$(nix show-derivation ${craneOverride} '.#dummy' 2>/dev/null |
+  jq -r 'to_entries[].value.inputSrcs[]')
+
+# And lastly make sure we DO NOT find the flake root source listed
+# or else the dummy derivation will depend on _too much_ (and get
+# invalidated with irrelevant changes)
+if echo "${drvSrcs}" | grep -q -F "${flakeSrc}"; then
+  echo "error: dummy derivation depends on flake source"
+  exit 1
+fi

extra-tests/test.sh

@@ -0,0 +1,24 @@
+#!/bin/sh
+
+set -eu
+
+anyFailed=0
+
+runTest() {
+  local testPath="$1"
+
+  if ${testPath}; then
+    echo "pass: ${testPath}"
+  else
+    echo "fail: ${testPath}"
+    anyFailed=1
+  fi
+}
+
+scriptPath=$(dirname "$0")
+cd "${scriptPath}"
+
+runTest ./dummy-does-not-depend-on-flake-source-via-path/test.sh
+runTest ./dummy-does-not-depend-on-flake-source-via-self/test.sh
+
+exit ${anyFailed}

flake.nix

@@ -81,6 +81,7 @@
 
         devShell = pkgs.mkShell {
           nativeBuildInputs = with pkgs; [
+            jq
             nixpkgs-fmt
           ];
         };

lib/mkDummySrc.nix

@@ -7,7 +7,7 @@
 }:
 
 { src
-, cargoLock ? src + "/Cargo.lock"
+, cargoLock ? null
 , ...
 }:
 let
@@ -23,6 +23,93 @@ let
 
   inherit (lib.strings) concatStrings;
 
+  # A quick explanation of what is happening here and why it is done in the way
+  # that it is:
+  #
+  # We want to build a dummy version of the project source. The only things that
+  # we want to keep are:
+  # 1. the Cargo.lock file
+  # 2. any .cargo/config.toml files (unaltered)
+  # 3. any Cargo.toml files stripped down only the attributes that would affect
+  # caching dependencies
+  #
+  # Any other sources are completely ignored, and so, we want to avoid any of those ignored sources
+  # leading to invalidating our caches. Normally if a build script references any data from another
+  # derivation, Nix will consider that entire derivation as an input and any changes to it or its
+  # inputs would invalidate the consumer. But we can try to get a bit clever:
+  #
+  # If we "break up" the input source into smaller parts (i.e. only the parts we care about) we can
+  # avoid the "false dependency" invalidation. One trick to accomplishing this is by "laundering"
+  # the data at evaluation time: we have Nix read the data out, do some TOML transformations, write
+  # it to a fresh file, and then have other derivations consume _that result_. The only thing we
+  # have to be careful about is stripping away any "context" Nix may be tracking for the input as we
+  # work with it: specifically if the `src` input provided by the caller happens to point to a
+  # derivation output (e.g. including using an entire flake source like `{ src = self; }`). Nix
+  # carries this "context" to any other strings operations which touch the derivation output (e.g.
+  # appending path components). In most cases this is the right thing do to since a derivation which
+  # consumes any part of another derivation _probably_ needs to be rebuilt if the latter changes,
+  # but here we will explicitly strip the context out since we want to break this dependency.
+  # This way, adding a comment or editing an ignored field won't lead to rebuilding everything from
+  # scratch!
+  #
+  # The other trick to accomplishing a similar feat (but without rewriting the files at evaluation
+  # time) is to use Nix's source filtering. We give Nix some source path and a function, and Nix
+  # will create a brand new entry in the store after while asking the function whether each and
+  # every file or directory under said path should be kept or not. The result is that only changes
+  # to the kept files would result in rebuilding the consumers.
+  #
+  # Thus to avoid accidental rebuilds, we need to explicitly filter the source to only contain the
+  # files we care about (namely, the .cargo/config.toml files and the Cargo.lock file, we're already
+  # cleaning up the Cargo.toml files during evaluation). There is one extra hurdle we have to clear:
+  # Nix's source filtering operates in a top-down lazy manner. For every directory it encounters it
+  # will ask "should this be kept or not?" If the answer is "no" it skips the directory entirely,
+  # which is reasonable. The problem is the function won't know what files that directory may or may
+  # not contain unless it indicates the directory should be kept. If that happens but the function
+  # rejects all other files under the directory, Nix just keeps the (now empty) directory and moves
+  # on. This isn't a huge problem, except that adding a new directory _anywhere_ in the flake root
+  # would also invalidate everything again.
+  #
+  # Finally we pull one last trick up our sleeve: we do the filtering in two passes! First, at
+  # evaluation time, we walk the input path and look for any interesting files (i.e.
+  # .cargo/config.toml and Cargo.lock files) and remember at what paths they appear relative to the
+  # input source. Then we run the source filtering and use that information to guide which files are
+  # kept. Namely, if the path being filtered is a regular file, we check if its path (relative to
+  # the source root) matches one of our interesting files. If the path being filtered is a
+  # directory, we check if it happens to be an ancestor for an interesting file (i.e. is a prefix of
+  # an interesting file). That way we are left with the smallest possible source needed for our
+  # dummy derivation, and we bring any cache invalidation to a minimum. Whew!
+  mkBasePath = p: (toString p) + "/";
+  uncleanSrcBasePath = mkBasePath src;
+
+  uncleanFiles = findCargoFiles src;
+
+  cargoTomlsBase = uncleanSrcBasePath;
+  inherit (uncleanFiles) cargoTomls;
+
+  cleanSrc =
+    let
+      adjustPaths = builtins.map (p: removePrefix uncleanSrcBasePath (toString p));
+      allUncleanFiles = map
+        (p: removePrefix uncleanSrcBasePath (toString p))
+        # Allow the default `Cargo.lock` location to be picked up here
+        # (if it exists) so it automattically appears in the cleaned source
+        (uncleanFiles.cargoConfigs ++ [ "Cargo.lock" ]);
+    in
+    lib.cleanSourceWith {
+      inherit src;
+      name = "cleaned-mkDummySrc";
+      filter = path: type:
+        let
+          strippedPath = removePrefix uncleanSrcBasePath path;
+          filter = x:
+            if type == "directory" then
+              lib.hasPrefix strippedPath x
+            else
+              x == strippedPath;
+        in
+        lib.any filter allUncleanFiles;
+    };
+
   dummyrs = writeText "dummy.rs" ''
     #![allow(dead_code)]
     pub fn main() {}
@@ -33,28 +120,14 @@ let
     cp -f ${dummyrs} ${prefix}/${path}
   '';
 
-  inherit (findCargoFiles src)
-    cargoTomls
-    cargoConfigs;
-
-  basePath = (toString src) + "/";
-  copyAllCargoConfigs = concatStrings (map
-    (p:
-      let
-        dest = removePrefix basePath (toString p);
-      in
-      ''
-        mkdir -p $out/${dirOf dest}
-        cp ${p} $out/${dest}
-      ''
-    )
-    cargoConfigs
-  );
-
   copyAndStubCargoTomls = concatStrings (map
     (p:
       let
-        cargoTomlDest = removePrefix basePath (toString p);
+        # Safety: all the paths here are fully processed/consumed at evaluation time, so it is is
+        # safe to throw away any context (to the Nix store) the original path may have carried.
+        # Given that we call `cleanSourceWith` earlier, we know that the input `src` must be valid
+        # (or else we would have other errors to deal with)
+        cargoTomlDest = builtins.unsafeDiscardStringContext (removePrefix cargoTomlsBase (toString p));
         parentDir = "$out/${dirOf cargoTomlDest}";
 
         trimmedCargoToml = cleanCargoToml {
@@ -94,14 +167,19 @@ let
     cargoTomls
   );
 
+  # Since we allow the caller to provide a path to *some* Cargo.lock file
+  # we include it in our dummy build only if it was explicitly specified
+  # and the path exists.
   copyCargoLock =
-    if pathExists cargoLock
+    if cargoLock == null
+    then ""
+    else if pathExists cargoLock
     then "cp ${cargoLock} $out/Cargo.lock"
-    else "echo could not find Cargo.lock at src root";
+    else throw "`cargoLock` was specified but the path it points to does not exist: ${cargoLock}";
 in
 runCommandLocal "dummy-src" { } ''
   mkdir -p $out
+  cp --recursive --no-preserve=mode,ownership ${cleanSrc}/. -t $out
   ${copyCargoLock}
-  ${copyAllCargoConfigs}
   ${copyAndStubCargoTomls}
 ''