From d0bede371af1c1af22c22a1ad08f9e0e83026750 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 22 Jul 2024 09:35:14 -0700 Subject: [PATCH] eval.rs: remove is_test and embed mock-nixpkgs.nix This commit does the following things: 1. Purify the environment variables entirely when calling `nix-instantiate`. Opt in to the five environment variables needed in order to run `nix-instantiate` in the Nix sandbox. 2. Remove the hard-coded `NIX_PATH` environment variables in `default.nix` and `package.nix`. These were implicitly sent through to `nix-instantiate`, along with `-I` arguments. Now, only `-I` is used. 3. Replace the `is_test` argument with `#[cfg(test)]` applied to `mutate_nix_instatiate_arguments_based_on_cfg`. While this is slightly more lines of code that the combination of Nix and Rust that it replaces, there is less Nix logic and more Rust logic. Additionally, the Rust logic that is test-only is contained to `cargo check` only rather than being present in the production binary. --- default.nix | 12 ++------- package.nix | 12 ++++++--- src/eval.rs | 71 ++++++++++++++++++++++++++++++++++++++++++----------- src/main.rs | 18 ++++++-------- 4 files changed, 76 insertions(+), 37 deletions(-) diff --git a/default.nix b/default.nix index 8bb7bac1..26feb6d3 100644 --- a/default.nix +++ b/default.nix @@ -14,9 +14,6 @@ let }; inherit (pkgs) lib; - testNixpkgsPath = ./tests/mock-nixpkgs.nix; - nixpkgsLibPath = nixpkgs + "/lib"; - # Needed to make Nix evaluation work inside nix builds initNix = '' export TEST_ROOT=$(pwd)/test-tmp @@ -49,18 +46,13 @@ let packages = { build = pkgs.callPackage ./package.nix { - inherit - nixpkgsLibPath - initNix - testNixpkgsPath - version - ; + inherit initNix version; nix = defaultNixPackage; }; shell = pkgs.mkShell { env.NIX_CHECK_BY_NAME_NIX_PACKAGE = lib.getBin defaultNixPackage; - env.NIX_PATH = "test-nixpkgs=${toString testNixpkgsPath}:test-nixpkgs/lib=${toString nixpkgsLibPath}"; + env.NIX_CHECK_BY_NAME_NIXPKGS_LIB = "${nixpkgs}/lib"; env.RUST_SRC_PATH = "${pkgs.rustPlatform.rustLibSrc}"; inputsFrom = [ packages.build ]; nativeBuildInputs = with pkgs; [ diff --git a/package.nix b/package.nix index a2e1da5b..f93f4bb6 100644 --- a/package.nix +++ b/package.nix @@ -1,6 +1,7 @@ { lib, rustPlatform, + path, nix, nixVersions, clippy, @@ -12,9 +13,7 @@ nixVersions.minimum ], - nixpkgsLibPath, initNix, - testNixpkgsPath, version, }: let @@ -23,6 +22,7 @@ in rustPlatform.buildRustPackage { pname = "nixpkgs-check-by-name"; inherit version; + src = fs.toSource { root = ./.; fileset = fs.unions [ @@ -32,18 +32,24 @@ rustPlatform.buildRustPackage { ./tests ]; }; + cargoLock.lockFile = ./Cargo.lock; + nativeBuildInputs = [ clippy makeWrapper ]; + env.NIX_CHECK_BY_NAME_NIX_PACKAGE = lib.getBin nix; - env.NIX_PATH = "test-nixpkgs=${testNixpkgsPath}:test-nixpkgs/lib=${nixpkgsLibPath}"; + env.NIX_CHECK_BY_NAME_NIXPKGS_LIB = "${path}/lib"; + checkPhase = '' # This path will be symlinked to the current version that is being tested nixPackage=$(mktemp -d)/nix + # For initNix export PATH=$nixPackage/bin:$PATH + # This is what nixpkgs-check-by-name uses export NIX_CHECK_BY_NAME_NIX_PACKAGE=$nixPackage diff --git a/src/eval.rs b/src/eval.rs index b9dfeb97..8094d5cf 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -11,13 +11,11 @@ use crate::validation::ResultIteratorExt as _; use crate::validation::{self, Validation::Success}; use crate::NixFileStore; use relative_path::RelativePathBuf; -use std::fs; -use std::path::Path; +use std::path::{Path, PathBuf}; +use std::{env, fs, process}; use anyhow::Context; use serde::Deserialize; -use std::path::PathBuf; -use std::process; use tempfile::Builder; const EVAL_NIX: &[u8] = include_bytes!("eval.nix"); @@ -100,6 +98,56 @@ pub enum DefinitionVariant { }, } +/// Pass through variables needed to make Nix evaluation work inside Nix build. See `initNix`. +/// If these variables don't exist, assume we're not in a Nix sandbox. +fn pass_through_environment_variables_for_nix_eval_in_nix_build(command: &mut process::Command) { + for variable in [ + "NIX_CONF_DIR", + "NIX_LOCALSTATE_DIR", + "NIX_LOG_DIR", + "NIX_STATE_DIR", + "NIX_STORE_DIR", + ] { + if let Ok(value) = env::var(variable) { + command.env(variable, value); + } + } +} + +#[cfg(not(test))] +fn mutate_nix_instatiate_arguments_based_on_cfg( + _work_dir_path: &Path, + command: &mut process::Command, +) -> anyhow::Result<()> { + command.arg("--show-trace"); + + Ok(()) +} + +/// Tests need to be able to mock out ``; do that for them. +#[cfg(test)] +fn mutate_nix_instatiate_arguments_based_on_cfg( + work_dir_path: &Path, + command: &mut process::Command, +) -> anyhow::Result<()> { + const MOCK_NIXPKGS: &[u8] = include_bytes!("../tests/mock-nixpkgs.nix"); + let mock_nixpkgs_path = work_dir_path.join("mock-nixpkgs.nix"); + fs::write(&mock_nixpkgs_path, MOCK_NIXPKGS)?; + + // Wire it up so that it can be imported as `import { }`. + command.arg("-I"); + command.arg(&format!("test-nixpkgs={}", mock_nixpkgs_path.display())); + + // Retrieve the path to the real nixpkgs lib, then wire it up to `import `. + let nixpkgs_lib = env::var("NIX_CHECK_BY_NAME_NIXPKGS_LIB") + .with_context(|| "Could not get environment variable NIX_CHECK_BY_NAME_NIXPKGS_LIB")?; + + command.arg("-I"); + command.arg(&format!("test-nixpkgs/lib={nixpkgs_lib}")); + + Ok(()) +} + /// Check that the Nixpkgs attribute values corresponding to the packages in `pkgs/by-name` are of /// the form `callPackage { ... }`. See the `./eval.nix` file for how this is /// achieved on the Nix side. @@ -107,7 +155,6 @@ pub fn check_values( nixpkgs_path: &Path, nix_file_store: &mut NixFileStore, package_names: Vec, - is_test: bool, ) -> validation::Result { let work_dir = Builder::new() .prefix("nixpkgs-check-by-name") @@ -132,7 +179,7 @@ pub fn check_values( fs::write(&eval_nix_path, EVAL_NIX)?; // Pinning Nix in this way makes the tool more reproducible - let nix_package = std::env::var("NIX_CHECK_BY_NAME_NIX_PACKAGE") + let nix_package = env::var("NIX_CHECK_BY_NAME_NIX_PACKAGE") .with_context(|| "Could not get environment variable NIX_CHECK_BY_NAME_NIX_PACKAGE")?; // With restrict-eval, only paths in NIX_PATH can be accessed. We explicitly specify them here. @@ -140,6 +187,8 @@ pub fn check_values( command // Capture stderr so that it can be printed later in case of failure .stderr(process::Stdio::piped()) + // Clear environment so that nothing from the outside influences this `nix-instantiate`. + .env_clear() .args([ "--eval", "--json", @@ -159,14 +208,8 @@ pub fn check_values( .arg("-I") .arg(nixpkgs_path); - // Only do these actions if we're not running tests. - if !is_test { - // Clear NIX_PATH to be sure it doesn't influence the result. - // During tests we need to have available. - command.env_remove("NIX_PATH"); - // Show the full Nix error trace, but not in tests because the full trace is super impure. - command.arg("--show-trace"); - } + pass_through_environment_variables_for_nix_eval_in_nix_build(&mut command); + mutate_nix_instatiate_arguments_based_on_cfg(&work_dir_path, &mut command)?; command.arg(eval_nix_path); diff --git a/src/main.rs b/src/main.rs index d71057ed..523d6997 100644 --- a/src/main.rs +++ b/src/main.rs @@ -47,7 +47,7 @@ pub struct Args { fn main() -> ExitCode { let args = Args::parse(); - match process(args.base, args.nixpkgs, false, &mut io::stderr()) { + match process(args.base, args.nixpkgs, &mut io::stderr()) { Ok(true) => ExitCode::SUCCESS, Ok(false) => ExitCode::from(1), Err(e) => { @@ -62,8 +62,6 @@ fn main() -> ExitCode { /// # Arguments /// - `base_nixpkgs`: Path to the base Nixpkgs to run ratchet checks against. /// - `main_nixpkgs`: Path to the main Nixpkgs to check. -/// - `is_test`: Whether we're running a test right now, which e.g. allows access to the -/// NIX_PATH entry /// - `error_writer`: An `io::Write` value to write validation errors to, if any. /// /// # Return value @@ -73,12 +71,11 @@ fn main() -> ExitCode { pub fn process( base_nixpkgs: PathBuf, main_nixpkgs: PathBuf, - is_test: bool, error_writer: &mut W, ) -> anyhow::Result { // Very easy to parallelise this, since it's totally independent - let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs, is_test)); - let main_result = check_nixpkgs(&main_nixpkgs, is_test)?; + let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs)); + let main_result = check_nixpkgs(&main_nixpkgs)?; let base_result = match base_thread.join() { Ok(res) => res?, @@ -154,7 +151,7 @@ pub fn process( /// This does not include ratchet checks, see ../README.md#ratchet-checks /// Instead a `ratchet::Nixpkgs` value is returned, whose `compare` method allows performing the /// ratchet check against another result. -pub fn check_nixpkgs(nixpkgs_path: &Path, is_test: bool) -> validation::Result { +pub fn check_nixpkgs(nixpkgs_path: &Path) -> validation::Result { let mut nix_file_store = NixFileStore::default(); Ok({ @@ -169,9 +166,10 @@ pub fn check_nixpkgs(nixpkgs_path: &Path, is_test: bool) -> validation::Result anyhow::Result<_> { let mut writer = vec![]; - process(base_nixpkgs.to_owned(), path.to_owned(), true, &mut writer) + process(base_nixpkgs.to_owned(), path.to_owned(), &mut writer) .with_context(|| format!("Failed test case {name}"))?; Ok(writer) },