BoomMicrophone/nixpkgs-suyu
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

tests.nixpkgs-check-by-name: Better error for non-syntactic callPackage

Browse source
This commit is contained in:
Silvan Mosberger 2024-02-19 23:35:18 +01:00
parent a61c8c9f5c
commit 24069b982d
5 changed files with 111 additions and 53 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

58
pkgs/test/nixpkgs-check-by-name/src/eval.rs
View file

@ -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<String> {
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 `<attr> = { ... }`
(false, None)
// Something like `<attr> = foo`
(_, Err(definition)) => NixpkgsProblem::NonSyntacticCallPackage {
package_name: attribute_name.to_owned(),
location: location.to_relative_string(nixpkgs_path)?,
definition,
}
.into(),
// Something like `<attr> = pythonPackages.callPackage ...`
| (false, Some(_))
// Something like `<attr> = 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()
}
.into()
}
// Something like `<attr> = 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()
}
.into()
}
} else {
NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}.into()
}
.into()
}
}
}
@ -423,16 +453,16 @@ fn handle_non_by_name_attribute(
// `callPackage`
match (is_semantic_call_package, optional_syntactic_call_package) {
// Something like `<attr> = { }`
(false, None)
(false, Err(_))
// Something like `<attr> = pythonPackages.callPackage ...`
| (false, Some(_))
| (false, Ok(_))
// Something like `<attr> = 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 `<attr> = 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) => {

75
pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
View file

@ -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 `<attr> = callPackage <arg1> <arg2>;`, `None` is
/// returned.
/// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `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<Option<CallPackageArgumentInfo>> {
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<Result<CallPackageArgumentInfo, String>> {
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<Option<ast::AttrpathValue>> {
) -> anyhow::Result<Result<ast::AttrpathValue, String>> {
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 `<attr-path> = <value>`, 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(())

13
pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
View file

@ -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,

3
pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
View file

@ -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;

3
pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
View file

@ -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;