tests.nixpkgs-check-by-name: Custom Validation type and improvements (82e708c1) · Commits · nix / nixpkgs

pkgs/test/nixpkgs-check-by-name/src/check_result.rs

deleted100644 → 0

+0 −74

Original line number	Diff line number	Diff line
		use crate::nixpkgs_problem::NixpkgsProblem;
		use itertools::concat;
		use itertools::{
		Either,
		Either::{Left, Right},
		Itertools,
		};

		/// A type alias representing the result of a check, either:
		/// - Err(anyhow::Error): A fatal failure, typically I/O errors.
		/// Such failures are not caused by the files in Nixpkgs.
		/// This hints at a bug in the code or a problem with the deployment.
		/// - Ok(Left(Vec<NixpkgsProblem>)): A non-fatal problem with the Nixpkgs files.
		/// Further checks can be run even with this result type.
		/// Such problems can be fixed by changing the Nixpkgs files.
		/// - Ok(Right(A)): A successful (potentially intermediate) result with an arbitrary value.
		/// No fatal errors have occurred and no problems have been found with Nixpkgs.
		pub type CheckResult<A> = anyhow::Result<Either<Vec<NixpkgsProblem>, A>>;

		/// Map a `CheckResult<I>` to a `CheckResult<O>` by applying a function to the
		/// potentially contained value in case of success.
		pub fn map<I, O>(check_result: CheckResult<I>, f: impl FnOnce(I) -> O) -> CheckResult<O> {
		Ok(check_result?.map_right(f))
		}

		/// Create a successfull `CheckResult<A>` from a value `A`.
		pub fn ok<A>(value: A) -> CheckResult<A> {
		Ok(Right(value))
		}

		impl NixpkgsProblem {
		/// Create a `CheckResult<A>` from a single check problem
		pub fn into_result<A>(self) -> CheckResult<A> {
		Ok(Left(vec![self]))
		}
		}

		/// Combine two check results, both of which need to be successful for the return value to be successful.
		/// The `NixpkgsProblem`s of both sides are returned concatenated.
		pub fn and<A>(first: CheckResult<()>, second: CheckResult<A>) -> CheckResult<A> {
		match (first?, second?) {
		(Right(_), Right(right_value)) => Ok(Right(right_value)),
		(Left(errors), Right(_)) => Ok(Left(errors)),
		(Right(_), Left(errors)) => Ok(Left(errors)),
		(Left(errors_l), Left(errors_r)) => Ok(Left(concat([errors_l, errors_r]))),
		}
		}

		/// Combine many checks results into a single one.
		/// All given check results need to be successful in order for the returned check result to be successful,
		/// in which case the returned check result value contains each a `Vec` of each individual result value.
		/// The `NixpkgsProblem`s of all results are returned concatenated.
		pub fn sequence<A>(check_results: impl IntoIterator<Item = CheckResult<A>>) -> CheckResult<Vec<A>> {
		let (errors, values): (Vec<_>, Vec<_>) = check_results
		.into_iter()
		.collect::<anyhow::Result<Vec<_>>>()?
		.into_iter()
		.partition_map(\|r\| r);

		// To combine the errors from the results we flatten all the error Vec's into a new Vec
		// This is not very efficient, but doesn't matter because generally we should have no errors
		let flattened_errors = errors.into_iter().flatten().collect::<Vec<_>>();

		if flattened_errors.is_empty() {
		Ok(Right(values))
		} else {
		Ok(Left(flattened_errors))
		}
		}

		/// Like `sequence`, but replace the resulting value with ()
		pub fn sequence_<A>(check_results: impl IntoIterator<Item = CheckResult<A>>) -> CheckResult<()> {
		map(sequence(check_results), \|_\| ())
		}

pkgs/test/nixpkgs-check-by-name/src/eval.rs

+45 −44

