buildPackage: split installation steps to avoid check phase interfere…

…nce (#771)

CHANGELOG.md

@@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Changed
 * **Breaking**: dropped compatibility for Nix versions below 2.24.10
 * **Breaking**: dropped compatibility for nixpkgs-23.11
+* **Breaking** (technically): `buildPackage`'s installation behavior has been
+  split into two steps: binaries are now installed into a temporary directory as
+  a post build hook (to avoid interference from the check phase clobbering
+  resultant binaries with development features enabled) followed by an actual
+  installation (from said directory) during the install phase. If you use a
+  custom build phase with `buildPackage` you may need to ensure the additional
+  post build hook defined in `installFromCargoBuildLogHook` runs (or follow the
+  error messages to resolve any build issues).
 * `mkDummySrc` has been reworked to match cargo's `autobin` detection logic,
   meaning that only real binary targets defined by the project will be dummified
   if they exist (no more injecting `src/bin/crane-dummy-*`). This does mean that

checks/behaviorChangesWithFeatures/Cargo.toml

@@ -0,0 +1,3 @@
+[workspace]
+members = ["crates/*"]
+resolver = "2"

checks/behaviorChangesWithFeatures/crates/app/Cargo.toml

@@ -0,0 +1,11 @@
+[package]
+name = "app"
+version = "0.1.0"
+edition = "2021"
+publish = false
+
+[dependencies]
+some_dep = { path = "../some_dep" }
+
+[dev-dependencies]
+some_dep = { path = "../some_dep", features = ["dev"] }

checks/behaviorChangesWithFeatures/crates/app/src/main.rs

@@ -0,0 +1,3 @@
+pub fn main() {
+    some_dep::fun();
+}

checks/behaviorChangesWithFeatures/crates/app/tests/test.rs

@@ -0,0 +1,4 @@
+#[test]
+fn some_test() {
+    assert!(std::env!("CARGO_BIN_EXE_app").ends_with("app"));
+}

checks/behaviorChangesWithFeatures/crates/some_dep/Cargo.toml

@@ -0,0 +1,8 @@
+[package]
+name = "some_dep"
+version = "0.1.0"
+edition = "2021"
+publish = false
+
+[features]
+dev = []

checks/behaviorChangesWithFeatures/crates/some_dep/src/lib.rs

@@ -0,0 +1,4 @@
+pub fn fun() {
+    let msg = if cfg!(feature = "dev") { "dev" } else { "prod" };
+    println!("{msg}");
+}

checks/compilesFresh.nix

@@ -36,6 +36,7 @@ let
     inherit installCargoArtifactsMode;
 
     doInstallCargoArtifacts = false;
+    doNotPostBuildInstallCargoBinaries = true;
 
     # NB: explicit call here so that the buildDepsOnly call
     # doesn't inherit our build commands

checks/default.nix

@@ -15,6 +15,20 @@ let
   aarch64Darwin = pkgs.hostPlatform.system == "aarch64-darwin";
 in
 {
+  behaviorChangesWithFeatures =
+    let
+      cmd = myLib.buildPackage {
+        pname = "behaviorChangesWithFeatures";
+        version = "0.0.1";
+        src = myLib.cleanCargoSource ./behaviorChangesWithFeatures;
+        doCheck = true; # Explicitly repro original report
+      };
+    in
+    pkgs.runCommand "behaviorChangesWithFeatures" { } ''
+      diff <(echo prod) <(${cmd}/bin/app)
+      touch $out
+    '';
+
   # https://github.com/ipetkov/crane/issues/411
   bzip2Sys = myLib.buildPackage {
     src = ./bzip2-sys;

docs/API.md

@@ -223,8 +223,8 @@ install hooks.
   - Default value: `false`
 * `installPhaseCommand`: the command(s) which are expected to install the
   derivation's outputs.
-  - Default value: will look for a cargo build log and install all binary
-    targets listed there
+  - Default value: will look for a temporary installation directory created by
+    `installFromCargoBuildLogHook` and then install all of its contents
 
 #### Remove attributes
 The following attributes will be removed before being lowered to
@@ -1877,7 +1877,13 @@ takes two positional arguments:
    * This log can be captured, for example, via `cargo build --message-format
      json-render-diagnostics >cargo-build.json`
 
-**Automatic behavior:** none
+Defines `postBuildInstallFromCargoBuildLog()` which will use a build log produced by
+cargo to find and install any binaries and libraries which have been built into
+a temporary location defined by `$postBuildInstallFromCargoBuildLogOut`
+
+**Automatic behavior:** if `doNotPostBuildInstallCargoBinaries` is not set, then
+`$postBuildInstallFromCargoBuildLogOut` will be set to a temporary directory and
+`postBuildInstallFromCargoBuildLog` will be run as a post build hook.
 
 **Required nativeBuildInputs**: assumes `cargo` is available on the `$PATH`
 

lib/buildPackage.nix

@@ -55,20 +55,29 @@ mkCargoDerivation (cleanedArgs // memoizedArgs // {
   '';
 
   installPhaseCommand = args.installPhaseCommand or ''
-    if [ -n "$cargoBuildLog" -a -f "$cargoBuildLog" ]; then
-      installFromCargoBuildLog "$out" "$cargoBuildLog"
+    if [ -n "$postBuildInstallFromCargoBuildLogOut" -a -d "$postBuildInstallFromCargoBuildLogOut" ]; then
+      echo "actually installing contents of $postBuildInstallFromCargoBuildLogOut to $out"
+      mkdir -p $out
+      find "$postBuildInstallFromCargoBuildLogOut" -mindepth 1 -maxdepth 1 | xargs -r mv -t $out
     else
       echo ${lib.strings.escapeShellArg ''
-        !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-        $cargoBuildLog is either undefined or does not point to a valid file location!
-        By default `buildPackage` will capture cargo's output and use it to determine which binaries
-        should be installed (instead of just guessing based on what is present in cargo's target directory).
-        If you are overriding the derivation with a custom build step, you have two options:
+        !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+        $postBuildInstallFromCargoBuildLogOut is either undefined or does not point to a
+        valid location! By default `buildPackage` will expect that cargo's output was
+        captured and the resulting binaries preinstalled in a temporary location to avoid
+        interference by the check phase.
+
+        If you are defining your own custom build step, you have two options:
         1. override `installPhaseCommand` with the appropriate installation steps
-        2. ensure that cargo's build log is captured in a file and point $cargoBuildLog at it
-        At a minimum, the latter option can be achieved with running:
-            cargoBuildLog=$(mktemp cargoBuildLogXXXX.json)
-            cargo build --release --message-format json-render-diagnostics >"$cargoBuildLog"
+        2. ensure that cargo's build log is captured in a file and point
+           $postBuildInstallFromCargoBuildLogOut at it
+
+        At a minimum, the latter option can be achieved with a build phase that runs:
+             cargoBuildLog=$(mktemp cargoBuildLogXXXX.json)
+             cargo build --release --message-format json-render-diagnostics >"$cargoBuildLog"
+             postBuildInstallFromCargoBuildLogOut=$(mktemp -d cargoBuildTempOutXXXX)
+             installFromCargoBuildLog "$postBuildInstallFromCargoBuildLogOut" "$cargoBuildLog"
+        !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
       ''}
 
       false

lib/setupHooks/installFromCargoBuildLogHook.sh

@@ -42,7 +42,7 @@ function installFromCargoBuildLog() (
     mkdir -p "${loc}"
 
     while IFS= read -r to_install; do
-      echo installing ${to_install}
+      echo installing ${to_install} in "${loc}"
       cp "${to_install}" "${loc}"
     done
 
@@ -54,3 +54,36 @@ function installFromCargoBuildLog() (
 
   echo searching for bins/libs complete
 )
+
+# NB: unfortunately it seems possible for a `cargo test` run to clobber the actual artifacts
+# we are planning to install (see https://github.com/ipetkov/crane/issues/765). To work around
+# this we'll capture any installables immediately after running and actually install them later
+function postBuildInstallFromCargoBuildLog() (
+  if [ -n "${cargoBuildLog:-}" -a -f "${cargoBuildLog}" ]; then
+    installFromCargoBuildLog "${postBuildInstallFromCargoBuildLogOut}" "${cargoBuildLog}"
+  else
+    cat <<-'EOF'
+!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+$cargoBuildLog is either undefined or does not point to a valid file location!
+By default the installFromCargoBuildLogHook will expect that cargo's output
+was captured and can be used to determine which binaries should be installed
+(instead of just guessing based on what is present in cargo's target directory)
+
+If you are defining your own custom build step, you have two options:
+1. Set `doNotPostBuildInstallCargoBinaries = true;` and ensure the installation
+   steps are handled as appropriate.
+2. ensure that cargo's build log is captured in a file and point $cargoBuildLog at it
+
+At a minimum, the latter option can be achieved with a build phase that runs:
+     cargoBuildLog=$(mktemp cargoBuildLogXXXX.json)
+     cargo build --release --message-format json-render-diagnostics >"$cargoBuildLog"
+!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+EOF
+    false
+  fi
+)
+
+if [ -z "${doNotPostBuildInstallCargoBinaries:-}" ]; then
+  postBuildInstallFromCargoBuildLogOut=$(mktemp -d postBuildInstallFromCargoBuildLogOutTempXXX)
+  postBuildHooks+=(postBuildInstallFromCargoBuildLog)
+fi