From 210faf516688a2c66ed7bac07dd072d34b4f0b11 Mon Sep 17 00:00:00 2001 From: Thiago Kenji Okada Date: Thu, 12 Dec 2024 17:33:40 +0000 Subject: [PATCH] nixos-rebuild-ng: change copy closure logic when copying from_host -> to_host - If user is using Nix 2.18+ (the default), we will switch to use `nix copy` since it supports using `--from` and `--to` at the same time - If user is using older versions of Nix, we will call `nix-copy-closure` twice, once with `--from` and another with `--to` The reason for this change is because the previous logic, that SSH'd to from_host to copy to to_host using `nix-copy-closure --to` means that `from_host` needs to have credentials to communicate with `to_host`, that is not always possible nor expected. It breaks even in simple cases like `--from` and `--to` is the same host because it is not common for a host to SSH'd itself. This also makes the behavior more consistent, since `--from` and `--to` is interpreted from the eval host point of view EXCEPT when they're used together. This may of course break in the cases where this old behavior was assumed, e.g.: deploying with a bastion host. I am not sure how common this was, and I think this change in behavior should enable more cases than break, but let's see. For that particular case, running `nixos-rebuild-ng` inside SSH command (e.g.: `ssh build_host -- nixos-rebuild-ng switch --target-host target_host`) should do the trick. --- pkgs/by-name/ni/nixos-rebuild-ng/package.nix | 8 +- .../src/nixos_rebuild/__init__.py | 14 ++-- .../src/nixos_rebuild/constants.py | 6 ++ .../nixos-rebuild-ng/src/nixos_rebuild/nix.py | 71 +++++++++++------ .../ni/nixos-rebuild-ng/src/tests/test_nix.py | 77 +++++++++++++------ 5 files changed, 120 insertions(+), 56 deletions(-) create mode 100644 pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/package.nix b/pkgs/by-name/ni/nixos-rebuild-ng/package.nix index 7dc84de9984e..6cd7eca28691 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/package.nix +++ b/pkgs/by-name/ni/nixos-rebuild-ng/package.nix @@ -19,6 +19,11 @@ }: let executable = if withNgSuffix then "nixos-rebuild-ng" else "nixos-rebuild"; + # This version is kind of arbitrary, we use some features that were + # implemented in newer versions of Nix, but not necessary 2.18. + # However, Lix is a fork of Nix 2.18, so this looks like a good version + # to cut specific functionality. + withNix218 = lib.versionAtLeast nix.version "2.18"; in python3Packages.buildPythonApplication rec { pname = "nixos-rebuild-ng"; @@ -53,8 +58,9 @@ python3Packages.buildPythonApplication rec { ]; postPatch = '' - substituteInPlace nixos_rebuild/__init__.py \ + substituteInPlace nixos_rebuild/constants.py \ --subst-var-by executable ${executable} \ + --subst-var-by withNix218 ${lib.boolToString withNix218} \ --subst-var-by withReexec ${lib.boolToString withReexec} \ --subst-var-by withShellFiles ${lib.boolToString withShellFiles} diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py index 49ba193a03ad..8ce9c9b2d7f0 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py @@ -9,6 +9,7 @@ from subprocess import CalledProcessError, run from typing import assert_never from . import nix +from .constants import EXECUTABLE, WITH_NIX_2_18, WITH_REEXEC, WITH_SHELL_FILES from .models import Action, BuildAttr, Flake, NRError, Profile from .process import Remote, cleanup_ssh from .utils import Args, LogFormatter @@ -16,12 +17,6 @@ from .utils import Args, LogFormatter logger = logging.getLogger() logger.setLevel(logging.INFO) -# Build-time flags -# Strings to avoid breaking standalone (e.g.: `python -m nixos_rebuild`) usage -EXECUTABLE = "@executable@" -WITH_REEXEC = "@withReexec@" -WITH_SHELL_FILES = "@withShellFiles@" - def get_parser() -> tuple[argparse.ArgumentParser, dict[str, argparse.ArgumentParser]]: common_flags = argparse.ArgumentParser(add_help=False) @@ -187,7 +182,7 @@ def parse_args( } if args.help or args.action is None: - if WITH_SHELL_FILES == "true": + if WITH_SHELL_FILES: r = run(["man", "8", EXECUTABLE], check=False) parser.exit(r.returncode) else: @@ -281,6 +276,9 @@ def reexec( def execute(argv: list[str]) -> None: args, args_groups = parse_args(argv) + if not WITH_NIX_2_18: + logger.warning("you're using Nix <2.18, some features will not work correctly") + atexit.register(cleanup_ssh) common_flags = vars(args_groups["common_flags"]) @@ -303,7 +301,7 @@ def execute(argv: list[str]) -> None: # Re-exec to a newer version of the script before building to ensure we get # the latest fixes if ( - WITH_REEXEC == "true" + WITH_REEXEC and can_run and not args.fast and not os.environ.get("_NIXOS_REBUILD_REEXEC") diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py new file mode 100644 index 000000000000..883c8b41b360 --- /dev/null +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py @@ -0,0 +1,6 @@ +# Build-time flags +# Strings to avoid breaking standalone (e.g.: `python -m nixos_rebuild`) usage +EXECUTABLE = "@executable@" +WITH_NIX_2_18 = "@withNix218@" == "true" # type: ignore +WITH_REEXEC = "@withReexec@" == "true" # type: ignore +WITH_SHELL_FILES = "@withShellFiles@" == "true" # type: ignore diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py index 3100496e2eee..bca9a5b12c71 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py @@ -7,6 +7,7 @@ from string import Template from subprocess import PIPE, CalledProcessError from typing import Final +from .constants import WITH_NIX_2_18 from .models import ( Action, BuildAttr, @@ -139,30 +140,56 @@ def copy_closure( """Copy a nix closure to or from host to localhost. Also supports copying a closure from a remote to another remote.""" - host = to_host or from_host - if not host: - return sshopts = os.getenv("NIX_SSHOPTS", "") - run_wrapper( - [ - "nix-copy-closure", - *dict_to_flags(copy_flags), - "--to" if to_host else "--from", - host.host, - closure, - ], - extra_env={ - # Using raw NIX_SSHOPTS here to avoid messing up with the passed - # parameters, and we do not add the SSH_DEFAULT_OPTS in the remote - # to remote case, otherwise it will fail because of ControlPath - # will not exist in remote - "NIX_SSHOPTS": sshopts - if from_host and to_host - else " ".join(filter(lambda x: x, [*SSH_DEFAULT_OPTS, sshopts])) - }, - remote=from_host if to_host else None, - ) + extra_env = { + "NIX_SSHOPTS": " ".join(filter(lambda x: x, [*SSH_DEFAULT_OPTS, sshopts])) + } + + def nix_copy_closure(host: Remote, to: bool) -> None: + run_wrapper( + [ + "nix-copy-closure", + *dict_to_flags(copy_flags), + "--to" if to else "--from", + host.host, + closure, + ], + extra_env=extra_env, + ) + + def nix_copy(to_host: Remote, from_host: Remote) -> None: + run_wrapper( + [ + "nix", + "copy", + "--from", + f"ssh://{from_host.host}", + "--to", + f"ssh://{to_host.host}", + closure, + ], + extra_env=extra_env, + ) + + match (to_host, from_host): + case (None, None): + return + case (Remote(_) as host, None) | (None, Remote(_) as host): + nix_copy_closure(host, to=bool(to_host)) + case (Remote(_), Remote(_)): + if WITH_NIX_2_18: + # With newer Nix, use `nix copy` instead of `nix-copy-closure` + # since it supports `--to` and `--from` at the same time + # TODO: once we drop Nix 2.3 from nixpkgs, remove support for + # `nix-copy-closure` + nix_copy(to_host, from_host) + else: + # With older Nix, we need to copy from to local and local to + # host. This means it is slower and need additional disk space + # in local + nix_copy_closure(from_host, to=False) + nix_copy_closure(to_host, to=True) def edit(flake: Flake | None, **flake_flags: Args) -> None: diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py index a5310a1df8a8..0e1a0ecdcc68 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py @@ -112,7 +112,6 @@ def test_remote_build(mock_run: Any, monkeypatch: Any) -> None: extra_env={ "NIX_SSHOPTS": " ".join(p.SSH_DEFAULT_OPTS + ["--ssh opts"]) }, - remote=None, ), call( ["nix-store", "--realise", Path("/path/to/file"), "--build"], @@ -166,7 +165,6 @@ def test_remote_build_flake(mock_run: Any, monkeypatch: Any) -> None: extra_env={ "NIX_SSHOPTS": " ".join(p.SSH_DEFAULT_OPTS + ["--ssh opts"]) }, - remote=None, ), call( [ @@ -185,37 +183,66 @@ def test_remote_build_flake(mock_run: Any, monkeypatch: Any) -> None: ) -@patch(get_qualified_name(n.run_wrapper, n), autospec=True) -def test_copy_closure(mock_run: Any, monkeypatch: Any) -> None: +def test_copy_closure(monkeypatch: Any) -> None: closure = Path("/path/to/closure") - n.copy_closure(closure, None) - mock_run.assert_not_called() + with patch(get_qualified_name(n.run_wrapper, n), autospec=True) as mock_run: + n.copy_closure(closure, None) + mock_run.assert_not_called() target_host = m.Remote("user@target.host", [], None) build_host = m.Remote("user@build.host", [], None) - - n.copy_closure(closure, target_host) - mock_run.assert_called_with( - ["nix-copy-closure", "--to", "user@target.host", closure], - extra_env={"NIX_SSHOPTS": " ".join(p.SSH_DEFAULT_OPTS)}, - remote=None, - ) + with patch(get_qualified_name(n.run_wrapper, n), autospec=True) as mock_run: + n.copy_closure(closure, target_host) + mock_run.assert_called_with( + ["nix-copy-closure", "--to", "user@target.host", closure], + extra_env={"NIX_SSHOPTS": " ".join(p.SSH_DEFAULT_OPTS)}, + ) monkeypatch.setenv("NIX_SSHOPTS", "--ssh build-opt") - n.copy_closure(closure, None, build_host) - mock_run.assert_called_with( - ["nix-copy-closure", "--from", "user@build.host", closure], - extra_env={"NIX_SSHOPTS": " ".join(p.SSH_DEFAULT_OPTS + ["--ssh build-opt"])}, - remote=None, - ) + with patch(get_qualified_name(n.run_wrapper, n), autospec=True) as mock_run: + n.copy_closure(closure, None, build_host) + mock_run.assert_called_with( + ["nix-copy-closure", "--from", "user@build.host", closure], + extra_env={ + "NIX_SSHOPTS": " ".join(p.SSH_DEFAULT_OPTS + ["--ssh build-opt"]) + }, + ) monkeypatch.setenv("NIX_SSHOPTS", "--ssh build-target-opt") - n.copy_closure(closure, target_host, build_host) - mock_run.assert_called_with( - ["nix-copy-closure", "--to", "user@target.host", closure], - remote=build_host, - extra_env={"NIX_SSHOPTS": "--ssh build-target-opt"}, - ) + monkeypatch.setattr(n, "WITH_NIX_2_18", True) + extra_env = { + "NIX_SSHOPTS": " ".join(p.SSH_DEFAULT_OPTS + ["--ssh build-target-opt"]) + } + with patch(get_qualified_name(n.run_wrapper, n), autospec=True) as mock_run: + n.copy_closure(closure, target_host, build_host) + mock_run.assert_called_with( + [ + "nix", + "copy", + "--from", + "ssh://user@build.host", + "--to", + "ssh://user@target.host", + closure, + ], + extra_env=extra_env, + ) + + monkeypatch.setattr(n, "WITH_NIX_2_18", False) + with patch(get_qualified_name(n.run_wrapper, n), autospec=True) as mock_run: + n.copy_closure(closure, target_host, build_host) + mock_run.assert_has_calls( + [ + call( + ["nix-copy-closure", "--from", "user@build.host", closure], + extra_env=extra_env, + ), + call( + ["nix-copy-closure", "--to", "user@target.host", closure], + extra_env=extra_env, + ), + ] + ) @patch(get_qualified_name(n.run_wrapper, n), autospec=True)