Skip to content

nixos-rebuild-ng: fix switch inside a path symlinked to nix store #417191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
thiagokokada wants to merge 2 commits into from
Closed

nixos-rebuild-ng: fix switch inside a path symlinked to nix store #417191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96bc121
nixos-rebuild-ng: fix switch inside a path symlinked to nix store
thiagokokada Jun 16, 2025
616d7ff
nixos-rebuild-ng: simplify `Flake.parse()`
thiagokokada Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 36 additions & 73 deletions pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from dataclasses import dataclass
from enum import Enum
from pathlib import Path
from typing import Any, Callable, ClassVar, Self, TypedDict, override
from typing import Any, ClassVar, Self, TypedDict, override

from .process import Remote, run_wrapper

Expand Down Expand Up @@ -61,43 +61,18 @@ def from_arg(cls, attr: str | None, file: str | None) -> Self:
return cls(Path(file or "default.nix"), attr)


def discover_git(location: Path) -> Path | None:
"""
Discover the current git repository in the given location.
"""
current = location.resolve()
previous = None

while current.is_dir() and current != previous:
dotgit = current / ".git"
if dotgit.is_dir():
return current
elif dotgit.is_file(): # this is a worktree
with dotgit.open() as f:
dotgit_content = f.read().strip()
if dotgit_content.startswith("gitdir: "):
return Path(dotgit_content.split("gitdir: ")[1])
previous = current
current = current.parent

return None


def discover_closest_flake(location: Path) -> Path | None:
"""
Discover the closest flake.nix file starting from the given location upwards.
"""
current = location.resolve()
previous = None

while current.is_dir() and current != previous:
flake_file = current / "flake.nix"
if flake_file.is_file():
return current
previous = current
current = current.parent

return None
def _get_hostname(target_host: Remote | None) -> str | None:
if target_host:
try:
return run_wrapper(
["uname", "-n"],
stdout=subprocess.PIPE,
remote=target_host,
).stdout.strip()
except (AttributeError, subprocess.CalledProcessError):
return None
else:
return platform.node()


@dataclass(frozen=True)
Expand All @@ -114,53 +89,41 @@ def __str__(self) -> str:
return f"{self.path}#{self.attr}"

@classmethod
def parse(
cls,
flake_str: str,
hostname_fn: Callable[[], str | None] = lambda: None,
) -> Self:
def parse(cls, flake_str: str, target_host: Remote | None = None) -> Self:
Copy link
Copy Markdown
Contributor Author

@thiagokokada thiagokokada Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: sorry for adding a refactor for a supposedly revertion PR only but this code was really bothering me.

I understand why I wrote it that way (I wanted to make hostname computation to be lazy since it can be somewhat expensive, especially in the --target-host case), but I ended up creating a code that can only be summed up as "it was probably wrote by a drunk person", and I don't even drink alcohol.

If reviewing the changes is difficult, please review by commit, it should be easier to understand what is going on.

m = cls._re.match(flake_str)
assert m is not None, f"got no matches for {flake_str}"
attr = m.group("attr")
nixos_attr = f'nixosConfigurations."{attr or hostname_fn() or "default"}"'
nixos_attr = (
f'nixosConfigurations."{attr or _get_hostname(target_host) or "default"}"'
)
path_str = m.group("path")
if ":" in path_str:
return cls(path_str, nixos_attr)
else:
path = Path(path_str)
git_repo = discover_git(path)
if git_repo is not None:
url = f"git+file://{git_repo}"
flake_path = discover_closest_flake(path)
if (
flake_path is not None
and flake_path != git_repo
and flake_path.is_relative_to(git_repo)
):
url += f"?dir={flake_path.relative_to(git_repo)}"
return cls(url, nixos_attr)
return cls(path, nixos_attr)
# Since we use builtins.getFlake we have behavior differences
# between normal nix build and the nix repl because
# builtins.getFlake won't pick up local flakes as git+file
# but assumes path:// flakes instead.
# This can have surprising effects such as beeing able to access
# untracked files that would lead to build failures otherwise
# or copying large files to the nix store.
# We could force git+file:// protocol to fix this, but thanks to
# another issue in nix this causes a nasty issue where
# nixos-rebuild is run inside a symlink to a nix store path
# (e.g. /run/opengl-driver/lib).
# See:
# - https://github.com/NixOS/nixpkgs/pull/375493
# - https://github.com/NixOS/nixpkgs/pull/410498
# - https://github.com/NixOS/nixpkgs/issues/93694
return cls(Path(path_str), nixos_attr)

