From 39191e84c6dc365ef36fe30b2419867251a4c9e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Mon, 2 Mar 2026 16:42:37 +0100 Subject: [PATCH] Don't evaluate packages in `nix build` Re-evaluating all attributes can significantly increase the memory consumption of `nix build` by several GB during the entire runtime of the process and also can take some time for large reviews. The required derivations are already known from `multi_system_eval`, so pass them directly to `nix build`. Sadly, we can't pass derivations or store paths directly to `mkShell`/`nix-shell`, so we need to turn the required outputs to store paths with `builtins.storePath` and pass them to `mkShell`. --- nixpkgs_review/nix.py | 126 ++++++++-------------------- nixpkgs_review/nix/review-shell.nix | 37 ++------ nixpkgs_review/report.py | 7 +- nixpkgs_review/review.py | 4 - nixpkgs_review/utils.py | 13 ++- 5 files changed, 57 insertions(+), 130 deletions(-) diff --git a/nixpkgs_review/nix.py b/nixpkgs_review/nix.py index f36e9c67..6e87d73b 100644 --- a/nixpkgs_review/nix.py +++ b/nixpkgs_review/nix.py @@ -24,7 +24,6 @@ class BuildConfig: allow: AllowedFeatures nix_path: str - local_system: str nixpkgs_config: Path num_eval_workers: int = 1 max_memory_size: int = 4096 @@ -99,17 +98,14 @@ class ShellConfig: """Configuration for launching a nix-shell.""" cache_directory: Path - local_system: str build_graph: str nix_path: str - nixpkgs_config: Path - nixpkgs_overlay: Path run: str | None = None sandbox: bool = False def nix_shell( - attrs_per_system: dict[System, list[str]], + attrs_per_system: dict[System, list[Attr]], config: ShellConfig, ) -> None: bin_name = f"{config.build_graph}-shell" @@ -121,8 +117,6 @@ def nix_shell( shell_file_args = build_shell_file_args( cache_dir=config.cache_directory, attrs_per_system=attrs_per_system, - local_system=config.local_system, - nixpkgs_config=config.nixpkgs_config, ) if config.sandbox: args = _nix_shell_sandbox(nix_shell_bin, shell_file_args, config) @@ -200,9 +194,6 @@ def tmpfs(path: Path | str, *, is_dir: bool = True) -> list[str]: *bind("/"), *bind("/dev", dev=True), *tmpfs("/tmp"), # noqa: S108 - # Required for evaluation - *bind(config.nixpkgs_config), - *bind(config.nixpkgs_overlay), # /run (also cover sockets for wayland/pulseaudio and pipewires) *bind(Path("/run/user").joinpath(uid), dev=True, try_=True), # HOME @@ -295,6 +286,8 @@ def _nix_eval_filter(packages: NixEvalResult) -> list[Attr]: def multi_system_eval( attr_names_per_system: dict[System, set[str]], build_config: BuildConfig, + *, + instantiate: bool = False, ) -> dict[System, list[Attr]]: attr_json = NamedTemporaryFile(mode="w+", delete=False) # noqa: SIM115 delete = True @@ -311,7 +304,7 @@ def multi_system_eval( str(build_config.num_eval_workers), "--max-memory-size", str(build_config.max_memory_size), - "--no-instantiate", + *(() if instantiate else ("--no-instantiate",)), *_nix_common_flags(build_config.allow, build_config.nix_path), "--expr", f"(import {eval_script} {{ attr-json = {attr_json.name}; }})", @@ -369,24 +362,26 @@ def nix_build( attrs_per_system: dict[System, list[Attr]] = multi_system_eval( attr_names_per_system, build_config, + instantiate=True, ) - filtered_per_system = { - system: [attr.name for attr in attrs if not (attr.broken or attr.blacklisted)] - for system, attrs in attrs_per_system.items() - } + filtered_drv_paths = [ + f"{attr.drv_path}^*" + for attrs in attrs_per_system.values() + 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(filtered_drv_paths) == 0: return attrs_per_system command = [ build_graph, "build", - "--file", - REVIEW_SHELL, *_nix_common_flags(build_config.allow, build_config.nix_path), "--no-link", "--keep-going", + "--stdin", ] if platform == "linux": @@ -397,88 +392,37 @@ def nix_build( "relaxed", ] - shell_file_args = build_shell_file_args( - cache_dir=cache_directory, - attrs_per_system=filtered_per_system, - local_system=build_config.local_system, - nixpkgs_config=build_config.nixpkgs_config, - ) - _write_review_shell_drv( - cache_directory=cache_directory, - shell_file_args=shell_file_args, - allow=build_config.allow, - nix_path=build_config.nix_path, - ) + command += shlex.split(args) - command += shell_file_args + shlex.split(args) + rebuilds_file = cache_directory / "rebuilds.txt" + with rebuilds_file.open("w+") as f: + f.write("".join(f"{p}\n" for p in filtered_drv_paths)) + f.flush() + f.seek(0, os.SEEK_SET) + + sh(command, stdin=f) - sh(command) return attrs_per_system def build_shell_file_args( cache_dir: Path, - attrs_per_system: dict[System, list[str]], - local_system: str, - nixpkgs_config: Path, + attrs_per_system: dict[System, list[Attr]], ) -> list[str]: - attrs_file = cache_dir.joinpath("attrs.nix") - with attrs_file.open("w+") as f: - f.write("{\n") - for system, attrs in attrs_per_system.items(): - f.write(f" {system} = [\n") - for attr in attrs: - f.write(f' "{attr}"\n') - f.write(" ];\n") - f.write("}") + outputs_file = cache_dir.joinpath("outputs.json") + with outputs_file.open("w+") as f: + json.dump( + [ + str(output) + for attrs in attrs_per_system.values() + for attr in attrs + for output in (attr.outputs or {}).values() + ], + f, + ) return [ "--argstr", - "local-system", - local_system, - "--argstr", - "nixpkgs-path", - str(cache_dir.joinpath("nixpkgs/")), - "--argstr", - "nixpkgs-config-path", - str(nixpkgs_config), - "--argstr", - "attrs-path", - str(attrs_file), - ] - - -def _write_review_shell_drv( - cache_directory: Path, - shell_file_args: list[str], - allow: AllowedFeatures, - nix_path: str, -) -> None: - review_drv_link: Path = cache_directory / "review-shell.drv" - - cmd: list[str] = [ - "nix-instantiate", - *_nix_common_flags(allow, nix_path), - *shell_file_args, - REVIEW_SHELL, + "outputs-path", + str(outputs_file), ] - res = subprocess.run( - cmd, - capture_output=True, - text=True, - check=False, - ) - if res.returncode != 0: - msg = "Failed to instantiate review shell derivation for caching" - if res.stderr: - msg = f"{msg}: {res.stderr.strip()}" - raise NixpkgsReviewError(msg) - - drv_lines = [line.strip() for line in res.stdout.splitlines() if line.strip()] - if not drv_lines: - msg = "No review shell derivation path produced for caching" - raise NixpkgsReviewError(msg) - - drv_path = drv_lines[-1] - review_drv_link.unlink(missing_ok=True) - review_drv_link.symlink_to(drv_path) diff --git a/nixpkgs_review/nix/review-shell.nix b/nixpkgs_review/nix/review-shell.nix index 76040824..227fd9a9 100644 --- a/nixpkgs_review/nix/review-shell.nix +++ b/nixpkgs_review/nix/review-shell.nix @@ -1,36 +1,17 @@ { - local-system, - nixpkgs-config-path, - # Path to Nix file containing the Nixpkgs config - attrs-path, - # Path to Nix file containing a list of attributes to build - nixpkgs-path, - # Path to this review's nixpkgs - local-pkgs ? import nixpkgs-path { - system = local-system; - config = import nixpkgs-config-path; - }, - lib ? local-pkgs.lib, + # Path to json file containing the outputs of all built packages + outputs-path, }: let + inherit (import { }) lib buildEnv mkShell; - nixpkgs-config = import nixpkgs-config-path; - extractPackagesForSystem = - system: system-attrs: - let - system-pkg = import nixpkgs-path { - inherit system; - config = nixpkgs-config; - }; - in - map (attrString: lib.attrByPath (lib.splitString "." attrString) null system-pkg) system-attrs; - attrs = lib.flatten (lib.mapAttrsToList extractPackagesForSystem (import attrs-path)); - supportIgnoreSingleFileOutputs = (lib.functionArgs local-pkgs.buildEnv) ? ignoreSingleFileOutputs; - env = local-pkgs.buildEnv ( + outputs = map builtins.storePath (builtins.fromJSON (builtins.readFile outputs-path)); + supportIgnoreSingleFileOutputs = (lib.functionArgs buildEnv) ? ignoreSingleFileOutputs; + env = buildEnv ( { name = "env"; - paths = attrs; + paths = outputs; ignoreCollisions = true; } // lib.optionalAttrs supportIgnoreSingleFileOutputs { @@ -38,11 +19,11 @@ let } ); in -(import nixpkgs-path { }).mkShell { +mkShell { name = "review-shell"; preferLocalBuild = true; allowSubstitutes = false; dontWrapQtApps = true; # see test_rev_command_with_pkg_count - packages = if builtins.length attrs > 50 then [ env ] else attrs; + packages = if builtins.length outputs > 50 then [ env ] else outputs; } diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index 9dcfb5f9..25685f7a 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -350,11 +350,8 @@ def __init__( reports[system] = SystemReport(attrs) self.system_reports: dict[System, SystemReport] = order_reports(reports) - def built_packages(self) -> dict[System, list[str]]: - return { - system: [a.name for a in report.built] - for system, report in self.system_reports.items() - } + def built_packages(self) -> dict[System, list[Attr]]: + return {system: report.built for system, report in self.system_reports.items()} def write(self, directory: Path, pr: int | None) -> None: # write logs first because snippets from them may be needed for the report diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index 47a0f3d4..6dd0ab5f 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -638,11 +638,8 @@ def start_review( if not self.shell_options.no_shell: shell_config = ShellConfig( cache_directory=path, - local_system=self.build_config.local_system, build_graph=self.shell_options.build_graph, nix_path=self.builddir.nix_path, - nixpkgs_config=self.build_config.nixpkgs_config, - nixpkgs_overlay=self.builddir.overlay.path, run=self.shell_options.run, sandbox=self.shell_options.sandbox, ) @@ -1037,7 +1034,6 @@ def build_config_from_args( return BuildConfig( allow=allow, nix_path=nix_path, - local_system=current_system(), nixpkgs_config=nixpkgs_config, num_eval_workers=args.num_eval_workers, max_memory_size=args.max_memory_size, diff --git a/nixpkgs_review/utils.py b/nixpkgs_review/utils.py index fb526aba..d83a8646 100644 --- a/nixpkgs_review/utils.py +++ b/nixpkgs_review/utils.py @@ -69,7 +69,7 @@ def sh( # noqa: PLR0913 *, cwd: Path | str | None = None, env: dict[str, str] | None = None, - stdin: str | None = None, + stdin: str | int | IO[Any] | None = None, stdout: int | None = None, stderr: int | None = None, quiet: bool = False, @@ -77,13 +77,22 @@ def sh( # noqa: PLR0913 if not quiet: info("$ " + shlex.join(command)) env = os.environ | env if env else None + + input_: str | None = None + stdin_: int | IO[Any] | None = None + if isinstance(stdin, str): + input_ = stdin + else: + stdin_ = stdin + return subprocess.run( command, cwd=cwd, text=True, check=False, env=env, - input=stdin, + input=input_, + stdin=stdin_, stdout=stdout, stderr=stderr, )