Skip to content

Commit

Permalink
WIP
Browse files Browse the repository at this point in the history
  • Loading branch information
@infinisil
infinisil committed Feb 19, 2024
1 parent 03bd6ed commit 3805b88
Show file tree
Hide file tree
Showing 6 changed files with 340 additions and 87 deletions.
5 changes: 5 additions & 0 deletions notes.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
- SyntaxNodes are either mutable or not
- We want them to be mutable, but only when we actually intend on changing them
This means, only for the `migrate` command, and only for the attr name location files, not the reference check files
- We should be able to re-render the file without needing backward-replacements
- Mutable syntax nodes can be changed without absolute locations/indices. So any absolute locations need to be resolved before modifying it
96 changes: 55 additions & 41 deletions pkgs/test/nixpkgs-check-by-name/src/eval.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::nixpkgs_problem::NixpkgsProblem;
pub(crate) use crate::nixpkgs_problem::NixpkgsProblem;
use crate::ratchet;
use crate::structure;
use crate::utils;
Expand Down Expand Up @@ -405,59 +405,73 @@ fn handle_non_by_name_attribute(
location: Some(location),
}) = non_by_name_attribute {

let nix_file = nix_file_store.get(&location.file)?;

// Parse the Nix file in the location and figure out whether it's an
// attribute definition of the form `= callPackage <arg1> <arg2>`,
// returning the arguments if so.
let optional_syntactic_call_package = nix_file_store
.get(&location.file)?
.call_package_argument_info_at(
location.line,
location.column,
if let Some(attrpath_value_node) = nix_file.attrpath_value_at(location.line, location.column)? {

// Parse the Nix file in the location and figure out whether it's an
// attribute definition of the form `= callPackage <arg1> <arg2>`,
// returning the arguments if so.
let optional_syntactic_call_package = nix_file.attrpath_value_call_package_argument_info(
&attrpath_value_node,
// Passing the Nixpkgs path here both checks that the <arg1> is within Nixpkgs, and
// strips the absolute Nixpkgs path from it, such that
// syntactic_call_package.relative_path is relative to Nixpkgs
nixpkgs_path
)?;

// At this point, we completed two different checks for whether it's a
// `callPackage`
match (is_semantic_call_package, optional_syntactic_call_package) {
// Something like `<attr> = { }`
(false, None)
// Something like `<attr> = pythonPackages.callPackage ...`
| (false, Some(_))
// Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
| (true, None) => {
// In all of these cases, it's not possible to migrate the package to `pkgs/by-name`
NonApplicable
}
// Something like `<attr> = pkgs.callPackage ...`
(true, Some(syntactic_call_package)) => {
// It's only possible to migrate such a definitions if..
match syntactic_call_package.relative_path {
Some(ref rel_path) if rel_path.starts_with(utils::BASE_SUBPATH) => {
// ..the path is not already within `pkgs/by-name` like
//
// foo-variant = callPackage ../by-name/fo/foo/package.nix {
// someFlag = true;
// }
//
// While such definitions could be moved to `pkgs/by-name` by using
// `.override { someFlag = true; }` instead, this changes the semantics in
// relation with overlays, so migration is generally not possible.
//
// See also "package variants" in RFC 140:
// https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants
NonApplicable
}
_ => {
// Otherwise, the path is outside `pkgs/by-name`, which means it can be
// migrated
Loose(syntactic_call_package)
// At this point, we completed two different checks for whether it's a
// `callPackage`
match (is_semantic_call_package, optional_syntactic_call_package) {
// Something like `<attr> = { }`
(false, None)
// Something like `<attr> = pythonPackages.callPackage ...`
| (false, Some(_))
// Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
| (true, None) => {
// In all of these cases, it's not possible to migrate the package to `pkgs/by-name`
NonApplicable
}
// Something like `<attr> = pkgs.callPackage ...`
(true, Some(syntactic_call_package)) => {
// It's only possible to migrate such a definitions if..
match syntactic_call_package.relative_path {
Some(ref rel_path) if rel_path.starts_with(utils::BASE_SUBPATH) => {
// ..the path is not already within `pkgs/by-name` like
//
// foo-variant = callPackage ../by-name/fo/foo/package.nix {
// someFlag = true;
// }
//
// While such definitions could be moved to `pkgs/by-name` by using
// `.override { someFlag = true; }` instead, this changes the semantics in
// relation with overlays, so migration is generally not possible.
//
// See also "package variants" in RFC 140:
// https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants
NonApplicable
}
_ => {
// Otherwise, the path is outside `pkgs/by-name`, which means it can be
// migrated
Loose(ratchet::UsesByNameContext {
call_package_argument_info: syntactic_call_package,
file: location.file.to_owned(),
line: location.line,
syntax_node: attrpath_value_node,
})
}
}
}
}
} else {
// Inherit
NonApplicable
}

} else {
// This catches all the cases not matched by the above `if let`, falling back to not being
// able to migrate such attributes
Expand Down
100 changes: 71 additions & 29 deletions pkgs/test/nixpkgs-check-by-name/src/main.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::io::Write;
use crate::nix_file::NixFileStore;
mod eval;
mod nix_file;
Expand All @@ -13,13 +14,24 @@ use crate::validation::Validation::Failure;
use crate::validation::Validation::Success;
use anyhow::Context;
use clap::Parser;
use clap::Subcommand;
use colored::Colorize;
use std::io;
use std::path::{Path, PathBuf};
use std::process::ExitCode;

/// Program to check the validity of pkgs/by-name
///
#[derive(Parser, Debug)]
#[command(about, verbatim_doc_comment)]
pub struct Args {
#[command(subcommand)]
command: Commands,

/// Path to the main Nixpkgs to check.
/// For PRs, this should be set to a checkout of the PR branch.
nixpkgs: PathBuf,
}

/// This CLI interface may be changed over time if the CI workflow making use of
/// it is adjusted to deal with the change appropriately.
///
Expand All @@ -31,33 +43,60 @@ use std::process::ExitCode;
/// Standard error:
/// - Informative messages
/// - Detected problems if validation is not successful
#[derive(Parser, Debug)]
#[command(about, verbatim_doc_comment)]
pub struct Args {
/// Path to the main Nixpkgs to check.
/// For PRs, this should be set to a checkout of the PR branch.
nixpkgs: PathBuf,
#[derive(Subcommand, Debug)]
enum Commands {
/// Checks whether PR is valid
CheckPR {
/// Path to the base Nixpkgs to run ratchet checks against.
/// For PRs, this should be set to a checkout of the PRs base branch.
#[arg(long)]
base: PathBuf,
},

/// Path to the base Nixpkgs to run ratchet checks against.
/// For PRs, this should be set to a checkout of the PRs base branch.
#[arg(long)]
base: PathBuf,
/// Migrates a Nixpkgs
Migrate,
}

fn main() -> ExitCode {
let args = Args::parse();
match process(&args.base, &args.nixpkgs, false, &mut io::stderr()) {
Ok(true) => {
eprintln!("{}", "Validated successfully".green());
ExitCode::SUCCESS
}
Ok(false) => {
eprintln!("{}", "Validation failed, see above errors".yellow());
ExitCode::from(1)
let mut nix_file_store = NixFileStore::default();
match args.command {
Commands::CheckPR { base } => {
match process(&base, &args.nixpkgs, &mut nix_file_store, false, &mut io::stderr()) {
Ok(true) => {
eprintln!("{}", "Validated successfully".green());
ExitCode::SUCCESS
}
Ok(false) => {
eprintln!("{}", "Validation failed, see above errors".yellow());
ExitCode::from(1)
}
Err(e) => {
eprintln!("{} {:#}", "I/O error: ".yellow(), e);
ExitCode::from(2)
}
}
}
Err(e) => {
eprintln!("{} {:#}", "I/O error: ".yellow(), e);
ExitCode::from(2)
Commands::Migrate => {
let mut error_writer = &mut io::stderr();
match check_nixpkgs(&args.nixpkgs, &mut nix_file_store, false, &mut error_writer) {
Ok(validation::Validation::Success(nixpkgs)) => {
nixpkgs.migrate(&mut nix_file_store);
eprintln!("{}", "Migration done".green());
ExitCode::SUCCESS
}
Ok(validation::Validation::Failure(errors)) => {
for error in errors {
writeln!(error_writer, "{}", error.to_string().red()).unwrap()
}
eprintln!("{}", "Validation failed, see above errors".yellow());
ExitCode::from(1)
}
Err(e) => {
eprintln!("{} {:#}", "I/O error: ".yellow(), e);
ExitCode::from(2)
}
}
}
}
}
Expand All @@ -79,15 +118,16 @@ fn main() -> ExitCode {
pub fn process<W: io::Write>(
base_nixpkgs: &Path,
main_nixpkgs: &Path,
nix_file_store: &mut NixFileStore,
keep_nix_path: bool,
error_writer: &mut W,
) -> anyhow::Result<bool> {
// Check the main Nixpkgs first
let main_result = check_nixpkgs(main_nixpkgs, keep_nix_path, error_writer)?;
let main_result = check_nixpkgs(main_nixpkgs, nix_file_store, keep_nix_path, 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
check_nixpkgs(base_nixpkgs, keep_nix_path, error_writer)?.result_map(
check_nixpkgs(base_nixpkgs, nix_file_store, keep_nix_path, error_writer)?.result_map(
|base_nixpkgs_version| {
Ok(ratchet::Nixpkgs::compare(
base_nixpkgs_version,
Expand Down Expand Up @@ -115,10 +155,10 @@ pub fn process<W: io::Write>(
/// ratchet check against another result.
pub fn check_nixpkgs<W: io::Write>(
nixpkgs_path: &Path,
nix_file_store: &mut NixFileStore,
keep_nix_path: bool,
error_writer: &mut W,
) -> validation::Result<ratchet::Nixpkgs> {
let mut nix_file_store = NixFileStore::default();

Ok({
let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| {
Expand All @@ -136,16 +176,17 @@ pub fn check_nixpkgs<W: io::Write>(
)?;
Success(ratchet::Nixpkgs::default())
} else {
check_structure(&nixpkgs_path, &mut nix_file_store)?.result_map(|package_names|
check_structure(&nixpkgs_path, nix_file_store)?.result_map(|package_names|
// Only if we could successfully parse the structure, we do the evaluation checks
eval::check_values(&nixpkgs_path, &mut nix_file_store, package_names, keep_nix_path))?
eval::check_values(&nixpkgs_path, nix_file_store, package_names, keep_nix_path))?
}
})
}

#[cfg(test)]
mod tests {
use crate::process;
use crate::NixFileStore;
use crate::process;
use crate::utils;
use anyhow::Context;
use std::fs;
Expand Down Expand Up @@ -240,7 +281,8 @@ mod tests {
// 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![];
process(base_nixpkgs, &path, true, &mut writer)
let mut nix_file_store = NixFileStore::default();
process(base_nixpkgs, &path, &mut nix_file_store, true, &mut writer)
.with_context(|| format!("Failed test case {name}"))?;
Ok(writer)
})?;
Expand Down

0 comments on commit 3805b88

Please sign in to comment.