colin/nixpkgs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #272395 from tweag/by-name-migrate-empty-arg

Browse Source 
tests.nixpkgs-check-by-name: Implement gradual empty arg check migration
This commit is contained in:
Silvan Mosberger 2023-12-15 19:08:11 +01:00 committed by GitHub
commit 2a107bc64b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
17 changed files with 275 additions and 110 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
pkgs/test/nixpkgs-check-by-name/README.md
View File

@ -4,27 +4,14 @@ This directory implements a program to check the [validity](#validity-checks) of
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).
## API
## Interface
This API may be changed over time if the CI workflow making use of it is adjusted to deal with the change appropriately.
The interface of the tool is shown with `--help`:
```
cargo run -- --help
```
- Command line: `nixpkgs-check-by-name <NIXPKGS>`
- Arguments:
- `<NIXPKGS>`: The path to the Nixpkgs to check
- `--version <VERSION>`: The version of the checks to perform.
Possible values:
- `v0` (default)
- `v1`
See [validation](#validity-checks) for the differences.
- Exit code:
- `0`: If the [validation](#validity-checks) is successful
- `1`: If the [validation](#validity-checks) is not successful
- `2`: If an unexpected I/O error occurs
- Standard error:
- Informative messages
- Detected problems if validation is not successful
The interface may be changed over time only if the CI workflow making use of it is adjusted to deal with the change appropriately.
## Validity checks
@ -32,7 +19,7 @@ These checks are performed by this tool:
### File structure checks
- `pkgs/by-name` must only contain subdirectories of the form `${shard}/${name}`, called _package directories_.
- The `name`'s of package directories must be unique when lowercased
- The `name`'s of package directories must be unique when lowercased.
- `name` is a string only consisting of the ASCII characters `a-z`, `A-Z`, `0-9`, `-` or `_`.
- `shard` is the lowercased first two letters of `name`, expressed in Nix: `shard = toLower (substring 0 2 name)`.
- Each package directory must contain a `package.nix` file and may contain arbitrary other files.
@ -41,9 +28,21 @@ These checks are performed by this tool:
- Each package directory must not refer to files outside itself using symlinks or Nix path expressions.
### Nix evaluation checks
- `pkgs.${name}` is defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`.
- **Only after --version v1**: If `pkgs.${name}` is not auto-called from `pkgs/by-name`, `args` must not be empty
- `pkgs.lib.isDerivation pkgs.${name}` is `true`.
- For each package directory, the `pkgs.${name}` attribute must be defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`.
- For each package directory, `pkgs.lib.isDerivation pkgs.${name}` must be `true`.
### Ratchet checks
Furthermore, this tool implements certain [ratchet](https://qntm.org/ratchet) checks.
This allows gradually phasing out deprecated patterns without breaking the base branch or having to migrate it all at once.
It works by not allowing new instances of the pattern to be introduced, but allowing already existing instances.
The existing instances are coming from `<BASE_NIXPKGS>`, which is then checked against `<NIXPKGS>` for new instances.
Ratchets should be removed eventually once the pattern is not used anymore.
The current ratchets are:
- New manual definitions of `pkgs.${name}` (e.g. in `pkgs/top-level/all-packages.nix`) with `args = { }`
(see [nix evaluation checks](#nix-evaluation-checks)) must not be introduced.
## Development
@ -86,6 +85,10 @@ Tests are declared in [`./tests`](./tests) as subdirectories imitating Nixpkgs w
allowing the simulation of package overrides to the real [`pkgs/top-level/all-packages.nix`](../../top-level/all-packages.nix`).
The default is an empty overlay.
- `base` (optional):
Contains another subdirectory imitating Nixpkgs with potentially any of the above structures.
This is used for [ratchet checks](#ratchet-checks).
- `expected` (optional):
A file containing the expected standard output.
The default is expecting an empty standard output.

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

@ -1,7 +1,7 @@
use crate::nixpkgs_problem::NixpkgsProblem;
use crate::ratchet;
use crate::structure;
use crate::validation::{self, Validation::Success};
use crate::Version;
use std::path::Path;
use anyhow::Context;
@ -39,11 +39,10 @@ enum AttributeVariant {
/// of the form `callPackage <package_file> { ... }`.
/// See the `eval.nix` file for how this is achieved on the Nix side
pub fn check_values(
version: Version,
nixpkgs_path: &Path,
package_names: Vec<String>,
eval_accessible_paths: Vec<&Path>,
) -> validation::Result<()> {
eval_accessible_paths: &[&Path],
) -> validation::Result<ratchet::Nixpkgs> {
// 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")?;
@ -110,39 +109,13 @@ pub fn check_values(
String::from_utf8_lossy(&result.stdout)
))?;
Ok(validation::sequence_(package_names.iter().map(
|package_name| {
let relative_package_file = structure::relative_file_for_package(package_name);
Ok(
validation::sequence(package_names.into_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 {
NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.clone(),
package_name: package_name.clone(),
}
.into()
} else if !attribute_info.is_derivation {
if let Some(attribute_info) = actual_files.get(&package_name) {
let check_result = if !attribute_info.is_derivation {
NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.clone(),
package_name: package_name.clone(),
@ -150,7 +123,44 @@ pub fn check_values(
.into()
} else {
Success(())
}
};
let check_result = check_result.and(match &attribute_info.variant {
AttributeVariant::AutoCalled => Success(ratchet::Package {
empty_non_auto_called: ratchet::EmptyNonAutoCalled::Valid,
}),
AttributeVariant::CallPackage { path, empty_arg } => {
let correct_file = if let Some(call_package_path) = path {
absolute_package_file == *call_package_path
} else {
false
};
if correct_file {
Success(ratchet::Package {
// Empty arguments for non-auto-called packages are not allowed anymore.
empty_non_auto_called: if *empty_arg {
ratchet::EmptyNonAutoCalled::Invalid
} else {
ratchet::EmptyNonAutoCalled::Valid
},
})
} else {
NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.clone(),
package_name: package_name.clone(),
}
.into()
}
}
AttributeVariant::Other => NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.clone(),
package_name: package_name.clone(),
}
.into(),
});
check_result.map(|value| (package_name.clone(), value))
} else {
NixpkgsProblem::UndefinedAttr {
relative_package_file: relative_package_file.clone(),
@ -158,6 +168,9 @@ pub fn check_values(
}
.into()
}
},
)))
}))
.map(|elems| ratchet::Nixpkgs {
packages: elems.into_iter().collect(),
}),
)
}

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

@ -1,5 +1,6 @@
mod eval;
mod nixpkgs_problem;
mod ratchet;
mod references;
mod structure;
mod utils;
@ -9,37 +10,43 @@ use crate::structure::check_structure;
use crate::validation::Validation::Failure;
use crate::validation::Validation::Success;
use anyhow::Context;
use clap::{Parser, ValueEnum};
use clap::Parser;
use colored::Colorize;
use std::io;
use std::path::{Path, PathBuf};
use std::process::ExitCode;
/// Program to check the validity of pkgs/by-name
///
/// This CLI interface may be changed over time if the CI workflow making use of
/// it is adjusted to deal with the change appropriately.
///
/// Exit code:
/// - `0`: If the validation is successful
/// - `1`: If the validation is not successful
/// - `2`: If an unexpected I/O error occurs
///
/// Standard error:
/// - Informative messages
/// - Detected problems if validation is not successful
#[derive(Parser, Debug)]
#[command(about)]
#[command(about, verbatim_doc_comment)]
pub struct Args {
/// Path to nixpkgs
/// Path to the main Nixpkgs to check.
/// For PRs, this should be set to a checkout of the PR branch.
nixpkgs: PathBuf,
/// The version of the checks
/// Increasing this may cause failures for a Nixpkgs that succeeded before
/// TODO: Remove default once Nixpkgs CI passes this argument
#[arg(long, value_enum, default_value_t = Version::V0)]
version: Version,
}
/// The version of the checks
#[derive(Debug, Clone, PartialEq, PartialOrd, ValueEnum)]
pub enum Version {
/// Initial version
V0,
/// Empty argument check
V1,
/// Path to the base Nixpkgs to run ratchet checks against.
/// For PRs, this should be set to a checkout of the PRs base branch.
/// If not specified, no ratchet checks will be performed.
/// However, this flag will become required once CI uses it.
#[arg(long)]
base: Option<PathBuf>,
}
fn main() -> ExitCode {
let args = Args::parse();
match check_nixpkgs(&args.nixpkgs, args.version, vec![], &mut io::stderr()) {
match process(args.base.as_deref(), &args.nixpkgs, &[], &mut io::stderr()) {
Ok(true) => {
eprintln!("{}", "Validated successfully".green());
ExitCode::SUCCESS
@ -55,10 +62,11 @@ fn main() -> ExitCode {
}
}
/// Checks whether the pkgs/by-name structure in Nixpkgs is valid.
/// Does the actual work. This is the abstraction used both by `main` and the tests.
///
/// # Arguments
/// - `nixpkgs_path`: The path to the Nixpkgs to check
/// - `base_nixpkgs`: Path to the base Nixpkgs to run ratchet checks against.
/// - `main_nixpkgs`: Path to the main Nixpkgs to check.
/// - `eval_accessible_paths`:
/// Extra paths that need to be accessible to evaluate Nixpkgs using `restrict-eval`.
/// This is used to allow the tests to access the mock-nixpkgs.nix file
@ -68,33 +76,30 @@ fn main() -> ExitCode {
/// - `Err(e)` if an I/O-related error `e` occurred.
/// - `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,
eval_accessible_paths: Vec<&Path>,
pub fn process<W: io::Write>(
base_nixpkgs: Option<&Path>,
main_nixpkgs: &Path,
eval_accessible_paths: &[&Path],
error_writer: &mut W,
) -> anyhow::Result<bool> {
let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
"Nixpkgs path {} could not be resolved",
nixpkgs_path.display()
))?;
let check_result = if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
eprintln!(
"Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
utils::BASE_SUBPATH
);
Success(())
} else {
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)?
}
// Check the main Nixpkgs first
let main_result = check_nixpkgs(main_nixpkgs, eval_accessible_paths, error_writer)?;
let check_result = main_result.result_map(|nixpkgs_version| {
// If the main Nixpkgs doesn't have any problems, run the ratchet checks against the base
// Nixpkgs
if let Some(base) = base_nixpkgs {
check_nixpkgs(base, eval_accessible_paths, error_writer)?.result_map(
|base_nixpkgs_version| {
Ok(ratchet::Nixpkgs::compare(
Some(base_nixpkgs_version),
nixpkgs_version,
))
},
)
} else {
Ok(ratchet::Nixpkgs::compare(None, nixpkgs_version))
}
};
})?;
match check_result {
Failure(errors) => {
@ -103,15 +108,45 @@ pub fn check_nixpkgs<W: io::Write>(
}
Ok(false)
}
Success(_) => Ok(true),
Success(()) => Ok(true),
}
}
/// Checks whether the pkgs/by-name structure in Nixpkgs is valid.
///
/// 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<W: io::Write>(
nixpkgs_path: &Path,
eval_accessible_paths: &[&Path],
error_writer: &mut W,
) -> validation::Result<ratchet::Nixpkgs> {
Ok({
let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
"Nixpkgs path {} could not be resolved",
nixpkgs_path.display()
))?;
if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
writeln!(
error_writer,
"Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
utils::BASE_SUBPATH
)?;
Success(ratchet::Nixpkgs::default())
} else {
check_structure(&nixpkgs_path)?.result_map(|package_names|
// Only if we could successfully parse the structure, we do the evaluation checks
eval::check_values(&nixpkgs_path, package_names, eval_accessible_paths))?
}
})
}
#[cfg(test)]
mod tests {
use crate::check_nixpkgs;
use crate::process;
use crate::utils;
use crate::Version;
use anyhow::Context;
use std::fs;
use std::path::Path;
@ -197,10 +232,17 @@ mod tests {
fn test_nixpkgs(name: &str, path: &Path, expected_errors: &str) -> anyhow::Result<()> {
let extra_nix_path = Path::new("tests/mock-nixpkgs.nix");
let base_path = path.join("base");
let base_nixpkgs = if base_path.exists() {
Some(base_path.as_path())
} else {
None
};
// 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![];
check_nixpkgs(&path, Version::V1, vec![&extra_nix_path], &mut writer)
process(base_nixpkgs, &path, &[&extra_nix_path], &mut writer)
.context(format!("Failed test case {name}"))?;
Ok(writer)
})?;

85
pkgs/test/nixpkgs-check-by-name/src/ratchet.rs Normal file
View File

@ -0,0 +1,85 @@
//! This module implements the ratchet checks, see ../README.md#ratchet-checks
//!
//! Each type has a `compare` method that validates the ratchet checks for that item.
use crate::nixpkgs_problem::NixpkgsProblem;
use crate::structure;
use crate::validation::{self, Validation, Validation::Success};
use std::collections::HashMap;
/// The ratchet value for the entirety of Nixpkgs.
#[derive(Default)]
pub struct Nixpkgs {
/// The ratchet values for each package in `pkgs/by-name`
pub packages: HashMap<String, Package>,
}
impl Nixpkgs {
/// Validates the ratchet checks for Nixpkgs
pub fn compare(optional_from: Option<Self>, to: Self) -> Validation<()> {
validation::sequence_(
// We only loop over the current attributes,
// we don't need to check ones that were removed
to.packages.into_iter().map(|(name, attr_to)| {
let attr_from = if let Some(from) = &optional_from {
from.packages.get(&name)
} else {
// This pretends that if there's no base version to compare against, all
// attributes existed without conforming to the new strictness check for
// backwards compatibility.
// TODO: Remove this case. This is only needed because the `--base`
// argument is still optional, which doesn't need to be once CI is updated
// to pass it.
Some(&Package {
empty_non_auto_called: EmptyNonAutoCalled::Invalid,
})
};
Package::compare(&name, attr_from, &attr_to)
}),
)
}
}
/// The ratchet value for a single package in `pkgs/by-name`
pub struct Package {
/// The ratchet value for the check for non-auto-called empty arguments
pub empty_non_auto_called: EmptyNonAutoCalled,
}
impl Package {
/// Validates the ratchet checks for a single package defined in `pkgs/by-name`
pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
EmptyNonAutoCalled::compare(
name,
optional_from.map(|x| &x.empty_non_auto_called),
&to.empty_non_auto_called,
)
}
}
/// The ratchet value of a single package in `pkgs/by-name`
/// for the non-auto-called empty argument check of a single.
///
/// This checks that packages defined in `pkgs/by-name` cannot be overridden
/// with an empty second argument like `callPackage ... { }`.
#[derive(PartialEq, PartialOrd)]
pub enum EmptyNonAutoCalled {
Invalid,
Valid,
}
impl EmptyNonAutoCalled {
/// Validates the non-auto-called empty argument ratchet check for a single package defined in `pkgs/by-name`
fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
let from = optional_from.unwrap_or(&Self::Valid);
if to >= from {
Success(())
} else {
NixpkgsProblem::WrongCallPackage {
relative_package_file: structure::relative_file_for_package(name),
package_name: name.to_owned(),
}
.into()
}
}
}

