From 8ab5566e8255e7d0883eafbac3dfa1aa94b18e56 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 14:02:24 -0700 Subject: [PATCH 01/10] Add rustfmt to the development shell This makes `cargo fmt` work. --- default.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/default.nix b/default.nix index acd2ae5..75d8928 100644 --- a/default.nix +++ b/default.nix @@ -67,6 +67,7 @@ let cargo-outdated npins rust-analyzer + rustfmt treefmtEval.config.build.wrapper ]; }; From 4d7d713fcc24781ddaaea11b1b1b539f6cd451a9 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 14:13:35 -0700 Subject: [PATCH 02/10] main.rs: line wrap to 100 characters --- src/main.rs | 49 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/src/main.rs b/src/main.rs index e4ce598..870e7dc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -22,8 +22,8 @@ use std::thread; /// 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. +/// 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 @@ -36,12 +36,11 @@ use std::thread; #[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. + /// Path to the main Nixpkgs to check. For PRs, set this to a checkout of the PR branch. nixpkgs: 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. + /// For PRs, set this to a checkout of the PRs base branch. #[arg(long)] base: PathBuf, } @@ -88,11 +87,19 @@ pub fn process( match (base_result, main_result) { (Failure(_), Failure(errors)) => { - // Base branch fails and the PR doesn't fix it and may also introduce additional problems + // The base branch fails, the PR doesn't fix it, and the PR may also introduce + // additional problems. for error in errors { writeln!(error_writer, "{}", error.to_string().red())? } - writeln!(error_writer, "{}", "The base branch is broken and still has above problems with this PR, which need to be fixed first.\nConsider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.".yellow())?; + writeln!( + error_writer, + "{}", + "The base branch is broken and still has above problems with this PR, \ + which need to be fixed first.\nConsider reverting the PR that introduced \ + these problems in order to prevent more failures of unrelated PRs." + .yellow() + )?; Ok(false) } (Failure(_), Success(_)) => { @@ -110,7 +117,8 @@ pub fn process( writeln!( error_writer, "{}", - "This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break." + "This PR introduces the problems listed above. \ + Please fix them before merging, otherwise the base branch would break." .yellow() )?; Ok(false) @@ -122,7 +130,13 @@ pub fn process( for error in errors { writeln!(error_writer, "{}", error.to_string().red())? } - writeln!(error_writer, "{}", "This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.".yellow())?; + writeln!( + error_writer, + "{}", + "This PR introduces additional instances of discouraged patterns as \ + listed above. Merging is discouraged but would not break the base branch." + .yellow() + )?; Ok(false) } @@ -197,8 +211,8 @@ mod tests { Ok(temp_env::with_vars(empty_list, tempfile::tempdir)?) } - // We cannot check case-conflicting files into Nixpkgs (the channel would fail to - // build), so we generate the case-conflicting file instead. + // We cannot check case-conflicting files into Nixpkgs (the channel would fail to build), + // so we generate the case-conflicting file instead. #[test] fn test_case_sensitive() -> anyhow::Result<()> { let temp_nixpkgs = tempdir()?; @@ -220,19 +234,23 @@ mod tests { test_nixpkgs( "case_sensitive", path, - "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\nThis PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.\n", + "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\n\ + This PR introduces the problems listed above. Please fix them before merging, \ + otherwise the base branch would break.\n", )?; Ok(()) } /// Tests symlinked temporary directories. - /// This is needed because on darwin, `/tmp` is a symlink to `/private/tmp`, and Nix's + /// + /// This is needed because on Darwin, `/tmp` is a symlink to `/private/tmp`, and Nix's /// restrict-eval doesn't also allow access to the canonical path when you allow the /// non-canonical one. /// /// The error if we didn't do this would look like this: - /// error: access to canonical path '/private/var/folders/[...]/.tmpFbcNO0' is forbidden in restricted mode + /// error: access to canonical path + /// '/private/var/folders/[...]/.tmpFbcNO0' is forbidden in restricted mode #[test] fn test_symlinked_tmpdir() -> anyhow::Result<()> { // Create a directory with two entries: @@ -277,7 +295,8 @@ mod tests { if actual_errors != expected_errors { panic!( - "Failed test case {name}, expected these errors:\n=======\n{}\n=======\nbut got these:\n=======\n{}\n=======", + "Failed test case {name}, expected these errors:\n=======\n{}\n=======\n\ + but got these:\n=======\n{}\n=======", expected_errors, actual_errors ); } From f7d4dc481d1886e02f3a7c95050832d1bbb33299 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 14:18:06 -0700 Subject: [PATCH 03/10] eval.nix: line wrap to 100 characters --- src/eval.nix | 58 ++++++++++++++++------------------ tests/by-name-failure/expected | 8 ++--- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/eval.nix b/src/eval.nix index 5165c8c..f3c6f5d 100644 --- a/src/eval.nix +++ b/src/eval.nix @@ -1,15 +1,14 @@ # Takes a path to nixpkgs and a path to the json-encoded list of `pkgs/by-name` attributes. -# 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 +# +# 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. { attrsPath, nixpkgsPath }: 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 `callPackage` and - # `_internalCallByNamePackageFile` with our own version that adds this + # 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 + # `callPackage` and `_internalCallByNamePackageFile` with our own version that adds this # information to the result, and then try to access it. overlay = final: prev: { @@ -17,41 +16,41 @@ let callPackage = fn: args: addVariantInfo (prev.callPackage fn args) { - # This is a manual definition of the attribute, and it's a callPackage, specifically a semantic callPackage + # This is a manual definition of the attribute, and it's a `1callPackage`, specifically a + # semantic `callPackage`. ManualDefinition.is_semantic_call_package = true; }; - # Adds information to each attribute about whether it's automatically - # defined by the `pkgs/by-name` overlay. This internal attribute is only - # used by that overlay. - # This overrides the above `callPackage` information (we don't need that - # one, since `pkgs/by-name` always uses `callPackage` underneath. + # Adds information to each attribute about whether it's automatically defined by the + # `pkgs/by-name` overlay. This internal attribute is only used by that overlay. + # + # This overrides the above `callPackage` information. It's OK because we don't need that one, + # since `pkgs/by-name` always uses `callPackage` underneath. _internalCallByNamePackageFile = file: addVariantInfo (prev._internalCallByNamePackageFile file) { AutoDefinition = null; }; }; - # We can't just replace attribute values with their info in the overlay, - # because attributes can depend on other attributes, so this would break evaluation. + # We can't just replace attribute values with their info in the overlay, because attributes can + # depend on other attributes, so this would break evaluation. addVariantInfo = value: variant: if builtins.isAttrs value then value // { _callPackageVariant = variant; } else - # It's very rare that callPackage doesn't return an attribute set, but it can occur. - # In such a case we can't really return anything sensible that would include the info, - # so just don't return the value directly and treat it as if it wasn't a callPackage. + # It's very rare that `callPackage` doesn't return an attribute set, but it can occur. + # In such a case we can't really return anything sensible that would include the info, so just + # don't return the value directly and treat it as if it wasn't a `callPackage`. value; pkgs = import nixpkgsPath { - # Don't let the users home directory influence this result + # Don't let the user's home directory influence this result. config = { }; overlays = [ overlay ]; - # We check evaluation and callPackage only for x86_64-linux. - # Not ideal, but hard to fix + # We check evaluation and `callPackage` only for x86_64-linux. Not ideal, but hard to fix. system = "x86_64-linux"; }; - # See AttributeInfo in ./eval.rs for the meaning of this + # See AttributeInfo in ./eval.rs for the meaning of this. attrInfo = name: value: { location = builtins.unsafeGetAttrPos name pkgs; attribute_variant = @@ -70,7 +69,7 @@ let }; }; - # Information on all attributes that are in pkgs/by-name. + # Information on all attributes that are in `pkgs/by-name`. byNameAttrs = builtins.listToAttrs ( map (name: { inherit name; @@ -78,18 +77,17 @@ let if !pkgs ? ${name} then { Missing = null; } else - # Evaluation failures are not allowed, so don't try to catch them + # Evaluation failures are not allowed, so don't try to catch them. { Existing = attrInfo name pkgs.${name}; }; }) 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 `pkgs/by-name`. + # We need this to enforce `pkgs/by-name` 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 `pkgs/by-name` often fail evaluation, so we need to handle that. output = attrInfo name value; result = builtins.tryEval (builtins.deepSeq output null); in @@ -101,8 +99,8 @@ let # All attributes attributes = byNameAttrs // nonByNameAttrs; in -# We output them in the form [ [ ] ]` such that the Rust side -# doesn't need to sort them again to get deterministic behavior (good for testing) +# We output them in the form [ [ ] ]` such that the Rust side doesn't need to sort +# them again to get deterministic behavior. This is good for testing. map (name: [ name attributes.${name} diff --git a/tests/by-name-failure/expected b/tests/by-name-failure/expected index dbf6317..97c5e98 100644 --- a/tests/by-name-failure/expected +++ b/tests/by-name-failure/expected @@ -6,12 +6,12 @@ error: … while evaluating attribute 'ByName' - at src/eval.nix:77:7: + at src/eval.nix:76:7: - 76| inherit name; - 77| value.ByName = + 75| inherit name; + 76| value.ByName = | ^ - 78| if !pkgs ? ${name} then + 77| if !pkgs ? ${name} then (stack trace truncated; use '--show-trace' to show the full trace) From b408c5abd14a0976dfe056c357677719e6511940 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 14:34:52 -0700 Subject: [PATCH 04/10] eval.rs: line wrap to 100 characters The only thing over 100 character now is a URL. --- src/eval.rs | 208 ++++++++++++++++++++++++++++------------------------ 1 file changed, 113 insertions(+), 95 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index dba47a9..eba7c44 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -19,37 +19,37 @@ use std::path::PathBuf; use std::process; use tempfile::NamedTempFile; -/// Attribute set of this structure is returned by 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 via `pkgs/by-name`. ByName(ByNameAttribute), - /// An attribute not defined via pkgs/by-name + /// An attribute not defined via `pkgs/by-name`. NonByName(NonByNameAttribute), } #[derive(Deserialize)] enum NonByNameAttribute { - /// The attribute doesn't evaluate + /// The attribute doesn't evaluate. EvalFailure, EvalSuccess(AttributeInfo), } #[derive(Deserialize)] enum ByNameAttribute { - /// The attribute doesn't exist at all + /// The attribute doesn't exist at all. Missing, Existing(AttributeInfo), } #[derive(Deserialize)] struct AttributeInfo { - /// The location of the attribute as returned by `builtins.unsafeGetAttrPos` + /// The location of the attribute as returned by `builtins.unsafeGetAttrPos`. location: Option, attribute_variant: AttributeVariant, } -/// The structure returned by a successful `builtins.unsafeGetAttrPos` +/// The structure returned by a successful `builtins.unsafeGetAttrPos`. #[derive(Deserialize, Clone, Debug)] struct Location { pub file: PathBuf, @@ -58,7 +58,7 @@ struct Location { } impl Location { - // Returns the [file] field, but relative to Nixpkgs + /// Returns the [file] field, but relative to Nixpkgs. fn relative_file(&self, nixpkgs_path: &Path) -> anyhow::Result { let path = self.file.strip_prefix(nixpkgs_path).with_context(|| { format!( @@ -73,33 +73,33 @@ impl Location { #[derive(Deserialize)] pub enum AttributeVariant { - /// The attribute is not an attribute set, we're limited in the amount of information we can get - /// from it (though it's obviously not a derivation) + /// The attribute is not an attribute set, so we're limited in the amount of information we can + /// get from it. Since all derivations are attribute sets, it's obviously not a derivation. NonAttributeSet, AttributeSet { /// Whether the attribute is a derivation (`lib.isDerivation`) is_derivation: bool, - /// The type of callPackage + /// The type of `callPackage` used. definition_variant: DefinitionVariant, }, } #[derive(Deserialize)] pub enum DefinitionVariant { - /// An automatic definition by the `pkgs/by-name` overlay - /// Though it's detected using the internal _internalCallByNamePackageFile attribute, - /// which can in theory also be used by other code + /// An automatic definition by the `pkgs/by-name` overlay, though it's detected using the + /// internal `_internalCallByNamePackageFile` attribute, which can in theory also be used by + /// other code. AutoDefinition, - /// A manual definition of the attribute, typically in `all-packages.nix` + /// A manual definition of the attribute, typically in `all-packages.nix`. ManualDefinition { /// Whether the attribute is defined as `pkgs.callPackage ...` or something else. is_semantic_call_package: bool, }, } -/// Check that the Nixpkgs attribute values corresponding to the packages in pkgs/by-name are -/// of the form `callPackage { ... }`. -/// See the `eval.nix` file for how this is achieved on the Nix side +/// Check that the Nixpkgs attribute values corresponding to the packages in `pkgs/by-name` are of +/// the form `callPackage { ... }`. See the `./eval.nix` file for how this is +/// achieved on the Nix side. pub fn check_values( nixpkgs_path: &Path, nix_file_store: &mut NixFileStore, @@ -109,10 +109,10 @@ pub fn check_values( // 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().with_context(|| "Failed to create a temporary file")?; - // We need to canonicalise this path because if it's a symlink (which can be the case on - // Darwin), Nix would need to read both the symlink and the target path, therefore need 2 - // NIX_PATH entries for restrict-eval. But if we resolve the symlinks then only one predictable - // entry is needed. + + // We need to canonicalise this path. If it's a symlink (which can be the case on Darwin), + // Nix would need to read both the symlink and the target path, and therefore need two NIX_PATH + // entries for restrict-eval. If we resolve the symlinks, we only need one predictable entry. let attrs_file_path = attrs_file.path().canonicalize()?; serde_json::to_writer(&attrs_file, &package_names).with_context(|| { @@ -124,8 +124,8 @@ pub fn check_values( let expr_path = std::env::var("NIX_CHECK_BY_NAME_EXPR_PATH") .with_context(|| "Could not get environment variable NIX_CHECK_BY_NAME_EXPR_PATH")?; - // With restrict-eval, only paths in NIX_PATH can be accessed, so we explicitly specify the - // ones needed needed + + // With restrict-eval, only paths in NIX_PATH can be accessed. We explicitly specify them here. let mut command = process::Command::new("nix-instantiate"); command // Capture stderr so that it can be printed later in case of failure @@ -138,23 +138,23 @@ pub fn check_values( "--restrict-eval", ]) // Pass the path to the attrs_file as an argument and add it to the NIX_PATH so it can be - // accessed in restrict-eval mode + // accessed in restrict-eval mode. .args(["--arg", "attrsPath"]) .arg(&attrs_file_path) .arg("-I") .arg(&attrs_file_path) - // Same for the nixpkgs to test + // Same for the nixpkgs to test. .args(["--arg", "nixpkgsPath"]) .arg(nixpkgs_path) .arg("-I") .arg(nixpkgs_path); - // Only do these actions if we're not running tests + // Only do these actions if we're not running tests. if !is_test { - // Clear NIX_PATH to be sure it doesn't influence the result (during tests we need to have - // available though + // Clear NIX_PATH to be sure it doesn't influence the result. + // During tests we need to have available. command.env_remove("NIX_PATH"); - // Show the full Nix error trace, but not in tests because the full trace is super impure + // Show the full Nix error trace, but not in tests because the full trace is super impure. command.arg("--show-trace"); } @@ -208,8 +208,7 @@ pub fn check_values( })) } -/// Handles the evaluation result for an attribute in `pkgs/by-name`, -/// turning it into a validation result. +/// Handle the evaluation result for an attribute in `pkgs/by-name`, making it a validation result. fn by_name( nix_file_store: &mut NixFileStore, nixpkgs_path: &Path, @@ -227,11 +226,11 @@ fn by_name( .into() }; - // At this point we know that `pkgs/by-name/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 + // At this point we know that `pkgs/by-name/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 + // The attribute is missing. Missing => { // This indicates a bug in the `pkgs/by-name` overlay, because it's supposed to // automatically defined attributes in `pkgs/by-name` @@ -261,7 +260,7 @@ fn by_name( }, location, }) => { - // Only derivations are allowed in `pkgs/by-name` + // Only derivations are allowed in `pkgs/by-name`. let is_derivation_result = if is_derivation { Success(()) } else { @@ -275,8 +274,9 @@ fn by_name( // `_internalCallByNamePackageFile` was used DefinitionVariant::AutoDefinition => { if let Some(_location) = location { - // Such an automatic definition should definitely not have a location - // Having one indicates that somebody is using `_internalCallByNamePackageFile`, + // Such an automatic definition should definitely not have a location. + // Having one indicates that somebody is using + // `_internalCallByNamePackageFile`, to_validation(ByNameErrorKind::InternalCallPackageUsed) } else { Success(Tight) @@ -295,16 +295,27 @@ fn by_name( let nix_file = nix_file_store.get(&location.file)?; // The relative path of the Nix file, for error messages - let relative_location_file = location.relative_file(nixpkgs_path).with_context(|| { - format!("Failed to resolve the file where attribute {attribute_name} is defined") - })?; + let relative_location_file = + location.relative_file(nixpkgs_path).with_context(|| { + format!( + "Failed to resolve the file where attribute {} is defined", + attribute_name + ) + })?; - // Figure out whether it's an attribute definition of the form `= callPackage `, - // returning the arguments if so. + // Figure out whether it's an attribute definition of the form + // `= callPackage `, returning the arguments if so. let (optional_syntactic_call_package, definition) = nix_file - .call_package_argument_info_at(location.line, location.column, nixpkgs_path) + .call_package_argument_info_at( + location.line, + location.column, + nixpkgs_path, + ) .with_context(|| { - format!("Failed to get the definition info for attribute {attribute_name}") + format!( + "Failed to get the definition info for attribute {}", + attribute_name + ) })?; by_name_override( @@ -318,20 +329,21 @@ fn by_name( } else { // If manual definitions don't have a location, it's likely `mapAttrs`'d // over, e.g. if it's defined in aliases.nix. - // We can't verify whether its of the expected `callPackage`, so error out + // We can't verify whether its of the expected `callPackage`, so error out. to_validation(ByNameErrorKind::CannotDetermineAttributeLocation) } } }; - // Independently report problems about whether it's a derivation and the callPackage variant + // Independently report problems about whether it's a derivation and the callPackage + // variant. is_derivation_result.and(variant_result) } }; Ok( // Packages being checked in this function are _always_ already defined in `pkgs/by-name`, // so instead of repeating ourselves all the time to define `uses_by_name`, just set it - // once at the end with a map + // once at the end with a map. manual_definition_result.map(|manual_definition| ratchet::Package { manual_definition, uses_by_name: Tight, @@ -339,8 +351,8 @@ fn by_name( ) } -/// Handles the case for packages in `pkgs/by-name` that are manually overridden, e.g. in -/// all-packages.nix +/// Handles the case for packages in `pkgs/by-name` that are manually overridden, +/// e.g. in `pkgs/top-level/all-packages.nix`. fn by_name_override( attribute_name: &str, is_semantic_call_package: bool, @@ -363,13 +375,14 @@ fn by_name_override( }) }; - // At this point, we completed two different checks for whether it's a - // `callPackage` + // 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 ` = foo` (_, None) => to_problem(ByNameOverrideErrorKind::NonSyntacticCallPackage).into(), + // Something like ` = pythonPackages.callPackage ...` (false, Some(_)) => to_problem(ByNameOverrideErrorKind::NonToplevelCallPackage).into(), + // Something like ` = pkgs.callPackage ...` (true, Some(syntactic_call_package)) => { if let Some(actual_package_path) = syntactic_call_package.relative_path { @@ -380,28 +393,28 @@ fn by_name_override( }) .into() } else { - // Manual definitions with empty arguments are not allowed - // anymore, but existing ones should continue to be allowed + // Manual definitions with empty arguments are not allowed anymore, + // but existing ones should continue to be allowed. let manual_definition_ratchet = if syntactic_call_package.empty_arg { - // This is the state to migrate away from + // This is the state to migrate away from. Loose(to_problem(ByNameOverrideErrorKind::EmptyArgument)) } else { - // This is the state to migrate to + // This is the state to migrate to. Tight }; Success(manual_definition_ratchet) } } else { - // No path + // No path... to_problem(ByNameOverrideErrorKind::NonPath).into() } } } } -/// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`, -/// turning it into a validation result. +/// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`, turning it into a +/// validation result. fn handle_non_by_name_attribute( nixpkgs_path: &Path, nix_file_store: &mut NixFileStore, @@ -412,47 +425,52 @@ fn handle_non_by_name_attribute( use NonByNameAttribute::*; // The ratchet state whether this attribute uses `pkgs/by-name`. + // // This is never `Tight`, because we only either: // - Know that the attribute _could_ be migrated to `pkgs/by-name`, which is `Loose` - // - Or we're unsure, in which case we use NonApplicable + // - 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 // 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`. + // 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`. // - // For now this isn't our problem, but in the future we - // might have another check to enforce that evaluation must not be broken. + // 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 really justified. + // 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 + // really justified. if let EvalSuccess(AttributeInfo { - // We're only interested in attributes that are attribute sets (which includes - // derivations). Anything else can't be in `pkgs/by-name`. + // We're only interested in attributes that are attribute sets, which all derivations + // are. Anything else can't be in `pkgs/by-name`. attribute_variant: AttributeVariant::AttributeSet { - // Indeed, we only care about derivations, non-derivation attribute sets can't be - // in `pkgs/by-name` + // As of today, non-derivation attribute sets can't be in `pkgs/by-name`. is_derivation: true, // Of the two definition variants, really only the manual one makes sense here. + // // Special cases are: + // // - Manual aliases to auto-called packages are not treated as manual definitions, - // due to limitations in the semantic callPackage detection. So those should be - // ignored. - // - Manual definitions using the internal _internalCallByNamePackageFile are - // not treated as manual definitions, since _internalCallByNamePackageFile is + // due to limitations in the semantic `callPackage` detection. + // So those should be ignored. + // + // - Manual definitions using the internal `_internalCallByNamePackageFile` are + // not treated as manual definitions, since `_internalCallByNamePackageFile` is // used to detect automatic ones. We can't distinguish from the above case, so we // just need to ignore this one too, even if that internal attribute should never // be called manually. - definition_variant: DefinitionVariant::ManualDefinition { is_semantic_call_package } + definition_variant: DefinitionVariant::ManualDefinition { + is_semantic_call_package + } }, - // We need the location of the manual definition, because otherwise - // we can't figure out whether it's a syntactic callPackage + // We need the location of the manual definition, because otherwise we can't figure out + // whether it's a syntactic `callPackage`. location: Some(location), }) = non_by_name_attribute { @@ -464,23 +482,22 @@ fn handle_non_by_name_attribute( format!("Failed to resolve the file where attribute {attribute_name} is defined") })?; - // Figure out whether it's an attribute definition of the form `= callPackage `, - // returning the arguments if so. + // Figure out whether it's an attribute definition of the form + // `= callPackage `, returning the arguments if so. let (optional_syntactic_call_package, _definition) = nix_file .call_package_argument_info_at( location.line, location.column, - // Passing the Nixpkgs path here both checks that the is within Nixpkgs, and - // strips the absolute Nixpkgs path from it, such that + // Passing the Nixpkgs path here both checks that the is within Nixpkgs, + // and strips the absolute Nixpkgs path from it, such that // syntactic_call_package.relative_path is relative to Nixpkgs nixpkgs_path - ) + ) .with_context(|| { - format!("Failed to get the definition info for attribute {attribute_name}") + format!("Failed to get the definition info for attribute {}", attribute_name) })?; - // At this point, we completed two different checks for whether it's a - // `callPackage` + // 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 ` = { }` (false, None) @@ -488,9 +505,11 @@ fn handle_non_by_name_attribute( | (false, Some(_)) // 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` + // In all of these cases, it's not possible to migrate the package to + // `pkgs/by-name`. NonApplicable } + // Something like ` = pkgs.callPackage ...` (true, Some(syntactic_call_package)) => { // It's only possible to migrate such a definitions if.. @@ -512,7 +531,7 @@ fn handle_non_by_name_attribute( } _ => { // Otherwise, the path is outside `pkgs/by-name`, which means it can be - // migrated + // migrated. Loose((syntactic_call_package, relative_location_file)) } } @@ -520,14 +539,13 @@ fn handle_non_by_name_attribute( } } else { // This catches all the cases not matched by the above `if let`, falling back to not being - // able to migrate such attributes + // able to migrate such attributes. NonApplicable }; 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 ourselves all the time to define `manual_definition`, - // just set it once at the end here + // they're not using `pkgs/by-name` 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, })) From 0f2d05de703371d9aa8b85e55a109de5b739c802 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 14:41:25 -0700 Subject: [PATCH 05/10] nix_file.rs: line wrap to 100 characters --- src/nix_file.rs | 176 ++++++++++++++++++++++++++++-------------------- 1 file changed, 104 insertions(+), 72 deletions(-) diff --git a/src/nix_file.rs b/src/nix_file.rs index e2dc1e1..3b1695f 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -16,8 +16,8 @@ use std::fs::read_to_string; use std::path::Path; use std::path::PathBuf; -/// A structure to store parse results of Nix files in memory, -/// making sure that the same file never has to be parsed twice +/// A structure to store parse results of Nix files in memory, making sure that the same file never +/// has to be parsed twice. #[derive(Default)] pub struct NixFileStore { entries: HashMap, @@ -25,10 +25,10 @@ pub struct NixFileStore { impl NixFileStore { /// Get the store entry for a Nix file if it exists, otherwise parse the file, insert it into - /// the store, and return the value + /// the store, and return the value. /// - /// Note that this function only gives an anyhow::Result::Err for I/O errors. - /// A parse error is anyhow::Result::Ok(Result::Err(error)) + /// Note that this function only gives an `anyhow::Result::Err` for I/O errors. + /// A parse error is `anyhow::Result::Ok(Result::Err(error))` pub fn get(&mut self, path: &Path) -> anyhow::Result<&NixFile> { match self.entries.entry(path.to_owned()) { Entry::Occupied(entry) => Ok(entry.into_mut()), @@ -37,18 +37,18 @@ impl NixFileStore { } } -/// A structure for storing a successfully parsed Nix file +/// A structure for storing a successfully parsed Nix file. pub struct NixFile { - /// The parent directory of the Nix file, for more convenient error handling + /// The parent directory of the Nix file, for more convenient error handling. pub parent_dir: PathBuf, - /// The path to the file itself, for errors + /// The path to the file itself, for errors. pub path: PathBuf, pub syntax_root: rnix::Root, pub line_index: LineIndex, } impl NixFile { - /// Creates a new NixFile, failing for I/O or parse errors + /// Creates a new NixFile, failing for I/O or parse errors. fn new(path: impl AsRef) -> anyhow::Result { let Some(parent_dir) = path.as_ref().parent() else { anyhow::bail!("Could not get parent of path {}", path.as_ref().display()) @@ -56,6 +56,7 @@ impl NixFile { let contents = read_to_string(&path) .with_context(|| format!("Could not read file {}", path.as_ref().display()))?; + let line_index = LineIndex::new(&contents); // NOTE: There's now another Nixpkgs CI check to make sure all changed Nix files parse @@ -63,8 +64,8 @@ impl NixFile { // errors. In the future we should unify these two checks, ideally moving the other CI // check into this tool as well and checking for both mainline Nix and rnix. rnix::Root::parse(&contents) - // rnix's ::ok returns Result<_, _> , so no error is thrown away like it would be with - // std::result's ::ok + // rnix's `rnix::Parse::ok` returns `Result<_, _>`, so no error is thrown away like it + // would be with `std::result::Result::ok`. .ok() .map(|syntax_root| NixFile { parent_dir: parent_dir.to_path_buf(), @@ -76,40 +77,52 @@ impl NixFile { } } -/// Information about callPackage arguments +/// Information about `callPackage` arguments. #[derive(Debug, PartialEq)] pub struct CallPackageArgumentInfo { /// The relative path of the first argument, or `None` if it's not a path. pub relative_path: Option, - /// Whether the second argument is an empty attribute set + + /// Whether the second argument is an empty attribute set. pub empty_arg: bool, } impl NixFile { - /// Returns information about callPackage arguments for an attribute at a specific line/column - /// index. - /// If the definition at the given location is not of the form ` = callPackage ;`, - /// `Ok((None, String))` is returned, with `String` being the definition itself. + /// Returns information about `callPackage` arguments for an attribute at a specific + /// line/column index. + /// + /// If the definition at the given location is not of the form + /// ` = callPackage ;`, `Ok((None, String))` is returned, with `String` + /// being the definition itself. /// /// This function only returns `Err` for problems that can't be caused by the Nix contents, /// but rather problems in this programs code itself. /// /// This is meant to be used with the location returned from `builtins.unsafeGetAttrPos`, e.g.: - /// - Create file `default.nix` with contents + /// + /// - Create file `default.nix` with this contents: + /// /// ```nix /// self: { /// foo = self.callPackage ./default.nix { }; /// } /// ``` - /// - Evaluate + /// + /// - Evaluate using `nix-instantiate`: + /// /// ```nix /// builtins.unsafeGetAttrPos "foo" (import ./default.nix { }) /// ``` + /// /// results in `{ file = ./default.nix; line = 2; column = 3; }` + /// /// - Get the NixFile for `.file` from a `NixFileStore` - /// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current directory + /// + /// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current + /// directory. /// /// You'll get back + /// /// ```rust /// Ok(( /// Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }), @@ -157,13 +170,18 @@ impl NixFile { // This is the token offset, we get both the (newline + indentation) on the left side, // and the attribute name on the right side. let TokenAtOffset::Between(_space, token) = token_at_offset else { - anyhow::bail!("Line {line} column {column} in {} is not the start of a token, but rather {token_at_offset:?}", self.path.display()) + anyhow::bail!( + "Line {line} column {column} in {} is not the start of a token, but rather \ + {token_at_offset:?}", + self.path.display() + ) }; - // token looks like "foo" + // Token looks like "foo" let Some(node) = token.parent() else { anyhow::bail!( - "Token on line {line} column {column} in {} does not have a parent node: {token:?}", + "Token on line {line} column {column} in {} does not have a parent node: \ + {token:?}", self.path.display() ) }; @@ -171,20 +189,20 @@ impl NixFile { if ast::Attr::can_cast(node.kind()) { // Something like `foo`, `"foo"` or `${"foo"}` } else if ast::Inherit::can_cast(node.kind()) { - // Something like `inherit ` or `inherit () ` + // Something like `inherit ` or `inherit () `. // This is the only other way how `builtins.unsafeGetAttrPos` can return - // attribute positions, but we only look for ones like ` = `, so - // ignore this + // attribute positions, but we only look for ones like ` = `, + // so ignore this. return Ok(Left(node.to_string())); } else { - // However, anything else is not expected and smells like a bug + // However, anything else is not expected and smells like a bug. anyhow::bail!( "Node in {} is neither an attribute node nor an inherit node: {node:?}", self.path.display() ) } - // node looks like "foo" + // Node looks like "foo" let Some(attrpath_node) = node.parent() else { anyhow::bail!( "Node in {} does not have a parent node: {node:?}", @@ -193,7 +211,7 @@ impl NixFile { }; if !ast::Attrpath::can_cast(attrpath_node.kind()) { - // We know that `node` is an attribute, its parent should be an attribute path + // We know that `node` is an attribute, so its parent should be an attribute path. anyhow::bail!( "In {}, attribute parent node is not an attribute path node: {attrpath_node:?}", self.path.display() @@ -214,17 +232,17 @@ impl NixFile { self.path.display() ) } - // attrpath_value_node looks like "foo.bar = 10;" - // unwrap is fine because we confirmed that we can cast with the above check. - // We could avoid this `unwrap` for a `clone`, since `cast` consumes the argument, - // but we still need it for the error message when the cast fails. + // attrpath_value_node looks like `foo.bar = 10;`. `unwrap` is fine because we confirmed + // that we can cast with the above check. We could avoid this `unwrap` for a `clone`, + // since `cast` consumes the argument, but we still need it for the error message when the + // cast fails. Ok(Right( ast::AttrpathValue::cast(attrpath_value_node).unwrap(), )) } - // Internal function mainly to make attrpath_value_at independently testable + // Internal function mainly to make `attrpath_value_at` independently testable. fn attrpath_value_call_package_argument_info( &self, attrpath_value: ast::AttrpathValue, @@ -244,8 +262,9 @@ impl NixFile { // paths and we can't really know which one it is. We could have a case like // `foo.bar = callPackage ... { }` and trying to determine if `bar` is a `callPackage`, // where this is not correct. - // However, this case typically doesn't occur anyways, - // because top-level packages wouldn't be nested under an attribute set. + // + // However, this case typically doesn't occur anyways, because top-level packages + // wouldn't be nested under an attribute set. return Ok(None); } let Some(value) = attrpath_value.value() else { @@ -258,87 +277,95 @@ impl NixFile { // Not even a function call, instead something like `foo = null` return Ok(None); }; + let Some(function1) = apply1.lambda() else { anyhow::bail!("apply node doesn't have a lambda: {apply1:?}") }; + let Some(arg1) = apply1.argument() else { anyhow::bail!("apply node doesn't have an argument: {apply1:?}") }; // At this point we know it's something like `foo = `. - // For a callPackage, `` would be `callPackage ./file` and `` would be `{ }` + // For a callPackage, `` would be `callPackage ./file` and `` would be `{ }`. let empty_arg = if let Expr::AttrSet(attrset) = arg1 { // We can only statically determine whether the argument is empty if it's an attribute - // set _expression_, even though other kind of expressions could evaluate to an attribute - // set _value_. But this is what we want anyways + // set _expression_, even though other kind of expressions could evaluate to an + // attribute set _value_. But this is what we want anyway. attrset.entries().next().is_none() } else { false }; - // Because callPackage takes two curried arguments, the first function needs to be a - // function call itself + // Because `callPackage` takes two curried arguments, the first function needs to be a + // function call itself. let Expr::Apply(apply2) = function1 else { - // Not a callPackage, instead something like `foo = import ./foo` + // Not a `callPackage`, instead something like `foo = import ./foo`. return Ok(None); }; + let Some(function2) = apply2.lambda() else { anyhow::bail!("apply node doesn't have a lambda: {apply2:?}") }; + let Some(arg2) = apply2.argument() else { anyhow::bail!("apply node doesn't have an argument: {apply2:?}") }; // At this point we know it's something like `foo = `. - // For a callPackage, `` would be `callPackage`, `` would be `./file` + // For a callPackage, `` would be `callPackage`, `` would be `./file`. - // Check that is a path expression + // Check that is a path expression. let path = if let Expr::Path(actual_path) = arg2 { - // Try to statically resolve the path and turn it into a nixpkgs-relative path + // Try to statically resolve the path and turn it into a nixpkgs-relative path. if let ResolvedPath::Within(p) = self.static_resolve_path(actual_path, relative_to) { Some(p) } else { - // We can't statically know an existing path inside Nixpkgs used as + // We can't statically know an existing path inside Nixpkgs used as . None } } else { - // is not a path, but rather e.g. an inline expression + // is not a path, but rather e.g. an inline expression. None }; - // Check that is an identifier, or an attribute path with an identifier at the end + // Check that is an identifier, or an attribute path with an identifier at the end. let ident = match function2 { Expr::Ident(ident) => { - // This means it's something like `foo = callPackage ` + // This means it's something like `foo = callPackage `. ident } + Expr::Select(select) => { // This means it's something like `foo = self.callPackage `. // We also end up here for e.g. `pythonPackages.callPackage`, but the - // callPackage-mocking method will take care of not triggering for this case. - + // `callPackage`-mocking method will take care of not triggering for this case. if select.default_expr().is_some() { - // Very odd case, but this would be `foo = self.callPackage or true ./test.nix {} - // (yes this is valid Nix code) + // Very odd case: `foo = self.callPackage or true ./test.nix {}`. + // Yes, this is valid Nix code! return Ok(None); } + let Some(attrpath) = select.attrpath() else { anyhow::bail!("select node doesn't have an attrpath: {select:?}") }; + let Some(last) = attrpath.attrs().last() else { // This case shouldn't be possible, it would be `foo = self. ./test.nix {}`, - // which shouldn't parse + // which shouldn't parse. anyhow::bail!("select node has an empty attrpath: {select:?}") }; + if let ast::Attr::Ident(ident) = last { ident } else { - // Here it's something like `foo = self."callPackage" /test.nix {}` - // which we're not gonna bother with + // Here it's something like `foo = self."callPackage" /test.nix {}` which we're + // not going to bother with. return Ok(None); } } + // Any other expression we're not gonna treat as callPackage _ => return Ok(None), }; @@ -358,27 +385,31 @@ impl NixFile { } } -/// The result of trying to statically resolve a Nix path expression +/// The result of trying to statically resolve a Nix path expression. pub enum ResolvedPath { - /// Something like `./foo/${bar}/baz`, can't be known statically + /// Something like `./foo/${bar}/baz`. This can't be known statically. Interpolated, - /// Something like ``, can't be known statically + + /// Something like ``. This can't be known statically. SearchPath, - /// Path couldn't be resolved due to an IO error, - /// e.g. if the path doesn't exist or you don't have the right permissions + + /// Path couldn't be resolved due to an IO error, e.g. if the path doesn't exist or you don't + /// have the right permissions. Unresolvable(std::io::Error), - /// The path is outside the given absolute path + + /// The path is outside the given absolute path. Outside, - /// The path is within the given absolute path. - /// The `RelativePathBuf` is the relative path under the given absolute path. + + /// The path is within the given absolute path. The `RelativePathBuf` is the relative path + /// under the given absolute path. Within(RelativePathBuf), } impl NixFile { - /// Statically resolves a Nix path expression and checks that it's within an absolute path + /// Statically resolves a Nix path expression and checks that it's within an absolute path. /// - /// E.g. for the path expression `./bar.nix` in `./foo.nix` and an absolute path of the - /// current directory, the function returns `ResolvedPath::Within(./bar.nix)` + /// Given the path expression `./bar.nix` in `./foo.nix` and an absolute path of the + /// current directory, the function returns `ResolvedPath::Within(./bar.nix)`. pub fn static_resolve_path(&self, node: ast::Path, relative_to: &Path) -> ResolvedPath { if node.parts().count() != 1 { // If there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`. @@ -389,18 +420,19 @@ impl NixFile { if text.starts_with('<') { // A search path like ``. There doesn't appear to be better way to detect - // these in rnix + // these in rnix. return ResolvedPath::SearchPath; } - // Join the file's parent directory and the path expression, then resolve it + // Join the file's parent directory and the path expression, then resolve it. + // // FIXME: Expressions like `../../../../foo/bar/baz/qux` or absolute paths // may resolve close to the original file, but may have left the relative_to. - // That should be checked more strictly + // That should be checked more strictly. match self.parent_dir.join(Path::new(&text)).canonicalize() { Err(resolution_error) => ResolvedPath::Unresolvable(resolution_error), Ok(resolved) => { - // Check if it's within relative_to + // Check if it's within relative_to. match resolved.strip_prefix(relative_to) { Err(_prefix_error) => ResolvedPath::Outside, Ok(suffix) => ResolvedPath::Within( @@ -450,7 +482,7 @@ mod tests { let nix_file = NixFile::new(&file)?; - // These are builtins.unsafeGetAttrPos locations for the attributes + // These are `builtins.unsafeGetAttrPos` locations for the attributes let cases = [ (2, 3, Right("foo = 1;")), (3, 3, Right(r#""bar" = 2;"#)), From c0df13bcda29ebad3a63ac3a89a22123bf039baa Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 15:04:45 -0700 Subject: [PATCH 06/10] ratchet.rs: line wrap to 100 characters --- src/ratchet.rs | 58 +++++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/src/ratchet.rs b/src/ratchet.rs index e7cb57e..4e6ce04 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -59,27 +59,28 @@ impl Package { /// The ratchet state of a generic ratchet check. pub enum RatchetState { - /// The ratchet is loose, it can be tightened more. - /// In other words, this is the legacy state we're trying to move away from. - /// Introducing new instances is not allowed but previous instances will continue to be allowed. - /// The `Context` is context for error messages in case a new instance of this state is - /// introduced + /// The ratchet is loose. It can be tightened more. In other words, this is the legacy state + /// we're trying to move away from. + /// + /// Introducing new instances is not allowed but previous instances will continue to be + /// allowed. The `Context` is context for error messages in case a new instance of this state + /// is introduced. Loose(Ratchet::ToContext), - /// The ratchet is tight, it can't be tightened any further. - /// This is either because we already use the latest state, or because the ratchet isn't - /// relevant. + + /// The ratchet is tight. It can't be tightened any further. This is either because we already + /// use the latest state, or because the ratchet isn't relevant. Tight, - /// This ratchet can't be applied. - /// State transitions from/to NonApplicable are always allowed + + /// This ratchet can't be applied. State transitions from/to NonApplicable are always allowed. NonApplicable, } -/// A trait that can convert an attribute-specific error context into a NixpkgsProblem +/// A trait that can convert an attribute-specific error context into a NixpkgsProblem. pub trait ToNixpkgsProblem { - /// Context relating to the Nixpkgs that is being transitioned _to_ + /// Context relating to the Nixpkgs that is being transitioned _to_. type ToContext; - /// How to convert an attribute-specific error context into a NixpkgsProblem + /// How to convert an attribute-specific error context into a NixpkgsProblem. fn to_nixpkgs_problem( name: &str, optional_from: Option<()>, @@ -92,12 +93,12 @@ impl RatchetState { /// The previous state may be `None` in case the attribute is new. fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> { match (optional_from, to) { - // Loosening a ratchet is now allowed + // Loosening a ratchet is not allowed. (Some(RatchetState::Tight), RatchetState::Loose(loose_context)) => { Context::to_nixpkgs_problem(name, Some(()), loose_context).into() } - // Introducing a loose ratchet is also not allowed + // Introducing a loose ratchet is also not allowed. (None, RatchetState::Loose(loose_context)) => { Context::to_nixpkgs_problem(name, None, loose_context).into() } @@ -111,19 +112,22 @@ impl RatchetState { } } -/// The ratchet to check whether a top-level attribute has/needs -/// a manual definition, e.g. in all-packages.nix. +/// The ratchet to check whether a top-level attribute has/needs a manual definition, e.g. in +/// `pkgs/top-level/all-packages.nix`. /// /// This ratchet is only tight for attributes that: -/// - Are not defined in `pkgs/by-name`, and rely on a manual definition -/// - Are defined in `pkgs/by-name` without any manual definition, -/// (no custom argument overrides) +/// +/// - Are not defined in `pkgs/by-name`, and rely on a manual definition. +/// +/// - Are defined in `pkgs/by-name` without any manual definition (no custom argument overrides). +/// /// - Are defined with `pkgs/by-name` with a manual definition that can't be removed -/// because it provides custom argument overrides +/// 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 custom argument overrides +/// +/// - Are defined in `pkgs/by-name` with a manual definition that doesn't have any +/// custom argument overrides. pub enum ManualDefinition {} impl ToNixpkgsProblem for ManualDefinition { @@ -138,11 +142,11 @@ impl ToNixpkgsProblem 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 `pkgs/by-name`. /// -/// 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 to all-packages.nix +/// 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 +/// to `pkgs/top-level/all-packages.nix`. pub enum UsesByName {} impl ToNixpkgsProblem for UsesByName { From 4c5c6e0d359435fcee79f15885b0098b35e92a33 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 15:06:37 -0700 Subject: [PATCH 07/10] references.rs: line wrap to 100 characters --- src/references.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/references.rs b/src/references.rs index 9a50d19..2016561 100644 --- a/src/references.rs +++ b/src/references.rs @@ -20,8 +20,8 @@ pub fn check_references( ) -> validation::Result<()> { // The first subpath to check is the package directory itself, which we can represent as an // empty path, since the absolute package directory gets prepended to this. - // We don't use `./.` to keep the error messages cleaner - // (there's no canonicalisation going on underneath) + // We don't use `./.` to keep the error messages cleaner, since there's no canonicalisation + // going on underneath. let subpath = RelativePath::new(""); check_path( nix_file_store, @@ -37,7 +37,7 @@ pub fn check_references( }) } -/// Checks for a specific path to not have references outside +/// Checks for a specific path to not have references outside. /// /// The subpath is the relative path within the package directory we're currently checking. /// A relative path so that the error messages don't get absolute paths (which are messy in CI). @@ -59,11 +59,11 @@ fn check_path( }; Ok(if path.is_symlink() { - // Check whether the symlink resolves to outside the package directory + // Check whether the symlink resolves to outside the package directory. match path.canonicalize() { Ok(target) => { - // No need to handle the case of it being inside the directory, since we scan through the - // entire directory recursively anyways + // No need to handle the case of it being inside the directory, since we scan + // through the entire directory recursively in any case. if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) { to_validation(PathErrorKind::OutsideSymlink) } else { @@ -114,8 +114,8 @@ fn check_path( }) } -/// Check whether a nix file contains path expression references pointing outside the package -/// directory +/// Check whether a Nix file contains path expression references pointing outside the package +/// directory. fn check_nix_file( nix_file_store: &mut NixFileStore, relative_package_dir: &RelativePath, @@ -159,8 +159,8 @@ fn check_nix_file( }) } ResolvedPath::Within(..) => { - // No need to handle the case of it being inside the directory, since we scan through the - // entire directory recursively anyways + // No need to handle the case of it being inside the directory, since we scan + // through the entire directory recursively in any case. Success(()) } } From dab080ad19c20cf2a7d7c76958f24a6f07c6d2f6 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 15:08:01 -0700 Subject: [PATCH 08/10] structure.rs: line wrap to 100 characters --- src/structure.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/structure.rs b/src/structure.rs index fea8a85..d70e0fa 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -52,7 +52,6 @@ pub fn check_structure( Ok(if shard_name == "README.md" { // README.md is allowed to be a file and not checked - Success(vec![]) } else if !shard_path.is_dir() { NixpkgsProblem::Shard(ShardError { @@ -60,7 +59,8 @@ pub fn check_structure( kind: ShardErrorKind::ShardNonDir, }) .into() - // we can't check for any other errors if it's a file, since there's no subdirectories to check + // We can't check for any other errors if it's a file, since there's no + // subdirectories to check. } else { let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); let result = if !shard_name_valid { @@ -112,7 +112,7 @@ pub fn check_structure( }) .collect_vec()?; - // Combine the package names conatained within each shard into a longer list + // Combine the package names contained within each shard into a longer list. Ok(validation::sequence(shard_results).map(concat)) } @@ -153,8 +153,8 @@ 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 { - // Only show this error if we have a valid shard and package name - // Because if one of those is wrong, you should fix that first + // 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 { to_validation(PackageErrorKind::IncorrectShard { correct_relative_package_dir: correct_relative_package_dir.clone(), From f1cf585fb992b8827177ca77a91ce197c9a0e2ac Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 15:09:57 -0700 Subject: [PATCH 09/10] utils.rs: line wrap to 100 characters --- src/utils.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/utils.rs b/src/utils.rs index 9a5d127..8645cc9 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -6,22 +6,25 @@ use std::path::Path; pub const BASE_SUBPATH: &str = "pkgs/by-name"; pub const PACKAGE_NIX_FILENAME: &str = "package.nix"; -/// Deterministic file listing so that tests are reproducible +/// Deterministic file listing so that tests are reproducible. pub fn read_dir_sorted(base_dir: &Path) -> anyhow::Result> { let listing = base_dir .read_dir() .with_context(|| format!("Could not list directory {}", base_dir.display()))?; + let mut shard_entries = listing .collect::>>() .with_context(|| format!("Could not list directory {}", base_dir.display()))?; + shard_entries.sort_by_key(|entry| entry.file_name()); + Ok(shard_entries) } /// A simple utility for calculating the line for a string offset. -/// This doesn't do any Unicode handling, though that probably doesn't matter -/// because newlines can't split up Unicode characters. Also this is only used -/// for error reporting +/// +/// This doesn't do any Unicode handling, though that probably doesn't matter because newlines +/// can't split up Unicode characters. This is only used for error reporting. pub struct LineIndex { /// Stores the indices of newlines newlines: Vec, @@ -41,7 +44,7 @@ impl LineIndex { } /// Returns the line number for a string index. - /// If the index points to a newline, returns the line number before the newline + /// If the index points to a newline, returns the line number before the newline. pub fn line(&self, index: usize) -> usize { match self.newlines.binary_search(&index) { // +1 because lines are 1-indexed @@ -60,7 +63,7 @@ impl LineIndex { // For the nth line, we add the index of the (n-1)st newline to the column, // and remove one more from the index since arrays are 0-indexed. // Then add the 1-indexed column to get not the newline index itself, - // but rather the index of the position on the next line + // but rather the index of the position on the next line. self.newlines[line - 2] + column } } From 61245dbba206a2ae32da3fe4023b6aefed90b6f5 Mon Sep 17 00:00:00 2001 From: Philip Taron Date: Mon, 8 Jul 2024 15:12:05 -0700 Subject: [PATCH 10/10] validation.rs: line wrap to 100 characters --- src/validation.rs | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/validation.rs b/src/validation.rs index b14bfb9..ea1fa65 100644 --- a/src/validation.rs +++ b/src/validation.rs @@ -6,12 +6,11 @@ use itertools::{ }; use Validation::*; -/// The validation result of a check. -/// Instead of exiting at the first failure, -/// this type can accumulate multiple failures. -/// This can be achieved using the functions `and`, `sequence` and `sequence_` +/// The validation result of a check. Instead of exiting at the first failure, this type can +/// accumulate multiple failures. This can be achieved using the functions `and`, `sequence` and +/// `sequence_`. /// -/// This leans on https://hackage.haskell.org/package/validation +/// This leans on . pub enum Validation { Failure(Vec), Success(A), @@ -25,14 +24,18 @@ impl From for Validation { } /// A type alias representing the result of a check, either: +/// /// - Err(anyhow::Error): A fatal failure, typically I/O errors. /// Such failures are not caused by the files in Nixpkgs. /// This hints at a bug in the code or a problem with the deployment. +/// /// - Ok(Failure(Vec)): A non-fatal validation problem with the Nixpkgs files. /// Further checks can be run even with this result type. /// Such problems can be fixed by changing the Nixpkgs files. +/// /// - Ok(Success(A)): A successful (potentially intermediate) result with an arbitrary value. /// No fatal errors have occurred and no validation problems have been found with Nixpkgs. +/// pub type Result = anyhow::Result>; pub trait ResultIteratorExt: Sized + Iterator> { @@ -43,15 +46,15 @@ impl ResultIteratorExt for I where I: Sized + Iterator>, { - /// A convenience version of `collect` specialised to a vector + /// A convenience version of `collect` specialised to a vector. fn collect_vec(self) -> std::result::Result, E> { self.collect() } } impl Validation { - /// Map a `Validation` to a `Validation` by applying a function to the - /// potentially contained value in case of success. + /// Map a `Validation` to a `Validation` by applying a function to the potentially + /// contained value in case of success. pub fn map(self, f: impl FnOnce(A) -> B) -> Validation { match self { Failure(err) => Failure(err), @@ -59,8 +62,8 @@ impl Validation { } } - /// Map a `Validation` to a `Result` by applying a function `A -> Result` - /// only if there is a `Success` value + /// Map a `Validation` to a `Result` by applying a function `A -> Result` only if + /// there is a `Success` value. pub fn result_map(self, f: impl FnOnce(A) -> Result) -> Result { match self { Failure(err) => Ok(Failure(err)), @@ -70,8 +73,8 @@ impl Validation { } impl Validation<()> { - /// Combine two validations, both of which need to be successful for the return value to be successful. - /// The `NixpkgsProblem`s of both sides are returned concatenated. + /// Combine two validations, both of which need to be successful for the return value to be + /// successful. The `NixpkgsProblem`s of both sides are returned concatenated. pub fn and(self, other: Validation) -> Validation { match (self, other) { (Success(_), Success(right_value)) => Success(right_value), @@ -83,9 +86,12 @@ impl Validation<()> { } /// Combine many validations into a single one. -/// All given validations need to be successful in order for the returned validation to be successful, -/// in which case the returned validation value contains a `Vec` of each individual value. -/// Otherwise the `NixpkgsProblem`s of all validations are returned concatenated. +/// +/// All given validations need to be successful in order for the returned validation to be +/// successful, in which case the returned validation value contains a `Vec` of each individual +/// value. +/// +/// Otherwise, the `NixpkgsProblem`s of all validations are returned concatenated. pub fn sequence(check_results: impl IntoIterator>) -> Validation> { let (errors, values): (Vec>, Vec) = check_results .into_iter()