From f9ae5666ec2d91600775416a8530f8e4f5e51506 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 19 Jul 2024 22:21:22 +0200 Subject: [PATCH 1/3] Use a fixed Nix version This makes the binary always use the same Nix version, instead of getting it from PATH. This makes it more reproducible, because issues from different Nix versions can't occur anymore, e.g. https://github.com/NixOS/nixpkgs-check-by-name/issues/78 --- default.nix | 6 ++++++ package.nix | 4 +++- src/eval.rs | 6 +++++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/default.nix b/default.nix index ee61fbf..404a707 100644 --- a/default.nix +++ b/default.nix @@ -45,6 +45,9 @@ let settings.formatter.shfmt.options = [ "--space-redirects" ]; }; + # The resulting package is built to always use this Nix version, such that the result is reproducible + defaultNixPackage = pkgs.nix; + packages = { build = pkgs.callPackage ./package.nix { inherit @@ -54,10 +57,12 @@ let testNixpkgsPath version ; + nix = defaultNixPackage; }; shell = pkgs.mkShell { env.NIX_CHECK_BY_NAME_EXPR_PATH = toString runtimeExprPath; + env.NIX_CHECK_BY_NAME_NIX_PACKAGE = lib.getBin defaultNixPackage; env.NIX_PATH = "test-nixpkgs=${toString testNixpkgsPath}:test-nixpkgs/lib=${toString nixpkgsLibPath}"; env.RUST_SRC_PATH = "${pkgs.rustPlatform.rustLibSrc}"; inputsFrom = [ packages.build ]; @@ -69,6 +74,7 @@ let rust-analyzer rustfmt treefmtEval.config.build.wrapper + defaultNixPackage ]; }; diff --git a/package.nix b/package.nix index 479d99b..b539f2e 100644 --- a/package.nix +++ b/package.nix @@ -33,6 +33,7 @@ rustPlatform.buildRustPackage { makeWrapper ]; env.NIX_CHECK_BY_NAME_EXPR_PATH = "${runtimeExprPath}"; + env.NIX_CHECK_BY_NAME_NIX_PACKAGE = lib.getBin nix; env.NIX_PATH = "test-nixpkgs=${testNixpkgsPath}:test-nixpkgs/lib=${nixpkgsLibPath}"; preCheck = initNix; postCheck = '' @@ -41,6 +42,7 @@ rustPlatform.buildRustPackage { ''; postInstall = '' wrapProgram $out/bin/nixpkgs-check-by-name \ - --set NIX_CHECK_BY_NAME_EXPR_PATH "$NIX_CHECK_BY_NAME_EXPR_PATH" + --set NIX_CHECK_BY_NAME_EXPR_PATH "$NIX_CHECK_BY_NAME_EXPR_PATH" \ + --set NIX_CHECK_BY_NAME_NIX_PACKAGE ${lib.getBin nix} ''; } diff --git a/src/eval.rs b/src/eval.rs index eba7c44..c72546b 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -125,8 +125,12 @@ pub fn check_values( let expr_path = std::env::var("NIX_CHECK_BY_NAME_EXPR_PATH") .with_context(|| "Could not get environment variable NIX_CHECK_BY_NAME_EXPR_PATH")?; + // Pinning nix in this way makes the tool more reproducible + let nix_package = std::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. - let mut command = process::Command::new("nix-instantiate"); + let mut command = process::Command::new(format!("{nix_package}/bin/nix-instantiate")); command // Capture stderr so that it can be printed later in case of failure .stderr(process::Stdio::piped()) From d551581649c5029a270a15a5a6a67b6ba23a1c03 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 19 Jul 2024 22:25:37 +0200 Subject: [PATCH 2/3] Set up for testing multiple Nix versions While the parent commit makes it use a fixed Nix version, we should also run test for other Nix versions. --- package.nix | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/package.nix b/package.nix index b539f2e..37ff64c 100644 --- a/package.nix +++ b/package.nix @@ -2,9 +2,15 @@ lib, rustPlatform, nix, + nixVersions, clippy, makeWrapper, + nixVersionsToTest ? [ + nix + nixVersions.stable + ], + nixpkgsLibPath, initNix, runtimeExprPath, @@ -28,15 +34,28 @@ rustPlatform.buildRustPackage { }; cargoLock.lockFile = ./Cargo.lock; nativeBuildInputs = [ - nix clippy makeWrapper ]; env.NIX_CHECK_BY_NAME_EXPR_PATH = "${runtimeExprPath}"; env.NIX_CHECK_BY_NAME_NIX_PACKAGE = lib.getBin nix; env.NIX_PATH = "test-nixpkgs=${testNixpkgsPath}:test-nixpkgs/lib=${nixpkgsLibPath}"; - preCheck = initNix; - postCheck = '' + 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 + + ${lib.concatMapStringsSep "\n" (nix: '' + ln -s ${lib.getBin nix} "$nixPackage" + echo "Testing with $(nix --version)" + ${initNix} + runHook cargoCheckHook + rm "$nixPackage" + '') (lib.unique nixVersionsToTest)} + # --tests or --all-targets include tests for linting cargo clippy --all-targets -- -D warnings ''; From 743d860e57ecdae9528e08f7450e07163823d235 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 19 Jul 2024 22:46:43 +0200 Subject: [PATCH 3/3] Fix tests for nixVersions.minimum This is important because that's the Nix version that should really be used in CI --- CONTRIBUTING.md | 4 ++++ package.nix | 1 + src/main.rs | 34 +++++++++++++++++++++++++++------- tests/by-name-failure/expected | 18 +----------------- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e67fdb8..2b47d54 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -61,6 +61,10 @@ Integration tests are declared in [`./tests`](./tests) as subdirectories imitati A file containing the expected standard output. The default is expecting an empty standard output. + This file is matched against the error almost literally, + with the only exception being that the `@REDACTED@` string can match anything, + which is useful for non-deterministic errors. + ## Automation Pinned dependencies are [regularly updated automatically](./.github/workflows/update.yml). diff --git a/package.nix b/package.nix index 37ff64c..b37d2f2 100644 --- a/package.nix +++ b/package.nix @@ -9,6 +9,7 @@ nixVersionsToTest ? [ nix nixVersions.stable + nixVersions.minimum ], nixpkgsLibPath, diff --git a/src/main.rs b/src/main.rs index 870e7dc..33111f9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -181,6 +181,7 @@ mod tests { use crate::process; use crate::utils; use anyhow::Context; + use std::ffi::OsStr; use std::fs; use std::path::Path; use tempfile::{tempdir_in, TempDir}; @@ -277,14 +278,24 @@ mod tests { } else { Path::new("tests/empty-base") }; + // Empty dir, needed so that no warnings are printed when testing older Nix versions + // that don't recognise certain newer keys in nix.conf + let nix_conf_dir = tempdir()?; // We don't want coloring to mess up the tests - let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> { - let mut writer = vec![]; - process(base_nixpkgs.to_owned(), path.to_owned(), true, &mut writer) - .with_context(|| format!("Failed test case {name}"))?; - Ok(writer) - })?; + let writer = temp_env::with_vars( + [ + ("NO_COLOR", Some(OsStr::new("1"))), + // See above comment on nix_conf_dir + ("NIX_CONF_DIR", Some(nix_conf_dir.path().as_os_str())), + ], + || -> anyhow::Result<_> { + let mut writer = vec![]; + process(base_nixpkgs.to_owned(), path.to_owned(), true, &mut writer) + .with_context(|| format!("Failed test case {name}"))?; + Ok(writer) + }, + )?; let expr_path = std::env::var("NIX_CHECK_BY_NAME_EXPR_PATH") .with_context(|| "Could not get environment variable NIX_CHECK_BY_NAME_EXPR_PATH")?; @@ -293,7 +304,16 @@ mod tests { // on the output, which we need to relativise for the tests to succeed everywhere let actual_errors = String::from_utf8_lossy(&writer).replace(&expr_path, "src/eval.nix"); - if actual_errors != expected_errors { + let pattern = format!( + "^{}$", + regex::escape(expected_errors).replace("@REDACTED@", ".*") + ); + + let expected_errors_regex = regex::RegexBuilder::new(&pattern) + .dot_matches_new_line(true) + .build()?; + + if !expected_errors_regex.is_match(&actual_errors) { panic!( "Failed test case {name}, expected these errors:\n=======\n{}\n=======\n\ but got these:\n=======\n{}\n=======", diff --git a/tests/by-name-failure/expected b/tests/by-name-failure/expected index 97c5e98..e76e502 100644 --- a/tests/by-name-failure/expected +++ b/tests/by-name-failure/expected @@ -1,20 +1,4 @@ trace: This should be on stderr! -error: - … while evaluating list element at index 3 - - … while evaluating list element at index 1 - - … while evaluating attribute 'ByName' - - at src/eval.nix:76:7: - - 75| inherit name; - 76| value.ByName = - | ^ - 77| if !pkgs ? ${name} then - - (stack trace truncated; use '--show-trace' to show the full trace) - - error: This is an error! +@REDACTED@error: This is an error!@REDACTED@ - Nix evaluation failed for some package in `pkgs/by-name`, see error above This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.