9
pkgs/test/nixpkgs-check-by-name/src/validation.rs
View File

@ -58,6 +58,15 @@ impl<A> Validation<A> {
Success(value) => Success(f(value)),
}
}
/// Map a `Validation<A>` to a `Result<B>` by applying a function `A -> Result<B>`
/// only if there is a `Success` value
pub fn result_map<B>(self, f: impl FnOnce(A) -> Result<B>) -> Result<B> {
match self {
Failure(err) => Ok(Failure(err)),
Success(value) => f(value),
}
}
}
impl Validation<()> {

1
pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected Normal file
View File

@ -0,0 +1 @@
Given Nixpkgs path does not contain a pkgs/by-name subdirectory, no check necessary.

3
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/all-packages.nix Normal file
View File

@ -0,0 +1,3 @@
self: super: {
nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
}

3
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/all-packages.nix Normal file
View File

@ -0,0 +1,3 @@
self: super: {
nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
}

1
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/default.nix Normal file
View File

@ -0,0 +1 @@
import ../../mock-nixpkgs.nix { root = ./.; }

1
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/pkgs/by-name/no/nonDerivation/package.nix Normal file
View File

@ -0,0 +1 @@
{ someDrv }: someDrv

1
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/default.nix Normal file
View File

@ -0,0 +1 @@
import ../mock-nixpkgs.nix { root = ./.; }

0
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected Normal file
View File

1
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/pkgs/by-name/no/nonDerivation/package.nix Normal file
View File

@ -0,0 +1 @@
{ someDrv }: someDrv

1
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix Normal file
View File

@ -0,0 +1 @@
import ../../mock-nixpkgs.nix { root = ./.; }

1
pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md Normal file
View File

@ -0,0 +1 @@
(this is just here so the directory can get tracked by git)

2
pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix
View File

@ -1,3 +1,3 @@
self: super: {
nonDerivation = null;
nonDerivation = self.someDrv;
}

2
pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix
View File

@ -1,3 +1,3 @@
self: super: {
nonDerivation = self.callPackage ({ }: { }) { };
nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
}