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

tests.nixpkgs-check-by-name: Improve docs, introduce "ratchet" term

Browse Source
This commit is contained in:
Silvan Mosberger
2023-12-15 02:14:48 +01:00
parent 413dd9c03e
commit 79618ff8cb
4 changed files with 73 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkgs/test/nixpkgs-check-by-name/README.md
+21 -9
View File
@@ -10,13 +10,15 @@ This API may be changed over time if the CI workflow making use of it is adjuste
- Command line: `nixpkgs-check-by-name [--base <BASE_NIXPKGS>] <NIXPKGS>`
- Arguments:
- `<NIXPKGS>`: The path to the Nixpkgs to check.
- `<BASE_NIXPKGS>`: The path to the Nixpkgs to use as the base to compare `<NIXPKGS>` against.
This allows the strictness of checks to increase over time by only preventing _new_ violations from being introduced,
while allowing violations that already existed.
- `<NIXPKGS>`:
The path to the Nixpkgs to check.
For PRs, this should be set to a checkout of the PR branch.
- `<BASE_NIXPKGS>`:
The path to the Nixpkgs to use as the [ratchet check](#ratchet-checks) base.
For PRs, this should be set to a checkout of the PRs base branch.
If not specified, all violations of stricter checks are allowed.
However, this flag will become required once CI passes it.
If not specified, no ratchet checks will be performed.
However, this flag will become required once CI uses it.
- Exit code:
- `0`: If the [validation](#validity-checks) is successful
- `1`: If the [validation](#validity-checks) is not successful
@@ -41,10 +43,20 @@ These checks are performed by this tool:
### Nix evaluation checks
- `pkgs.${name}` is defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`.
- If `pkgs.${name}` is not auto-called from `pkgs/by-name`, `args` must not be empty,
with the exception that if `BASE_NIXPKGS` also has a definition for the same package with empty `args`, it's allowed
- `pkgs.lib.isDerivation pkgs.${name}` is `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:
- If `pkgs.${name}` is not auto-called from `pkgs/by-name`, the `args` in its `callPackage` must not be empty,
## Development
Enter the development environment in this directory either automatically with `direnv` or with
@@ -88,7 +100,7 @@ Tests are declared in [`./tests`](./tests) as subdirectories imitating Nixpkgs w
- `base` (optional):
Contains another subdirectory imitating Nixpkgs with potentially any of the above structures.
This will be used as the `--base` argument, allowing tests of gradual transitions.
This is used for [ratchet checks](#ratchet-checks).
- `expected` (optional):
A file containing the expected standard output.
pkgs/test/nixpkgs-check-by-name/src/eval.rs
+10 -9
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;
@@ -42,7 +42,7 @@ pub fn check_values(
nixpkgs_path: &Path,
package_names: Vec<String>,
eval_accessible_paths: &[&Path],
) -> validation::Result<version::Nixpkgs> {
) -> 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")?;
@@ -126,8 +126,8 @@ pub fn check_values(
};
let check_result = check_result.and(match &attribute_info.variant {
AttributeVariant::AutoCalled => Success(version::Attribute {
empty_non_auto_called: version::EmptyNonAutoCalled::Valid,
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 {
@@ -137,11 +137,12 @@ pub fn check_values(
};
if correct_file {
Success(version::Attribute {
Success(ratchet::Package {
// Empty arguments for non-auto-called packages are not allowed anymore.
empty_non_auto_called: if *empty_arg {
version::EmptyNonAutoCalled::Invalid
ratchet::EmptyNonAutoCalled::Invalid
} else {
version::EmptyNonAutoCalled::Valid
ratchet::EmptyNonAutoCalled::Valid
},
})
} else {
@@ -168,8 +169,8 @@ pub fn check_values(
.into()
}
}))
.map(|elems| version::Nixpkgs {
attributes: elems.into_iter().collect(),
.map(|elems| ratchet::Nixpkgs {
packages: elems.into_iter().collect(),
}),
)
}
pkgs/test/nixpkgs-check-by-name/src/main.rs
+19 -15
View File
@@ -1,15 +1,14 @@
mod eval;
mod nixpkgs_problem;
mod ratchet;
mod references;
mod structure;
mod utils;
mod validation;
mod version;
use crate::structure::check_structure;
use crate::validation::Validation::Failure;
use crate::validation::Validation::Success;
use crate::version::Nixpkgs;
use anyhow::Context;
use clap::Parser;
use colored::Colorize;
@@ -21,10 +20,14 @@ use std::process::ExitCode;
#[derive(Parser, Debug)]
#[command(about)]
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,
/// Path to the base Nixpkgs to compare against
/// 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>,
}
@@ -50,8 +53,8 @@ fn main() -> ExitCode {
/// Does the actual work. This is the abstraction used both by `main` and the tests.
///
/// # Arguments
/// - `base_nixpkgs`: The path to the base Nixpkgs to compare against
/// - `main_nixpkgs`: The path to the main 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
@@ -67,19 +70,22 @@ pub fn process<W: io::Write>(
eval_accessible_paths: &[&Path],
error_writer: &mut W,
) -> anyhow::Result<bool> {
// 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(Nixpkgs::compare(
Ok(ratchet::Nixpkgs::compare(
Some(base_nixpkgs_version),
nixpkgs_version,
))
},
)
} else {
Ok(Nixpkgs::compare(None, nixpkgs_version))
Ok(ratchet::Nixpkgs::compare(None, nixpkgs_version))
}
})?;
@@ -96,16 +102,14 @@ pub fn process<W: io::Write>(
/// Checks whether the pkgs/by-name structure in Nixpkgs is valid.
///
/// This does not include checks that depend on the base version of Nixpkgs to compare against,
/// which is used for checks that were only introduced later and increased strictness.
///
/// Instead a `version::Nixpkgs` is returned, whose `compare` method allows comparing the
/// result of this function for the base Nixpkgs against the one for the main Nixpkgs.
/// 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<version::Nixpkgs> {
) -> validation::Result<ratchet::Nixpkgs> {
Ok({
let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
"Nixpkgs path {} could not be resolved",
@@ -118,7 +122,7 @@ pub fn check_nixpkgs<W: io::Write>(
"Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
utils::BASE_SUBPATH
)?;
Success(version::Nixpkgs::default())
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
pkgs/test/nixpkgs-check-by-name/src/version.rs → pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+23 -21
View File
@@ -1,30 +1,28 @@
//! 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 check version conformity of a Nixpkgs path:
/// When the strictness of the check increases, this structure should be extended to distinguish
/// between parts that are still valid, and ones that aren't valid anymore.
/// The ratchet value for the entirety of Nixpkgs.
#[derive(Default)]
pub struct Nixpkgs {
/// The package attributes tracked in `pkgs/by-name`
pub attributes: HashMap<String, Attribute>,
/// The ratchet values for each package in `pkgs/by-name`
pub packages: HashMap<String, Package>,
}
impl Nixpkgs {
/// Compares two Nixpkgs versions against each other, returning validation errors only if the
/// `from` version satisfied the stricter checks, while the `to` version doesn't satisfy them
/// anymore.
/// This enables a gradual transition from weaker to stricter checks, by only allowing PRs to
/// increase strictness.
/// 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.attributes.into_iter().map(|(name, attr_to)| {
to.packages.into_iter().map(|(name, attr_to)| {
let attr_from = if let Some(from) = &optional_from {
from.attributes.get(&name)
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
@@ -32,22 +30,24 @@ impl Nixpkgs {
// 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(&Attribute {
Some(&Package {
empty_non_auto_called: EmptyNonAutoCalled::Invalid,
})
};
Attribute::compare(&name, attr_from, &attr_to)
Package::compare(&name, attr_from, &attr_to)
}),
)
}
}
/// The check version conformity of an attribute defined by `pkgs/by-name`
pub struct Attribute {
/// 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 Attribute {
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,
@@ -57,17 +57,19 @@ impl Attribute {
}
}
/// Whether an attribute conforms to the new strictness check that
/// `callPackage ... {}` is not allowed anymore in `all-package.nix`
/// 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 {
/// The attribute is not valid anymore with the new check
Invalid,
/// The attribute is still valid with the new check
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 {