Original line number	Diff line number	Diff line
		use crate::check_result;
		use crate::check_result::CheckResult;
		use crate::nixpkgs_problem::NixpkgsProblem;
		use crate::structure;
		use crate::validation::{self, Validation::Success};
		use crate::Version;
		use std::path::Path;

		@@ -46,7 +45,7 @@ pub fn check_values(
		nixpkgs_path: &Path,
		package_names: Vec<String>,
		eval_accessible_paths: Vec<&Path>,
		) -> CheckResult<()> {
		) -> validation::Result<()> {
		// Write the list of packages we need to check into a temporary JSON file.
		// This can then get read by the Nix evaluation.
		let attrs_file = NamedTempFile::new().context("Failed to create a temporary file")?;
		@@ -112,7 +111,8 @@ pub fn check_values(
		String::from_utf8_lossy(&result.stdout)
		))?;

		check_result::sequence_(package_names.iter().map(\|package_name\| {
		Ok(validation::sequence_(package_names.iter().map(
		\|package_name\| {
		let relative_package_file = structure::relative_file_for_package(package_name);
		let absolute_package_file = nixpkgs_path.join(&relative_package_file);

		@@ -142,22 +142,23 @@ pub fn check_values(
		relative_package_file: relative_package_file.clone(),
		package_name: package_name.clone(),
		}
		.into_result()
		.into()
		} else if !attribute_info.is_derivation {
		NixpkgsProblem::NonDerivation {
		relative_package_file: relative_package_file.clone(),
		package_name: package_name.clone(),
		}
		.into_result()
		.into()
		} else {
		check_result::ok(())
		Success(())
		}
		} else {
		NixpkgsProblem::UndefinedAttr {
		relative_package_file: relative_package_file.clone(),
		package_name: package_name.clone(),
		}
		.into_result()
		.into()
		}
		}))
		},
		)))
		}

pkgs/test/nixpkgs-check-by-name/src/main.rs

+10 −12

