Merge pull request #281390 from tweag/by-name-alias-fix

tests.nixpkgs-check-by-name: Fix for aliases to packages in `pkgs/by-name` and better testing
NixOS · Jan 17, 2024 · a48d8ea · a48d8ea
2 parents efbacad + 0f27917
commit a48d8ea
Show file tree

Hide file tree

Showing 9 changed files with 61 additions and 28 deletions.
diff --git a/pkgs/test/nixpkgs-check-by-name/default.nix b/pkgs/test/nixpkgs-check-by-name/default.nix
@@ -6,10 +6,26 @@
   clippy,
   mkShell,
   makeWrapper,
+  runCommand,
 }:
 let
   runtimeExprPath = ./src/eval.nix;
   nixpkgsLibPath = ../../../lib;
+
+  # Needed to make Nix evaluation work inside nix builds
+  initNix = ''
+    export TEST_ROOT=$(pwd)/test-tmp
+    export NIX_CONF_DIR=$TEST_ROOT/etc
+    export NIX_LOCALSTATE_DIR=$TEST_ROOT/var
+    export NIX_LOG_DIR=$TEST_ROOT/var/log/nix
+    export NIX_STATE_DIR=$TEST_ROOT/var/nix
+    export NIX_STORE_DIR=$TEST_ROOT/store
+
+    # Ensure that even if tests run in parallel, we don't get an error
+    # We'd run into https://github.com/NixOS/nix/issues/2706 unless the store is initialised first
+    nix-store --init
+  '';
+
   package =
     rustPlatform.buildRustPackage {
       name = "nixpkgs-check-by-name";
@@ -22,21 +38,8 @@ let
         makeWrapper
       ];
       env.NIX_CHECK_BY_NAME_EXPR_PATH = "${runtimeExprPath}";
-      # Needed to make Nix evaluation work inside the nix build
-      preCheck = ''
-        export TEST_ROOT=$(pwd)/test-tmp
-        export NIX_CONF_DIR=$TEST_ROOT/etc
-        export NIX_LOCALSTATE_DIR=$TEST_ROOT/var
-        export NIX_LOG_DIR=$TEST_ROOT/var/log/nix
-        export NIX_STATE_DIR=$TEST_ROOT/var/nix
-        export NIX_STORE_DIR=$TEST_ROOT/store
-
-        export NIXPKGS_LIB_PATH=${nixpkgsLibPath}
-
-        # Ensure that even if tests run in parallel, we don't get an error
-        # We'd run into https://github.com/NixOS/nix/issues/2706 unless the store is initialised first
-        nix-store --init
-      '';
+      env.NIXPKGS_LIB_PATH = "${nixpkgsLibPath}";
+      preCheck = initNix;
       postCheck = ''
         cargo fmt --check
         cargo clippy -- -D warnings
@@ -50,6 +53,19 @@ let
         env.NIXPKGS_LIB_PATH = toString nixpkgsLibPath;
         inputsFrom = [ package ];
       };
+
+      # Tests the tool on the current Nixpkgs tree, this is a good sanity check
+      passthru.tests.nixpkgs = runCommand "test-nixpkgs-check-by-name" {
+        nativeBuildInputs = [
+          package
+          nix
+        ];
+        nixpkgsPath = lib.cleanSource ../../..;
+      } ''
+        ${initNix}
+        nixpkgs-check-by-name --base "$nixpkgsPath" "$nixpkgsPath"
+        touch $out
+      '';
     };
 in
 package
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -168,14 +168,23 @@ pub fn check_values(
                         // so the UsesByName ratchet is already as tight as it can be
                         NonAttributeSet => Success(Tight),
                         NonCallPackage => Success(Tight),
-                        // This is an odd case when _internalCallByNamePackageFile is used to define a package.
+                        // This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile
+                        // is used for a package outside `pkgs/by-name`
                         CallPackage(CallPackageInfo {
                             call_package_variant: Auto,
                             ..
-                        }) => NixpkgsProblem::InternalCallPackageUsed {
-                            attr_name: attribute_name.clone(),
+                        }) => {
+                            // With the current detection mechanism, this also triggers for aliases
+                            // to pkgs/by-name packages, and there's no good method of
+                            // distinguishing alias vs non-alias.
+                            // Using `config.allowAliases = false` at least currently doesn't work
+                            // because there's nothing preventing people from defining aliases that
+                            // are present even with that disabled.
+                            // In the future we could kind of abuse this behavior to have better
+                            // enforcement of conditional aliases, but for now we just need to not
+                            // give an error.
+                            Success(Tight)
                         }
-                        .into(),
                         // Only derivations can be in pkgs/by-name,
                         // so this attribute doesn't qualify
                         CallPackage(CallPackageInfo {

diff --git a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -88,9 +88,6 @@ pub enum NixpkgsProblem {
         text: String,
         io_error: io::Error,
     },
-    InternalCallPackageUsed {
-        attr_name: String,
-    },
     MovedOutOfByName {
         package_name: String,
         call_package_path: Option<PathBuf>,
@@ -227,11 +224,6 @@ impl fmt::Display for NixpkgsProblem {
                     subpath.display(),
                     text,
                 ),
-            NixpkgsProblem::InternalCallPackageUsed { attr_name } =>
-                write!(
-                    f,
-                    "pkgs.{attr_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.",
-                ),
             NixpkgsProblem::MovedOutOfByName { package_name, call_package_path, empty_arg } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {

diff --git a/pkgs/test/nixpkgs-check-by-name/tests/aliases/aliases.nix b/pkgs/test/nixpkgs-check-by-name/tests/aliases/aliases.nix
@@ -0,0 +1,3 @@
+self: super: {
+  baz = self.foo;
+}
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/aliases/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/aliases/all-packages.nix
@@ -0,0 +1,3 @@
+self: super: {
+  bar = self.foo;
+}
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/aliases/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/aliases/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/aliases/pkgs/by-name/fo/foo/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/aliases/pkgs/by-name/fo/foo/package.nix
@@ -0,0 +1 @@
+{ someDrv }: someDrv
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix b/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix
@@ -77,12 +77,21 @@ let
     else
       [ ];
 
+  # A list optionally containing the `aliases.nix` file from the test case as an overlay
+  # But only if config.allowAliases is not false
+  optionalAliasesOverlay =
+    if (config.allowAliases or true) && builtins.pathExists (root + "/aliases.nix") then
+      [ (import (root + "/aliases.nix")) ]
+    else
+      [ ];
+
   # All the overlays in the right order, including the user-supplied ones
   allOverlays =
     [
       autoCalledPackages
     ]
     ++ optionalAllPackagesOverlay
+    ++ optionalAliasesOverlay
     ++ overlays;
 
   # Apply all the overlays in order to the base fixed-point function pkgsFun