nixos-rebuild-ng: improve developer experience (#356468)

pkgs/by-name/ni/nixos-rebuild-ng/README.md

@@ -53,6 +53,38 @@ an attempt of the rewrite.
 
 And use `nixos-rebuild-ng` instead of `nixos-rebuild`.
 
+## Development
+
+Run:
+
+```console
+nix-build -A nixos-rebuild-ng.tests.ci
+```
+
+The command above will run the unit tests and linters, and also check if the
+code is formatted. However, sometimes is more convenient to run just a few
+tests to debug, in this case you can run:
+
+```console
+nix-shell -A nixos-rebuild-ng.devShell
+```
+
+The command above should automatically put you inside `src` directory, and you
+can run:
+
+```console
+# run program
+python -m nixos_rebuild
+# run tests
+python -m pytest
+# check types
+mypy .
+# fix lint issues
+ruff check --fix .
+# format code
+ruff format .
+```
+
 ## Current caveats
 
 - For now we will install it in `nixos-rebuild-ng` path by default, to avoid
@@ -96,4 +128,10 @@ And use `nixos-rebuild-ng` instead of `nixos-rebuild`.
 - [ ] Improve documentation
 - [ ] `nixos-rebuild repl` (calling old `nixos-rebuild` for now)
 - [ ] `nix` build/bootstrap
-- [ ] Reduce build closure
+- [ ] Generate tab completion via [`shtab`](https://docs.iterative.ai/shtab/)
+- [x] Reduce build closure
+
+## TODON'T
+
+- Reimplement `systemd-run` logic (will be moved to the new
+  [`apply`](https://github.com/NixOS/nixpkgs/pull/344407) script)

pkgs/by-name/ni/nixos-rebuild-ng/package.nix

@@ -1,24 +1,26 @@
 {
   lib,
   installShellFiles,
+  mkShell,
   nix,
   nixos-rebuild,
   python3,
+  python3Packages,
+  runCommand,
   withNgSuffix ? true,
 }:
-python3.pkgs.buildPythonApplication {
+python3Packages.buildPythonApplication rec {
   pname = "nixos-rebuild-ng";
   version = "0.0.0";
   src = ./src;
   pyproject = true;
 
-  build-system = with python3.pkgs; [
+  build-system = with python3Packages; [
     setuptools
   ];
 
-  dependencies = with python3.pkgs; [
+  dependencies = with python3Packages; [
     tabulate
-    types-tabulate
   ];
 
   nativeBuildInputs = [
@@ -53,22 +55,48 @@ python3.pkgs.buildPythonApplication {
       mv $out/bin/nixos-rebuild $out/bin/nixos-rebuild-ng
     '';
 
-  nativeCheckInputs = with python3.pkgs; [
+  nativeCheckInputs = with python3Packages; [
     pytestCheckHook
-    mypy
-    ruff
   ];
 
   pytestFlagsArray = [ "-vv" ];
 
-  postCheck = ''
-    echo -e "\x1b[32m## run mypy\x1b[0m"
-    mypy nixos_rebuild tests
-    echo -e "\x1b[32m## run ruff\x1b[0m"
-    ruff check nixos_rebuild tests
-    echo -e "\x1b[32m## run ruff format\x1b[0m"
-    ruff format --check nixos_rebuild tests
+  passthru =
+    let
+      python-with-pkgs = python3.withPackages (
+        ps: with ps; [
+          mypy
+          pytest
+          ruff
+          types-tabulate
+          # dependencies
+          tabulate
+        ]
+      );
+    in
+    {
+      devShell = mkShell {
+        packages = [ python-with-pkgs ];
+        shellHook = ''
+          cd pkgs/by-name/ni/nixos-rebuild-ng/src || true
         '';
+      };
+
+      # NOTE: this is a passthru test rather than a build-time test because we
+      # want to keep the build closures small
+      tests.ci = runCommand "${pname}-ci" { nativeBuildInputs = [ python-with-pkgs ]; } ''
+        export RUFF_CACHE_DIR="$(mktemp -d)"
+
+        echo -e "\x1b[32m## run mypy\x1b[0m"
+        mypy ${src}
+        echo -e "\x1b[32m## run ruff\x1b[0m"
+        ruff check ${src}
+        echo -e "\x1b[32m## run ruff format\x1b[0m"
+        ruff format --check ${src}
+
+        touch $out
+      '';
+    };
 
   meta = {
     description = "Rebuild your NixOS configuration and switch to it, on local hosts and remote";

pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py

@@ -7,11 +7,11 @@ import sys
 from subprocess import run
 from typing import assert_never
 
-from tabulate import tabulate
-
 from .models import Action, Flake, NRError, Profile
 from .nix import (
     edit,
+    find_file,
+    get_nixpkgs_rev,
     list_generations,
     nixos_build,
     nixos_build_flake,
@@ -88,7 +88,20 @@ def execute(argv: list[str]) -> None:
     if args.upgrade or args.upgrade_all:
         upgrade_channels(bool(args.upgrade_all))
 
-    match action := Action(args.action):
+    action = Action(args.action)
+    # Only run shell scripts from the Nixpkgs tree if the action is
+    # "switch", "boot", or "test". With other actions (such as "build"),
+    # the user may reasonably expect that no code from the Nixpkgs tree is
+    # executed, so it's safe to run nixos-rebuild against a potentially
+    # untrusted tree.
+    can_run = action in (Action.SWITCH, Action.BOOT, Action.TEST)
+    if can_run and not flake:
+        nixpkgs_path = find_file("nixpkgs", nix_flags)
+        rev = get_nixpkgs_rev(nixpkgs_path)
+        if nixpkgs_path and rev:
+            (nixpkgs_path / ".version-suffix").write_text(rev)
+
+    match action:
         case Action.SWITCH | Action.BOOT:
             info("building the system configuration...")
             if args.rollback:
@@ -177,6 +190,8 @@ def execute(argv: list[str]) -> None:
             if args.json:
                 print(json.dumps(generations, indent=2))
             else:
+                from tabulate import tabulate
+
                 headers = {
                     "generation": "Generation",
                     "date": "Build-date",
@@ -216,7 +231,3 @@ def main() -> None:
             raise ex
         else:
             sys.exit(str(ex))
-
-
-if __name__ == "__main__":
-    main()

pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__main__.py

@@ -0,0 +1,4 @@
+from . import main
+
+if __name__ == "__main__":
+    main()

pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/models.py

@@ -46,9 +46,7 @@ class Action(Enum):
 class Flake:
     path: Path
     attr: str
-    _re: ClassVar[re.Pattern[str]] = re.compile(
-        r"^(?P<path>[^\#]*)\#?(?P<attr>[^\#\"]*)$"
-    )
+    _re: ClassVar = re.compile(r"^(?P<path>[^\#]*)\#?(?P<attr>[^\#\"]*)$")
 
     @override
     def __str__(self) -> str:
@@ -59,11 +57,8 @@ class Flake:
         m = cls._re.match(flake_str)
         assert m is not None, f"got no matches for {flake_str}"
         attr = m.group("attr")
-        if not attr:
-            attr = f"nixosConfigurations.{hostname or "default"}"
-        else:
-            attr = f"nixosConfigurations.{attr}"
-        return Flake(Path(m.group("path")), attr)
+        nixos_attr = f"nixosConfigurations.{attr or hostname or "default"}"
+        return Flake(Path(m.group("path")), nixos_attr)
 
     @classmethod
     def from_arg(cls, flake_arg: Any) -> Flake | None:

pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import os
+import shutil
 from datetime import datetime
 from pathlib import Path
 from subprocess import PIPE, CalledProcessError, run
@@ -14,7 +15,7 @@ from .models import (
     NRError,
     Profile,
 )
-from .utils import dict_to_flags
+from .utils import dict_to_flags, info
 
 FLAKE_FLAGS: Final = ["--extra-experimental-features", "nix-command flakes"]
 
@@ -48,6 +49,49 @@ def edit(flake: Flake | None, nix_flags: list[str] | None = None) -> None:
             raise NRError("cannot find NixOS config file")
 
 
+def find_file(file: str, nix_flags: list[str] | None = None) -> Path | None:
+    "Find classic Nixpkgs location."
+    r = run(
+        ["nix-instantiate", "--find-file", file, *(nix_flags or [])],
+        stdout=PIPE,
+        check=False,
+        text=True,
+    )
+    if r.returncode:
+        return None
+    return Path(r.stdout.strip())
+
+
+def get_nixpkgs_rev(nixpkgs_path: Path | None) -> str | None:
+    """Get Nixpkgs path as a Git revision.
+
+    Can be used to generate `.version-suffix` file."""
+    if not nixpkgs_path:
+        return None
+
+    # Git is not included in the closure for nixos-rebuild so we need to check
+    if not shutil.which("git"):
+        info(f"warning: Git not found; cannot figure out revision of '{nixpkgs_path}'")
+        return None
+
+    # Get current revision
+    r = run(
+        ["git", "-C", nixpkgs_path, "rev-parse", "--short", "HEAD"],
+        check=False,
+        stdout=PIPE,
+        text=True,
+    )
+    rev = r.stdout.strip()
+
+    if rev:
+        # Check if repo is dirty
+        if run(["git", "-C", nixpkgs_path, "diff", "--quiet"], check=False).returncode:
+            rev += "M"
+        return f".git.{rev}"
+    else:
+        return None
+
+
 def _parse_generation_from_nix_store(path: Path, profile: Profile) -> Generation:
     entry_id = path.name.split("-")[1]
     current = path.name == profile.path.readlink().name

pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_main.py

@@ -67,10 +67,17 @@ def test_parse_args() -> None:
 
 
 @patch(get_qualified_name(nr.nix.run, nr.nix), autospec=True)
-def test_execute_nix_boot(mock_run: Any, tmp_path: Path) -> None:
+@patch(get_qualified_name(nr.nix.shutil.which), autospec=True, return_value="/bin/git")
+def test_execute_nix_boot(mock_which: Any, mock_run: Any, tmp_path: Path) -> None:
+    nixpkgs_path = tmp_path / "nixpkgs"
+    nixpkgs_path.mkdir()
     config_path = tmp_path / "test"
     config_path.touch()
     mock_run.side_effect = [
+        # update_nixpkgs_rev
+        CompletedProcess([], 0, str(nixpkgs_path)),
+        CompletedProcess([], 0, "nixpkgs-rev"),
+        CompletedProcess([], 0),
         # nixos_build
         CompletedProcess([], 0, str(config_path)),
         # set_profile
@@ -82,9 +89,25 @@ def test_execute_nix_boot(mock_run: Any, tmp_path: Path) -> None:
     nr.execute(["nixos-rebuild", "boot", "--no-flake", "-vvv"])
 
     assert nr.VERBOSE is True
-    assert mock_run.call_count == 3
+    assert mock_run.call_count == 6
     mock_run.assert_has_calls(
         [
+            call(
+                ["nix-instantiate", "--find-file", "nixpkgs", "-vvv"],
+                stdout=PIPE,
+                check=False,
+                text=True,
+            ),
+            call(
+                ["git", "-C", nixpkgs_path, "rev-parse", "--short", "HEAD"],
+                check=False,
+                stdout=PIPE,
+                text=True,
+            ),
+            call(
+                ["git", "-C", nixpkgs_path, "diff", "--quiet"],
+                check=False,
+            ),
             call(
                 [
                     "nix-build",
@@ -180,11 +203,15 @@ def test_execute_nix_switch_flake(mock_run: Any, tmp_path: Path) -> None:
 
 
 @patch(get_qualified_name(nr.nix.run, nr.nix), autospec=True)
-def test_execute_switch_rollback(mock_run: Any) -> None:
+def test_execute_switch_rollback(mock_run: Any, tmp_path: Path) -> None:
+    nixpkgs_path = tmp_path / "nixpkgs"
+    nixpkgs_path.touch()
+
     nr.execute(["nixos-rebuild", "switch", "--rollback", "--install-bootloader"])
 
     assert nr.VERBOSE is False
-    assert mock_run.call_count == 2
+    assert mock_run.call_count == 3
+    # ignoring update_nixpkgs_rev calls
     mock_run.assert_has_calls(
         [
             call(

pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py

@@ -42,6 +42,61 @@ def test_edit(mock_run: Any, monkeypatch: Any, tmpdir: Any) -> None:
         mock_run.assert_called_with(["editor", default_nix], check=False)
 
 
+@patch(get_qualified_name(n.shutil.which), autospec=True, return_value="/bin/git")
+def test_get_nixpkgs_rev(mock_which: Any) -> None:
+    assert n.get_nixpkgs_rev(None) is None
+
+    path = Path("/path/to/nix")
+
+    with patch(
+        get_qualified_name(n.run, n),
+        autospec=True,
+        side_effect=[CompletedProcess([], 0, "")],
+    ) as mock_run:
+        assert n.get_nixpkgs_rev(path) is None
+        mock_run.assert_called_with(
+            ["git", "-C", path, "rev-parse", "--short", "HEAD"],
+            check=False,
+            stdout=PIPE,
+            text=True,
+        )
+
+    expected_calls = [
+        call(
+            ["git", "-C", path, "rev-parse", "--short", "HEAD"],
+            check=False,
+            stdout=PIPE,
+            text=True,
+        ),
+        call(
+            ["git", "-C", path, "diff", "--quiet"],
+            check=False,
+        ),
+    ]
+
+    with patch(
+        get_qualified_name(n.run, n),
+        autospec=True,
+        side_effect=[
+            CompletedProcess([], 0, "0f7c82403fd6"),
+            CompletedProcess([], returncode=0),
+        ],
+    ) as mock_run:
+        assert n.get_nixpkgs_rev(path) == ".git.0f7c82403fd6"
+        mock_run.assert_has_calls(expected_calls)
+
+    with patch(
+        get_qualified_name(n.run, n),
+        autospec=True,
+        side_effect=[
+            CompletedProcess([], 0, "0f7c82403fd6"),
+            CompletedProcess([], returncode=1),
+        ],
+    ) as mock_run:
+        assert n.get_nixpkgs_rev(path) == ".git.0f7c82403fd6M"
+        mock_run.assert_has_calls(expected_calls)
+
+
 def test_get_generations_from_nix_store(tmp_path: Path) -> None:
     nixos_path = tmp_path / "nixos-system"
     nixos_path.mkdir()