@classmethod
def from_arg(cls, flake_arg: Any, target_host: Remote | None) -> Self | None:
def get_hostname() -> str | None:
if target_host:
try:
return run_wrapper(
["uname", "-n"],
stdout=subprocess.PIPE,
remote=target_host,
).stdout.strip()
except (AttributeError, subprocess.CalledProcessError):
return None
else:
return platform.node()

match flake_arg:
case str(s):
return cls.parse(s, get_hostname)
return cls.parse(s, target_host)
case True:
return cls.parse(".", get_hostname)
return cls.parse(".", target_host)
case False:
return None
case _:
Expand All @@ -169,7 +132,7 @@ def get_hostname() -> str | None:
if default_path.exists():
# It can be a symlink to the actual flake.
default_path = default_path.resolve()
return cls.parse(str(default_path.parent), get_hostname)
return cls.parse(str(default_path.parent), target_host)
else:
return None

Expand Down
81 changes: 19 additions & 62 deletions pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
from pathlib import Path
from unittest.mock import Mock, patch

from pytest import MonkeyPatch

import nixos_rebuild.models as m

from .helpers import get_qualified_name
Expand All @@ -30,45 +28,32 @@ def test_build_attr_to_attr() -> None:
)


def test_flake_parse(tmpdir: Path, monkeypatch: MonkeyPatch) -> None:
@patch("platform.node", autospec=True, return_value="hostname")
def test_flake_parse(mock_node: Mock) -> None:
assert m.Flake.parse("/path/to/flake#attr") == m.Flake(
Path("/path/to/flake"), 'nixosConfigurations."attr"'
)
assert m.Flake.parse("/path/ to /flake", lambda: "hostname") == m.Flake(
assert m.Flake.parse("/path/ to /flake") == m.Flake(
Path("/path/ to /flake"), 'nixosConfigurations."hostname"'
)
assert m.Flake.parse("/path/to/flake", lambda: "hostname") == m.Flake(
assert m.Flake.parse("/path/to/flake") == m.Flake(
Path("/path/to/flake"), 'nixosConfigurations."hostname"'
)
# change directory to tmpdir
with monkeypatch.context() as patch_context:
patch_context.chdir(tmpdir)
assert m.Flake.parse(".#attr") == m.Flake(
Path("."), 'nixosConfigurations."attr"'
)
assert m.Flake.parse("#attr") == m.Flake(
Path("."), 'nixosConfigurations."attr"'
)
assert m.Flake.parse(".") == m.Flake(Path("."), 'nixosConfigurations."default"')
assert m.Flake.parse("path:/to/flake#attr") == m.Flake(
"path:/to/flake", 'nixosConfigurations."attr"'
)
assert m.Flake.parse("github:user/repo/branch") == m.Flake(
"github:user/repo/branch", 'nixosConfigurations."default"'
)
git_root = tmpdir / "git_root"
git_root.mkdir()
(git_root / ".git").mkdir()
assert m.Flake.parse(str(git_root)) == m.Flake(
f"git+file://{git_root}", 'nixosConfigurations."default"'
)

work_tree = tmpdir / "work_tree"
work_tree.mkdir()
(work_tree / ".git").write_text("gitdir: /path/to/git", "utf-8")
assert m.Flake.parse(str(work_tree)) == m.Flake(
"git+file:///path/to/git", 'nixosConfigurations."default"'
"github:user/repo/branch", 'nixosConfigurations."hostname"'
)
with patch(
get_qualified_name(m.run_wrapper, m),
autospec=True,
return_value=subprocess.CompletedProcess([], 0, stdout="remote\n"),
):
target_host = m.Remote("remote", [], None)
assert m.Flake.parse("/path/to/flake", target_host) == m.Flake(
Path("/path/to/flake"), 'nixosConfigurations."remote"'
)


def test_flake_to_attr() -> None:
Expand All @@ -80,12 +65,8 @@ def test_flake_to_attr() -> None:
)


@patch("platform.node", autospec=True)
def test_flake_from_arg(
mock_node: Mock, monkeypatch: MonkeyPatch, tmpdir: Path
) -> None:
mock_node.return_value = "hostname"

@patch("platform.node", autospec=True, return_value="hostname")
def test_flake_from_arg(mock_node: Mock) -> None:
# Flake string
assert m.Flake.from_arg("/path/to/flake#attr", None) == m.Flake(
Path("/path/to/flake"), 'nixosConfigurations."attr"'
Expand All @@ -95,11 +76,9 @@ def test_flake_from_arg(
assert m.Flake.from_arg(False, None) is None

# True
with monkeypatch.context() as patch_context:
patch_context.chdir(tmpdir)
assert m.Flake.from_arg(True, None) == m.Flake(
Path("."), 'nixosConfigurations."hostname"'
)
assert m.Flake.from_arg(True, None) == m.Flake(
Path("."), 'nixosConfigurations."hostname"'
)

# None when we do not have /etc/nixos/flake.nix
with patch(
Expand All @@ -109,28 +88,6 @@ def test_flake_from_arg(
):
assert m.Flake.from_arg(None, None) is None

# None when we have a file in /etc/nixos/flake.nix
with (
patch(
"pathlib.Path.exists",
autospec=True,
return_value=True,
),
patch(
"pathlib.Path.is_symlink",
autospec=True,
return_value=False,
),
patch(
get_qualified_name(m.discover_git),
autospec=True,
return_value="/etc/nixos",
),
):
assert m.Flake.from_arg(None, None) == m.Flake(
"git+file:///etc/nixos", 'nixosConfigurations."hostname"'
)

with (
patch(
"pathlib.Path.exists",
Expand Down
12 changes: 4 additions & 8 deletions pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ def test_build(mock_run: Mock) -> None:
autospec=True,
return_value=CompletedProcess([], 0, stdout=" \n/path/to/file\n "),
)
def test_build_flake(mock_run: Mock, monkeypatch: MonkeyPatch, tmpdir: Path) -> None:
monkeypatch.chdir(tmpdir)
def test_build_flake(mock_run: Mock) -> None:
flake = m.Flake.parse(".#hostname")

assert n.build_flake(
Expand Down Expand Up @@ -193,10 +192,7 @@ def run_wrapper_side_effect(
autospec=True,
return_value=CompletedProcess([], 0, stdout=" \n/path/to/file\n "),
)
def test_build_remote_flake(
mock_run: Mock, monkeypatch: MonkeyPatch, tmpdir: Path
) -> None:
monkeypatch.chdir(tmpdir)
def test_build_remote_flake(mock_run: Mock, monkeypatch: MonkeyPatch) -> None:
flake = m.Flake.parse(".#hostname")
build_host = m.Remote("user@host", [], None)
monkeypatch.setenv("NIX_SSHOPTS", "--ssh opts")
Expand Down Expand Up @@ -318,7 +314,7 @@ def test_copy_closure(monkeypatch: MonkeyPatch) -> None:
@patch(get_qualified_name(n.run_wrapper, n), autospec=True)
def test_edit(mock_run: Mock, monkeypatch: MonkeyPatch, tmpdir: Path) -> None:
# Flake
flake = m.Flake.parse(f"{tmpdir}#attr")
flake = m.Flake.parse("#attr")
n.edit(flake, {"commit_lock_file": True})
mock_run.assert_called_with(
[
Expand All @@ -328,7 +324,7 @@ def test_edit(mock_run: Mock, monkeypatch: MonkeyPatch, tmpdir: Path) -> None:
"edit",
"--commit-lock-file",
"--",
f'{tmpdir}#nixosConfigurations."attr"',
'.#nixosConfigurations."attr"',
],
check=False,
)
Expand Down
Loading