Switch to nix-eval-jobs for local evaluation

README.md

@@ -323,6 +323,10 @@ attribute set:
 $ nixpkgs-review pr -p nixosTests.ferm 47077
 ```
 
+Many packages also specify their associated `nixosTests` in the `passthru.tests`
+attribute set. Adding the `--tests` argument will run those tests for all
+packages that will be rebuild.
+
 ## Ignoring ofborg evaluations
 
 By default, nixpkgs-review will use ofborg's evaluation result if available to

default.nix

@@ -10,6 +10,18 @@ let
   withNom' =
     withNom
     && (builtins.tryEval (builtins.elem buildPlatform.system pkgs.ghc.meta.platforms)).value or false;
+
+  # at least v2.26.0 (currently unreleased) is required for the '--apply' flag
+  nix-eval-jobs =
+    if lib.versionOlder pkgs.nix-eval-jobs.version "2.26.0" then
+      callPackage (fetchFromGitHub {
+        owner = "nix-community";
+        repo = "nix-eval-jobs";
+        rev = "6d4fd5a93d7bc953ffa4dcd6d53ad7056a71eff7";
+        hash = "sha256-1dZLPw+nlFQzzswfyTxW+8VF1AJ4ZvoYvLTjlHiz1SA=";
+      }) { nix = nixVersions.nix_2_25; }
+    else
+      pkgs.nix-eval-jobs;
 in
 python3Packages.buildPythonApplication {
   name = "nixpkgs-review";
@@ -46,6 +58,7 @@ python3Packages.buildPythonApplication {
         [
           pkgs.nixVersions.stable or nix_2_4
           git
+          nix-eval-jobs
         ]
         ++ lib.optional withSandboxSupport bubblewrap
         ++ lib.optional withNom' nix-output-monitor;

flake.nix

@@ -3,6 +3,7 @@
 
   inputs = {
     nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
+
     flake-parts.url = "github:hercules-ci/flake-parts";
     flake-parts.inputs.nixpkgs-lib.follows = "nixpkgs";
 

nixpkgs_review/cli/__init__.py

@@ -179,6 +179,11 @@ def common_flags() -> list[CommonFlag]:
             type=regex_type,
             help="Regular expression that package attributes have to match (can be passed multiple times)",
         ),
+        CommonFlag(
+            "--tests",
+            action="store_true",
+            help="For all packages to be built, also build their `passthru.tests`",
+        ),
         CommonFlag(
             "-r",
             "--remote",
@@ -250,7 +255,13 @@ def common_flags() -> list[CommonFlag]:
             "--num-parallel-evals",
             type=int,
             default=1,
-            help="Number of parallel `nix-env`/`nix eval` processes to run simultaneously (warning, can imply heavy RAM usage)",
+            help="Number of parallel `nix-eval-jobs` workers to run simultaneously (warning, can imply heavy RAM usage)",
+        ),
+        CommonFlag(
+            "--max-memory-size",
+            type=int,
+            default=4096,
+            help="Maximum `nix-eval-jobs` per worker memory size in megabyte (warning, workers can shortly exceed the limit)",
         ),
     ]
 

nixpkgs_review/cli/pr.py

@@ -110,7 +110,9 @@ def pr_command(args: argparse.Namespace) -> str:
                     build_graph=args.build_graph,
                     nixpkgs_config=nixpkgs_config,
                     extra_nixpkgs_config=args.extra_nixpkgs_config,
+                    build_passthru_tests=args.tests,
                     num_parallel_evals=args.num_parallel_evals,
+                    max_memory_size=args.max_memory_size,
                     show_header=not args.no_headers,
                 )
                 contexts.append((pr, builddir.path, review.build_pr(pr)))

nixpkgs_review/nix.py

@@ -1,4 +1,3 @@
-import concurrent.futures
 import json
 import os
 import shlex
@@ -21,13 +20,13 @@ class Attr:
     exists: bool
     broken: bool
     blacklisted: bool
-    path: Path | None
-    drv_path: str | None
+    outputs: dict[str, Path] | None
+    drv_path: Path | None
     aliases: list[str] = field(default_factory=list)
     _path_verified: bool | None = field(init=False, default=None)
 
     def was_build(self) -> bool:
-        if self.path is None:
+        if self.outputs is None or len(self.outputs) == 0:
             return False
 
         if self._path_verified is not None:
@@ -42,7 +41,7 @@ def was_build(self) -> bool:
                 "verify",
                 "--no-contents",
                 "--no-trust",
-                self.path,
+                *self.outputs.values(),
             ],
             stderr=subprocess.DEVNULL,
             check=False,
@@ -51,7 +50,15 @@ def was_build(self) -> bool:
         return self._path_verified
 
     def is_test(self) -> bool:
-        return self.name.startswith("nixosTests")
+        return self.name.startswith("nixosTests") or ".passthru.tests." in self.name
+
+    def outputs_with_name(self) -> dict[str, Path]:
+        def with_output(output: str) -> str:
+            if output == "out":
+                return self.name
+            return f"{self.name}.{output}"
+
+        return {with_output(output): path for output, path in self.outputs.items()}
 
 
 REVIEW_SHELL: Final[str] = str(ROOT.joinpath("nix/review-shell.nix"))
@@ -187,7 +194,7 @@ def tmpfs(path: Path | str, is_dir: bool = True) -> list[str]:
     ]
 
 
-def _nix_eval_filter(json: dict[str, Any]) -> list[Attr]:
+def _nix_eval_filter(json: list[Any]) -> list[Attr]:
     # workaround https://github.com/NixOS/ofborg/issues/269
     blacklist = {
         "appimage-run-tests",
@@ -200,27 +207,37 @@ def _nix_eval_filter(json: dict[str, Any]) -> list[Attr]:
         "tests.trivial",
         "tests.writers",
     }
+
+    def is_blacklisted(name: str) -> bool:
+        return name in blacklist or any(
+            name.startswith(f"{entry}.") for entry in blacklist
+        )
+
     attr_by_path: dict[Path, Attr] = {}
     broken = []
-    for name, props in json.items():
-        path = props.get("path", None)
-        if path is not None:
-            path = Path(path)
-
+    for props in json:
+        drv_path = None
+        outputs = None
+        if not props["broken"]:
+            drv_path = Path(props["drvPath"])
+            outputs = {output: Path(path) for output, path in props["outputs"].items()}
+
+        # the 'name' field might be quoted, so get the unqoted one from 'attrPath'
+        name = props["attrPath"][1]
         attr = Attr(
             name=name,
             exists=props["exists"],
             broken=props["broken"],
-            blacklisted=name in blacklist,
-            path=path,
-            drv_path=props["drvPath"],
+            blacklisted=is_blacklisted(name),
+            outputs=outputs,
+            drv_path=drv_path,
         )
-        if attr.path is not None:
-            other = attr_by_path.get(attr.path, None)
+        if attr.drv_path is not None:
+            other = attr_by_path.get(attr.drv_path, None)
             if other is None:
-                attr_by_path[attr.path] = attr
+                attr_by_path[attr.drv_path] = attr
             elif len(other.name) > len(attr.name):
-                attr_by_path[attr.path] = attr
+                attr_by_path[attr.drv_path] = attr
                 attr.aliases.append(other.name)
             else:
                 other.aliases.append(attr.name)
@@ -234,31 +251,60 @@ def nix_eval(
     system: str,
     allow: AllowedFeatures,
     nix_path: str,
+    num_parallel_evals: int,
+    max_memory_size: int,
+    include_passthru_tests: bool = False,
 ) -> list[Attr]:
+    return multi_system_eval(
+        {system: attrs},
+        allow=allow,
+        nix_path=nix_path,
+        num_parallel_evals=num_parallel_evals,
+        max_memory_size=max_memory_size,
+        include_passthru_tests=include_passthru_tests,
+    ).get(system, [])
+
+
+def multi_system_eval(
+    attr_names_per_system: dict[System, set[str]],
+    allow: AllowedFeatures,
+    nix_path: str,
+    num_parallel_evals: int,
+    max_memory_size: int,
+    include_passthru_tests: bool = False,
+) -> dict[System, list[Attr]]:
     attr_json = NamedTemporaryFile(mode="w+", delete=False)  # noqa: SIM115
     delete = True
     try:
-        json.dump(list(attrs), attr_json)
+        json.dump(
+            {system: list(attrs) for system, attrs in attr_names_per_system.items()},
+            attr_json,
+        )
         eval_script = str(ROOT.joinpath("nix/evalAttrs.nix"))
         attr_json.flush()
         cmd = [
-            "nix",
+            "nix-eval-jobs",
+            "--workers",
+            str(num_parallel_evals),
+            "--max-memory-size",
+            str(max_memory_size),
             "--extra-experimental-features",
-            "nix-command" if allow.url_literals else "nix-command no-url-literals",
-            "--system",
-            system,
-            "eval",
+            "" if allow.url_literals else "no-url-literals",
+            "--expr",
+            f"""(import {eval_script} {{
+              attr-json = {attr_json.name};
+              include-passthru-tests = {str(include_passthru_tests).lower()};
+            }})""",
             "--nix-path",
             nix_path,
-            "--json",
-            "--impure",
             "--allow-import-from-derivation"
             if allow.ifd
             else "--no-allow-import-from-derivation",
-            "--expr",
-            f"(import {eval_script} {{ attr-json = {attr_json.name}; }})",
+            "--apply",
+            "d: { inherit (d) exists broken; }",
         ]
 
+        info("$ " + shlex.join(cmd))
         nix_eval = subprocess.run(cmd, stdout=subprocess.PIPE, text=True, check=False)
         if nix_eval.returncode != 0:
             delete = False
@@ -267,48 +313,30 @@ def nix_eval(
             )
             raise NixpkgsReviewError(msg)
 
-        return _nix_eval_filter(json.loads(nix_eval.stdout))
+        systems_packages = {}
+        for line in nix_eval.stdout.splitlines():
+            attrs = json.loads(line)
+            system = attrs["attrPath"][0]
+            systems_packages.setdefault(system, list()).append(attrs)
+
+        return {
+            system: _nix_eval_filter(packages)
+            for system, packages in systems_packages.items()
+        }
     finally:
         attr_json.close()
         if delete:
             Path(attr_json.name).unlink()
 
 
-def multi_system_eval(
-    attr_names_per_system: dict[System, set[str]],
-    allow: AllowedFeatures,
-    nix_path: str,
-    n_threads: int,
-) -> dict[System, list[Attr]]:
-    results: dict[System, list[Attr]] = {}
-    with concurrent.futures.ThreadPoolExecutor(max_workers=n_threads) as executor:
-        future_to_system = {
-            executor.submit(
-                nix_eval,
-                attrs=attrs,
-                system=system,
-                allow=allow,
-                nix_path=nix_path,
-            ): system
-            for system, attrs in attr_names_per_system.items()
-        }
-        for future in concurrent.futures.as_completed(future_to_system):
-            system = future_to_system[future]
-            results[system] = future.result()
-
-    return results
-
-
 def nix_build(
     attr_names_per_system: dict[System, set[str]],
     args: str,
-    cache_directory: Path,
-    local_system: System,
     allow: AllowedFeatures,
     build_graph: str,
     nix_path: str,
-    nixpkgs_config: Path,
-    n_threads: int,
+    num_parallel_evals: int,
+    max_memory_size: int,
 ) -> dict[System, list[Attr]]:
     if not attr_names_per_system:
         info("Nothing to be built.")
@@ -318,33 +346,29 @@ def nix_build(
         attr_names_per_system,
         allow,
         nix_path,
-        n_threads=n_threads,
+        num_parallel_evals=num_parallel_evals,
+        max_memory_size=max_memory_size,
     )
 
-    filtered_per_system: dict[System, list[str]] = {}
-    for system, attrs in attrs_per_system.items():
-        filtered_per_system[system] = []
-        for attr in attrs:
-            if not (attr.broken or attr.blacklisted):
-                filtered_per_system[system].append(attr.name)
+    paths = []
+    for attrs in attrs_per_system.values():
+        paths.extend(
+            f"{attr.drv_path}^*"
+            for attr in attrs
+            if not (attr.broken or attr.blacklisted)
+        )
 
-    if all(len(filtered) == 0 for filtered in filtered_per_system.values()):
+    if len(paths) == 0:
         return attrs_per_system
 
     command = [
         build_graph,
         "build",
-        "--file",
-        REVIEW_SHELL,
-        "--nix-path",
-        nix_path,
         "--extra-experimental-features",
-        "nix-command" if allow.url_literals else "nix-command no-url-literals",
+        "nix-command",
         "--no-link",
         "--keep-going",
-        "--allow-import-from-derivation"
-        if allow.ifd
-        else "--no-allow-import-from-derivation",
+        "--stdin",
     ]
 
     if platform == "linux":
@@ -355,14 +379,9 @@ def nix_build(
             "relaxed",
         ]
 
-    command += build_shell_file_args(
-        cache_dir=cache_directory,
-        attrs_per_system=filtered_per_system,
-        local_system=local_system,
-        nixpkgs_config=nixpkgs_config,
-    ) + shlex.split(args)
+    command += shlex.split(args)
 
-    sh(command)
+    sh(command, input="\n".join(str(p) for p in paths))
     return attrs_per_system