From 1d8cfeafa55bc61b7a25999f58b49c7122fc94ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Mon, 24 Feb 2025 22:30:37 +0100 Subject: [PATCH 1/7] Fix Review.build_commit merge behavior --- nixpkgs_review/review.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index 969fe829..7fa4c636 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -221,7 +221,7 @@ def build_commit( if self.only_packages: if reviewed_commit is None: self.apply_unstaged(staged) - elif self.checkout == CheckoutOption.MERGE: + elif self.checkout == CheckoutOption.COMMIT: self.git_checkout(reviewed_commit) else: self.git_merge(reviewed_commit) @@ -246,7 +246,7 @@ def build_commit( if reviewed_commit is None: self.apply_unstaged(staged) - elif self.checkout == CheckoutOption.MERGE: + elif self.checkout == CheckoutOption.COMMIT: self.git_checkout(reviewed_commit) else: self.git_merge(reviewed_commit) From c23571920b67b0b38f91eeb6a7b7aedddf3cd5ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Wed, 26 Feb 2025 15:12:30 +0100 Subject: [PATCH 2/7] Fix typo in console report --- nixpkgs_review/report.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index a5ce15b7..60db1f4a 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -275,5 +275,5 @@ def print_console(self, pr: int | None) -> None: ) print_number(report.blacklisted, "blacklisted", log=skipped) print_number(report.failed, "failed to build") - print_number(report.tests, "built", what="tests", log=print) + print_number(report.tests, "built", what="test", log=print) print_number(report.built, "built", log=print) From f4d26b9cd270ec13112e20db21ea4e8f17e2644c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Wed, 26 Feb 2025 15:12:37 +0100 Subject: [PATCH 3/7] Report failed tests as failed build Until now, failed tests would not be reported separately and simply shown in the "tests build" list. --- nixpkgs_review/report.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index 60db1f4a..9687d5ab 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -116,10 +116,10 @@ def __init__(self, attrs: list[Attr]) -> None: self.blacklisted.append(attr) elif not attr.exists: self.non_existent.append(attr) - elif attr.name.startswith("nixosTests."): - self.tests.append(attr) elif not attr.was_build(): self.failed.append(attr) + elif attr.is_test(): + self.tests.append(attr) else: self.built.append(attr) From e02c1483a8b334fa6117667521d091239e9d1267 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Wed, 26 Feb 2025 15:12:51 +0100 Subject: [PATCH 4/7] Switch to nix-eval-jobs for local evaluation --- default.nix | 13 +++ flake.nix | 1 + nixpkgs_review/cli/__init__.py | 8 +- nixpkgs_review/cli/pr.py | 1 + nixpkgs_review/review.py | 148 ++++++++++++--------------------- tests/test_pr.py | 2 +- tests/test_rev.py | 2 +- tests/test_wip.py | 2 +- 8 files changed, 79 insertions(+), 98 deletions(-) diff --git a/default.nix b/default.nix index c647e7b3..cb52849e 100644 --- a/default.nix +++ b/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; diff --git a/flake.nix b/flake.nix index b25fcf1f..3c49f543 100644 --- a/flake.nix +++ b/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"; diff --git a/nixpkgs_review/cli/__init__.py b/nixpkgs_review/cli/__init__.py index 2531267e..d1adabce 100644 --- a/nixpkgs_review/cli/__init__.py +++ b/nixpkgs_review/cli/__init__.py @@ -250,7 +250,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)", ), ] diff --git a/nixpkgs_review/cli/pr.py b/nixpkgs_review/cli/pr.py index 42e90416..de40624b 100644 --- a/nixpkgs_review/cli/pr.py +++ b/nixpkgs_review/cli/pr.py @@ -111,6 +111,7 @@ def pr_command(args: argparse.Namespace) -> str: nixpkgs_config=nixpkgs_config, extra_nixpkgs_config=args.extra_nixpkgs_config, 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))) diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index 7fa4c636..99549c7a 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -1,6 +1,7 @@ import argparse -import concurrent.futures +import json import os +import shlex import subprocess import sys import tempfile @@ -10,7 +11,6 @@ from pathlib import Path from re import Pattern from typing import IO -from xml.etree import ElementTree as ET from .allow import AllowedFeatures from .builddir import Builddir @@ -107,6 +107,7 @@ def __init__( checkout: CheckoutOption = CheckoutOption.MERGE, sandbox: bool = False, num_parallel_evals: int = 1, + max_memory_size: int = 4096, show_header: bool = True, ) -> None: if skip_packages_regex is None: @@ -146,6 +147,7 @@ def __init__( self.nixpkgs_config = nixpkgs_config self.extra_nixpkgs_config = extra_nixpkgs_config self.num_parallel_evals = num_parallel_evals + self.max_memory_size = max_memory_size self.show_header = show_header def _process_aliases_for_systems(self, system: str) -> set[str]: @@ -241,7 +243,8 @@ def build_commit( self.builddir.nix_path, self.systems, self.allow, - n_threads=self.num_parallel_evals, + num_parallel_evals=self.num_parallel_evals, + max_memory_size=self.max_memory_size, ) if reviewed_commit is None: @@ -256,7 +259,8 @@ def build_commit( self.builddir.nix_path, self.systems, self.allow, - n_threads=self.num_parallel_evals, + num_parallel_evals=self.num_parallel_evals, + max_memory_size=self.max_memory_size, check_meta=True, ) @@ -446,119 +450,74 @@ def review_commit( ) -def parse_packages_xml(stdout: IO[str]) -> list[Package]: - packages: list[Package] = [] - path = None - attrs = None - homepage = None - description = None - position = None - context = ET.iterparse(stdout, events=("start", "end")) # noqa: S314 - for event, elem in context: - if elem.tag == "item": - if event == "start": - attrs = elem.attrib - homepage = None - description = None - position = None - path = None - else: - assert attrs is not None - if path is None: - # architecture not supported - continue - pkg = Package( - pname=attrs["pname"], - version=attrs["version"], - attr_path=attrs["attrPath"], - store_path=path, - homepage=homepage, - description=description, - position=position, - ) - packages.append(pkg) - elif event == "start" and elem.tag == "output" and elem.attrib["name"] == "out": - path = elem.attrib["path"] - elif event == "start" and elem.tag == "meta": - name = elem.attrib["name"] - if name not in ["homepage", "description", "position"]: - continue - if elem.attrib["type"] == "strings": - values = (e.attrib["value"] for e in elem) - value = ", ".join(values) - else: - value = elem.attrib["value"] - if name == "homepage": - homepage = value - elif name == "description": - description = value - elif name == "position": - position = value +def parse_packages_json(stdout: IO[str]) -> dict[System, list[Package]]: + packages = dict() + + for line in stdout: + attrs = json.loads(line) + if "error" in attrs: + continue + + system, attr_path = attrs["attr"].split(".", 1) + + packages.setdefault(system, list()).append( + Package( + pname=attrs["pname"] or attrs["name"], + version=attrs["version"], + attr_path=attr_path, + store_path=(attrs["outputs"].get("out") or attrs["outputs"].popitem()), + homepage=attrs.get("meta", {}).get("homepage"), + description=attrs.get("meta", {}).get("description"), + position=attrs.get("meta", {}).get("position"), + ) + ) + return packages -def _list_packages_system( - system: System, +def list_packages( nix_path: str, + systems: set[System], allow: AllowedFeatures, + num_parallel_evals: int, + max_memory_size: int, check_meta: bool = False, -) -> list[Package]: +) -> dict[System, list[Package]]: + systems_str = "{ " + " ".join(f"{system} = null;" for system in systems) + " }" cmd = [ - "nix-env", + "nix-eval-jobs", + "--workers", + str(num_parallel_evals), + "--max-memory-size", + str(max_memory_size), "--extra-experimental-features", "" if allow.url_literals else "no-url-literals", - "--option", - "system", - system, - "-f", - "", + "--expr", + f"builtins.mapAttrs (system: _: import {{ inherit system; }} // {{ recurseForDerivations = true; }}) {systems_str}", "--nix-path", nix_path, - "-qaP", - "--xml", - "--out-path", "--show-trace", "--allow-import-from-derivation" if allow.ifd else "--no-allow-import-from-derivation", + "--apply", + 'd: { pname = if d ? pname then d.pname else null; version = if d ? version then d.version else ""; }', ] if check_meta: cmd.append("--meta") - info("$ " + " ".join(cmd)) + info("$ " + shlex.join(cmd)) with tempfile.NamedTemporaryFile(mode="w") as tmp: - res = subprocess.run(cmd, stdout=tmp, check=False) + res = subprocess.run(cmd, stdout=tmp, stderr=subprocess.DEVNULL, check=False) if res.returncode != 0: - msg = f"Failed to list packages: nix-env failed with exit code {res.returncode}" + msg = f"Failed to list packages: nix-eval-jobs failed with exit code {res.returncode}" raise NixpkgsReviewError(msg) tmp.flush() with Path(tmp.name).open() as f: - return parse_packages_xml(f) - - -def list_packages( - nix_path: str, - systems: set[System], - allow: AllowedFeatures, - n_threads: int, - check_meta: bool = False, -) -> dict[System, list[Package]]: - results: dict[System, list[Package]] = {} - with concurrent.futures.ThreadPoolExecutor(max_workers=n_threads) as executor: - future_to_system = { - executor.submit( - _list_packages_system, - system=system, - nix_path=nix_path, - allow=allow, - check_meta=check_meta, - ): system - for system in systems - } - for future in concurrent.futures.as_completed(future_to_system): - system = future_to_system[future] - results[system] = future.result() - - return results + packages = parse_packages_json(f) + if len(packages) == 0: + msg = "Failed to list packages: nix-eval-jobs returned no packages" + raise NixpkgsReviewError(msg) + return packages def package_attrs( @@ -744,6 +703,7 @@ def review_local_revision( nixpkgs_config=nixpkgs_config, extra_nixpkgs_config=args.extra_nixpkgs_config, num_parallel_evals=args.num_parallel_evals, + max_memory_size=args.max_memory_size, ) review.review_commit(builddir.path, args.branch, commit, staged, print_result) return builddir.path diff --git a/tests/test_pr.py b/tests/test_pr.py index 5c873638..16c4117e 100644 --- a/tests/test_pr.py +++ b/tests/test_pr.py @@ -253,7 +253,7 @@ def test_pr_github_action_eval( helpers.assert_built(pkg_name="pkg1", path=path) -@patch("nixpkgs_review.review._list_packages_system") +@patch("nixpkgs_review.review.list_packages") def test_pr_only_packages_does_not_trigger_an_eval( mock_eval: MagicMock, helpers: Helpers, diff --git a/tests/test_rev.py b/tests/test_rev.py index 3f665d47..c463924d 100644 --- a/tests/test_rev.py +++ b/tests/test_rev.py @@ -43,7 +43,7 @@ def test_rev_command_without_nom(helpers: Helpers) -> None: helpers.assert_built(pkg_name="pkg1", path=path) -@patch("nixpkgs_review.review._list_packages_system") +@patch("nixpkgs_review.review.list_packages") def test_rev_only_packages_does_not_trigger_an_eval( mock_eval: MagicMock, helpers: Helpers, diff --git a/tests/test_wip.py b/tests/test_wip.py index 9db856c1..e63b31d7 100644 --- a/tests/test_wip.py +++ b/tests/test_wip.py @@ -43,7 +43,7 @@ def test_wip_command_without_nom( assert "$ nix build" in captured.out -@patch("nixpkgs_review.review._list_packages_system") +@patch("nixpkgs_review.review.list_packages") def test_wip_only_packages_does_not_trigger_an_eval( mock_eval: MagicMock, helpers: Helpers, From 5d0b0dc59f3aebd1067202d860b19d52275eaf91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Wed, 26 Feb 2025 15:13:33 +0100 Subject: [PATCH 5/7] Use nix-eval-jobs in multi_system_eval This switches `nix.Attr` from using `path` as the unique package identifier to `drv_path`. --- nixpkgs_review/nix.py | 130 ++++++++++++++++++------------- nixpkgs_review/nix/evalAttrs.nix | 90 +++++++++++---------- nixpkgs_review/report.py | 47 ++++++----- nixpkgs_review/review.py | 33 ++++++-- 4 files changed, 171 insertions(+), 129 deletions(-) diff --git a/nixpkgs_review/nix.py b/nixpkgs_review/nix.py index c23b180e..dddb13fa 100644 --- a/nixpkgs_review/nix.py +++ b/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, @@ -53,6 +52,14 @@ def was_build(self) -> bool: def is_test(self) -> bool: return self.name.startswith("nixosTests") + 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", @@ -202,25 +209,29 @@ def _nix_eval_filter(json: dict[str, Any]) -> list[Attr]: } 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"], + 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 +245,54 @@ def nix_eval( system: str, allow: AllowedFeatures, nix_path: str, + num_parallel_evals: int, + max_memory_size: int, ) -> 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, + ).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, +) -> 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}; }})", "--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,38 +301,22 @@ 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, @@ -308,7 +326,8 @@ def nix_build( 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,7 +337,8 @@ 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]] = {} diff --git a/nixpkgs_review/nix/evalAttrs.nix b/nixpkgs_review/nix/evalAttrs.nix index 64b8f8bd..36bef464 100644 --- a/nixpkgs_review/nix/evalAttrs.nix +++ b/nixpkgs_review/nix/evalAttrs.nix @@ -1,52 +1,56 @@ { attr-json }: with builtins; -let - pkgs = import { - config = import (getEnv "NIXPKGS_CONFIG") // { - allowBroken = false; +mapAttrs ( + system: attrs: + let + pkgs = import { + inherit system; + config = import (getEnv "NIXPKGS_CONFIG") // { + allowBroken = false; + }; }; - }; - inherit (pkgs) lib; + inherit (pkgs) lib; - attrs = fromJSON (readFile attr-json); - getProperties = - name: - let - attrPath = lib.splitString "." name; - pkg = lib.attrByPath attrPath null pkgs; - exists = lib.hasAttrByPath attrPath pkgs; - in - if pkg == null then - [ - (lib.nameValuePair name { - inherit exists; - broken = true; - path = null; - drvPath = null; - }) - ] - else if !lib.isDerivation pkg then - if builtins.typeOf pkg != "set" then - # if it is not a package, ignore it (it is probably something like overrideAttrs) - [ ] + # nix-eval-jobs only shows derivations, so create an empty one to return + fake = derivation { + name = "fake"; + system = "fake"; + builder = "fake"; + }; + + getProperties = + name: + let + attrPath = lib.splitString "." name; + maybePkg = tryEval (lib.attrByPath attrPath null pkgs); + pkg = maybePkg.value; + exists = lib.hasAttrByPath attrPath pkgs; + in + # some packages are set to null or throw if they aren't compatible with a platform or package set + if !maybePkg.success || pkg == null then + [ + (lib.nameValuePair name ( + fake + // { + inherit exists; + broken = true; + } + )) + ] + else if !lib.isDerivation pkg then + if builtins.typeOf pkg != "set" then + # if it is not a package, ignore it (it is probably something like overrideAttrs) + [ ] + else + lib.flatten (lib.mapAttrsToList (name': _: getProperties ("${name}.${name'}")) pkg) else - lib.flatten (lib.mapAttrsToList (name': _: getProperties ("${name}.${name'}")) pkg) - else - lib.flip map pkg.outputs or [ "out" ] ( - output: let - # some packages are set to null if they aren't compatible with a platform or package set - maybePath = tryEval "${lib.getOutput output pkg}"; - broken = !exists || !maybePath.success; + maybePath = tryEval "${pkg}"; + broken = !maybePath.success; in - lib.nameValuePair (if output == "out" then name else "${name}.${output}") { - inherit exists broken; - path = if !broken then maybePath.value else null; - drvPath = if !broken then pkg.drvPath else null; - } - ); -in - -listToAttrs (concatMap getProperties attrs) + [ (lib.nameValuePair name (pkg // { inherit exists broken; })) ]; + in + listToAttrs (concatMap getProperties attrs) // { recurseForDerivations = true; } +) (fromJSON (readFile attr-json)) diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index 9687d5ab..3e14431d 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -68,32 +68,31 @@ def write_error_logs(attrs_per_system: dict[str, list[Attr]], directory: Path) - attr_name: str = f"{attr.name}-{system}" - if attr.path is not None and attr.path.exists(): + if not attr.broken: if attr.was_build(): - symlink_source = results.ensure().joinpath(attr_name) + symlink_folder = results.ensure() else: - symlink_source = failed_results.ensure().joinpath(attr_name) - if os.path.lexists(symlink_source): - symlink_source.unlink() - symlink_source.symlink_to(attr.path) - - for path in [f"{attr.drv_path}^*", attr.path]: - if not path: - continue - with logs.ensure().joinpath(attr_name + ".log").open("w+") as f: - nix_log = subprocess.run( - [ - "nix", - "--extra-experimental-features", - "nix-command", - "log", - path, - ], - stdout=f, - check=False, - ) - if nix_log.returncode == 0: - break + symlink_folder = failed_results.ensure() + + for name, path in attr.outputs_with_name().items(): + symlink_source = symlink_folder.joinpath(f"{name}-{system}") + + if os.path.lexists(symlink_source): + symlink_source.unlink() + symlink_source.symlink_to(path) + + with logs.ensure().joinpath(attr_name + ".log").open("w+") as f: + subprocess.run( + [ + "nix", + "--extra-experimental-features", + "nix-command", + "log", + f"{attr.drv_path}^*", + ], + stdout=f, + check=False, + ) def _serialize_attrs(attrs: list[Attr]) -> list[str]: diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index 99549c7a..997746d0 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -238,7 +238,6 @@ def build_commit( print("Local evaluation for computing rebuilds") - # TODO: nix-eval-jobs ? base_packages: dict[System, list[Package]] = list_packages( self.builddir.nix_path, self.systems, @@ -254,7 +253,6 @@ def build_commit( else: self.git_merge(reviewed_commit) - # TODO: nix-eval-jobs ? merged_packages: dict[System, list[Package]] = list_packages( self.builddir.nix_path, self.systems, @@ -301,6 +299,8 @@ def build( system, self.allow, self.builddir.nix_path, + self.num_parallel_evals, + self.max_memory_size, ) return nix_build( packages_per_system, @@ -312,6 +312,7 @@ def build( self.builddir.nix_path, self.nixpkgs_config, self.num_parallel_evals, + self.max_memory_size, ) def build_pr(self, pr_number: int) -> dict[System, list[Attr]]: @@ -496,7 +497,6 @@ def list_packages( f"builtins.mapAttrs (system: _: import {{ inherit system; }} // {{ recurseForDerivations = true; }}) {systems_str}", "--nix-path", nix_path, - "--show-trace", "--allow-import-from-derivation" if allow.ifd else "--no-allow-import-from-derivation", @@ -525,18 +525,27 @@ def package_attrs( system: str, allow: AllowedFeatures, nix_path: str, + num_parallel_evals: int, + max_memory_size: int, ignore_nonexisting: bool = True, ) -> dict[Path, Attr]: attrs: dict[Path, Attr] = {} nonexisting = [] - for attr in nix_eval(package_set, system, allow, nix_path): + for attr in nix_eval( + package_set, + system, + allow, + nix_path, + num_parallel_evals, + max_memory_size, + ): if not attr.exists: nonexisting.append(attr.name) elif not attr.broken: - assert attr.path is not None - attrs[attr.path] = attr + assert attr.drv_path is not None + attrs[attr.drv_path] = attr if not ignore_nonexisting and len(nonexisting) > 0: warn("These packages do not exist:") @@ -551,13 +560,19 @@ def join_packages( system: str, allow: AllowedFeatures, nix_path: str, + num_parallel_evals: int, + max_memory_size: int, ) -> set[str]: - changed_attrs = package_attrs(changed_packages, system, allow, nix_path) + changed_attrs = package_attrs( + changed_packages, system, allow, nix_path, num_parallel_evals, max_memory_size + ) specified_attrs = package_attrs( specified_packages, system, allow, nix_path, + num_parallel_evals, + max_memory_size, ignore_nonexisting=False, ) @@ -586,6 +601,8 @@ def filter_packages( system: str, allow: AllowedFeatures, nix_path: str, + num_parallel_evals: int, + max_memory_size: int, ) -> set[str]: packages: set[str] = set() assert isinstance(changed_packages, set) @@ -605,6 +622,8 @@ def filter_packages( system, allow, nix_path, + num_parallel_evals, + max_memory_size, ) for attr in changed_packages: From 72cc6d54cf19ea08cab132cb1b331c1c5280cfdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Wed, 26 Feb 2025 15:14:34 +0100 Subject: [PATCH 6/7] Perform nix_build without review shell Re-evaluating all attributes can significantly increase the memory consumption of `nix build` by several GB during the entire runtime of the process. Therefore, simply give the derivations that have already been evaluated to the process. --- nixpkgs_review/nix.py | 37 ++++++++++++------------------------- nixpkgs_review/review.py | 3 --- nixpkgs_review/utils.py | 6 ++++-- 3 files changed, 16 insertions(+), 30 deletions(-) diff --git a/nixpkgs_review/nix.py b/nixpkgs_review/nix.py index dddb13fa..218f795e 100644 --- a/nixpkgs_review/nix.py +++ b/nixpkgs_review/nix.py @@ -320,12 +320,9 @@ def multi_system_eval( 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, num_parallel_evals: int, max_memory_size: int, ) -> dict[System, list[Attr]]: @@ -341,30 +338,25 @@ def nix_build( 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": @@ -375,14 +367,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 diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index 997746d0..d53bac6a 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -305,12 +305,9 @@ def build( return nix_build( packages_per_system, args, - self.builddir.path, - self.local_system, self.allow, self.build_graph, self.builddir.nix_path, - self.nixpkgs_config, self.num_parallel_evals, self.max_memory_size, ) diff --git a/nixpkgs_review/utils.py b/nixpkgs_review/utils.py index 17d69593..d717728b 100644 --- a/nixpkgs_review/utils.py +++ b/nixpkgs_review/utils.py @@ -30,10 +30,12 @@ def wrapper(text: str) -> None: def sh( - command: list[str], cwd: Path | str | None = None + command: list[str], + cwd: Path | str | None = None, + input: str | None = None, # noqa: A002 ) -> "subprocess.CompletedProcess[str]": info("$ " + shlex.join(command)) - return subprocess.run(command, cwd=cwd, text=True, check=False) + return subprocess.run(command, cwd=cwd, text=True, check=False, input=input) def verify_commit_hash(commit: str) -> str: From 58b0c55353bc93444b9669d2d35353bed7852d65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Wed, 26 Feb 2025 15:37:59 +0100 Subject: [PATCH 7/7] Add flag to build package passthru.tests --- README.md | 4 ++++ nixpkgs_review/cli/__init__.py | 5 +++++ nixpkgs_review/cli/pr.py | 1 + nixpkgs_review/nix.py | 18 +++++++++++++++--- nixpkgs_review/nix/evalAttrs.nix | 23 +++++++++++++++++++++-- nixpkgs_review/review.py | 24 ++++++++++++++++++++++++ 6 files changed, 70 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 49142508..553a642a 100644 --- a/README.md +++ b/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 diff --git a/nixpkgs_review/cli/__init__.py b/nixpkgs_review/cli/__init__.py index d1adabce..555aefda 100644 --- a/nixpkgs_review/cli/__init__.py +++ b/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", diff --git a/nixpkgs_review/cli/pr.py b/nixpkgs_review/cli/pr.py index de40624b..39728790 100644 --- a/nixpkgs_review/cli/pr.py +++ b/nixpkgs_review/cli/pr.py @@ -110,6 +110,7 @@ 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, diff --git a/nixpkgs_review/nix.py b/nixpkgs_review/nix.py index 218f795e..d9f03145 100644 --- a/nixpkgs_review/nix.py +++ b/nixpkgs_review/nix.py @@ -50,7 +50,7 @@ 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: @@ -207,6 +207,12 @@ def _nix_eval_filter(json: list[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 props in json: @@ -222,7 +228,7 @@ def _nix_eval_filter(json: list[Any]) -> list[Attr]: name=name, exists=props["exists"], broken=props["broken"], - blacklisted=name in blacklist, + blacklisted=is_blacklisted(name), outputs=outputs, drv_path=drv_path, ) @@ -247,6 +253,7 @@ def nix_eval( nix_path: str, num_parallel_evals: int, max_memory_size: int, + include_passthru_tests: bool = False, ) -> list[Attr]: return multi_system_eval( {system: attrs}, @@ -254,6 +261,7 @@ def nix_eval( nix_path=nix_path, num_parallel_evals=num_parallel_evals, max_memory_size=max_memory_size, + include_passthru_tests=include_passthru_tests, ).get(system, []) @@ -263,6 +271,7 @@ def multi_system_eval( 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 @@ -282,7 +291,10 @@ def multi_system_eval( "--extra-experimental-features", "" if allow.url_literals else "no-url-literals", "--expr", - f"(import {eval_script} {{ attr-json = {attr_json.name}; }})", + f"""(import {eval_script} {{ + attr-json = {attr_json.name}; + include-passthru-tests = {str(include_passthru_tests).lower()}; + }})""", "--nix-path", nix_path, "--allow-import-from-derivation" diff --git a/nixpkgs_review/nix/evalAttrs.nix b/nixpkgs_review/nix/evalAttrs.nix index 36bef464..579d370b 100644 --- a/nixpkgs_review/nix/evalAttrs.nix +++ b/nixpkgs_review/nix/evalAttrs.nix @@ -1,4 +1,7 @@ -{ attr-json }: +{ + attr-json, + include-passthru-tests, +}: with builtins; mapAttrs ( @@ -50,7 +53,23 @@ mapAttrs ( maybePath = tryEval "${pkg}"; broken = !maybePath.success; in - [ (lib.nameValuePair name (pkg // { inherit exists broken; })) ]; + [ (lib.nameValuePair name (pkg // { inherit exists broken; })) ] + ++ lib.optionals include-passthru-tests ( + lib.flip lib.mapAttrsToList pkg.passthru.tests or { } ( + test: drv: + let + maybePath = tryEval drv.outPath; + broken = !maybePath.success || !lib.isDerivation drv; + in + lib.nameValuePair "${name}.passthru.tests.${test}" ( + drv + // { + inherit exists broken; + recurseForDerivations = false; # dont recurse into 'broken' tests + } + ) + ) + ); in listToAttrs (concatMap getProperties attrs) // { recurseForDerivations = true; } ) (fromJSON (readFile attr-json)) diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index d53bac6a..f4b25f4b 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -106,6 +106,7 @@ def __init__( skip_packages_regex: list[Pattern[str]] | None = None, checkout: CheckoutOption = CheckoutOption.MERGE, sandbox: bool = False, + build_passthru_tests: bool = False, num_parallel_evals: int = 1, max_memory_size: int = 4096, show_header: bool = True, @@ -146,6 +147,7 @@ def __init__( self.build_graph = build_graph self.nixpkgs_config = nixpkgs_config self.extra_nixpkgs_config = extra_nixpkgs_config + self.build_passthru_tests = build_passthru_tests self.num_parallel_evals = num_parallel_evals self.max_memory_size = max_memory_size self.show_header = show_header @@ -301,6 +303,7 @@ def build( self.builddir.nix_path, self.num_parallel_evals, self.max_memory_size, + self.build_passthru_tests, ) return nix_build( packages_per_system, @@ -525,6 +528,7 @@ def package_attrs( num_parallel_evals: int, max_memory_size: int, ignore_nonexisting: bool = True, + build_passthru_tests: bool = False, ) -> dict[Path, Attr]: attrs: dict[Path, Attr] = {} @@ -537,6 +541,7 @@ def package_attrs( nix_path, num_parallel_evals, max_memory_size, + build_passthru_tests, ): if not attr.exists: nonexisting.append(attr.name) @@ -559,6 +564,7 @@ def join_packages( nix_path: str, num_parallel_evals: int, max_memory_size: int, + build_passthru_tests: bool, ) -> set[str]: changed_attrs = package_attrs( changed_packages, system, allow, nix_path, num_parallel_evals, max_memory_size @@ -571,6 +577,7 @@ def join_packages( num_parallel_evals, max_memory_size, ignore_nonexisting=False, + build_passthru_tests=build_passthru_tests, ) # ofborg does not include tests and manual evaluation is too expensive @@ -600,10 +607,25 @@ def filter_packages( nix_path: str, num_parallel_evals: int, max_memory_size: int, + build_passthru_tests: bool, ) -> set[str]: packages: set[str] = set() assert isinstance(changed_packages, set) + # reduce number of times the passthru.tests are evaluated, by either doing + # it here or in join_packages + if build_passthru_tests and len(specified_packages) == 0: + changed_attrs = package_attrs( + changed_packages, + system, + allow, + nix_path, + num_parallel_evals, + max_memory_size, + build_passthru_tests=build_passthru_tests, + ) + changed_packages = {changed_attrs[path].name for path in changed_attrs} + if ( len(specified_packages) == 0 and len(package_regexes) == 0 @@ -621,6 +643,7 @@ def filter_packages( nix_path, num_parallel_evals, max_memory_size, + build_passthru_tests, ) for attr in changed_packages: @@ -718,6 +741,7 @@ def review_local_revision( 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, )