Implement check to disallow pkgs/by-name package definitions in all-packages.nix with an empty argument

pkgs/test/nixpkgs-check-by-name/README.md

@@ -11,6 +11,13 @@ This API may be changed over time if the CI workflow making use of it is adjuste
 - Command line: `nixpkgs-check-by-name <NIXPKGS>`
 - Arguments:
   - `<NIXPKGS>`: The path to the Nixpkgs to check
+  - `--version <VERSION>`: The version of the checks to perform.
+
+    Possible values:
+    - `v0` (default)
+    - `v1`
+
+    See [validation](#validity-checks) for the differences.
 - Exit code:
   - `0`: If the [validation](#validity-checks) is successful
   - `1`: If the [validation](#validity-checks) is not successful
@@ -35,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

pkgs/test/nixpkgs-check-by-name/src/eval.nix

@@ -18,19 +18,42 @@ let
     callPackage = fn: args:
       let
         result = super.callPackage fn args;
+        variantInfo._attributeVariant = {
+          # These names are used by the deserializer on the Rust side
+          CallPackage.path =
+            if builtins.isPath fn then
+              toString fn
+            else
+              null;
+          CallPackage.empty_arg =
+            args == { };
+        };
       in
       if builtins.isAttrs result then
         # If this was the last overlay to be applied, we could just only return the `_callPackagePath`,
         # but that's not the case because stdenv has another overlays on top of user-provided ones.
         # So to not break the stdenv build we need to return the mostly proper result here
-        result // {
-          _callPackagePath = fn;
-        }
+        result // variantInfo
       else
         # It's very rare that callPackage doesn't return an attribute set, but it can occur.
-        {
-          _callPackagePath = fn;
+        variantInfo;
+
+    _internalCallByNamePackageFile = file:
+      let
+        result = super._internalCallByNamePackageFile file;
+        variantInfo._attributeVariant = {
+          # This name is used by the deserializer on the Rust side
+          AutoCalled = null;
         };
+      in
+      if builtins.isAttrs result then
+        # If this was the last overlay to be applied, we could just only return the `_callPackagePath`,
+        # but that's not the case because stdenv has another overlays on top of user-provided ones.
+        # So to not break the stdenv build we need to return the mostly proper result here
+        result // variantInfo
+      else
+        # It's very rare that callPackage doesn't return an attribute set, but it can occur.
+        variantInfo;
   };
 
   pkgs = import nixpkgsPath {
@@ -39,14 +62,14 @@ let
     overlays = [ callPackageOverlay ];
   };
 
-  attrInfo = attr: {
+  attrInfo = attr:
+    let
+      value = pkgs.${attr};
+    in
+    {
     # These names are used by the deserializer on the Rust side
-    call_package_path =
-      if pkgs.${attr} ? _callPackagePath && builtins.isPath pkgs.${attr}._callPackagePath then
-        toString pkgs.${attr}._callPackagePath
-      else
-        null;
-    is_derivation = pkgs.lib.isDerivation pkgs.${attr};
+    variant = value._attributeVariant or { Other = null; };
+    is_derivation = pkgs.lib.isDerivation value;
   };
 
   attrInfos = builtins.listToAttrs (map (name: {

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;
@@ -13,16 +14,34 @@ use tempfile::NamedTempFile;
 /// Attribute set of this structure is returned by eval.nix
 #[derive(Deserialize)]
 struct AttributeInfo {
-    call_package_path: Option<PathBuf>,
+    variant: AttributeVariant,
     is_derivation: bool,
 }
 
+#[derive(Deserialize)]
+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 <path> <args>,
+    /// and overridden by all-packages.nix
+    CallPackage {
+        /// The <path> argument or None if it's not a path
+        path: Option<PathBuf>,
+        /// true if <args> is { }
+        empty_arg: bool,
+    },
+    /// The attribute is not defined as pkgs.callPackage
+    Other,
+}
+
 const EXPR: &str = include_str!("eval.nix");
 
 /// Check that the Nixpkgs attribute values corresponding to the packages in pkgs/by-name are
 /// of the form `callPackage <package_file> { ... }`.
 /// See the `eval.nix` file for how this is achieved on the Nix side
 pub fn check_values<W: io::Write>(
+    version: Version,
     error_writer: &mut ErrorWriter<W>,
     nixpkgs: &structure::Nixpkgs,
     eval_accessible_paths: Vec<&Path>,
@@ -97,16 +116,29 @@ pub fn check_values<W: io::Write>(
         let absolute_package_file = nixpkgs.path.join(&relative_package_file);
 
         if let Some(attribute_info) = actual_files.get(package_name) {
-            let is_expected_file =
-                if let Some(call_package_path) = &attribute_info.call_package_path {
-                    absolute_package_file == *call_package_path
-                } else {
-                    false
-                };
+            let valid = match &attribute_info.variant {
+                AttributeVariant::AutoCalled => true,
+                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 !is_expected_file {
+            if !valid {
                 error_writer.write(&format!(
-                    "pkgs.{package_name}: This attribute is not defined as `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;

pkgs/test/nixpkgs-check-by-name/src/main.rs

@@ -4,7 +4,7 @@ mod structure;
 mod utils;
 
 use anyhow::Context;
-use clap::Parser;
+use clap::{Parser, ValueEnum};
 use colored::Colorize;
 use std::io;
 use std::path::{Path, PathBuf};
@@ -15,14 +15,28 @@ use utils::ErrorWriter;
 /// Program to check the validity of pkgs/by-name
 #[derive(Parser, Debug)]
 #[command(about)]
-struct Args {
+pub struct Args {
     /// Path to nixpkgs
     nixpkgs: PathBuf,
+    /// The version of the checks
+    /// Increasing this may cause failures for a Nixpkgs that succeeded before
+    /// TODO: Remove default once Nixpkgs CI passes this argument
+    #[arg(long, value_enum, default_value_t = Version::V0)]
+    version: Version,
+}
+
+/// The version of the checks
+#[derive(Debug, Clone, PartialEq, PartialOrd, ValueEnum)]
+pub enum Version {
+    /// Initial version
+    V0,
+    /// Empty argument check
+    V1,
 }
 
 fn main() -> ExitCode {
     let args = Args::parse();
-    match check_nixpkgs(&args.nixpkgs, vec![], &mut io::stderr()) {
+    match check_nixpkgs(&args.nixpkgs, args.version, vec![], &mut io::stderr()) {
         Ok(true) => {
             eprintln!("{}", "Validated successfully".green());
             ExitCode::SUCCESS
@@ -53,6 +67,7 @@ fn main() -> ExitCode {
 /// - `Ok(true)` if the structure is valid, nothing will have been written to `error_writer`.
 pub fn check_nixpkgs<W: io::Write>(
     nixpkgs_path: &Path,
+    version: Version,
     eval_accessible_paths: Vec<&Path>,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
@@ -75,7 +90,7 @@ pub fn check_nixpkgs<W: io::Write>(
 
         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)?;
         }
     }
@@ -86,6 +101,7 @@ pub fn check_nixpkgs<W: io::Write>(
 mod tests {
     use crate::check_nixpkgs;
     use crate::structure;
+    use crate::Version;
     use anyhow::Context;
     use std::fs;
     use std::path::Path;
@@ -174,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, 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)
         })?;

pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix

@@ -75,9 +75,14 @@ let
 
   # Turns autoCalledPackageFiles into an overlay that `callPackage`'s all of them
   autoCalledPackages = self: super:
-    builtins.mapAttrs (name: file:
-      self.callPackage file { }
-    ) autoCalledPackageFiles;
+    {
+      # Needed to be able to detect empty arguments in all-packages.nix
+      # See a more detailed description in pkgs/top-level/by-name-overlay.nix
+      _internalCallByNamePackageFile = file: self.callPackage file { };
+    }
+    // builtins.mapAttrs
+      (name: self._internalCallByNamePackageFile)
+      autoCalledPackageFiles;
 
   # A list optionally containing the `all-packages.nix` file from the test case as an overlay
   optionalAllPackagesOverlay =

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 { ... }` with a non-empty second argument.

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 { };
+}

pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/default.nix

@@ -0,0 +1 @@
+import ../mock-nixpkgs.nix { root = ./.; }

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.

pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/pkgs/by-name/no/nonDerivation/package.nix

@@ -0,0 +1 @@
+{ someDrv }: someDrv

pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/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 { ... }` with a non-empty second argument.

pkgs/test/nixpkgs-check-by-name/tests/override-no-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 { ... }` with a non-empty second argument.

pkgs/test/nixpkgs-check-by-name/tests/override-success/all-packages.nix

@@ -0,0 +1,5 @@
+self: super: {
+  foo = self.callPackage ./pkgs/by-name/fo/foo/package.nix {
+    enableBar = true;
+  };
+}

pkgs/test/nixpkgs-check-by-name/tests/override-success/default.nix

@@ -0,0 +1 @@
+import ../mock-nixpkgs.nix { root = ./.; }

pkgs/test/nixpkgs-check-by-name/tests/override-success/pkgs/by-name/fo/foo/package.nix

@@ -0,0 +1,8 @@
+{
+  someDrv,
+  enableBar ? false,
+}:
+if enableBar then
+  someDrv
+else
+  {}

pkgs/top-level/by-name-overlay.nix

@@ -45,6 +45,15 @@ in
 # Currently this would be hard to measure until we have more packages
 # and ideally https://github.com/NixOS/nix/pull/8895
 self: super:
-mapAttrs (name: file:
-  self.callPackage file { }
-) packageFiles
+{
+  # This attribute is necessary to allow CI to ensure that all packages defined in `pkgs/by-name`
+  # don't have an overriding definition in `all-packages.nix` with an empty (`{ }`) second `callPackage` argument.
+  # It achieves that with an overlay that modifies both `callPackage` and this attribute to signal whether `callPackage` is used
+  # and whether it's defined by this file here or `all-packages.nix`.
+  # TODO: This can be removed once `pkgs/by-name` can handle custom `callPackage` arguments without `all-packages.nix` (or any other way of achieving the same result).
+  # Because at that point the code in ./stage.nix can be changed to not allow definitions in `all-packages.nix` to override ones from `pkgs/by-name` anymore and throw an error if that happens instead.
+  _internalCallByNamePackageFile = file: self.callPackage file { };
+}
+// mapAttrs
+  (name: self._internalCallByNamePackageFile)
+  packageFiles