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

File ratchet checks #144

Merged
merged 3 commits into from
Jan 29, 2025
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: 2 additions & 2 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,5 @@ jobs:
- uses: serokell/xrefcheck-action@v1
with: # Invalid symlinks for testing purposes.
xrefcheck-args: >
--ignore tests/symlink-invalid/pkgs/by-name/fo/foo/foo
--ignore tests/multiple-failures/pkgs/by-name/A/fo@/foo
--ignore tests/symlink-invalid/main/pkgs/by-name/fo/foo/foo
--ignore tests/multiple-failures/main/pkgs/by-name/A/fo@/foo
47 changes: 24 additions & 23 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,31 +47,32 @@ The most important tools and commands in this environment are:
### Integration tests

Integration tests are declared in [`./tests`](./tests) as subdirectories imitating Nixpkgs with these files:
- `default.nix`:
Always contains
```nix
import <test-nixpkgs> { root = ./.; }
```
which makes
```
nix-instantiate <subdir> --eval -A <attr> --arg overlays <overlays>
```
work very similarly to the real Nixpkgs, just enough for the program to be able to test it.
- `pkgs/by-name`:
The `pkgs/by-name` directory to check.

- `pkgs/top-level/all-packages.nix` (optional):
Contains an overlay of the form
```nix
self: super: {
# ...
}
```
allowing the simulation of package overrides to the real [`pkgs/top-level/all-packages.nix`](https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/all-packages.nix).
The default is an empty overlay.
- `main`: A Nixpkgs root directory with:
- `default.nix`:
Always contains
```nix
import <test-nixpkgs> { root = ./.; }
```
which makes
```
nix-instantiate <subdir> --eval -A <attr> --arg overlays <overlays>
```
work very similarly to the real Nixpkgs, just enough for the program to be able to test it.
- `pkgs/by-name`:
The `pkgs/by-name` directory to check.