Original line number	Diff line number	Diff line
		mod check_result;
		mod eval;
		mod nixpkgs_problem;
		mod references;
		mod structure;
		mod utils;
		mod validation;

		use crate::structure::check_structure;
		use crate::validation::Validation::Failure;
		use crate::validation::Validation::Success;
		use anyhow::Context;
		use clap::{Parser, ValueEnum};
		use colored::Colorize;
		use itertools::{
		Either,
		Either::{Left, Right},
		};
		use std::io;
		use std::path::{Path, PathBuf};
		use std::process::ExitCode;
		@@ -86,26 +84,26 @@ pub fn check_nixpkgs<W: io::Write>(
		"Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
		utils::BASE_SUBPATH
		);
		check_result::ok(())
		Success(())
		} else {
		match check_structure(&nixpkgs_path)? {
		Left(errors) => Ok(Left(errors)),
		Right(package_names) =>
		Failure(errors) => Failure(errors),
		Success(package_names) =>
		// Only if we could successfully parse the structure, we do the evaluation checks
		{
		eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths)
		eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths)?
		}
		}
		};

		match check_result? {
		Either::Left(errors) => {
		match check_result {
		Failure(errors) => {
		for error in errors {
		writeln!(error_writer, "{}", error.to_string().red())?
		}
		Ok(false)
		}
		Either::Right(_) => Ok(true),
		Success(_) => Ok(true),
		}
		}

pkgs/test/nixpkgs-check-by-name/src/references.rs

+77 −69

Original line number	Diff line number	Diff line
		use crate::check_result;
		use crate::check_result::CheckResult;
		use crate::nixpkgs_problem::NixpkgsProblem;
		use crate::utils;
		use crate::utils::LineIndex;
		use crate::validation::{self, ResultIteratorExt, Validation::Success};

		use anyhow::Context;
		use rnix::{Root, SyntaxKind::NODE_PATH};
		@@ -23,7 +22,7 @@ struct PackageContext<'a> {
		pub fn check_references(
		relative_package_dir: &Path,
		absolute_package_dir: &Path,
		) -> CheckResult<()> {
		) -> validation::Result<()> {
		let context = PackageContext {
		relative_package_dir: &relative_package_dir.to_path_buf(),
		absolute_package_dir: &absolute_package_dir.to_path_buf(),
		@@ -38,10 +37,10 @@ pub fn check_references(
		}

		/// Checks for a specific path to not have references outside
		fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		fn check_path(context: &PackageContext, subpath: &Path) -> validation::Result<()> {
		let path = context.absolute_package_dir.join(subpath);

		if path.is_symlink() {
		Ok(if path.is_symlink() {
		// Check whether the symlink resolves to outside the package directory
		match path.canonicalize() {
		Ok(target) => {
		@@ -52,9 +51,9 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		relative_package_dir: context.relative_package_dir.clone(),
		subpath: subpath.to_path_buf(),
		}
		.into_result()
		.into()
		} else {
		check_result::ok(())
		Success(())
		}
		}
		Err(io_error) => NixpkgsProblem::UnresolvableSymlink {
		@@ -62,15 +61,20 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		subpath: subpath.to_path_buf(),
		io_error,
		}
		.into_result(),
		.into(),
		}
		} else if path.is_dir() {
		// Recursively check each entry
		check_result::sequence_(utils::read_dir_sorted(&path)?.into_iter().map(\|entry\| {
		validation::sequence_(
		utils::read_dir_sorted(&path)?
		.into_iter()
		.map(\|entry\| {
		let entry_subpath = subpath.join(entry.file_name());
		check_path(context, &entry_subpath)
		.context(format!("Error while recursing into {}", subpath.display()))
		}))
		})
		.collect_vec()?,
		)
		} else if path.is_file() {
		// Only check Nix files
		if let Some(ext) = path.extension() {
		@@ -78,22 +82,22 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		check_nix_file(context, subpath).context(format!(
		"Error while checking Nix file {}",
		subpath.display()
		))
		))?
		} else {
		check_result::ok(())
		Success(())
		}
		} else {
		check_result::ok(())
		Success(())
		}
		} else {
		// This should never happen, git doesn't support other file types
		anyhow::bail!("Unsupported file type for path {}", subpath.display());
		}
		})
		}

		/// Check whether a nix file contains path expression references pointing outside the package
		/// directory
		fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		fn check_nix_file(context: &PackageContext, subpath: &Path) -> validation::Result<()> {
		let path = context.absolute_package_dir.join(subpath);
		let parent_dir = path.parent().context(format!(
		"Could not get parent of path {}",
		@@ -105,23 +109,24 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> {

		let root = Root::parse(&contents);
		if let Some(error) = root.errors().first() {
		return NixpkgsProblem::CouldNotParseNix {
		return Ok(NixpkgsProblem::CouldNotParseNix {
		relative_package_dir: context.relative_package_dir.clone(),
		subpath: subpath.to_path_buf(),
		error: error.clone(),
		}
		.into_result();
		.into());
		}

		let line_index = LineIndex::new(&contents);

		check_result::sequence_(root.syntax().descendants().map(\|node\| {
		Ok(validation::sequence_(root.syntax().descendants().map(
		\|node\| {
		let text = node.text().to_string();
		let line = line_index.line(node.text_range().start().into());

		if node.kind() != NODE_PATH {
		// We're only interested in Path expressions
		check_result::ok(())
		Success(())
		} else if node.children().count() != 0 {
		// Filters out ./foo/${bar}/baz
		// TODO: We can just check ./foo
		@@ -131,7 +136,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		line,
		text,
		}
		.into_result()
		.into()
		} else if text.starts_with('<') {
		// Filters out search paths like <nixpkgs>
		NixpkgsProblem::SearchPath {
		@@ -140,7 +145,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		line,
		text,
		}
		.into_result()
		.into()
		} else {
		// Resolves the reference of the Nix path
		// turning `../baz` inside `/foo/bar/default.nix` to `/foo/baz`
		@@ -149,16 +154,18 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		// Then checking if it's still in the package directory
		// No need to handle the case of it being inside the directory, since we scan through the
		// entire directory recursively anyways
		if let Err(_prefix_error) = target.strip_prefix(context.absolute_package_dir) {
		if let Err(_prefix_error) =
		target.strip_prefix(context.absolute_package_dir)
		{
		NixpkgsProblem::OutsidePathReference {
		relative_package_dir: context.relative_package_dir.clone(),
		subpath: subpath.to_path_buf(),
		line,
		text,
		}
		.into_result()
		.into()
		} else {
		check_result::ok(())
		Success(())
		}
		}
		Err(e) => NixpkgsProblem::UnresolvablePathReference {
		@@ -168,8 +175,9 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> {
		text,
		io_error: e,
		}
		.into_result(),
		.into(),
		}
		}
		}))
		},
		)))
		}

pkgs/test/nixpkgs-check-by-name/src/structure.rs

+59 −61

Original line number	Diff line number	Diff line
		use crate::check_result;
		use crate::check_result::CheckResult;
		use crate::nixpkgs_problem::NixpkgsProblem;
		use crate::references;
		use crate::utils;
		use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME};
		use crate::validation::{self, ResultIteratorExt, Validation::Success};
		use itertools::concat;
		use lazy_static::lazy_static;
		use regex::Regex;
		@@ -35,24 +34,25 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf {

		/// Check the structure of Nixpkgs, returning the attribute names that are defined in
		/// `pkgs/by-name`
		pub fn check_structure(path: &Path) -> CheckResult<Vec<String>> {
		pub fn check_structure(path: &Path) -> validation::Result<Vec<String>> {
		let base_dir = path.join(BASE_SUBPATH);

		let shard_results = utils::read_dir_sorted(&base_dir)?
		.into_iter()
		.map(\|shard_entry\| {
		.map(\|shard_entry\| -> validation::Result<_> {
		let shard_path = shard_entry.path();
		let shard_name = shard_entry.file_name().to_string_lossy().into_owned();
		let relative_shard_path = relative_dir_for_shard(&shard_name);

		if shard_name == "README.md" {
		Ok(if shard_name == "README.md" {
		// README.md is allowed to be a file and not checked
		check_result::ok(vec![])

		Success(vec![])
		} else if !shard_path.is_dir() {
		NixpkgsProblem::ShardNonDir {
		relative_shard_path: relative_shard_path.clone(),
		}
		.into_result()
		.into()
		// we can't check for any other errors if it's a file, since there's no subdirectories to check
		} else {
		let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name);
		@@ -61,9 +61,9 @@ pub fn check_structure(path: &Path) -> CheckResult<Vec<String>> {
		relative_shard_path: relative_shard_path.clone(),
		shard_name: shard_name.clone(),
		}
		.into_result()
		.into()
		} else {
		check_result::ok(())
		Success(())
		};

		let entries = utils::read_dir_sorted(&shard_path)?;
		@@ -80,21 +80,25 @@ pub fn check_structure(path: &Path) -> CheckResult<Vec<String>> {
		first: l.file_name(),
		second: r.file_name(),
		}
		.into_result::<()>()
		.into()
		});

		let result = check_result::and(result, check_result::sequence_(duplicate_results));
		let result = result.and(validation::sequence_(duplicate_results));

		let package_results = entries.into_iter().map(\|package_entry\| {
		let package_results = entries
		.into_iter()
		.map(\|package_entry\| {
		check_package(path, &shard_name, shard_name_valid, package_entry)
		});
		})
		.collect_vec()?;

		check_result::and(result, check_result::sequence(package_results))
		}
		});
		result.and(validation::sequence(package_results))
		})
		})
		.collect_vec()?;

		// Combine the package names conatained within each shard into a longer list
		check_result::map(check_result::sequence(shard_results), concat)
		Ok(validation::sequence(shard_results).map(concat))
		}

		fn check_package(
		@@ -102,16 +106,16 @@ fn check_package(
		shard_name: &str,
		shard_name_valid: bool,
		package_entry: DirEntry,
		) -> CheckResult<String> {
		) -> validation::Result<String> {
		let package_path = package_entry.path();
		let package_name = package_entry.file_name().to_string_lossy().into_owned();
		let relative_package_dir = PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}"));

		if !package_path.is_dir() {
		Ok(if !package_path.is_dir() {
		NixpkgsProblem::PackageNonDir {
		relative_package_dir: relative_package_dir.clone(),
		}
		.into_result()
		.into()
		} else {
		let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name);
		let result = if !package_name_valid {
		@@ -119,15 +123,13 @@ fn check_package(
		relative_package_dir: relative_package_dir.clone(),
		package_name: package_name.clone(),
		}
		.into_result()
		.into()
		} else {
		check_result::ok(())
		Success(())
		};

		let correct_relative_package_dir = relative_dir_for_package(&package_name);
		let result = check_result::and(
		result,
		if relative_package_dir != correct_relative_package_dir {
		let result = result.and(if relative_package_dir != correct_relative_package_dir {
		// Only show this error if we have a valid shard and package name
		// Because if one of those is wrong, you should fix that first
		if shard_name_valid && package_name_valid {
		@@ -135,38 +137,34 @@ fn check_package(
		relative_package_dir: relative_package_dir.clone(),
		correct_relative_package_dir: correct_relative_package_dir.clone(),
		}
		.into_result()
		.into()
		} else {
		check_result::ok(())
		Success(())
		}
		} else {
		check_result::ok(())
		},
		);
		Success(())
		});

		let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME);
		let result = check_result::and(
		result,
		if !package_nix_path.exists() {
		let result = result.and(if !package_nix_path.exists() {
		NixpkgsProblem::PackageNixNonExistent {
		relative_package_dir: relative_package_dir.clone(),
		}
		.into_result()
		.into()
		} else if package_nix_path.is_dir() {
		NixpkgsProblem::PackageNixDir {
		relative_package_dir: relative_package_dir.clone(),
		}
		.into_result()
		.into()
		} else {
		check_result::ok(())
		},
		);
		Success(())
		});

		let result = check_result::and(
		result,
		references::check_references(&relative_package_dir, &path.join(&relative_package_dir)),
		);
		let result = result.and(references::check_references(
		&relative_package_dir,
		&path.join(&relative_package_dir),
		)?);

		check_result::map(result, \|_\| package_name.clone())
		}
		result.map(\|_\| package_name.clone())
		})
		}