From 55597845789484cc3fd2aadda9ca4834492d2265 Mon Sep 17 00:00:00 2001 From: Ivan Petkov Date: Sat, 21 Dec 2024 19:05:45 +0000 Subject: [PATCH] buildPackage: split installation steps to avoid check phase interference (#771) --- CHANGELOG.md | 8 +++++ checks/behaviorChangesWithFeatures/Cargo.lock | 14 ++++++++ checks/behaviorChangesWithFeatures/Cargo.toml | 3 ++ .../crates/app/Cargo.toml | 11 ++++++ .../crates/app/src/main.rs | 3 ++ .../crates/app/tests/test.rs | 4 +++ .../crates/some_dep/Cargo.toml | 8 +++++ .../crates/some_dep/src/lib.rs | 4 +++ checks/compilesFresh.nix | 1 + checks/default.nix | 14 ++++++++ docs/API.md | 12 +++++-- lib/buildPackage.nix | 31 ++++++++++------ .../installFromCargoBuildLogHook.sh | 35 ++++++++++++++++++- 13 files changed, 133 insertions(+), 15 deletions(-) create mode 100644 checks/behaviorChangesWithFeatures/Cargo.lock create mode 100644 checks/behaviorChangesWithFeatures/Cargo.toml create mode 100644 checks/behaviorChangesWithFeatures/crates/app/Cargo.toml create mode 100644 checks/behaviorChangesWithFeatures/crates/app/src/main.rs create mode 100644 checks/behaviorChangesWithFeatures/crates/app/tests/test.rs create mode 100644 checks/behaviorChangesWithFeatures/crates/some_dep/Cargo.toml create mode 100644 checks/behaviorChangesWithFeatures/crates/some_dep/src/lib.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 024dcfdc..f08adddf 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/checks/behaviorChangesWithFeatures/Cargo.lock b/checks/behaviorChangesWithFeatures/Cargo.lock new file mode 100644 index 00000000..0d878d79 --- /dev/null +++ b/checks/behaviorChangesWithFeatures/Cargo.lock @@ -0,0 +1,14 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "app" +version = "0.1.0" +dependencies = [ + "some_dep", +] + +[[package]] +name = "some_dep" +version = "0.1.0" diff --git a/checks/behaviorChangesWithFeatures/Cargo.toml b/checks/behaviorChangesWithFeatures/Cargo.toml new file mode 100644 index 00000000..d7671484 --- /dev/null +++ b/checks/behaviorChangesWithFeatures/Cargo.toml @@ -0,0 +1,3 @@ +[workspace] +members = ["crates/*"] +resolver = "2" diff --git a/checks/behaviorChangesWithFeatures/crates/app/Cargo.toml b/checks/behaviorChangesWithFeatures/crates/app/Cargo.toml new file mode 100644 index 00000000..94ba4aaf --- /dev/null +++ b/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"] } diff --git a/checks/behaviorChangesWithFeatures/crates/app/src/main.rs b/checks/behaviorChangesWithFeatures/crates/app/src/main.rs new file mode 100644 index 00000000..a10f9ab3 --- /dev/null +++ b/checks/behaviorChangesWithFeatures/crates/app/src/main.rs @@ -0,0 +1,3 @@ +pub fn main() { + some_dep::fun(); +} diff --git a/checks/behaviorChangesWithFeatures/crates/app/tests/test.rs b/checks/behaviorChangesWithFeatures/crates/app/tests/test.rs new file mode 100644 index 00000000..0f477887 --- /dev/null +++ b/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")); +} diff --git a/checks/behaviorChangesWithFeatures/crates/some_dep/Cargo.toml b/checks/behaviorChangesWithFeatures/crates/some_dep/Cargo.toml new file mode 100644 index 00000000..5f24fdfe --- /dev/null +++ b/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 = [] diff --git a/checks/behaviorChangesWithFeatures/crates/some_dep/src/lib.rs b/checks/behaviorChangesWithFeatures/crates/some_dep/src/lib.rs new file mode 100644 index 00000000..207ce40b --- /dev/null +++ b/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}"); +} diff --git a/checks/compilesFresh.nix b/checks/compilesFresh.nix index 3e1f5e61..275a2385 100644 --- a/checks/compilesFresh.nix +++ b/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 diff --git a/checks/default.nix b/checks/default.nix index e4211940..aa71107e 100644 --- a/checks/default.nix +++ b/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; diff --git a/docs/API.md b/docs/API.md index 0e52c808..7f904cc6 100644 --- a/docs/API.md +++ b/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` diff --git a/lib/buildPackage.nix b/lib/buildPackage.nix index 79e946b8..c47e11ab 100644 --- a/lib/buildPackage.nix +++ b/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 diff --git a/lib/setupHooks/installFromCargoBuildLogHook.sh b/lib/setupHooks/installFromCargoBuildLogHook.sh index 071bbd9a..a340e250 100644 --- a/lib/setupHooks/installFromCargoBuildLogHook.sh +++ b/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