From 6e083fb0d957faf526ff3d5305ed5ab21bf679c0 Mon Sep 17 00:00:00 2001 From: Yueh-Shun Li Date: Wed, 19 Aug 2026 05:41:55 +0800 Subject: [PATCH 1/4] Add --store passthrough for building in an alternative Nix store Reviews always built into the ambient /nix/store. This is a prerequisite for isolating experimental builds (e.g. ca-derivations, which has known store-corruption bugs) from the user's main store. --store is passed through to every store-touching nix call and is a no-op when unset. The review shell warns but still attempts to launch with a custom store, since built binaries won't be at the real /nix/store path. Assisted-by: claude-code: claude-opus-4.8, claude-sonnet-4.6 --- nixpkgs_review/cli/__init__.py | 8 ++++++ nixpkgs_review/nix.py | 38 ++++++++++++++++++++++---- nixpkgs_review/report.py | 2 ++ nixpkgs_review/review.py | 2 ++ tests/test_store_flag.py | 50 ++++++++++++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 tests/test_store_flag.py diff --git a/nixpkgs_review/cli/__init__.py b/nixpkgs_review/cli/__init__.py index 492a0350..cfc8adba 100644 --- a/nixpkgs_review/cli/__init__.py +++ b/nixpkgs_review/cli/__init__.py @@ -321,6 +321,14 @@ def common_flags() -> list[CommonFlag]: default=None, help="Alternative package set to use for building, e.g. pkgsMusl, pkgsStatic, or pkgsCross.aarch64-multiplatform", ), + CommonFlag( + "--store", + type=str, + default=None, + help="Nix store to build in, passed verbatim to nix as `--store` " + "(e.g. `local?root=/tmp/review-store`). Defaults to the ambient store. " + "Useful for isolating risky/experimental builds from your main store.", + ), ] diff --git a/nixpkgs_review/nix.py b/nixpkgs_review/nix.py index c2c41f43..e8346d03 100644 --- a/nixpkgs_review/nix.py +++ b/nixpkgs_review/nix.py @@ -29,6 +29,7 @@ class BuildConfig: num_eval_workers: int = 1 max_memory_size: int = 4096 pkgs: str | None = None + store: str | None = None @dataclass @@ -40,6 +41,7 @@ class Attr: outputs: dict[str, Path] | None drv_path: Path | None aliases: list[str] = field(default_factory=list) + store: str | None = None _path_verified: bool | None = field(init=False, default=None) def was_build(self) -> bool: @@ -54,6 +56,7 @@ def was_build(self) -> bool: "nix", "--extra-experimental-features", "nix-command", + *_store_flags(self.store), "store", "verify", "--no-contents", @@ -86,7 +89,13 @@ def serialize(self) -> dict: REVIEW_SHELL: Final[str] = str(ROOT.joinpath("nix/review-shell.nix")) -def _nix_common_flags(allow: AllowedFeatures, nix_path: str) -> list[str]: +def _store_flags(store: str | None) -> list[str]: + return ["--store", store] if store else [] + + +def _nix_common_flags( + allow: AllowedFeatures, nix_path: str, store: str | None = None +) -> list[str]: return [ "--extra-experimental-features", "nix-command", @@ -96,6 +105,7 @@ def _nix_common_flags(allow: AllowedFeatures, nix_path: str) -> list[str]: "--allow-import-from-derivation" if allow.ifd else "--no-allow-import-from-derivation", + *_store_flags(store), ] @@ -112,6 +122,7 @@ class ShellConfig: run: str | None = None sandbox: bool = False pkgs: str | None = None + store: str | None = None def nix_shell( @@ -131,6 +142,12 @@ def nix_shell( nixpkgs_config=config.nixpkgs_config, pkgs=config.pkgs, ) + if config.store: + warn( + "Using a non-default --store: the review shell may fail to launch or " + "run binaries built into it, since they are not at the real /nix/store. " + "Consider --no-shell, or `nix copy` the results to the default store first." + ) if config.sandbox: with TemporaryDirectory(prefix="nixpkgs-review-links-") as dirname: bin_link = Path(dirname) / bin_name @@ -145,6 +162,7 @@ def nix_shell( *shell_file_args, "--nix-path", config.nix_path, + *_store_flags(config.store), REVIEW_SHELL, ] if config.run: @@ -240,6 +258,7 @@ def tmpfs(path: Path | str, *, is_dir: bool = True) -> list[str]: *shell_file_args, "--nix-path", config.nix_path, + *_store_flags(config.store), REVIEW_SHELL, ] @@ -259,7 +278,7 @@ class NixEvalPropsExtra(TypedDict): NixEvalResult = list[NixEvalProps] -def _nix_eval_filter(packages: NixEvalResult) -> list[Attr]: +def _nix_eval_filter(packages: NixEvalResult, store: str | None) -> list[Attr]: # workaround https://github.com/NixOS/ofborg/issues/269 blacklist = { "appimage-run-tests", @@ -293,6 +312,7 @@ def _nix_eval_filter(packages: NixEvalResult) -> list[Attr]: blacklisted=name in blacklist, outputs=outputs, drv_path=drv_path, + store=store, ) if attr.drv_path is not None: if (other := attr_by_path.get(attr.drv_path)) is None: @@ -327,7 +347,9 @@ def multi_system_eval( "--max-memory-size", str(build_config.max_memory_size), "--no-instantiate", - *_nix_common_flags(build_config.allow, build_config.nix_path), + *_nix_common_flags( + build_config.allow, build_config.nix_path, build_config.store + ), "--expr", f"(import {eval_script} {{ attr-json = {attr_json.name}; }})", "--apply", @@ -361,7 +383,7 @@ def multi_system_eval( systems_packages[system].append(eval_result) return { - system: _nix_eval_filter(packages) + system: _nix_eval_filter(packages, build_config.store) for system, packages in systems_packages.items() } finally: @@ -399,7 +421,9 @@ def nix_build( "build", "--file", REVIEW_SHELL, - *_nix_common_flags(build_config.allow, build_config.nix_path), + *_nix_common_flags( + build_config.allow, build_config.nix_path, build_config.store + ), "--no-link", "--keep-going", ] @@ -424,6 +448,7 @@ def nix_build( shell_file_args=shell_file_args, allow=build_config.allow, nix_path=build_config.nix_path, + store=build_config.store, ) command += shell_file_args + shlex.split(args) @@ -471,12 +496,13 @@ def _write_review_shell_drv( shell_file_args: list[str], allow: AllowedFeatures, nix_path: str, + store: str | None = None, ) -> None: review_drv_link: Path = cache_directory / "review-shell.drv" cmd: list[str] = [ "nix-instantiate", - *_nix_common_flags(allow, nix_path), + *_nix_common_flags(allow, nix_path, store), *shell_file_args, REVIEW_SHELL, ] diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index 52d9f3f2..994e2338 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -214,12 +214,14 @@ def _write_log_for_attr( logs: LazyDirectory, extra_nix_log_args: list[str], ) -> None: + store_flags = ["--store", attr.store] if attr.store else [] with logs.ensure().joinpath(get_log_filename(attr, system)).open("w+") as f: subprocess.run( [ "nix", "--extra-experimental-features", "nix-command", + *store_flags, "log", f"{attr.drv_path}^*", *extra_nix_log_args, diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index dadae06d..7bec829a 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -687,6 +687,7 @@ def start_review( run=self.shell_options.run, sandbox=self.shell_options.sandbox, pkgs=self.build_config.pkgs, + store=self.build_config.store, ) nix_shell(report.built_packages(), shell_config) @@ -1018,6 +1019,7 @@ def build_config_from_args( num_eval_workers=args.num_eval_workers, max_memory_size=args.max_memory_size, pkgs=args.pkgs, + store=args.store, ) diff --git a/tests/test_store_flag.py b/tests/test_store_flag.py new file mode 100644 index 00000000..f10b063e --- /dev/null +++ b/tests/test_store_flag.py @@ -0,0 +1,50 @@ +from __future__ import annotations + +from pathlib import Path + +from nixpkgs_review.allow import AllowedFeatures +from nixpkgs_review.cli import parse_args +from nixpkgs_review.nix import BuildConfig, _nix_common_flags, _store_flags +from nixpkgs_review.review import build_config_from_args + + +def test_store_flag_defaults_to_none() -> None: + args = parse_args("nixpkgs-review", ["rev", "HEAD"]) + assert args.store is None + + +def test_store_flag_parsed_for_pr_rev_wip() -> None: + for subcommand, extra in ( + ("pr", ["1"]), + ("rev", ["HEAD"]), + ("wip", []), + ): + args = parse_args( + "nixpkgs-review", [subcommand, *extra, "--store", "local?root=/tmp/x"] + ) + assert args.store == "local?root=/tmp/x" + + +def test_store_flags_helper() -> None: + assert _store_flags(None) == [] + assert _store_flags("local?root=/tmp/x") == ["--store", "local?root=/tmp/x"] + + +def test_nix_common_flags_includes_store_when_set() -> None: + allow = AllowedFeatures([]) + flags = _nix_common_flags(allow, "", "local?root=/tmp/x") + assert flags[-2:] == ["--store", "local?root=/tmp/x"] + + +def test_nix_common_flags_omits_store_when_unset() -> None: + allow = AllowedFeatures([]) + flags = _nix_common_flags(allow, "") + assert "--store" not in flags + + +def test_build_config_from_args_carries_store() -> None: + args = parse_args("nixpkgs-review", ["rev", "HEAD", "--store", "local?root=/tmp/x"]) + build_config: BuildConfig = build_config_from_args( + args, AllowedFeatures([]), nix_path="", nixpkgs_config=Path("/dev/null") + ) + assert build_config.store == "local?root=/tmp/x" From ed05157833f66947b2d2c3564dc357809005ba47 Mon Sep 17 00:00:00 2001 From: Yueh-Shun Li Date: Wed, 19 Aug 2026 06:26:21 +0800 Subject: [PATCH 2/4] Add --eval-store passthrough, decoupled from --store Remote/slow build stores (e.g. nixbuild.net) recommend keeping evaluation on a local store while builds go elsewhere. --store alone couldn't express that split. --eval-store is passed through wherever evaluation happens (eval, instantiate, nix build's own eval, the review shell); pure store queries (verify, log) are unaffected since they don't evaluate. Assisted-by: claude-code: claude-opus-4.8, claude-sonnet-4.6 --- nixpkgs_review/cli/__init__.py | 8 +++++++ nixpkgs_review/nix.py | 30 ++++++++++++++++++++++----- nixpkgs_review/review.py | 2 ++ tests/test_store_flag.py | 38 ++++++++++++++++++++++++++++++++-- 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/nixpkgs_review/cli/__init__.py b/nixpkgs_review/cli/__init__.py index cfc8adba..9914acfe 100644 --- a/nixpkgs_review/cli/__init__.py +++ b/nixpkgs_review/cli/__init__.py @@ -329,6 +329,14 @@ def common_flags() -> list[CommonFlag]: "(e.g. `local?root=/tmp/review-store`). Defaults to the ambient store. " "Useful for isolating risky/experimental builds from your main store.", ), + CommonFlag( + "--eval-store", + type=str, + default=None, + help="Store to write evaluation artifacts (drvs, IFD results) to, " + "passed verbatim to nix as `--eval-store` (e.g. `auto`). Useful with " + "a remote/slow --store (e.g. nixbuild.net) to keep evaluation local.", + ), ] diff --git a/nixpkgs_review/nix.py b/nixpkgs_review/nix.py index e8346d03..1ee0e680 100644 --- a/nixpkgs_review/nix.py +++ b/nixpkgs_review/nix.py @@ -30,6 +30,7 @@ class BuildConfig: max_memory_size: int = 4096 pkgs: str | None = None store: str | None = None + eval_store: str | None = None @dataclass @@ -93,8 +94,15 @@ def _store_flags(store: str | None) -> list[str]: return ["--store", store] if store else [] +def _eval_store_flags(eval_store: str | None) -> list[str]: + return ["--eval-store", eval_store] if eval_store else [] + + def _nix_common_flags( - allow: AllowedFeatures, nix_path: str, store: str | None = None + allow: AllowedFeatures, + nix_path: str, + store: str | None = None, + eval_store: str | None = None, ) -> list[str]: return [ "--extra-experimental-features", @@ -106,6 +114,7 @@ def _nix_common_flags( if allow.ifd else "--no-allow-import-from-derivation", *_store_flags(store), + *_eval_store_flags(eval_store), ] @@ -123,6 +132,7 @@ class ShellConfig: sandbox: bool = False pkgs: str | None = None store: str | None = None + eval_store: str | None = None def nix_shell( @@ -163,6 +173,7 @@ def nix_shell( "--nix-path", config.nix_path, *_store_flags(config.store), + *_eval_store_flags(config.eval_store), REVIEW_SHELL, ] if config.run: @@ -259,6 +270,7 @@ def tmpfs(path: Path | str, *, is_dir: bool = True) -> list[str]: "--nix-path", config.nix_path, *_store_flags(config.store), + *_eval_store_flags(config.eval_store), REVIEW_SHELL, ] @@ -348,7 +360,10 @@ def multi_system_eval( str(build_config.max_memory_size), "--no-instantiate", *_nix_common_flags( - build_config.allow, build_config.nix_path, build_config.store + build_config.allow, + build_config.nix_path, + build_config.store, + build_config.eval_store, ), "--expr", f"(import {eval_script} {{ attr-json = {attr_json.name}; }})", @@ -422,7 +437,10 @@ def nix_build( "--file", REVIEW_SHELL, *_nix_common_flags( - build_config.allow, build_config.nix_path, build_config.store + build_config.allow, + build_config.nix_path, + build_config.store, + build_config.eval_store, ), "--no-link", "--keep-going", @@ -449,6 +467,7 @@ def nix_build( allow=build_config.allow, nix_path=build_config.nix_path, store=build_config.store, + eval_store=build_config.eval_store, ) command += shell_file_args + shlex.split(args) @@ -491,18 +510,19 @@ def build_shell_file_args( ] -def _write_review_shell_drv( +def _write_review_shell_drv( # noqa: PLR0913 cache_directory: Path, shell_file_args: list[str], allow: AllowedFeatures, nix_path: str, store: str | None = None, + eval_store: str | None = None, ) -> None: review_drv_link: Path = cache_directory / "review-shell.drv" cmd: list[str] = [ "nix-instantiate", - *_nix_common_flags(allow, nix_path, store), + *_nix_common_flags(allow, nix_path, store, eval_store), *shell_file_args, REVIEW_SHELL, ] diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index 7bec829a..e22622ef 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -688,6 +688,7 @@ def start_review( sandbox=self.shell_options.sandbox, pkgs=self.build_config.pkgs, store=self.build_config.store, + eval_store=self.build_config.eval_store, ) nix_shell(report.built_packages(), shell_config) @@ -1020,6 +1021,7 @@ def build_config_from_args( max_memory_size=args.max_memory_size, pkgs=args.pkgs, store=args.store, + eval_store=args.eval_store, ) diff --git a/tests/test_store_flag.py b/tests/test_store_flag.py index f10b063e..f9771f8b 100644 --- a/tests/test_store_flag.py +++ b/tests/test_store_flag.py @@ -4,13 +4,19 @@ from nixpkgs_review.allow import AllowedFeatures from nixpkgs_review.cli import parse_args -from nixpkgs_review.nix import BuildConfig, _nix_common_flags, _store_flags +from nixpkgs_review.nix import ( + BuildConfig, + _eval_store_flags, + _nix_common_flags, + _store_flags, +) from nixpkgs_review.review import build_config_from_args def test_store_flag_defaults_to_none() -> None: args = parse_args("nixpkgs-review", ["rev", "HEAD"]) assert args.store is None + assert args.eval_store is None def test_store_flag_parsed_for_pr_rev_wip() -> None: @@ -25,26 +31,54 @@ def test_store_flag_parsed_for_pr_rev_wip() -> None: assert args.store == "local?root=/tmp/x" +def test_eval_store_flag_parsed_for_pr_rev_wip() -> None: + for subcommand, extra in ( + ("pr", ["1"]), + ("rev", ["HEAD"]), + ("wip", []), + ): + args = parse_args( + "nixpkgs-review", [subcommand, *extra, "--eval-store", "auto"] + ) + assert args.eval_store == "auto" + + def test_store_flags_helper() -> None: assert _store_flags(None) == [] assert _store_flags("local?root=/tmp/x") == ["--store", "local?root=/tmp/x"] +def test_eval_store_flags_helper() -> None: + assert _eval_store_flags(None) == [] + assert _eval_store_flags("auto") == ["--eval-store", "auto"] + + def test_nix_common_flags_includes_store_when_set() -> None: allow = AllowedFeatures([]) flags = _nix_common_flags(allow, "", "local?root=/tmp/x") assert flags[-2:] == ["--store", "local?root=/tmp/x"] +def test_nix_common_flags_includes_eval_store_when_set() -> None: + allow = AllowedFeatures([]) + flags = _nix_common_flags(allow, "", "local?root=/tmp/x", "auto") + assert flags[-4:] == ["--store", "local?root=/tmp/x", "--eval-store", "auto"] + + def test_nix_common_flags_omits_store_when_unset() -> None: allow = AllowedFeatures([]) flags = _nix_common_flags(allow, "") assert "--store" not in flags + assert "--eval-store" not in flags def test_build_config_from_args_carries_store() -> None: - args = parse_args("nixpkgs-review", ["rev", "HEAD", "--store", "local?root=/tmp/x"]) + args = parse_args( + "nixpkgs-review", + ["rev", "HEAD", "--store", "local?root=/tmp/x", "--eval-store", "auto"], + ) build_config: BuildConfig = build_config_from_args( args, AllowedFeatures([]), nix_path="", nixpkgs_config=Path("/dev/null") ) assert build_config.store == "local?root=/tmp/x" + assert build_config.eval_store == "auto" From 2ae3fd5dcfd9af81d39e4d327ac7417b0dc73129 Mon Sep 17 00:00:00 2001 From: Yueh-Shun Li Date: Wed, 19 Aug 2026 16:13:50 +0800 Subject: [PATCH 3/4] test: mock current_system() in store-flag test The test called build_config_from_args() directly. That shells out to a real `nix eval` for current_system(), with no environment isolation, unlike tests that use the helpers.nixpkgs() fixture. This passed in an interactive dev shell. It failed in a Nix build sandbox: no writable profile dir, so `nix eval` itself errors out. The test only cares about --store/--eval-store threading. Mock current_system() instead of depending on ambient Nix state. Assisted-by: claude-code: claude-opus-4.8, claude-sonnet-4.6 --- tests/test_store_flag.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/test_store_flag.py b/tests/test_store_flag.py index f9771f8b..31c09b82 100644 --- a/tests/test_store_flag.py +++ b/tests/test_store_flag.py @@ -1,6 +1,7 @@ from __future__ import annotations from pathlib import Path +from unittest.mock import MagicMock, patch from nixpkgs_review.allow import AllowedFeatures from nixpkgs_review.cli import parse_args @@ -72,7 +73,11 @@ def test_nix_common_flags_omits_store_when_unset() -> None: assert "--eval-store" not in flags -def test_build_config_from_args_carries_store() -> None: +@patch("nixpkgs_review.review.current_system", return_value="x86_64-linux") +def test_build_config_from_args_carries_store(mock_current_system: MagicMock) -> None: + # current_system() shells out to `nix eval`; mocked so this test doesn't + # depend on ambient Nix state (e.g. a writable profile dir), which isn't + # available in a Nix build sandbox. args = parse_args( "nixpkgs-review", ["rev", "HEAD", "--store", "local?root=/tmp/x", "--eval-store", "auto"], @@ -82,3 +87,4 @@ def test_build_config_from_args_carries_store() -> None: ) assert build_config.store == "local?root=/tmp/x" assert build_config.eval_store == "auto" + assert build_config.local_system == mock_current_system.return_value From 77f5aabfbc612c59b3fbc9e0569c377874e6cd8a Mon Sep 17 00:00:00 2001 From: Yueh-Shun Li Date: Wed, 19 Aug 2026 16:56:01 +0800 Subject: [PATCH 4/4] fix: pass --store/--eval-store to nix-env rebuild-diff eval list_packages()'s nix-env call computed base-vs-merged output paths without --store/--eval-store. Harmless without --allow ifd, but with IFD enabled it could build into the real /nix/store despite an isolated --store being set. Assisted-by: claude-code: claude-opus-4.8, claude-sonnet-4.6 --- nixpkgs_review/review.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index e22622ef..d73116bd 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -420,6 +420,8 @@ def build_commit( self.systems, self.build_config.allow, self.build_config.pkgs, + self.build_config.store, + self.build_config.eval_store, ) if head_commit is None: @@ -434,6 +436,8 @@ def build_commit( self.systems, self.build_config.allow, self.build_config.pkgs, + self.build_config.store, + self.build_config.eval_store, check_meta=True, ) @@ -766,11 +770,13 @@ def parse_packages_xml(stdout: IO[str]) -> list[Package]: return packages -def _list_packages_system( +def _list_packages_system( # noqa: PLR0913 system: System, nix_path: str, allow: AllowedFeatures, pkgs: str | None = None, + store: str | None = None, + eval_store: str | None = None, *, check_meta: bool = False, ) -> list[Package]: @@ -791,6 +797,8 @@ def _list_packages_system( "--allow-import-from-derivation" if allow.ifd else "--no-allow-import-from-derivation", + *(["--store", store] if store else []), + *(["--eval-store", eval_store] if eval_store else []), *(["-A", pkgs] if pkgs else []), ] if check_meta: @@ -806,11 +814,13 @@ def _list_packages_system( return parse_packages_xml(f) -def list_packages( +def list_packages( # noqa: PLR0913 nix_path: str, systems: set[System], allow: AllowedFeatures, pkgs: str | None = None, + store: str | None = None, + eval_store: str | None = None, *, check_meta: bool = False, ) -> dict[System, list[Package]]: @@ -822,6 +832,8 @@ def list_packages( allow=allow, check_meta=check_meta, pkgs=pkgs, + store=store, + eval_store=eval_store, ) return results