- `pkgs/top-level/all-packages.nix` (optional):
Contains an overlay of the form
```nix
self: super: {
# ...
}
```
allowing the simulation of package overrides to the real [`pkgs/top-level/all-packages.nix`](https://github.com/NixOS/nixpkgs/blob/master/pkgs/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.
Contains another Nixpkgs root directory with potentially any of the above structures.
This is used to test [ratchet checks](./README.md#ratchet-checks).

- `expected` (optional):
Expand Down
11 changes: 6 additions & 5 deletions src/eval.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::collections::BTreeMap;
use std::path::{Path, PathBuf};
use std::{env, fs, process};

Expand Down Expand Up @@ -152,11 +153,13 @@ fn mutate_nix_instatiate_arguments_based_on_cfg(
/// 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.
///
/// The validation result is a map from package names to a package ratchet state.
pub fn check_values(
nixpkgs_path: &Path,
nix_file_store: &mut NixFileStore,
package_names: &[String],
) -> validation::Result<ratchet::Nixpkgs> {
) -> validation::Result<BTreeMap<String, ratchet::Package>> {
let work_dir = tempfile::Builder::new()
.prefix("nixpkgs-vet")
.tempdir()
Expand Down Expand Up @@ -255,9 +258,7 @@ pub fn check_values(
.collect_vec()?,
);

Ok(check_result.map(|elems| ratchet::Nixpkgs {
packages: elems.into_iter().collect(),
}))
Ok(check_result.map(|elems| elems.into_iter().collect()))
}

/// Handle the evaluation result for an attribute in `pkgs/by-name`, making it a validation result.
Expand Down Expand Up @@ -372,7 +373,7 @@ fn by_name(

// Independently report problems about whether it's a derivation and the callPackage
// variant.
is_derivation_result.and(variant_result)
is_derivation_result.and_(variant_result)
}
};
Ok(
Expand Down
77 changes: 77 additions & 0 deletions src/files.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
use relative_path::RelativePath;
use relative_path::RelativePathBuf;
use std::collections::BTreeMap;
use std::path::Path;

use crate::nix_file::NixFileStore;
use crate::validation::ResultIteratorExt;
use crate::validation::Validation::Success;
use crate::{nix_file, ratchet, structure, validation};

/// Runs check on all Nix files, returning a ratchet result for each
pub fn check_files(
nixpkgs_path: &Path,
nix_file_store: &mut NixFileStore,
) -> validation::Result<BTreeMap<RelativePathBuf, ratchet::File>> {
process_nix_files(nixpkgs_path, nix_file_store, |_nix_file| {
// Noop for now, only boilerplate to make it easier to add future file-based checks
Ok(Success(ratchet::File {}))
Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

Or is this the place that #142 will go?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is where the logic goes yes :)

})
}

/// Processes all Nix files in a Nixpkgs directory according to a given function `f`, collecting the
/// results into a mapping from each file to a ratchet value.
fn process_nix_files(
nixpkgs_path: &Path,
nix_file_store: &mut NixFileStore,
f: impl Fn(&nix_file::NixFile) -> validation::Result<ratchet::File>,
) -> validation::Result<BTreeMap<RelativePathBuf, ratchet::File>> {
// Get all Nix files
let files = {
let mut files = vec![];
collect_nix_files(nixpkgs_path, &RelativePathBuf::new(), &mut files)?;
files
};

let results = files
.into_iter()
.map(|path| {
// Get the (optionally-cached) parsed Nix file
let nix_file = nix_file_store.get(&path.to_path(nixpkgs_path))?;
let result = f(nix_file)?;
let val = result.map(|ratchet| (path, ratchet));
Ok::<_, anyhow::Error>(val)
})
.collect_vec()?;

Ok(validation::sequence(results).map(|entries| {
// Convert the Vec to a BTreeMap
entries.into_iter().collect()
}))
}

/// Recursively collects all Nix files in the relative `dir` within `base`
/// into the `files` `Vec`.
fn collect_nix_files(
base: &Path,
dir: &RelativePath,
files: &mut Vec<RelativePathBuf>,
) -> anyhow::Result<()> {
for entry in structure::read_dir_sorted(&dir.to_path(base))? {
let mut relative_path = dir.to_relative_path_buf();
relative_path.push(entry.file_name().to_string_lossy().into_owned());

let absolute_path = entry.path();

// We'll get to every file based on directory recursion, no need to follow symlinks.
if absolute_path.is_symlink() {
continue;
}
if absolute_path.is_dir() {
collect_nix_files(base, &relative_path, files)?
} else if absolute_path.extension().is_some_and(|x| x == "nix") {
files.push(relative_path)
}
}
Ok(())
}
40 changes: 26 additions & 14 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
// #![allow(clippy::missing_const_for_fn)]

mod eval;
mod files;
mod location;
mod nix_file;
mod problem;
Expand All @@ -21,6 +22,7 @@ mod validation;

use anyhow::Context as _;
use clap::Parser;
use std::collections::BTreeMap;
use std::path::{Path, PathBuf};
use std::process::ExitCode;
use std::{panic, thread};
Expand Down Expand Up @@ -113,20 +115,30 @@ fn check_nixpkgs(nixpkgs_path: &Path) -> validation::Result<ratchet::Nixpkgs> {
)
})?;

if !nixpkgs_path.join(structure::BASE_SUBPATH).exists() {
// No pkgs/by-name directory, always valid
return Ok(Success(ratchet::Nixpkgs::default()));
}

let mut nix_file_store = NixFileStore::default();
let structure = check_structure(&nixpkgs_path, &mut nix_file_store)?;

// Only if we could successfully parse the structure, we do the evaluation checks
let result = structure.result_map(|package_names| {
eval::check_values(&nixpkgs_path, &mut nix_file_store, package_names.as_slice())
})?;
let package_result = {
if !nixpkgs_path.join(structure::BASE_SUBPATH).exists() {
// No pkgs/by-name directory, always valid
Success(BTreeMap::new())
} else {
let structure = check_structure(&nixpkgs_path, &mut nix_file_store)?;

// Only if we could successfully parse the structure, we do the evaluation checks
structure.result_map(|package_names| {
eval::check_values(&nixpkgs_path, &mut nix_file_store, package_names.as_slice())
})?
}
};

let file_result = files::check_files(&nixpkgs_path, &mut nix_file_store)?;

Ok(result)
Ok(
package_result.and(file_result, |packages, files| ratchet::Nixpkgs {
packages,
files,
}),
)
}

#[cfg(test)]
Expand Down Expand Up @@ -178,7 +190,7 @@ mod tests {
return Ok(());
}

let base = path.join(BASE_SUBPATH);
let base = path.join("main").join(BASE_SUBPATH);

