From f394f738faaee2a749eae7c507dccb1830dd440a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 12 Oct 2023 00:38:43 +0200 Subject: tests.nixpkgs-check-by-name: Improve an error message --- pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected') diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected index 1c6377d8aef..36fa40d18ea 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected @@ -1 +1 @@ -pkgs.nonDerivation: This attribute is not defined as `pkgs.callPackage pkgs/by-name/no/nonDerivation/package.nix { ... }`. +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 { ... }`. -- cgit 1.4.1 From d3bf6133e826a6901c50d8524cb3fbbce038df28 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 12 Oct 2023 01:20:56 +0200 Subject: tests.nixpkgs-check-by-name: Disallow empty all-packages.nix overrides Only enabled with `--version v1` --- pkgs/test/nixpkgs-check-by-name/README.md | 2 ++ pkgs/test/nixpkgs-check-by-name/src/eval.nix | 2 ++ pkgs/test/nixpkgs-check-by-name/src/eval.rs | 28 ++++++++++++++++------ pkgs/test/nixpkgs-check-by-name/src/main.rs | 8 ++++--- .../tests/override-different-file/expected | 2 +- .../tests/override-empty-arg/all-packages.nix | 3 +++ .../tests/override-empty-arg/default.nix | 1 + .../tests/override-empty-arg/expected | 1 + .../pkgs/by-name/no/nonDerivation/package.nix | 1 + .../tests/override-no-call-package/expected | 2 +- .../tests/override-no-file/expected | 2 +- 11 files changed, 39 insertions(+), 13 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/all-packages.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/pkgs/by-name/no/nonDerivation/package.nix (limited to 'pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected') diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index ebd1cd40aba..2d2db9a58bc 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/pkgs/test/nixpkgs-check-by-name/README.md @@ -15,6 +15,7 @@ This API may be changed over time if the CI workflow making use of it is adjuste Possible values: - `v0` (default) + - `v1` See [validation](#validity-checks) for the differences. - Exit code: @@ -41,6 +42,7 @@ These checks are performed by this tool: ### Nix evaluation checks - `pkgs.${name}` is defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`. + - **Only after --version v1**: If `pkgs.${name}` is not auto-called from `pkgs/by-name`, `args` must not be empty - `pkgs.lib.isDerivation pkgs.${name}` is `true`. ## Development diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix index 2fa0c5a9709..bf9f19d8e46 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix @@ -25,6 +25,8 @@ let toString fn else null; + CallPackage.empty_arg = + args == { }; }; in if builtins.isAttrs result then diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index b1c1a44f8c0..26836501c97 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,5 +1,6 @@ use crate::structure; use crate::utils::ErrorWriter; +use crate::Version; use std::path::Path; use anyhow::Context; @@ -22,10 +23,14 @@ enum AttributeVariant { /// The attribute is auto-called as pkgs.callPackage using pkgs/by-name, /// and it is not overridden by a definition in all-packages.nix AutoCalled, - /// The attribute is defined as a pkgs.callPackage , + /// The attribute is defined as a pkgs.callPackage , /// and overridden by all-packages.nix - /// The path is None when the argument isn't a path - CallPackage { path: Option }, + CallPackage { + /// The argument or None if it's not a path + path: Option, + /// true if is { } + empty_arg: bool, + }, /// The attribute is not defined as pkgs.callPackage Other, } @@ -36,6 +41,7 @@ const EXPR: &str = include_str!("eval.nix"); /// of the form `callPackage { ... }`. /// See the `eval.nix` file for how this is achieved on the Nix side pub fn check_values( + version: Version, error_writer: &mut ErrorWriter, nixpkgs: &structure::Nixpkgs, eval_accessible_paths: Vec<&Path>, @@ -112,19 +118,27 @@ pub fn check_values( if let Some(attribute_info) = actual_files.get(package_name) { let valid = match &attribute_info.variant { AttributeVariant::AutoCalled => true, - AttributeVariant::CallPackage { path } => { - if let Some(call_package_path) = path { + AttributeVariant::CallPackage { path, empty_arg } => { + let correct_file = if let Some(call_package_path) = path { absolute_package_file == *call_package_path } else { false - } + }; + // Only check for the argument to be non-empty if the version is V1 or + // higher + let non_empty = if version >= Version::V1 { + !empty_arg + } else { + true + }; + correct_file && non_empty } AttributeVariant::Other => false, }; if !valid { error_writer.write(&format!( - "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 {} {{ ... }}`.", + "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() ))?; continue; diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 5c51f32b5ce..5d28077ae4a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -30,6 +30,8 @@ pub struct Args { pub enum Version { /// Initial version V0, + /// Empty argument check + V1, } fn main() -> ExitCode { @@ -65,7 +67,7 @@ fn main() -> ExitCode { /// - `Ok(true)` if the structure is valid, nothing will have been written to `error_writer`. pub fn check_nixpkgs( nixpkgs_path: &Path, - _version: Version, + version: Version, eval_accessible_paths: Vec<&Path>, error_writer: &mut W, ) -> anyhow::Result { @@ -88,7 +90,7 @@ pub fn check_nixpkgs( if error_writer.empty { // Only if we could successfully parse the structure, we do the semantic checks - eval::check_values(&mut error_writer, &nixpkgs, eval_accessible_paths)?; + eval::check_values(version, &mut error_writer, &nixpkgs, eval_accessible_paths)?; references::check_references(&mut error_writer, &nixpkgs)?; } } @@ -188,7 +190,7 @@ mod tests { // We don't want coloring to mess up the tests let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> { let mut writer = vec![]; - check_nixpkgs(&path, Version::V0, vec![&extra_nix_path], &mut writer) + check_nixpkgs(&path, Version::V1, vec![&extra_nix_path], &mut writer) .context(format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected index 36fa40d18ea..51479e22d26 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected @@ -1 +1 @@ -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 { ... }`. +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. diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/all-packages.nix new file mode 100644 index 00000000000..d369dd7228d --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/all-packages.nix @@ -0,0 +1,3 @@ +self: super: { + nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { }; +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/default.nix new file mode 100644 index 00000000000..af25d145012 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/default.nix @@ -0,0 +1 @@ +import ../mock-nixpkgs.nix { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected new file mode 100644 index 00000000000..51479e22d26 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected @@ -0,0 +1 @@ +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. diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/pkgs/by-name/no/nonDerivation/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/pkgs/by-name/no/nonDerivation/package.nix new file mode 100644 index 00000000000..a1b92efbbad --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/pkgs/by-name/no/nonDerivation/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv 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 36fa40d18ea..51479e22d26 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 @@ -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 { ... }`. +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. diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected index 36fa40d18ea..51479e22d26 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected @@ -1 +1 @@ -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 { ... }`. +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. -- cgit 1.4.1