diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 1322f0faaf91..122c4b734897 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -51,6 +51,26 @@ struct Location { pub column: usize, } +impl Location { + // Creates a [String] from a [Location], in a format like `{file}:{line}:{column}`, + // where `file` is relative to the given [Path]. + fn to_relative_string(&self, nixpkgs_path: &Path) -> anyhow::Result { + let relative = self.file.strip_prefix(nixpkgs_path).with_context(|| { + format!( + "An attributes location ({}) is outside Nixpkgs ({})", + self.file.display(), + nixpkgs_path.display() + ) + })?; + Ok(format!( + "{}:{}:{}", + relative.display(), + self.line, + self.column + )) + } +} + #[derive(Deserialize)] pub enum AttributeVariant { /// The attribute is not an attribute set, we're limited in the amount of information we can get @@ -289,37 +309,47 @@ fn by_name( // 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) + // Something like ` = foo` + (_, Err(definition)) => NixpkgsProblem::NonSyntacticCallPackage { + package_name: attribute_name.to_owned(), + location: location.to_relative_string(nixpkgs_path)?, + definition, + } + .into(), // Something like ` = pythonPackages.callPackage ...` - | (false, Some(_)) - // Something like ` = bar` where `bar = pkgs.callPackage ...` - | (true, None) => { + (false, Ok(_)) => { // All of these are not of the expected form, so error out // TODO: Make error more specific, don't lump everything together NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.to_owned(), - package_name: attribute_name.to_owned(), - }.into() + relative_package_file: relative_package_file.to_owned(), + package_name: attribute_name.to_owned(), + } + .into() } // Something like ` = pkgs.callPackage ...` - (true, Some(syntactic_call_package)) => { + (true, Ok(syntactic_call_package)) => { if let Some(path) = syntactic_call_package.relative_path { if path == relative_package_file { // Manual definitions with empty arguments are not allowed // anymore - Success(if syntactic_call_package.empty_arg { Loose(()) } else { Tight }) + Success(if syntactic_call_package.empty_arg { + Loose(()) + } else { + Tight + }) } else { NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.to_owned(), - package_name: attribute_name.to_owned(), - }.into() + relative_package_file: relative_package_file.to_owned(), + package_name: attribute_name.to_owned(), + } + .into() } } else { NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.to_owned(), - package_name: attribute_name.to_owned(), - }.into() + relative_package_file: relative_package_file.to_owned(), + package_name: attribute_name.to_owned(), + } + .into() } } } @@ -423,16 +453,16 @@ fn handle_non_by_name_attribute( // `callPackage` match (is_semantic_call_package, optional_syntactic_call_package) { // Something like ` = { }` - (false, None) + (false, Err(_)) // Something like ` = pythonPackages.callPackage ...` - | (false, Some(_)) + | (false, Ok(_)) // Something like ` = bar` where `bar = pkgs.callPackage ...` - | (true, None) => { + | (true, Err(_)) => { // 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)) => { + (true, Ok(syntactic_call_package)) => { // It's only possible to migrate such a definitions if.. match syntactic_call_package.relative_path { Some(ref rel_path) if rel_path.starts_with(utils::BASE_SUBPATH) => { diff --git a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs index ea0833792f3c..3840f1dc479d 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs @@ -86,8 +86,9 @@ pub struct CallPackageArgumentInfo { impl NixFile { /// Returns information about callPackage arguments for an attribute at a specific line/column /// index. - /// If the location is not of the form ` = callPackage ;`, `None` is - /// returned. + /// If the location is not of the form ` = callPackage ;`, `Ok(Err(String))` is + /// returned, with String being how the definition looks like. + /// /// This function only returns `Err` for problems that can't be caused by the Nix contents, /// but rather problems in this programs code itself. /// @@ -108,7 +109,7 @@ impl NixFile { /// /// You'll get back /// ```rust - /// Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }) + /// Ok(Ok(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true })) /// ``` /// /// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an @@ -118,11 +119,15 @@ impl NixFile { line: usize, column: usize, relative_to: &Path, - ) -> anyhow::Result> { - let Some(attrpath_value) = self.attrpath_value_at(line, column)? else { - return Ok(None); - }; - self.attrpath_value_call_package_argument_info(attrpath_value, relative_to) + ) -> anyhow::Result> { + Ok(match self.attrpath_value_at(line, column)? { + Err(definition) => Err(definition), + Ok(attrpath_value) => { + let definition = attrpath_value.to_string(); + self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)? + .ok_or(definition) + } + }) } // Internal function mainly to make it independently testable @@ -130,7 +135,7 @@ impl NixFile { &self, line: usize, column: usize, - ) -> anyhow::Result> { + ) -> anyhow::Result> { let index = self.line_index.fromlinecolumn(line, column); let token_at_offset = self @@ -164,7 +169,7 @@ impl NixFile { // This is the only other way how `builtins.unsafeGetAttrPos` can return // attribute positions, but we only look for ones like ` = `, so // ignore this - return Ok(None); + return Ok(Err(node.to_string())); } else { // However, anything else is not expected and smells like a bug anyhow::bail!( @@ -208,7 +213,7 @@ impl NixFile { // 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(Some(ast::AttrpathValue::cast(attrpath_value_node).unwrap())) + Ok(Ok(ast::AttrpathValue::cast(attrpath_value_node).unwrap())) } // Internal function mainly to make attrpath_value_at independently testable @@ -437,14 +442,14 @@ mod tests { // These are builtins.unsafeGetAttrPos locations for the attributes let cases = [ - (2, 3, Some("foo = 1;")), - (3, 3, Some(r#""bar" = 2;"#)), - (4, 3, Some(r#"${"baz"} = 3;"#)), - (5, 3, Some(r#""${"qux"}" = 4;"#)), - (8, 3, Some("quux\n # B\n =\n # C\n 5\n # D\n ;")), - (17, 7, Some("quuux/**/=/**/5/**/;")), - (19, 10, None), - (20, 22, None), + (2, 3, Ok("foo = 1;")), + (3, 3, Ok(r#""bar" = 2;"#)), + (4, 3, Ok(r#"${"baz"} = 3;"#)), + (5, 3, Ok(r#""${"qux"}" = 4;"#)), + (8, 3, Ok("quux\n # B\n =\n # C\n 5\n # D\n ;")), + (17, 7, Ok("quuux/**/=/**/5/**/;")), + (19, 10, Err("inherit toInherit;")), + (20, 22, Err("inherit (toInherit) toInherit;")), ]; for (line, column, expected_result) in cases { @@ -452,7 +457,13 @@ mod tests { .attrpath_value_at(line, column) .context(format!("line {line}, column {column}"))? .map(|node| node.to_string()); - assert_eq!(actual_result.as_deref(), expected_result); + let owned_expected_result = expected_result + .map(|x| x.to_string()) + .map_err(|x| x.to_string()); + assert_eq!( + actual_result, owned_expected_result, + "line {line}, column {column}" + ); } Ok(()) @@ -481,41 +492,41 @@ mod tests { let nix_file = NixFile::new(&file)?; let cases = [ - (2, None), - (3, None), - (4, None), - (5, None), + (2, Err("a.sub = null;")), + (3, Err("b = null;")), + (4, Err("c = import ./file.nix;")), + (5, Err("d = import ./file.nix { };")), ( 6, - Some(CallPackageArgumentInfo { + Ok(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true, }), ), ( 7, - Some(CallPackageArgumentInfo { + Ok(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true, }), ), ( 8, - Some(CallPackageArgumentInfo { + Ok(CallPackageArgumentInfo { relative_path: None, empty_arg: true, }), ), ( 9, - Some(CallPackageArgumentInfo { + Ok(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: false, }), ), ( 10, - Some(CallPackageArgumentInfo { + Ok(CallPackageArgumentInfo { relative_path: None, empty_arg: false, }), @@ -525,8 +536,10 @@ mod tests { for (line, expected_result) in cases { let actual_result = nix_file .call_package_argument_info_at(line, 3, temp_dir.path()) - .context(format!("line {line}"))?; - assert_eq!(actual_result, expected_result); + .context(format!("line {line}"))? + .map_err(|x| x); + let owned_expected_result = expected_result.map_err(|x| x.to_string()); + assert_eq!(actual_result, owned_expected_result, "line {line}"); } Ok(()) diff --git a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs index e13869adaa41..18001eb44ecd 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs @@ -44,6 +44,11 @@ pub enum NixpkgsProblem { relative_package_file: PathBuf, package_name: String, }, + NonSyntacticCallPackage { + package_name: String, + location: String, + definition: String, + }, NonDerivation { relative_package_file: PathBuf, package_name: String, @@ -164,6 +169,14 @@ impl fmt::Display for NixpkgsProblem { "pkgs.{package_name}: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage {} {{ ... }}` with a non-empty second argument.", relative_package_file.display() ), + NixpkgsProblem::NonSyntacticCallPackage { package_name, location, definition } => + write!( + f, + "Because {} exists, the attribute `pkgs.{package_name}` must be defined as `callPackage {} {{ ... }}`. This is however not the case: The attribute is defined in {location} as\n\t{}", + structure::relative_dir_for_package(package_name).display(), + structure::relative_file_for_package(package_name).display(), + definition, + ), NixpkgsProblem::NonDerivation { relative_package_file, package_name } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected index 9df788191ece..f590ef4ff9fa 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected +++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected @@ -1 +1,2 @@ -pkgs.foo: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/fo/foo/package.nix { ... }` with a non-empty second argument. +Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined as `callPackage pkgs/by-name/fo/foo/package.nix { ... }`. This is however not the case: The attribute is defined in all-packages.nix:4:3 as + foo = self.bar; diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected index 51479e22d26f..25fc867b04fe 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected @@ -1 +1,2 @@ -pkgs.nonDerivation: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/no/nonDerivation/package.nix { ... }` with a non-empty second argument. +Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined as `callPackage pkgs/by-name/no/nonDerivation/package.nix { ... }`. This is however not the case: The attribute is defined in all-packages.nix:2:3 as + nonDerivation = self.someDrv;