Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Nix version and testing of multiple versions (and fix them for nixVersions.minimum) #83

Merged
merged 3 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
6 changes: 6 additions & 0 deletions default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 ];
Expand All @@ -69,6 +74,7 @@ let
rust-analyzer
rustfmt
treefmtEval.config.build.wrapper
defaultNixPackage
];
};

Expand Down
30 changes: 26 additions & 4 deletions package.nix
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,16 @@
lib,
rustPlatform,
nix,
nixVersions,
clippy,
makeWrapper,

nixVersionsToTest ? [
nix
nixVersions.stable
nixVersions.minimum
],

nixpkgsLibPath,
initNix,
runtimeExprPath,
Expand All @@ -28,19 +35,34 @@ 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
'';
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}
'';
}
6 changes: 5 additions & 1 deletion src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
34 changes: 27 additions & 7 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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()?;
Comment on lines +281 to +283
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A cute way to do this would be to take a dependency on pkgs.emptyDirectory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but since we're not in Nix here, it would be a bit cumbersome, so I think it's fine as-is :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally.


// 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")?;
Expand All @@ -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=======",
Expand Down
18 changes: 1 addition & 17 deletions tests/by-name-failure/expected
Original file line number Diff line number Diff line change
@@ -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.