fs::create_dir_all(base.join("fo/foo"))?;
fs::write(base.join("fo/foo/package.nix"), "{ someDrv }: someDrv")?;
Expand Down Expand Up @@ -237,7 +249,7 @@ mod tests {
.build()
.expect("valid regex");

let path = path.to_owned();
let main_path = path.join("main");
let base_path = path.join("base");
let base_nixpkgs = if base_path.exists() {
base_path
Expand All @@ -251,7 +263,7 @@ mod tests {
let nix_conf_dir = nix_conf_dir.path().as_os_str();

let status = temp_env::with_var("NIX_CONF_DIR", Some(nix_conf_dir), || {
process(base_nixpkgs, &path)
process(base_nixpkgs, &main_path)
});

let actual_errors = format!("{status}\n");
Expand Down
18 changes: 18 additions & 0 deletions src/ratchet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
//!
//! Each type has a `compare` method that validates the ratchet checks for that item.

use relative_path::RelativePath;
use std::collections::BTreeMap;

use relative_path::RelativePathBuf;
Expand All @@ -15,6 +16,7 @@ use crate::validation::{self, Validation, Validation::Success};
pub struct Nixpkgs {
/// The ratchet values for all packages
pub packages: BTreeMap<String, Package>,
pub files: BTreeMap<RelativePathBuf, File>,
}

impl Nixpkgs {
Expand All @@ -27,6 +29,9 @@ impl Nixpkgs {
.into_iter()
.map(|(name, pkg)| Package::compare(&name, from.packages.get(&name), &pkg)),
)
.and_(validation::sequence_(to.files.into_iter().map(
|(name, file)| File::compare(&name, from.files.get(&name), &file),
)))
}
}

Expand Down Expand Up @@ -57,6 +62,19 @@ impl Package {
}
}

pub struct File {}

impl File {
/// Validates the ratchet checks for a top-level package
pub fn compare(
_name: &RelativePath,
_optional_from: Option<&Self>,
_to: &Self,
) -> Validation<()> {
Success(())
}
}

/// The ratchet state of a generic ratchet check.
pub enum RatchetState<Ratchet: ToProblem> {
/// The ratchet is loose. It can be tightened more. In other words, this is the legacy state
Expand Down
10 changes: 5 additions & 5 deletions src/structure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ pub fn check_structure(
.into()
});

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

let package_results = entries
.into_iter()
Expand All @@ -111,7 +111,7 @@ pub fn check_structure(
})
.collect_vec()?;

result.and(validation::sequence(package_results))
result.and_(validation::sequence(package_results))
})
})
.collect_vec()?;
Expand Down Expand Up @@ -147,7 +147,7 @@ fn check_package(
};

let correct_relative_package_dir = relative_dir_for_package(&package_name);
let result = result.and(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.
// If one of those is wrong, you should fix that first.
if shard_name_valid && package_name_valid {
Expand All @@ -164,15 +164,15 @@ fn check_package(
});

let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME);
let result = result.and(if !package_nix_path.exists() {
let result = result.and_(if !package_nix_path.exists() {
npv_143::PackageNixMissing::new(package_name.clone()).into()
} else if !package_nix_path.is_file() {
npv_144::PackageNixIsNotFile::new(package_name.clone()).into()
} else {
Success(())
});

let result = result.and(references::check_references(
let result = result.and_(references::check_references(
nix_file_store,
&relative_package_dir,
&relative_package_dir.to_path(path),
Expand Down
12 changes: 10 additions & 2 deletions src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,17 @@ impl<A> Validation<A> {
impl Validation<()> {
/// Combine two validations, both of which need to be successful for the return value to be
/// successful. The `Problem`s of both sides are returned concatenated.
pub fn and<A>(self, other: Validation<A>) -> Validation<A> {
pub fn and_<B>(self, other: Validation<B>) -> Validation<B> {
self.and(other, |(), b| b)
}
}

impl<A> Validation<A> {
/// Combine two validations, both of which need to be successful for the return value to be
/// successful. The `Problem`s of both sides are returned concatenated.
pub fn and<B, C, F: FnOnce(A, B) -> C>(self, other: Validation<B>, f: F) -> Validation<C> {
match (self, other) {
(Success(_), Success(right_value)) => Success(right_value),
(Success(a), Success(b)) => Success(f(a, b)),
(Failure(errors_l), Failure(errors_r)) => Failure(concat([errors_l, errors_r])),
(Failure(errors), Success(_)) | (Success(_), Failure(errors)) => Failure(errors),
}
Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.