nixpkgs-check-by-name: Intermediate error representation refactor

pkgs/test/nixpkgs-check-by-name/Cargo.toml

@@ -13,6 +13,7 @@ serde = { version = "1.0.185", features = ["derive"] }
 anyhow = "1.0"
 lazy_static = "1.4.0"
 colored = "2.0.4"
+itertools = "0.11.0"
 
 [dev-dependencies]
 temp-env = "0.3.5"

pkgs/test/nixpkgs-check-by-name/README.md

@@ -1,6 +1,6 @@
 # Nixpkgs pkgs/by-name checker
 
-This directory implements a program to check the [validity](#validity-checks) of the `pkgs/by-name` Nixpkgs directory once introduced.
+This directory implements a program to check the [validity](#validity-checks) of the `pkgs/by-name` Nixpkgs directory.
 It is being used by [this GitHub Actions workflow](../../../.github/workflows/check-by-name.yml).
 This is part of the implementation of [RFC 140](https://github.com/NixOS/rfcs/pull/140).
 
@@ -24,7 +24,7 @@ This API may be changed over time if the CI workflow making use of it is adjuste
   - `2`: If an unexpected I/O error occurs
 - Standard error:
   - Informative messages
-  - Error messages if validation is not successful
+  - Detected problems if validation is not successful
 
 ## Validity checks
 

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

@@ -1,12 +1,12 @@
+use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::structure;
-use crate::utils::ErrorWriter;
+use crate::validation::{self, Validation::Success};
 use crate::Version;
 use std::path::Path;
 
 use anyhow::Context;
 use serde::Deserialize;
 use std::collections::HashMap;
-use std::io;
 use std::path::PathBuf;
 use std::process;
 use tempfile::NamedTempFile;
@@ -40,12 +40,12 @@ const EXPR: &str = include_str!("eval.nix");
 /// Check that the Nixpkgs attribute values corresponding to the packages in pkgs/by-name are
 /// of the form `callPackage <package_file> { ... }`.
 /// See the `eval.nix` file for how this is achieved on the Nix side
-pub fn check_values<W: io::Write>(
+pub fn check_values(
     version: Version,
-    error_writer: &mut ErrorWriter<W>,
-    nixpkgs: &structure::Nixpkgs,
+    nixpkgs_path: &Path,
+    package_names: Vec<String>,
     eval_accessible_paths: Vec<&Path>,
-) -> anyhow::Result<()> {
+) -> 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")?;
@@ -55,7 +55,7 @@ pub fn check_values<W: io::Write>(
     // entry is needed.
     let attrs_file_path = attrs_file.path().canonicalize()?;
 
-    serde_json::to_writer(&attrs_file, &nixpkgs.package_names).context(format!(
+    serde_json::to_writer(&attrs_file, &package_names).context(format!(
         "Failed to serialise the package names to the temporary path {}",
         attrs_file_path.display()
     ))?;
@@ -87,9 +87,9 @@ pub fn check_values<W: io::Write>(
         .arg(&attrs_file_path)
         // Same for the nixpkgs to test
         .args(["--arg", "nixpkgsPath"])
-        .arg(&nixpkgs.path)
+        .arg(nixpkgs_path)
         .arg("-I")
-        .arg(&nixpkgs.path);
+        .arg(nixpkgs_path);
 
     // Also add extra paths that need to be accessible
     for path in eval_accessible_paths {
@@ -111,52 +111,54 @@ pub fn check_values<W: io::Write>(
             String::from_utf8_lossy(&result.stdout)
         ))?;
 
-    for package_name in &nixpkgs.package_names {
-        let relative_package_file = structure::Nixpkgs::relative_file_for_package(package_name);
-        let absolute_package_file = nixpkgs.path.join(&relative_package_file);
+    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);
 
-        if let Some(attribute_info) = actual_files.get(package_name) {
-            let valid = match &attribute_info.variant {
-                AttributeVariant::AutoCalled => true,
-                AttributeVariant::CallPackage { path, empty_arg } => {
-                    let correct_file = if let Some(call_package_path) = path {
-                        absolute_package_file == *call_package_path
-                    } else {
-                        false
-                    };
-                    // Only check for the argument to be non-empty if the version is V1 or
-                    // higher
-                    let non_empty = if version >= Version::V1 {
-                        !empty_arg
-                    } else {
-                        true
-                    };
-                    correct_file && non_empty
-                }
-                AttributeVariant::Other => false,
-            };
-
-            if !valid {
-                error_writer.write(&format!(
-                    "pkgs.{package_name}: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage {} {{ ... }}` with a non-empty second argument.",
-                    relative_package_file.display()
-                ))?;
-                continue;
-            }
+            if let Some(attribute_info) = actual_files.get(package_name) {
+                let valid = match &attribute_info.variant {
+                    AttributeVariant::AutoCalled => true,
+                    AttributeVariant::CallPackage { path, empty_arg } => {
+                        let correct_file = if let Some(call_package_path) = path {
+                            absolute_package_file == *call_package_path
+                        } else {
+                            false
+                        };
+                        // Only check for the argument to be non-empty if the version is V1 or
+                        // higher
+                        let non_empty = if version >= Version::V1 {
+                            !empty_arg
+                        } else {
+                            true
+                        };
+                        correct_file && non_empty
+                    }
+                    AttributeVariant::Other => false,
+                };
 
-            if !attribute_info.is_derivation {
-                error_writer.write(&format!(
-                    "pkgs.{package_name}: This attribute defined by {} is not a derivation",
-                    relative_package_file.display()
-                ))?;
+                if !valid {
+                    NixpkgsProblem::WrongCallPackage {
+                        relative_package_file: relative_package_file.clone(),
+                        package_name: package_name.clone(),
+                    }
+                    .into()
+                } else if !attribute_info.is_derivation {
+                    NixpkgsProblem::NonDerivation {
+                        relative_package_file: relative_package_file.clone(),
+                        package_name: package_name.clone(),
+                    }
+                    .into()
+                } else {
+                    Success(())
+                }
+            } else {
+                NixpkgsProblem::UndefinedAttr {
+                    relative_package_file: relative_package_file.clone(),
+                    package_name: package_name.clone(),
+                }
+                .into()
             }
-        } else {
-            error_writer.write(&format!(
-                "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}",
-                relative_package_file.display()
-            ))?;
-            continue;
-        }
-    }
-    Ok(())
+        },
+    )))
 }

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

@@ -1,16 +1,19 @@
 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 std::io;
 use std::path::{Path, PathBuf};
 use std::process::ExitCode;
-use structure::Nixpkgs;
-use utils::ErrorWriter;
 
 /// Program to check the validity of pkgs/by-name
 #[derive(Parser, Debug)]
@@ -63,8 +66,8 @@ fn main() -> ExitCode {
 ///
 /// # Return value
 /// - `Err(e)` if an I/O-related error `e` occurred.
-/// - `Ok(false)` if the structure is invalid, all the structural errors have been written to `error_writer`.
-/// - `Ok(true)` if the structure is valid, nothing will have been written to `error_writer`.
+/// - `Ok(false)` if there are problems, all of which will be written to `error_writer`.
+/// - `Ok(true)` if there are no problems
 pub fn check_nixpkgs<W: io::Write>(
     nixpkgs_path: &Path,
     version: Version,
@@ -76,31 +79,38 @@ pub fn check_nixpkgs<W: io::Write>(
         nixpkgs_path.display()
     ))?;
 
-    // Wraps the error_writer to print everything in red, and tracks whether anything was printed
-    // at all. Later used to figure out if the structure was valid or not.
-    let mut error_writer = ErrorWriter::new(error_writer);
-
-    if !nixpkgs_path.join(structure::BASE_SUBPATH).exists() {
+    let check_result = if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
         eprintln!(
             "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
-            structure::BASE_SUBPATH
+            utils::BASE_SUBPATH
         );
+        Success(())
     } else {
-        let nixpkgs = Nixpkgs::new(&nixpkgs_path, &mut error_writer)?;
+        match check_structure(&nixpkgs_path)? {
+            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)?
+            }
+        }
+    };
 
-        if error_writer.empty {
-            // Only if we could successfully parse the structure, we do the semantic checks
-            eval::check_values(version, &mut error_writer, &nixpkgs, eval_accessible_paths)?;
-            references::check_references(&mut error_writer, &nixpkgs)?;
+    match check_result {
+        Failure(errors) => {
+            for error in errors {
+                writeln!(error_writer, "{}", error.to_string().red())?
+            }
+            Ok(false)
         }
+        Success(_) => Ok(true),
     }
-    Ok(error_writer.empty)
 }
 
 #[cfg(test)]
 mod tests {
     use crate::check_nixpkgs;
-    use crate::structure;
+    use crate::utils;
     use crate::Version;
     use anyhow::Context;
     use std::fs;
@@ -145,7 +155,7 @@ mod tests {
             return Ok(());
         }
 
-        let base = path.join(structure::BASE_SUBPATH);
+        let base = path.join(utils::BASE_SUBPATH);
 
         fs::create_dir_all(base.join("fo/foo"))?;
         fs::write(base.join("fo/foo/package.nix"), "{ someDrv }: someDrv")?;