diff --git a/src/eval.nix b/src/eval.nix index f3c6f5d..c59e7e8 100644 --- a/src/eval.nix +++ b/src/eval.nix @@ -1,4 +1,4 @@ -# Takes a path to nixpkgs and a path to the json-encoded list of `pkgs/by-name` attributes. +# Takes a path to nixpkgs and a path to the json-encoded list of attributes in the by-name subpath. # # Returns a value containing information on all Nixpkgs attributes which is decoded on the Rust # side. See ./eval.rs for the meaning of the returned values. @@ -7,11 +7,10 @@ let attrs = builtins.fromJSON (builtins.readFile attrsPath); # We need to check whether attributes are defined manually e.g. in `all-packages.nix`, - # automatically by the `pkgs/by-name` overlay, or neither. The only way to do so is to override + # automatically by the by-name overlay, or neither. The only way to do so is to override # `callPackage` and `_internalCallByNamePackageFile` with our own version that adds this # information to the result, and then try to access it. overlay = final: prev: { - # Adds information to each attribute about whether it's manually defined using `callPackage` callPackage = fn: args: @@ -69,7 +68,7 @@ let }; }; - # Information on all attributes that are in `pkgs/by-name`. + # Information on all attributes that are in the by-name subpath. byNameAttrs = builtins.listToAttrs ( map (name: { inherit name; @@ -82,12 +81,12 @@ let }) attrs ); - # Information on all attributes that exist but are not in `pkgs/by-name`. - # We need this to enforce `pkgs/by-name` for new packages. + # Information on all attributes that exist but are not in the by-name subpath. + # We need this to enforce the by-name subpath for new packages. nonByNameAttrs = builtins.mapAttrs ( name: value: let - # Packages outside `pkgs/by-name` often fail evaluation, so we need to handle that. + # Packages outside the by-name subpath often fail evaluation, so we need to handle that. output = attrInfo name value; result = builtins.tryEval (builtins.deepSeq output null); in diff --git a/src/eval.rs b/src/eval.rs index 58a8c4a..b31e3aa 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -12,7 +12,7 @@ use crate::problem::{ npv_100, npv_101, npv_102, npv_103, npv_104, npv_105, npv_106, npv_107, npv_108, npv_120, }; use crate::ratchet::RatchetState::{Loose, Tight}; -use crate::structure::{self, BASE_SUBPATH}; +use crate::structure; use crate::validation::ResultIteratorExt as _; use crate::validation::{self, Validation::Success}; use crate::{location, ratchet}; @@ -22,9 +22,9 @@ const EVAL_NIX: &[u8] = include_bytes!("eval.nix"); /// Attribute set of this structure is returned by `./eval.nix` #[derive(Deserialize)] enum Attribute { - /// An attribute that should be defined via `pkgs/by-name`. + /// An attribute that should be defined in the by-name subpath. ByName(ByNameAttribute), - /// An attribute not defined via `pkgs/by-name`. + /// An attribute not defined in the by-name subpath. NonByName(NonByNameAttribute), } @@ -88,7 +88,7 @@ pub enum AttributeVariant { #[derive(Deserialize)] pub enum DefinitionVariant { - /// An automatic definition by the `pkgs/by-name` overlay, though it's detected using the + /// An automatic definition by the by-name overlay, though it's detected using the /// internal `_internalCallByNamePackageFile` attribute, which can in theory also be used by /// other code. AutoDefinition, @@ -150,13 +150,14 @@ fn mutate_nix_instatiate_arguments_based_on_cfg( Ok(()) } -/// Check that the Nixpkgs attribute values corresponding to the packages in `pkgs/by-name` are of +/// Check that the Nixpkgs attribute values corresponding to the packages in the given by-name subpath are of /// the form `callPackage { ... }`. 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, + by_name_subpath: &str, nix_file_store: &mut NixFileStore, package_names: &[String], ) -> validation::Result> { @@ -223,7 +224,11 @@ pub fn check_values( if !result.status.success() { // Early return in case evaluation fails - return Ok(npv_120::NixEvalError::new(String::from_utf8_lossy(&result.stderr)).into()); + return Ok(npv_120::NixEvalError::new( + by_name_subpath, + String::from_utf8_lossy(&result.stderr), + ) + .into()); } // Parse the resulting JSON value @@ -245,12 +250,14 @@ pub fn check_values( nix_file_store, &attribute_name, non_by_name_attribute, + by_name_subpath, )?, Attribute::ByName(by_name_attribute) => by_name( nix_file_store, nixpkgs_path, &attribute_name, by_name_attribute, + by_name_subpath, )?, }; Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value))) @@ -261,22 +268,24 @@ pub fn check_values( 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. +/// Handle the evaluation result for an attribute in the by-name structure at the given by-name +/// subpath, making it a validation result. fn by_name( nix_file_store: &mut NixFileStore, nixpkgs_path: &Path, attribute_name: &str, by_name_attribute: ByNameAttribute, + by_name_subpath: &str, ) -> validation::Result { - // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists. This match + // At this point we know that `{by_name_subpath}/fo/foo/package.nix` has to exists. This match // decides whether the attribute `foo` is defined accordingly and whether a legacy manual // definition could be removed. let manual_definition_result = match by_name_attribute { // The attribute is missing. ByNameAttribute::Missing => { - // This indicates a bug in the `pkgs/by-name` overlay, because it's supposed to - // automatically defined attributes in `pkgs/by-name` - npv_100::ByNameUndefinedAttribute::new(attribute_name).into() + // This indicates a bug in the by-name overlay, because it's supposed to + // automatically define all attributes in the by-name subpath + npv_100::ByNameUndefinedAttribute::new(by_name_subpath, attribute_name).into() } // The attribute exists ByNameAttribute::Existing(AttributeInfo { @@ -290,7 +299,7 @@ fn by_name( // // We can't know whether the attribute is automatically or manually defined for sure, // and while we could check the location, the error seems clear enough as is. - npv_101::ByNameNonDerivation::new(attribute_name).into() + npv_101::ByNameNonDerivation::new(by_name_subpath, attribute_name).into() } // The attribute exists ByNameAttribute::Existing(AttributeInfo { @@ -302,16 +311,16 @@ fn by_name( }, location, }) => { - // Only derivations are allowed in `pkgs/by-name`. + // Only derivations are allowed in the by-name subpath. let is_derivation_result = if is_derivation { Success(()) } else { - npv_101::ByNameNonDerivation::new(attribute_name).into() + npv_101::ByNameNonDerivation::new(by_name_subpath, attribute_name).into() }; // If the definition looks correct let variant_result = match definition_variant { - // An automatic `callPackage` by the `pkgs/by-name` overlay. + // An automatic `callPackage` by the by-name overlay. // Though this gets detected by checking whether the internal // `_internalCallByNamePackageFile` was used DefinitionVariant::AutoDefinition => location.map_or_else( @@ -361,6 +370,7 @@ fn by_name( optional_syntactic_call_package, definition, location, + by_name_subpath, ) } else { // If manual definitions don't have a location, it's likely `mapAttrs`'d @@ -377,7 +387,7 @@ fn by_name( } }; Ok( - // Packages being checked in this function are _always_ already defined in `pkgs/by-name`, + // Packages being checked in this function are _always_ already defined in the by-name subpath, // so instead of repeating ourselves all the time to define `uses_by_name`, just set it // once at the end with a map. manual_definition_result.map(|manual_definition| ratchet::Package { @@ -387,7 +397,7 @@ fn by_name( ) } -/// Handles the case for packages in `pkgs/by-name` that are manually overridden, +/// Handles the case for packages in the given by-name subpath that are manually overridden, /// e.g. in `pkgs/top-level/all-packages.nix`. fn by_name_override( attribute_name: &str, @@ -395,10 +405,12 @@ fn by_name_override( optional_syntactic_call_package: Option, definition: String, location: location::Location, + by_name_subpath: &str, ) -> validation::Validation> { let Some(syntactic_call_package) = optional_syntactic_call_package else { // Something like ` = foo` return npv_104::ByNameOverrideOfNonSyntacticCallPackage::new( + by_name_subpath, attribute_name, location, definition, @@ -409,6 +421,7 @@ fn by_name_override( if !is_semantic_call_package { // Something like ` = pythonPackages.callPackage ...` return npv_105::ByNameOverrideOfNonTopLevelPackage::new( + by_name_subpath, attribute_name, location, definition, @@ -417,13 +430,20 @@ fn by_name_override( } let Some(actual_package_path) = syntactic_call_package.relative_path else { - return npv_108::ByNameOverrideContainsEmptyPath::new(attribute_name, location, definition) - .into(); + return npv_108::ByNameOverrideContainsEmptyPath::new( + by_name_subpath, + attribute_name, + location, + definition, + ) + .into(); }; - let expected_package_path = structure::relative_file_for_package(attribute_name); + let expected_package_path = + structure::relative_file_for_package(by_name_subpath, attribute_name); if actual_package_path != expected_package_path { return npv_106::ByNameOverrideContainsWrongCallPackagePath::new( + by_name_subpath, attribute_name, actual_package_path, location, @@ -435,8 +455,13 @@ fn by_name_override( // continue to be allowed. This is the state to migrate away from. if syntactic_call_package.empty_arg { Success(Loose( - npv_107::ByNameOverrideContainsEmptyArgument::new(attribute_name, location, definition) - .into(), + npv_107::ByNameOverrideContainsEmptyArgument::new( + by_name_subpath, + attribute_name, + location, + definition, + ) + .into(), )) } else { // This is the state to migrate to. @@ -444,44 +469,45 @@ fn by_name_override( } } -/// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`, turning it into a +/// Handles the evaluation result for an attribute _not_ in the given by-name subpath, turning it into a /// validation result. fn handle_non_by_name_attribute( nixpkgs_path: &Path, nix_file_store: &mut NixFileStore, attribute_name: &str, non_by_name_attribute: NonByNameAttribute, + by_name_subpath: &str, ) -> validation::Result { use NonByNameAttribute::EvalSuccess; use ratchet::RatchetState::{Loose, NonApplicable, Tight}; - // The ratchet state whether this attribute uses `pkgs/by-name`. + // The ratchet state whether this attribute uses the by-name subpath. // // This is never `Tight`, because we only either: - // - Know that the attribute _could_ be migrated to `pkgs/by-name`, which is `Loose` + // - Know that the attribute _could_ be migrated to the by-name subpath, which is `Loose` // - Or we're unsure, in which case we use `NonApplicable` let uses_by_name = // This is a big ol' match on various properties of the attribute // // First, it needs to succeed evaluation. We can't know whether an attribute could be - // migrated to `pkgs/by-name` if it doesn't evaluate, since we need to check that it's a + // migrated to the by-name subpath if it doesn't evaluate, since we need to check that it's a // derivation. // // This only has the minor negative effect that if a PR that breaks evaluation gets merged, - // fixing those failures won't force anything into `pkgs/by-name`. + // fixing those failures won't force anything into the by-name subpath. // // For now this isn't our problem, but in the future we might have another check to enforce // that evaluation must not be broken. // // The alternative of assuming that failing attributes would have been fit for - // `pkgs/by-name` has the problem that if a package evaluation gets broken temporarily, - // fixing it requires a move to pkgs/by-name, which could happen more often and isn't + // the by-name subpath has the problem that if a package evaluation gets broken temporarily, + // fixing it requires a move to the by-name subpath, which could happen more often and isn't // really justified. if let EvalSuccess(AttributeInfo { // We're only interested in attributes that are attribute sets, which all derivations - // are. Anything else can't be in `pkgs/by-name`. + // are. Anything else can't be in the by-name subpath. attribute_variant: AttributeVariant::AttributeSet { - // As of today, non-derivation attribute sets can't be in `pkgs/by-name`. + // As of today, non-derivation attribute sets can't be in by-name subpath. is_derivation: true, // Of the two definition variants, really only the manual one makes sense here. // @@ -537,7 +563,7 @@ fn handle_non_by_name_attribute( // Something like ` = bar` where `bar = pkgs.callPackage ...` | (true, None) => { // In all of these cases, it's not possible to migrate the package to - // `pkgs/by-name`. + // the by-name subpath. NonApplicable } @@ -545,14 +571,14 @@ fn handle_non_by_name_attribute( (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(BASE_SUBPATH) => { - // ..the path is not already within `pkgs/by-name` like + Some(ref rel_path) if rel_path.starts_with(by_name_subpath) => { + // ..the path is not already within the by-name subpath like // // foo-variant = callPackage ../by-name/fo/foo/package.nix { // someFlag = true; // } // - // While such definitions could be moved to `pkgs/by-name` by using + // While such definitions could be moved to the by-name subpath by using // `.override { someFlag = true; }` instead, this changes the semantics in // relation with overlays, so migration is generally not possible. // @@ -561,9 +587,9 @@ fn handle_non_by_name_attribute( NonApplicable } _ => { - // Otherwise, the path is outside `pkgs/by-name`, which means it can be - // migrated. - Loose((syntactic_call_package, location.file)) + // Otherwise, the path is outside of the by-name subpath, which means it can + // be migrated. + Loose((syntactic_call_package, location.file, by_name_subpath.into())) } } } @@ -575,7 +601,7 @@ fn handle_non_by_name_attribute( }; Ok(Success(ratchet::Package { // Packages being checked in this function _always_ need a manual definition, because - // they're not using `pkgs/by-name` which would allow avoiding it. So instead of repeating + // they're not using the by-name subpath which would allow avoiding it. So instead of repeating // ourselves all the time to define `manual_definition`, just set it once at the end here. manual_definition: Tight, uses_by_name, diff --git a/src/main.rs b/src/main.rs index 6656198..ba61b64 100644 --- a/src/main.rs +++ b/src/main.rs @@ -71,9 +71,10 @@ fn main() -> ExitCode { /// - `base_nixpkgs`: Path to the base Nixpkgs to run ratchet checks against. /// - `main_nixpkgs`: Path to the main Nixpkgs to check. fn process(base_nixpkgs: PathBuf, main_nixpkgs: &Path) -> Status { + let by_name_subpath = "pkgs/by-name"; // Very easy to parallelise this, since both operations are totally independent of each other. - let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs)); - let main_result = match check_nixpkgs(main_nixpkgs) { + let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs, by_name_subpath)); + let main_result = match check_nixpkgs(main_nixpkgs, by_name_subpath) { Ok(result) => result, Err(error) => { return error.into(); @@ -102,12 +103,15 @@ fn process(base_nixpkgs: PathBuf, main_nixpkgs: &Path) -> Status { } } -/// Checks whether the pkgs/by-name structure in Nixpkgs is valid. +/// Checks whether the by-name structure at the given path 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. -fn check_nixpkgs(nixpkgs_path: &Path) -> validation::Result { +fn check_nixpkgs( + nixpkgs_path: &Path, + by_name_subpath: &str, +) -> validation::Result { let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| { format!( "Nixpkgs path {} could not be resolved", @@ -118,15 +122,20 @@ fn check_nixpkgs(nixpkgs_path: &Path) -> validation::Result { let mut nix_file_store = NixFileStore::default(); let package_result = { - if !nixpkgs_path.join(structure::BASE_SUBPATH).exists() { - // No pkgs/by-name directory, always valid + if !nixpkgs_path.join(by_name_subpath).exists() { + // No directory at the given location (e.g. pkgs/by-name), always valid Success(BTreeMap::new()) } else { - let structure = check_structure(&nixpkgs_path, &mut nix_file_store)?; + let structure = check_structure(&nixpkgs_path, &mut nix_file_store, by_name_subpath)?; // 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()) + eval::check_values( + &nixpkgs_path, + by_name_subpath, + &mut nix_file_store, + package_names.as_slice(), + ) })? } }; @@ -150,7 +159,7 @@ mod tests { use pretty_assertions::StrComparison; use tempfile::{TempDir, tempdir_in}; - use super::{process, structure::BASE_SUBPATH}; + use super::process; #[test] fn tests_dir() -> anyhow::Result<()> { @@ -190,7 +199,7 @@ mod tests { return Ok(()); } - let base = path.join("main").join(BASE_SUBPATH); + let base = path.join("main/pkgs/by-name"); fs::create_dir_all(base.join("fo/foo"))?; fs::write(base.join("fo/foo/package.nix"), "{ someDrv }: someDrv")?; diff --git a/src/problem/npv_100.rs b/src/problem/npv_100.rs index 4c832f4..94c1476 100644 --- a/src/problem/npv_100.rs +++ b/src/problem/npv_100.rs @@ -6,14 +6,20 @@ use crate::structure; #[derive(Clone, new)] pub struct ByNameUndefinedAttribute { + #[new(into)] + by_name_subpath: String, #[new(into)] attribute_name: String, } impl fmt::Display for ByNameUndefinedAttribute { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let Self { attribute_name } = self; - let relative_package_file = structure::relative_file_for_package(attribute_name); + let Self { + by_name_subpath, + attribute_name, + } = self; + let relative_package_file = + structure::relative_file_for_package(by_name_subpath, attribute_name); write!( f, "- pkgs.{attribute_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}", diff --git a/src/problem/npv_101.rs b/src/problem/npv_101.rs index 1b92f31..27829fc 100644 --- a/src/problem/npv_101.rs +++ b/src/problem/npv_101.rs @@ -6,14 +6,20 @@ use crate::structure; #[derive(Clone, new)] pub struct ByNameNonDerivation { + #[new(into)] + by_name_subpath: String, #[new(into)] attribute_name: String, } impl fmt::Display for ByNameNonDerivation { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let Self { attribute_name } = self; - let relative_package_file = structure::relative_file_for_package(attribute_name); + let Self { + by_name_subpath, + attribute_name, + } = self; + let relative_package_file = + structure::relative_file_for_package(by_name_subpath, attribute_name); write!( f, "- pkgs.{attribute_name}: This attribute defined by {relative_package_file} is not a derivation", diff --git a/src/problem/npv_104.rs b/src/problem/npv_104.rs index 3aae406..f6678d2 100644 --- a/src/problem/npv_104.rs +++ b/src/problem/npv_104.rs @@ -10,6 +10,8 @@ use super::{create_path_expr, indent_definition}; #[derive(Clone, new)] pub struct ByNameOverrideOfNonSyntacticCallPackage { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, location: Location, @@ -20,13 +22,16 @@ pub struct ByNameOverrideOfNonSyntacticCallPackage { impl fmt::Display for ByNameOverrideOfNonSyntacticCallPackage { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, location, definition, } = self; let Location { file, line, column } = location; - let expected_package_path = structure::relative_file_for_package(package_name); - let relative_package_dir = structure::relative_dir_for_package(package_name); + let expected_package_path = + structure::relative_file_for_package(by_name_subpath, package_name); + let relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); let expected_path_expr = create_path_expr(file, expected_package_path); let indented_definition = indent_definition(*column, definition); diff --git a/src/problem/npv_105.rs b/src/problem/npv_105.rs index 1fcae39..b6d2984 100644 --- a/src/problem/npv_105.rs +++ b/src/problem/npv_105.rs @@ -10,6 +10,8 @@ use super::{create_path_expr, indent_definition}; #[derive(Clone, new)] pub struct ByNameOverrideOfNonTopLevelPackage { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, location: Location, @@ -20,13 +22,16 @@ pub struct ByNameOverrideOfNonTopLevelPackage { impl fmt::Display for ByNameOverrideOfNonTopLevelPackage { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, location, definition, } = self; let Location { file, line, column } = location; - let relative_package_dir = structure::relative_dir_for_package(package_name); - let expected_package_path = structure::relative_file_for_package(package_name); + let relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); + let expected_package_path = + structure::relative_file_for_package(by_name_subpath, package_name); let expected_path_expr = create_path_expr(file, expected_package_path); let indented_definition = indent_definition(*column, definition); diff --git a/src/problem/npv_106.rs b/src/problem/npv_106.rs index 727210a..3a3728e 100644 --- a/src/problem/npv_106.rs +++ b/src/problem/npv_106.rs @@ -11,6 +11,8 @@ use super::create_path_expr; #[derive(Clone, new)] pub struct ByNameOverrideContainsWrongCallPackagePath { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, #[new(into)] @@ -21,14 +23,17 @@ pub struct ByNameOverrideContainsWrongCallPackagePath { impl fmt::Display for ByNameOverrideContainsWrongCallPackagePath { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, location, actual_path, } = self; let Location { file, line, .. } = location; - let expected_package_path = structure::relative_file_for_package(package_name); + let expected_package_path = + structure::relative_file_for_package(by_name_subpath, package_name); let expected_path_expr = create_path_expr(file, expected_package_path); - let relative_package_dir = structure::relative_dir_for_package(package_name); + let relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); let actual_path_expr = create_path_expr(file, actual_path); writedoc!( f, diff --git a/src/problem/npv_107.rs b/src/problem/npv_107.rs index e62ead9..e45839b 100644 --- a/src/problem/npv_107.rs +++ b/src/problem/npv_107.rs @@ -10,6 +10,8 @@ use super::{create_path_expr, indent_definition}; #[derive(Clone, new)] pub struct ByNameOverrideContainsEmptyArgument { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, location: Location, @@ -20,14 +22,17 @@ pub struct ByNameOverrideContainsEmptyArgument { impl fmt::Display for ByNameOverrideContainsEmptyArgument { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, location, definition, } = self; let Location { file, line, column } = location; - let expected_package_path = structure::relative_file_for_package(package_name); + let expected_package_path = + structure::relative_file_for_package(by_name_subpath, package_name); let expected_path_expr = create_path_expr(file, expected_package_path); - let relative_package_dir = structure::relative_dir_for_package(package_name); + let relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); let indented_definition = indent_definition(*column, definition); writedoc!( diff --git a/src/problem/npv_108.rs b/src/problem/npv_108.rs index a73e899..705ce16 100644 --- a/src/problem/npv_108.rs +++ b/src/problem/npv_108.rs @@ -10,6 +10,8 @@ use super::{create_path_expr, indent_definition}; #[derive(Clone, new)] pub struct ByNameOverrideContainsEmptyPath { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, location: Location, @@ -20,13 +22,16 @@ pub struct ByNameOverrideContainsEmptyPath { impl fmt::Display for ByNameOverrideContainsEmptyPath { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, location, definition, } = self; let Location { file, line, column } = location; - let relative_package_dir = structure::relative_dir_for_package(package_name); - let expected_package_path = structure::relative_file_for_package(package_name); + let relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); + let expected_package_path = + structure::relative_file_for_package(by_name_subpath, package_name); let expected_path_expr = create_path_expr(file, expected_package_path); let indented_definition = indent_definition(*column, definition); diff --git a/src/problem/npv_109.rs b/src/problem/npv_109.rs index 67a1a3e..d18a714 100644 --- a/src/problem/npv_109.rs +++ b/src/problem/npv_109.rs @@ -6,13 +6,19 @@ use crate::structure; #[derive(Clone, new)] pub struct ByNameShardIsNotDirectory { + #[new(into)] + by_name_subpath: String, #[new(into)] shard_name: String, } impl fmt::Display for ByNameShardIsNotDirectory { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let relative_shard_path = structure::relative_dir_for_shard(&self.shard_name); + let Self { + by_name_subpath, + shard_name, + } = self; + let relative_shard_path = structure::relative_dir_for_shard(by_name_subpath, shard_name); write!( f, "- {relative_shard_path}: This is a file, but it should be a directory.", diff --git a/src/problem/npv_110.rs b/src/problem/npv_110.rs index 9b727f1..e3fde72 100644 --- a/src/problem/npv_110.rs +++ b/src/problem/npv_110.rs @@ -6,14 +6,19 @@ use crate::structure; #[derive(Clone, new)] pub struct ByNameShardIsInvalid { + #[new(into)] + by_name_subpath: String, #[new(into)] shard_name: String, } impl fmt::Display for ByNameShardIsInvalid { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let shard_name = &self.shard_name; - let relative_shard_path = structure::relative_dir_for_shard(shard_name); + let Self { + by_name_subpath, + shard_name, + } = self; + let relative_shard_path = structure::relative_dir_for_shard(by_name_subpath, shard_name); write!( f, "- {relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters, starting with a-z or \"_\", consisting of a-z, 0-9, \"-\" or \"_\".", diff --git a/src/problem/npv_111.rs b/src/problem/npv_111.rs index 5bee9b9..1a7f352 100644 --- a/src/problem/npv_111.rs +++ b/src/problem/npv_111.rs @@ -7,6 +7,8 @@ use crate::structure; #[derive(Clone, new)] pub struct ByNameShardIsCaseSensitiveDuplicate { + #[new(into)] + by_name_subpath: String, #[new(into)] shard_name: String, first: OsString, @@ -15,9 +17,15 @@ pub struct ByNameShardIsCaseSensitiveDuplicate { impl fmt::Display for ByNameShardIsCaseSensitiveDuplicate { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let relative_shard_path = structure::relative_dir_for_shard(&self.shard_name); - let first = self.first.to_string_lossy(); - let second = self.second.to_string_lossy(); + let Self { + by_name_subpath, + shard_name, + first, + second, + } = self; + let relative_shard_path = structure::relative_dir_for_shard(by_name_subpath, shard_name); + let first = first.to_string_lossy(); + let second = second.to_string_lossy(); write!( f, "- {relative_shard_path}: Duplicate case-sensitive package directories \"{first}\" and \"{second}\"." diff --git a/src/problem/npv_120.rs b/src/problem/npv_120.rs index db8ffb9..e0e4539 100644 --- a/src/problem/npv_120.rs +++ b/src/problem/npv_120.rs @@ -4,6 +4,8 @@ use derive_new::new; #[derive(Clone, new)] pub struct NixEvalError { + #[new(into)] + by_name_subpath: String, #[new(into)] stderr: String, } @@ -13,7 +15,8 @@ impl fmt::Display for NixEvalError { f.write_str(&self.stderr)?; write!( f, - "- Nix evaluation failed for some package in `pkgs/by-name`, see error above" + "- Nix evaluation failed for some package in `{}`, see error above", + self.by_name_subpath, ) } } diff --git a/src/problem/npv_140.rs b/src/problem/npv_140.rs index eb03d25..5f6765c 100644 --- a/src/problem/npv_140.rs +++ b/src/problem/npv_140.rs @@ -6,14 +6,20 @@ use crate::structure; #[derive(Clone, new)] pub struct PackageDirectoryIsNotDirectory { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, } impl fmt::Display for PackageDirectoryIsNotDirectory { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let Self { package_name } = self; - let relative_package_dir = structure::relative_dir_for_package(package_name); + let Self { + by_name_subpath, + package_name, + } = self; + let relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); write!( f, "- {relative_package_dir}: This path is a file, but it should be a directory.", diff --git a/src/problem/npv_142.rs b/src/problem/npv_142.rs index fd3e2b3..a81432a 100644 --- a/src/problem/npv_142.rs +++ b/src/problem/npv_142.rs @@ -7,6 +7,8 @@ use crate::structure; #[derive(Clone, new)] pub struct PackageInWrongShard { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, #[new(into)] @@ -16,10 +18,12 @@ pub struct PackageInWrongShard { impl fmt::Display for PackageInWrongShard { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, relative_package_dir, } = self; - let correct_relative_package_dir = structure::relative_dir_for_package(package_name); + let correct_relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); write!( f, "- {relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.", diff --git a/src/problem/npv_143.rs b/src/problem/npv_143.rs index fd49c78..62dc4bf 100644 --- a/src/problem/npv_143.rs +++ b/src/problem/npv_143.rs @@ -6,14 +6,20 @@ use crate::structure::{self, PACKAGE_NIX_FILENAME}; #[derive(Clone, new)] pub struct PackageNixMissing { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, } impl fmt::Display for PackageNixMissing { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let Self { package_name } = self; - let relative_package_dir = structure::relative_dir_for_package(package_name); + let Self { + by_name_subpath, + package_name, + } = self; + let relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); write!( f, "- {relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", diff --git a/src/problem/npv_144.rs b/src/problem/npv_144.rs index 740e822..b3f008e 100644 --- a/src/problem/npv_144.rs +++ b/src/problem/npv_144.rs @@ -6,14 +6,20 @@ use crate::structure::{self, PACKAGE_NIX_FILENAME}; #[derive(Clone, new)] pub struct PackageNixIsNotFile { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, } impl fmt::Display for PackageNixIsNotFile { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let Self { package_name } = self; - let relative_package_dir = structure::relative_dir_for_package(package_name); + let Self { + by_name_subpath, + package_name, + } = self; + let relative_package_dir = + structure::relative_dir_for_package(by_name_subpath, package_name); write!( f, "- {relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", diff --git a/src/problem/npv_160.rs b/src/problem/npv_160.rs index 5c8b7f6..aa5b100 100644 --- a/src/problem/npv_160.rs +++ b/src/problem/npv_160.rs @@ -8,6 +8,8 @@ use crate::structure; #[derive(Clone, new)] pub struct TopLevelPackageMovedOutOfByName { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, #[new(into)] @@ -19,11 +21,13 @@ pub struct TopLevelPackageMovedOutOfByName { impl fmt::Display for TopLevelPackageMovedOutOfByName { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, call_package_path, file, } = self; - let relative_package_file = structure::relative_file_for_package(package_name); + let relative_package_file = + structure::relative_file_for_package(by_name_subpath, package_name); let call_package_arg = call_package_path .as_ref() .map_or_else(|| "...".into(), |path| format!("./{}", path)); diff --git a/src/problem/npv_161.rs b/src/problem/npv_161.rs index 6dc7a66..48f9413 100644 --- a/src/problem/npv_161.rs +++ b/src/problem/npv_161.rs @@ -8,6 +8,8 @@ use crate::structure; #[derive(Clone, new)] pub struct TopLevelPackageMovedOutOfByNameWithCustomArguments { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, #[new(into)] @@ -19,11 +21,13 @@ pub struct TopLevelPackageMovedOutOfByNameWithCustomArguments { impl fmt::Display for TopLevelPackageMovedOutOfByNameWithCustomArguments { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, call_package_path, file, } = self; - let relative_package_file = structure::relative_file_for_package(package_name); + let relative_package_file = + structure::relative_file_for_package(by_name_subpath, package_name); let call_package_arg = call_package_path .as_ref() .map_or_else(|| "...".into(), |path| format!("./{}", path)); diff --git a/src/problem/npv_162.rs b/src/problem/npv_162.rs index cbc4708..54017fd 100644 --- a/src/problem/npv_162.rs +++ b/src/problem/npv_162.rs @@ -8,6 +8,8 @@ use crate::structure; #[derive(Clone, new)] pub struct NewTopLevelPackageShouldBeByName { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, #[new(into)] @@ -19,11 +21,13 @@ pub struct NewTopLevelPackageShouldBeByName { impl fmt::Display for NewTopLevelPackageShouldBeByName { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, call_package_path, file, } = self; - let relative_package_file = structure::relative_file_for_package(package_name); + let relative_package_file = + structure::relative_file_for_package(by_name_subpath, package_name); let call_package_arg = call_package_path .as_ref() .map_or_else(|| "...".into(), |path| format!("./{}", path)); diff --git a/src/problem/npv_163.rs b/src/problem/npv_163.rs index 81278e6..e7a5fe8 100644 --- a/src/problem/npv_163.rs +++ b/src/problem/npv_163.rs @@ -8,6 +8,8 @@ use crate::structure; #[derive(Clone, new)] pub struct NewTopLevelPackageShouldBeByNameWithCustomArgument { + #[new(into)] + by_name_subpath: String, #[new(into)] package_name: String, #[new(into)] @@ -19,11 +21,13 @@ pub struct NewTopLevelPackageShouldBeByNameWithCustomArgument { impl fmt::Display for NewTopLevelPackageShouldBeByNameWithCustomArgument { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let Self { + by_name_subpath, package_name, call_package_path, file, } = self; - let relative_package_file = structure::relative_file_for_package(package_name); + let relative_package_file = + structure::relative_file_for_package(by_name_subpath, package_name); let call_package_arg = call_package_path .as_ref() .map_or_else(|| "...".into(), |path| format!("./{}", path)); diff --git a/src/ratchet.rs b/src/ratchet.rs index e977bb0..600b09d 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -40,7 +40,7 @@ pub struct Package { /// The ratchet value for the check for non-auto-called empty arguments pub manual_definition: RatchetState, - /// The ratchet value for the check for new packages using pkgs/by-name + /// The ratchet value for the check for new packages using the by-name structure pub uses_by_name: RatchetState, } @@ -131,16 +131,17 @@ impl RatchetState { /// /// This ratchet is only tight for attributes that: /// -/// - Are not defined in `pkgs/by-name`, and rely on a manual definition. +/// - Are not defined in the by-name subpath, and rely on a manual definition. /// -/// - Are defined in `pkgs/by-name` without any manual definition (no custom argument overrides). +/// - Are defined in the by-name subpath without any manual definition (no custom argument +/// overrides). /// -/// - Are defined with `pkgs/by-name` with a manual definition that can't be removed +/// - Are defined with the by-name subpath with a manual definition that can't be removed /// because it provides custom argument overrides. /// /// In comparison, this ratchet is loose for attributes that: /// -/// - Are defined in `pkgs/by-name` with a manual definition that doesn't have any +/// - Are defined in the by-name subpath with a manual definition that doesn't have any /// custom argument overrides. pub enum ManualDefinition {} @@ -152,37 +153,51 @@ impl ToProblem for ManualDefinition { } } -/// The ratchet value of an attribute for the check that new packages use `pkgs/by-name`. +/// The ratchet value of an attribute for the check that new packages use the by-name subpath given +/// by ToContext. /// -/// This checks that all new package defined using `callPackage` must be defined via -/// `pkgs/by-name`. It also checks that once a package uses `pkgs/by-name`, it can't switch back +/// This checks that all new package defined using `callPackage` must be defined via the by-name +/// subpath. It also checks that once a package uses by-name subpath, it can't switch back /// to `pkgs/top-level/all-packages.nix`. pub enum UsesByName {} impl ToProblem for UsesByName { - type ToContext = (CallPackageArgumentInfo, RelativePathBuf); - - fn to_problem(name: &str, optional_from: Option<()>, (to, file): &Self::ToContext) -> Problem { + /// callPackage argument info, attribute location relative to the nixpkgs root, by-name subpath + type ToContext = (CallPackageArgumentInfo, RelativePathBuf, String); + + fn to_problem( + name: &str, + optional_from: Option<()>, + (to, file, by_name_subpath): &Self::ToContext, + ) -> Problem { let is_new = optional_from.is_none(); let is_empty = to.empty_arg; match (is_new, is_empty) { - (false, true) => { - npv_160::TopLevelPackageMovedOutOfByName::new(name, to.relative_path.clone(), file) - .into() - } - // This can happen if users mistakenly assume that `pkgs/by-name` can't be used + (false, true) => npv_160::TopLevelPackageMovedOutOfByName::new( + by_name_subpath, + name, + to.relative_path.clone(), + file, + ) + .into(), + // This can happen if users mistakenly assume that the by-name directory can't be used // for custom arguments. (false, false) => npv_161::TopLevelPackageMovedOutOfByNameWithCustomArguments::new( + by_name_subpath, + name, + to.relative_path.clone(), + file, + ) + .into(), + (true, true) => npv_162::NewTopLevelPackageShouldBeByName::new( + by_name_subpath, name, to.relative_path.clone(), file, ) .into(), - (true, true) => { - npv_162::NewTopLevelPackageShouldBeByName::new(name, to.relative_path.clone(), file) - .into() - } (true, false) => npv_163::NewTopLevelPackageShouldBeByNameWithCustomArgument::new( + by_name_subpath, name, to.relative_path.clone(), file, diff --git a/src/references.rs b/src/references.rs index 1c9185e..5c1938d 100644 --- a/src/references.rs +++ b/src/references.rs @@ -11,7 +11,7 @@ use crate::problem::{npv_121, npv_122, npv_123, npv_124, npv_125, npv_126, npv_1 use crate::structure::read_dir_sorted; use crate::validation::{self, ResultIteratorExt, Validation::Success}; -/// Check that every package directory in pkgs/by-name doesn't link to outside that directory. +/// Check that every package directory in the by-name structure doesn't link to outside that directory. /// Both symlinks and Nix path expressions are checked. pub fn check_references( nix_file_store: &mut NixFileStore, diff --git a/src/structure.rs b/src/structure.rs index 7a3edcf..dd19b41 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -12,7 +12,6 @@ use crate::problem::{npv_109, npv_110, npv_111, npv_140, npv_141, npv_142, npv_1 use crate::references; use crate::validation::{self, ResultIteratorExt, Validation::Success}; -pub const BASE_SUBPATH: &str = "pkgs/by-name"; pub const PACKAGE_NIX_FILENAME: &str = "package.nix"; static SHARD_NAME_REGEX: LazyLock = @@ -38,25 +37,26 @@ pub fn shard_for_package(package_name: &str) -> String { package_name.to_lowercase().chars().take(2).collect() } -pub fn relative_dir_for_shard(shard_name: &str) -> RelativePathBuf { - RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}")) +pub fn relative_dir_for_shard(by_name_subpath: &str, shard_name: &str) -> RelativePathBuf { + RelativePathBuf::from(format!("{by_name_subpath}/{shard_name}")) } -pub fn relative_dir_for_package(package_name: &str) -> RelativePathBuf { - relative_dir_for_shard(&shard_for_package(package_name)).join(package_name) +pub fn relative_dir_for_package(by_name_subpath: &str, package_name: &str) -> RelativePathBuf { + relative_dir_for_shard(by_name_subpath, &shard_for_package(package_name)).join(package_name) } -pub fn relative_file_for_package(package_name: &str) -> RelativePathBuf { - relative_dir_for_package(package_name).join(PACKAGE_NIX_FILENAME) +pub fn relative_file_for_package(by_name_subpath: &str, package_name: &str) -> RelativePathBuf { + relative_dir_for_package(by_name_subpath, package_name).join(PACKAGE_NIX_FILENAME) } /// Check the structure of Nixpkgs, returning the attribute names that are defined in -/// `pkgs/by-name` +/// the given by-name subpath pub fn check_structure( path: &Path, nix_file_store: &mut NixFileStore, + by_name_subpath: &str, ) -> validation::Result> { - let base_dir = path.join(BASE_SUBPATH); + let base_dir = path.join(by_name_subpath); let shard_results = read_dir_sorted(&base_dir)? .into_iter() @@ -70,11 +70,11 @@ pub fn check_structure( } else if !shard_path.is_dir() { // We can't check for any other errors if it's not a directory, since there are no // subdirectories to check. - npv_109::ByNameShardIsNotDirectory::new(shard_name).into() + npv_109::ByNameShardIsNotDirectory::new(by_name_subpath, shard_name).into() } else { let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); let result = if !shard_name_valid { - npv_110::ByNameShardIsInvalid::new(shard_name.clone()).into() + npv_110::ByNameShardIsInvalid::new(by_name_subpath, shard_name.clone()).into() } else { Success(()) }; @@ -87,6 +87,7 @@ pub fn check_structure( .filter(|(l, r)| l.file_name().eq_ignore_ascii_case(r.file_name())) .map(|(l, r)| { npv_111::ByNameShardIsCaseSensitiveDuplicate::new( + by_name_subpath, shard_name.clone(), l.file_name(), r.file_name(), @@ -105,6 +106,7 @@ pub fn check_structure( &shard_name, shard_name_valid, &package_entry, + by_name_subpath, ) }) .collect_vec()?; @@ -124,14 +126,15 @@ fn check_package( shard_name: &str, shard_name_valid: bool, package_entry: &DirEntry, + by_name_subpath: &str, ) -> validation::Result { let package_path = package_entry.path(); let package_name = package_entry.file_name().to_string_lossy().into_owned(); let relative_package_dir = - RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}")); + RelativePathBuf::from(format!("{by_name_subpath}/{shard_name}/{package_name}")); Ok(if !package_path.is_dir() { - npv_140::PackageDirectoryIsNotDirectory::new(package_name).into() + npv_140::PackageDirectoryIsNotDirectory::new(by_name_subpath, package_name).into() } else { let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); let result = if !package_name_valid { @@ -144,12 +147,13 @@ fn check_package( Success(()) }; - let correct_relative_package_dir = relative_dir_for_package(&package_name); + let correct_relative_package_dir = relative_dir_for_package(by_name_subpath, &package_name); 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 { npv_142::PackageInWrongShard::new( + by_name_subpath, package_name.clone(), relative_package_dir.clone(), ) @@ -163,9 +167,9 @@ fn check_package( let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); let result = result.and_(if !package_nix_path.exists() { - npv_143::PackageNixMissing::new(package_name.clone()).into() + npv_143::PackageNixMissing::new(by_name_subpath, package_name.clone()).into() } else if !package_nix_path.is_file() { - npv_144::PackageNixIsNotFile::new(package_name.clone()).into() + npv_144::PackageNixIsNotFile::new(by_name_subpath, package_name.clone()).into() } else { Success(()) });