From 3b75185d4302e98f019dd043b57d015dc5e99d05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 12 Jun 2025 17:40:11 +0200 Subject: [PATCH 01/14] optimise-store: Fix race condition in temporary link creation Replace rand() with atomic counter for unique temp link names and add retry loop to handle EEXIST errors when multiple processes optimize the store simultaneously. --- src/libstore/optimise-store.cc | 73 ++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 8073ee41bd7..4e92a197feb 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -234,38 +234,53 @@ void LocalStore::optimisePath_( its timestamp back to 0. */ MakeReadOnly makeReadOnly(mustToggle ? dirOfPath : ""); - std::filesystem::path tempLink = fmt("%1%/.tmp-link-%2%-%3%", config->realStoreDir, getpid(), rand()); - - try { - std::filesystem::create_hard_link(linkPath, tempLink); - inodeHash.insert(st.st_ino); - } catch (std::filesystem::filesystem_error & e) { - if (e.code() == std::errc::too_many_links) { - /* Too many links to the same file (>= 32000 on most file - systems). This is likely to happen with empty files. - Just shrug and ignore. */ - if (st.st_size) - printInfo("'%1%' has maximum number of links", linkPath); - return; + /* Retry loop for temporary link creation to handle race conditions */ + while (true) { + Path tempLink = makeTempPath(config->realStoreDir.get(), ".tmp-link"); + + try { + std::filesystem::create_hard_link(linkPath, tempLink); + inodeHash.insert(st.st_ino); + } catch (std::filesystem::filesystem_error & e) { + if (e.code() == std::errc::too_many_links) { + /* Too many links to the same file (>= 32000 on most file + systems). This is likely to happen with empty files. + Just shrug and ignore. */ + if (st.st_size) + printInfo("'%1%' has maximum number of links", linkPath); + return; + } + throw SysError("creating temporary link '%1%'", tempLink); } - throw; - } - /* Atomically replace the old file with the new hard link. */ - try { - std::filesystem::rename(tempLink, path); - } catch (std::filesystem::filesystem_error & e) { - std::filesystem::remove(tempLink); - printError("unable to unlink '%1%'", tempLink); - if (e.code() == std::errc::too_many_links) { - /* Some filesystems generate too many links on the rename, - rather than on the original link. (Probably it - temporarily increases the st_nlink field before - decreasing it again.) */ - debug("'%s' has reached maximum number of links", linkPath); - return; + /* Atomically replace the old file with the new hard link. */ + try { + std::filesystem::rename(tempLink, path); + break; /* Success! */ + } catch (std::filesystem::filesystem_error & e) { + try { + std::filesystem::remove(tempLink); + } catch (...) { + /* Ignore errors removing the temp link */ + } + + if (e.code() == std::errc::too_many_links) { + /* Some filesystems generate too many links on the rename, + rather than on the original link. (Probably it + temporarily increases the st_nlink field before + decreasing it again.) */ + debug("'%s' has reached maximum number of links", linkPath); + return; + } + + if (e.code() == std::errc::file_exists) { + /* Race condition: another process created the same temp link. + Try again with a different name. */ + continue; + } + + throw SysError("renaming temporary link '%1%' to '%2%'", tempLink, path); } - throw; } stats.filesLinked++; From 2acb2966c3ca3c218e5705827b0ca5e3534c2938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 12 Jun 2025 17:40:30 +0200 Subject: [PATCH 02/14] indirect-root-store: Fix race condition in makeSymlink Replace rand() with atomic counter and add retry loop to handle race conditions when creating temporary symlinks for garbage collector roots. --- src/libstore/indirect-root-store.cc | 31 ++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/libstore/indirect-root-store.cc b/src/libstore/indirect-root-store.cc index b882b2568a4..30a8b6d7bc0 100644 --- a/src/libstore/indirect-root-store.cc +++ b/src/libstore/indirect-root-store.cc @@ -1,4 +1,5 @@ #include "nix/store/indirect-root-store.hh" +#include "nix/util/file-system.hh" namespace nix { @@ -7,12 +8,32 @@ void IndirectRootStore::makeSymlink(const Path & link, const Path & target) /* Create directories up to `gcRoot'. */ createDirs(dirOf(link)); - /* Create the new symlink. */ - Path tempLink = fmt("%1%.tmp-%2%-%3%", link, getpid(), rand()); - createSymlink(target, tempLink); + /* Retry loop for temporary symlink creation to handle race conditions */ + while (true) { + Path tempLink = makeTempPath(dirOf(link), baseNameOf(link) + ".tmp"); - /* Atomically replace the old one. */ - std::filesystem::rename(tempLink, link); + createSymlink(target, tempLink); + + /* Atomically replace the old one. */ + try { + std::filesystem::rename(tempLink, link); + break; /* Success! */ + } catch (std::filesystem::filesystem_error & e) { + try { + std::filesystem::remove(tempLink); + } catch (...) { + /* Ignore errors removing the temp link */ + } + + if (e.code() == std::errc::file_exists) { + /* Race condition: another process created the same temp link. + Try again with a different name. */ + continue; + } + + throw SysError("failed to create symlink '%1%' -> '%2%'", link, target); + } + } } Path IndirectRootStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot) From b8ad8b1f3d47c7b3e047f57726b0ff95e405d06f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 12 Jun 2025 17:40:54 +0200 Subject: [PATCH 03/14] local-store: Fix potential race in build log creation Add atomic counter to temporary log file names and retry loop to handle EEXIST errors. If another process creates the log file first, we gracefully exit since the log already exists. --- src/libstore/local-store.cc | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 49c499e3fe4..11696e9bc73 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -1649,11 +1650,35 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log) createDirs(dirOf(logPath)); - auto tmpFile = fmt("%s.tmp.%d", logPath, getpid()); + /* Retry loop for temporary log file creation to handle race conditions */ + while (true) { + auto tmpFile = makeTempPath(dirOf(logPath), baseNameOf(logPath) + ".tmp"); - writeFile(tmpFile, compress("bzip2", log)); + try { + writeFile(tmpFile, compress("bzip2", log)); + } catch (Error & e) { + e.addTrace({}, "writing build log to '%s'", tmpFile); + throw; + } + + try { + std::filesystem::rename(tmpFile, logPath); + break; /* Success! */ + } catch (std::filesystem::filesystem_error & e) { + try { + std::filesystem::remove(tmpFile); + } catch (...) { + /* Ignore errors removing the temp file */ + } + + if (e.code() == std::errc::file_exists) { + /* Another process created the log file. That's fine, we're done. */ + break; + } - std::filesystem::rename(tmpFile, logPath); + throw SysError("renaming temporary file '%1%' to '%2%'", tmpFile, logPath); + } + } } std::optional LocalStore::getVersion() From 536780a35d979436acc16263fe69138c492f07c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 12 Jun 2025 17:41:13 +0200 Subject: [PATCH 04/14] local-binary-cache-store: Add retry loop to upsertFile Handle race conditions in binary cache file creation by retrying with a new temporary name on EEXIST errors. Uses existing makeTempPath which already has an atomic counter. --- src/libstore/local-binary-cache-store.cc | 33 +++++++++++++++++++----- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index f7511fdce0a..43c744022a2 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -53,13 +53,32 @@ struct LocalBinaryCacheStore : virtual BinaryCacheStore const std::string & mimeType) override { auto path2 = config->binaryCacheDir + "/" + path; - static std::atomic counter{0}; - Path tmp = fmt("%s.tmp.%d.%d", path2, getpid(), ++counter); - AutoDelete del(tmp, false); - StreamToSourceAdapter source(istream); - writeFile(tmp, source); - std::filesystem::rename(tmp, path2); - del.cancel(); + + /* Retry loop for handling race conditions */ + while (true) { + Path tmp = makeTempPath(dirOf(path2), baseNameOf(path2) + ".tmp"); + AutoDelete del(tmp, false); + + StreamToSourceAdapter source(istream); + try { + writeFile(tmp, source); + } catch (Error & e) { + e.addTrace({}, "while writing to temporary file '%s' for '%s'", tmp, path2); + } + + try { + std::filesystem::rename(tmp, path2); + del.cancel(); + break; /* Success! */ + } catch (std::filesystem::filesystem_error & e) { + if (e.code() == std::errc::file_exists) { + /* Race condition: another process created the same file. + Try again with a different name. */ + continue; + } + throw SysError("renaming '%s' to '%s'", tmp, path2); + } + } } void getFile(const std::string & path, Sink & sink) override From 9e4b2d2da27b5b8325e425859633a9003dafaa28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Fri, 13 Sep 2024 11:08:47 +0200 Subject: [PATCH 05/14] ci(Mergify): configuration update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörg Thalheim --- .mergify.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.mergify.yml b/.mergify.yml index f49144113da..efb469366e1 100644 --- a/.mergify.yml +++ b/.mergify.yml @@ -7,6 +7,7 @@ queue_rules: - check-success=installer test on macos - check-success=installer test on ubuntu - check-success=vm_tests + - check-success=buildbot/nix-build batch_size: 5 pull_request_rules: From 4ccfc396d5da61bbac51307d2d6474c8c76a9f85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Wed, 23 Oct 2024 18:42:08 +0200 Subject: [PATCH 06/14] doc/manual: improve remove_before_wrapper wrapper * type hints * use argparse * use pathlib * use TemporaryDirectory --- doc/manual/remove_before_wrapper.py | 40 ++++++++++++++--------------- 1 file changed, 19 insertions(+), 21 deletions(-) mode change 100644 => 100755 doc/manual/remove_before_wrapper.py diff --git a/doc/manual/remove_before_wrapper.py b/doc/manual/remove_before_wrapper.py old mode 100644 new mode 100755 index 6da4c19b0ce..b61645a96a7 --- a/doc/manual/remove_before_wrapper.py +++ b/doc/manual/remove_before_wrapper.py @@ -1,33 +1,31 @@ #!/usr/bin/env python3 -import os -import subprocess -import sys +import argparse import shutil -import typing as t +import subprocess +from pathlib import Path +from tempfile import TemporaryDirectory + -def main(): - if len(sys.argv) < 4 or '--' not in sys.argv: - print("Usage: remove-before-wrapper -- ") - sys.exit(1) +def main() -> None: + arg_parser = argparse.ArgumentParser(description="Remove before wrapper") + arg_parser.add_argument("output", type=Path, help="Output file") + arg_parser.add_argument("nix_command", nargs=argparse.REMAINDER, help="Nix command") + args = arg_parser.parse_args() - # Extract the parts - output: str = sys.argv[1] - nix_command_idx: int = sys.argv.index('--') + 1 - nix_command: t.List[str] = sys.argv[nix_command_idx:] + output = Path(args.output) + with TemporaryDirectory(prefix=str(output.parent.resolve() / "tmp")) as temp: + output_temp = Path(temp) / "output" - output_temp: str = output + '.tmp' + # Remove the output output in case it exist + shutil.rmtree(output, ignore_errors=True) - # Remove the output and temp output in case they exist - shutil.rmtree(output, ignore_errors=True) - shutil.rmtree(output_temp, ignore_errors=True) + # Execute nix command with `--write-to` tempary output + subprocess.run([*args.nix_command, "--write-to", output_temp], check=True) - # Execute nix command with `--write-to` tempary output - nix_command_write_to = nix_command + ['--write-to', output_temp] - subprocess.run(nix_command_write_to, check=True) + # Move the temporary output to the intended location + Path(output_temp).rename(output) - # Move the temporary output to the intended location - os.rename(output_temp, output) if __name__ == "__main__": main() From d3242d27d80999c14233445ebca398b8815b4c38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Mon, 30 Sep 2024 10:26:50 +0200 Subject: [PATCH 07/14] add new upload-release script This introduces a new release script for nix-based releases. Compared to the old version this improves the following aspects: - fully type checked - no longer depends on beeing executed on eelco's work station - no dependency on the third-party libraries compared to the old version (aws, LWP, json parser etc.) - self-test and dry run mode to test releases without having specific access to release buckets or docker registry. The results of a release can be visually inspected and debugged locally. - improved error handling and logging - ci-friendly: all dependencies are managed by nix and locked by the local flake - no dependency on an external docker daemon by using podman - more arguments and documented help to publish releases in different buckets/registry --- maintainers/upload-release.py | 585 ++++++++++++++++++++++++++++++++++ pyproject.toml | 25 ++ 2 files changed, 610 insertions(+) create mode 100755 maintainers/upload-release.py create mode 100644 pyproject.toml diff --git a/maintainers/upload-release.py b/maintainers/upload-release.py new file mode 100755 index 00000000000..dd23c5a27f2 --- /dev/null +++ b/maintainers/upload-release.py @@ -0,0 +1,585 @@ +#!/usr/bin/env nix +#! nix shell --inputs-from .# nixpkgs#bashInteractive nixpkgs#python3 nixpkgs#awscli2 nixpkgs#nix nixpkgs#podman --command python3 +import argparse +import json +import logging +import os +import shlex +import shutil +import socket +import subprocess +import sys +import time +import urllib.request +from collections.abc import Iterator +from contextlib import contextmanager +from dataclasses import dataclass, field +from pathlib import Path +from tempfile import TemporaryDirectory +from typing import Any + +SCRIPT_DIR = Path(__file__).resolve().parent + +logger = logging.getLogger("upload-release") + + +@dataclass +class Options: + eval_id: int + aws_region: str + aws_endpoint: str + release_bucket: str + channels_bucket: str + is_latest: bool + docker_owner: str + docker_authfile: Path + dry_run: bool + self_test: bool + project_root: Path + self_test_registry_port: int = 5000 + self_test_minio_port: int = 9000 + eval_url: str = field(init=False) + + def __post_init__(self) -> None: + self.eval_url = f"https://hydra.nixos.org/eval/{self.eval_id}" + + +@dataclass +class Platform: + job_name: str + can_fail: bool = False + + +class Error(Exception): + pass + + +def fetch_json(url: str) -> Any: + request = urllib.request.Request(url) + request.add_header("Accept", "application/json") + logger.info(f"fetching {url}") + with urllib.request.urlopen(request) as response: + return json.load(response) + + +def get_store_path(eval_url: str, job_name: str, output: str = "out") -> str: + build_info = fetch_json(f"{eval_url}/job/{job_name}") + path = build_info["buildoutputs"].get(output, {}).get("path", None) + if not path: + msg = f"job '{job_name}' lacks output '{output}'" + raise Error(msg) + return path + + +def run( + command: list[str], + check: bool = True, + dry_run: bool = False, + **kwargs: Any, +) -> subprocess.CompletedProcess: + logger.info(f"run {shlex.join(command)}") + if dry_run: + return subprocess.CompletedProcess(args=command, returncode=0, stdout=b"", stderr=b"") + return subprocess.run(command, check=check, text=True, **kwargs) + + +def copy_manual( + options: Options, + tmp_dir: Path, + release_name: str, + binary_cache: str, + release_dir: str, +) -> None: + try: + manual = get_store_path(options.eval_url, "build.nix.x86_64-linux", "doc") + except Exception: + logger.exception("Failed to get manual path") + return + + logger.info(f"Manual: {manual}") + + manual_nar = tmp_dir / f"{release_name}-manual.nar.xz" + logger.info(manual_nar) + + if not manual_nar.exists(): + tmp = manual_nar.with_suffix(".xz.tmp") + env = os.environ.copy() + env["NIX_REMOTE"] = binary_cache + run(["bash", "-c", "nix store dump-path $1 | xz > $2", "", manual, str(tmp)], env=env) + tmp.rename(manual_nar) + + manual_dir = tmp_dir / "manual" + if not manual_dir.exists(): + tmp = manual_dir.with_suffix(".tmp") + run(["bash", "-c", "xz -d < $1 | nix-store --restore $2", "", str(manual_nar), str(tmp)]) + (tmp / "share" / "doc" / "nix" / "manual").rename(manual_dir) + shutil.rmtree(manual_dir.parent / "manual.tmp") + + run( + [ + "aws", + "--endpoint", + options.aws_endpoint, + "s3", + "sync", + str(manual_dir), + f"s3://{options.release_bucket}/{release_dir}/manual", + ], + ) + + +def download_file( + eval_url: str, + binary_cache: str, + tmp_dir: Path, + job_name: str, + dst_name: str | None = None, +) -> str | None: + build_info = fetch_json(f"{eval_url}/job/{job_name}") + src_file = build_info["buildproducts"]["1"].get("path", None) + if not src_file: + msg = f"job '{job_name}' lacks product '1'" + raise Error(msg) + dst_name = dst_name or Path(src_file).name + tmp_file = tmp_dir / dst_name + + if not tmp_file.exists(): + logger.info(f"downloading {src_file} to {tmp_file}...") + file_info = json.loads( + run( + ["nix", "store", "ls", "--json", src_file], + env={"NIX_REMOTE": binary_cache}, + stdout=subprocess.PIPE, + ).stdout, + ) + if file_info["type"] == "symlink": + src_file = file_info["target"] + with tmp_file.with_suffix(".tmp").open("wb") as f: + run( + ["nix", "store", "cat", src_file], + env={"NIX_REMOTE": binary_cache}, + stdout=f, + ) + tmp_file.with_suffix(".tmp").rename(tmp_file) + + sha256_expected = build_info["buildproducts"]["1"]["sha256hash"] + sha256_actual = run( + ["nix", "hash", "file", "--base16", "--type", "sha256", str(tmp_file)], + stdout=subprocess.PIPE, + ).stdout.strip() + if sha256_expected and sha256_expected != sha256_actual: + msg = f"file {tmp_file} is corrupt, got {sha256_actual}, expected {sha256_expected}" + raise Error(msg) + + tmp_file.with_suffix(".sha256").write_text(sha256_actual) + return sha256_expected + + +def update_container_images( + options: Options, + binary_cache: str, + tmp_dir: Path, + release_dir: Path, + version: str, +) -> None: + docker_manifest = [] + docker_manifest_latest = [] + have_docker = False + podman_home = tmp_dir / "podman" + auth_file = podman_home / ".config" / "containers" / "auth.json" + auth_file.parent.mkdir(parents=True, exist_ok=True) + if options.docker_authfile.exists(): + shutil.copy(options.docker_authfile, auth_file) + policy = podman_home / ".config" / "containers" / "policy.json" + policy.parent.mkdir(parents=True, exist_ok=True) + policy.write_text( + json.dumps( + { + "default": [ + {"type": "insecureAcceptAnything"}, + ], + }, + ), + ) + if options.self_test: + registry = podman_home / ".config" / "containers" / "registries.conf" + registry.parent.mkdir(parents=True, exist_ok=True) + registry.write_text( + f""" + [[registry]] + location = "localhost:{options.self_test_registry_port}" + insecure = true + """, + ) + + docker_platforms = [ + ("x86_64-linux", "amd64"), + ("aarch64-linux", "arm64"), + ] + + def podman(command: list[str], dry_run: bool = False) -> subprocess.CompletedProcess: + env = os.environ.copy() + env["HOME"] = str(podman_home) + return run(["podman", *command], env=env, dry_run=dry_run) + + for system, docker_platform in docker_platforms: + file_name = f"nix-{version}-docker-image-{docker_platform}.tar.gz" + try: + download_file(options.eval_url, binary_cache, release_dir, f"dockerImage.{system}", file_name) + except subprocess.CalledProcessError: + logger.exception(f"Failed to build for {docker_platform}") + continue + + have_docker = True + logger.info(f"loading docker image for {docker_platform}...") + podman(["load", "-i", str(release_dir / file_name)]) + + tag = f"{options.docker_owner}/nix:{version}-{docker_platform}" + latest_tag = f"{options.docker_owner}/nix:latest-{docker_platform}" + + logger.info(f"tagging {version} docker image for {docker_platform}...") + podman(["tag", f"nix:{version}", tag]) + logger.info(f"pushing {version} docker image for {docker_platform}...") + podman(["push", "-q", tag], dry_run=options.dry_run) + + if options.is_latest: + logger.info(f"tagging latest docker image for {docker_platform}...") + podman(["tag", f"nix:{version}", latest_tag]) + logger.info(f"pushing latest docker image for {docker_platform}...") + podman(["push", "-q", latest_tag], dry_run=options.dry_run) + + docker_manifest += ["--amend", tag] + docker_manifest_latest += ["--amend", latest_tag] + + if not have_docker: + return + + logger.info("creating multi-platform docker manifest...") + podman(["manifest", "create", f"{options.docker_owner}/nix:{version}", *docker_manifest]) + if options.is_latest: + logger.info("creating latest multi-platform docker manifest...") + podman(["manifest", "create", f"{options.docker_owner}/nix:latest", *docker_manifest_latest]) + + logger.info("pushing multi-platform docker manifest...") + podman(["manifest", "push", f"{options.docker_owner}/nix:{version}"], dry_run=options.dry_run) + + if options.is_latest: + logger.info("pushing latest multi-platform docker manifest...") + podman(["manifest", "push", f"{options.docker_owner}/nix:latest"], dry_run=options.dry_run) + + +def upload_release(options: Options, tmp_dir: Path) -> None: + # check if git diff is clean + if run(["git", "diff", "--quiet"]).returncode != 0: + msg = "Git diff is not clean. Please commit or stash your changes or set --project-root to a clean directory." + raise Error(msg) + + nar_cache = tmp_dir / "nar-cache" + nar_cache.mkdir(parents=True, exist_ok=True) + binary_cache = f"https://cache.nixos.org/?local-nar-cache={nar_cache}" + + try: + run(["aws", "--endpoint", options.aws_endpoint, "s3", "ls", f"s3://{options.release_bucket}"]) + except subprocess.CalledProcessError as e: + msg = "Cannot access release buckets. Check your AWS credentials." + raise Error(msg) from e + + eval_info = fetch_json(options.eval_url) + flake_url = eval_info.get("flake") + flake_info = json.loads(run(["nix", "flake", "metadata", "--json", flake_url], stdout=subprocess.PIPE).stdout) + nix_rev = flake_info["locked"]["rev"] + + build_info = fetch_json(f"{options.eval_url}/job/build.nix.x86_64-linux") + release_name = build_info["nixname"] + release_dir = tmp_dir / "nix" / release_name + release_dir.mkdir(parents=True, exist_ok=True) + version = release_name.split("-")[-1] + + logger.info( + f"Flake URL is {flake_url}, Nix revision is {nix_rev}, version is {version}", + ) + update_container_images(options, binary_cache, tmp_dir, release_dir, version) + + release_location = f"nix/{release_name}" + copy_manual(options, tmp_dir, release_name, binary_cache, release_location) + + platforms = [ + Platform("binaryTarball.i686-linux"), + Platform("binaryTarball.x86_64-linux"), + Platform("binaryTarball.aarch64-linux"), + Platform("binaryTarball.x86_64-darwin"), + Platform("binaryTarball.aarch64-darwin"), + Platform("binaryTarballCross.x86_64-linux.armv6l-unknown-linux-gnueabihf", can_fail=True), + Platform("binaryTarballCross.x86_64-linux.armv7l-unknown-linux-gnueabihf", can_fail=True), + Platform("binaryTarballCross.x86_64-linux.riscv64-unknown-linux-gnu"), + Platform("installerScript"), + ] + + for platform in platforms: + try: + download_file(options.eval_url, binary_cache, release_dir, platform.job_name) + except subprocess.CalledProcessError: + if platform.can_fail: + logger.exception(f"Failed to build {platform.job_name}") + continue + raise + + fallback_paths = { + "x86_64-linux": get_store_path(options.eval_url, "build.nix.x86_64-linux"), + "i686-linux": get_store_path(options.eval_url, "build.nix.i686-linux"), + "aarch64-linux": get_store_path(options.eval_url, "build.nix.aarch64-linux"), + "riscv64-linux": get_store_path(options.eval_url, "buildCross.nix.riscv64-unknown-linux-gnu.x86_64-linux"), + "x86_64-darwin": get_store_path(options.eval_url, "build.nix.x86_64-darwin"), + "aarch64-darwin": get_store_path(options.eval_url, "build.nix.aarch64-darwin"), + } + + (release_dir / "fallback-paths.nix").write_text(json.dumps(fallback_paths, indent=2)) + + for file_name in release_dir.glob("*"): + name = file_name.name + dst_key = f"{release_location}/{name}" + has_object = ( + subprocess.run( + ["aws", "--endpoint", options.aws_endpoint, "s3", "ls", f"s3://{options.release_bucket}/{dst_key}"], + stdout=subprocess.PIPE, + check=False, + ).returncode + == 0 + ) + if not has_object: + logger.info(f"uploading {file_name} to s3://{options.release_bucket}/{dst_key}...") + content_type = "application/octet-stream" + if file_name.suffix in [".sha256", ".install", ".nix"]: + content_type = "text/plain" + run( + [ + "aws", + "--endpoint", + options.aws_endpoint, + "s3", + "cp", + str(file_name), + f"s3://{options.release_bucket}/{dst_key}", + "--content-type", + content_type, + ], + dry_run=options.dry_run, + ) + + if options.is_latest: + run( + [ + "aws", + "--endpoint", + options.aws_endpoint, + "s3api", + "put-object", + "--bucket", + options.channels_bucket, + "--key", + "nix-latest/install", + "--website-redirect-location", + f"https://releases.nixos.org/{release_location}/install", + ], + dry_run=options.dry_run, + ) + + run(["git", "remote", "update", "origin"]) + run( + [ + "git", + "tag", + "--force", + "--sign", + version, + nix_rev, + "-m", + f"Tagging release {version}", + ], + ) + run(["git", "push", "--tags"], dry_run=options.dry_run or options.self_test) + if options.is_latest: + run( + [ + "git", + "push", + "--force-with-lease", + "origin", + f"{nix_rev}:refs/heads/latest-release", + ], + dry_run=options.dry_run or options.self_test, + ) + + +def parse_args() -> Options: + parser = argparse.ArgumentParser(description="Upload a release to S3") + parser.add_argument("evalid", type=int, help="The evaluation ID to upload") + parser.add_argument( + "--aws-endpoint", + type=str, + default="https://s3-eu-west-1.amazonaws.com", + help="The AWS endpoint to use", + ) + parser.add_argument( + "--aws-region", + type=str, + default="s3-eu-west-1.amazonaws.com", + help="The AWS region to use", + ) + parser.add_argument( + "--release-bucket", + type=str, + default="nix-releases", + help="The S3 bucket to upload releases to", + ) + parser.add_argument( + "--channels-bucket", + type=str, + default="nix-channels", + help="The S3 bucket to upload channels to", + ) + parser.add_argument("--is-latest", action="store_true", help="Whether this is the latest release") + parser.add_argument("--dry-run", action="store_true", help="Don't actually upload anything") + parser.add_argument("--self-test", action="store_true", help="Don't actually upload anything") + parser.add_argument( + "--self-test-registry-port", + type=int, + default=5000, + help="The port to run the Docker registry on", + ) + parser.add_argument("--self-test-minio-port", type=int, default=9000, help="The port to run Minio on") + parser.add_argument("--docker-owner", type=str, default="docker.io/nixos", help="The owner of the Docker images") + parser.add_argument( + "--docker-authfile", + type=Path, + help="The path to the Docker authfile", + default=Path.home() / ".docker" / "config.json", + ) + parser.add_argument( + "--project-root", + type=Path, + default=SCRIPT_DIR.parent, + help="The root of the project (default: the directory of this script)", + ) + args = parser.parse_args() + return Options( + eval_id=args.evalid, + aws_region=args.aws_region, + aws_endpoint=args.aws_endpoint, + release_bucket=args.release_bucket, + channels_bucket=args.channels_bucket, + is_latest=args.is_latest, + docker_owner=args.docker_owner, + docker_authfile=args.docker_authfile, + dry_run=args.dry_run, + project_root=args.project_root, + self_test=args.self_test, + self_test_registry_port=args.self_test_registry_port, + self_test_minio_port=args.self_test_minio_port, + ) + + +def wait_tcp_server(port: int, process: subprocess.Popen) -> None: + while True: + try: + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.connect(("localhost", port)) + except OSError as e: + logger.info(f"Wait for port: {port}") + if res := process.poll() is not None: + msg = f"Process {process} exited with code {res}" + raise Error(msg) from e + else: + return + time.sleep(0.1) + + +@contextmanager +def setup_self_test(options: Options, tmp_dir: Path) -> Iterator[None]: + if not options.self_test: + yield + return + registry_dir = tmp_dir / "docker-registry" + registry_dir.mkdir() + registry_config = registry_dir / "config.yml" + registry_config.write_text( + json.dumps( + { + "version": "0.1", + "http": { + "addr": f"localhost:{options.self_test_registry_port}", + }, + "storage": { + "filesystem": {"rootdirectory": str(registry_dir)}, + }, + }, + ), + ) + + registry_command = [ + "nix", + "shell", + "--inputs-from", + options.project_root, + "nixpkgs#docker-distribution", + "-c", + "registry", + "serve", + registry_config, + ] + minio_command = ["nix", "run", "--inputs-from", options.project_root, "nixpkgs#minio", "--", "server", tmp_dir / "minio"] + + os.environ["MINIO_ROOT_USER"] = "minioadmin" + os.environ["MINIO_ROOT_PASSWORD"] = "minioadmin" + os.environ["AWS_ACCESS_KEY_ID"] = "minioadmin" + os.environ["AWS_SECRET_ACCESS_KEY"] = "minioadmin" + options.aws_endpoint = f"http://localhost:{options.self_test_minio_port}" + options.docker_owner = f"localhost:{options.self_test_registry_port}" + + with ( + subprocess.Popen( + registry_command, + cwd=registry_dir, + ) as registry, + subprocess.Popen(minio_command) as minio, + ): + try: + wait_tcp_server(options.self_test_registry_port, registry) + wait_tcp_server(options.self_test_minio_port, minio) + run(["aws", "--endpoint", options.aws_endpoint, "s3", "mb", f"s3://{options.release_bucket}"]) + run(["aws", "--endpoint", options.aws_endpoint, "s3", "mb", f"s3://{options.channels_bucket}"]) + yield + logger.info("############################### Finished self-test ###############################") + logger.info( + f"You can inspect the release at http://localhost:{options.self_test_minio_port}/{options.release_bucket}", + ) + logger.info("Username/password: minioadmin/minioadmin") + logger.info( + f"You can inspect the registry at http://localhost:{options.self_test_registry_port}/v2/_catalog", + ) + logger.info("Type 'exit' to stop the self-test") + subprocess.run(["bash", "--login"], check=False) + finally: + try: + registry.kill() + finally: + minio.kill() + + +def main() -> None: + options = parse_args() + logging.basicConfig(level=logging.INFO) + try: + with TemporaryDirectory() as _tmp_dir: + tmp_dir = Path(_tmp_dir) + os.chdir(options.project_root) + + with setup_self_test(options, tmp_dir): + upload_release(options, tmp_dir) + except Error as e: + print(e, file=sys.stderr) # noqa: T201 + sys.exit(1) + + +if __name__ == "__main__": + main() diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 00000000000..9766dacc8b4 --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,25 @@ +[project] +name = "nix" +requires-python = ">=3.12" + +[tool.ruff] +# Allow lines to be as long as 120. +line-length = 120 +[tool.ruff.lint] +select = ["ALL"] +ignore = [ + "D", + "S", + "FBT001", + "FBT002", + "LOG", + "G004", + "EXE003", + "EXE005", + "E501", + "ANN401", + "PLR0915", + "COM812", + "ISC001", + "ERA001", +] From e6ab521fb4d7167b61cf20eaca7e61324f5b1e67 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Sun, 5 Jan 2025 19:46:10 +0100 Subject: [PATCH 08/14] implement CLICOLOR_FORCE environment variable This allows to force colors but not disable other features such as the progress bar. This is for instance useful in CI environments. Adapted from https://git.lix.systems/lix-project/lix/commit/378ec5fb0611e314511a7afc806664205846fc2e and others. --- src/libcmd/markdown.cc | 2 +- src/libmain/progress-bar.cc | 2 +- src/libmain/shared.cc | 4 ++- src/libutil/include/nix/util/terminal.hh | 34 +++++++++++++++++++++++- src/libutil/logging.cc | 2 +- src/libutil/terminal.cc | 24 ++++++++++++++--- src/nix/main.cc | 2 +- src/nix/nix-env/nix-env.cc | 2 +- 8 files changed, 61 insertions(+), 11 deletions(-) diff --git a/src/libcmd/markdown.cc b/src/libcmd/markdown.cc index 09cd9c1fb54..bd6dc5d8de8 100644 --- a/src/libcmd/markdown.cc +++ b/src/libcmd/markdown.cc @@ -65,7 +65,7 @@ static std::string doRenderMarkdownToTerminal(std::string_view markdown) if (!rndr_res) throw Error("allocation error while rendering Markdown"); - return filterANSIEscapes(std::string(buf->data, buf->size), !isTTY()); + return filterANSIEscapes(std::string(buf->data, buf->size), !shouldANSI()); } std::string renderMarkdownToTerminal(std::string_view markdown) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index c00f5d86b4d..a461c00a667 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -608,7 +608,7 @@ class ProgressBar : public Logger std::unique_ptr makeProgressBar() { - return std::make_unique(isTTY()); + return std::make_unique(shouldANSI()); } } // namespace nix diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 7187e972059..316c6ad7a46 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -6,6 +6,8 @@ #include "nix/main/loggers.hh" #include "nix/main/progress-bar.hh" #include "nix/util/signals.hh" +#include "nix/util/terminal.hh" + #include #include @@ -356,7 +358,7 @@ int handleExceptions(const std::string & programName, std::function fun) RunPager::RunPager() { - if (!isatty(STDOUT_FILENO)) + if (!isOutputARealTerminal(StandardOutputStream::Stdout)) return; char * pager = getenv("NIX_PAGER"); if (!pager) diff --git a/src/libutil/include/nix/util/terminal.hh b/src/libutil/include/nix/util/terminal.hh index f19de268c8a..fb12b79a1ac 100644 --- a/src/libutil/include/nix/util/terminal.hh +++ b/src/libutil/include/nix/util/terminal.hh @@ -5,11 +5,43 @@ #include namespace nix { + +enum class StandardOutputStream { + Stdout = 1, + Stderr = 2, +}; + +/** + * Determine whether the output is a real terminal (i.e. not dumb, not a pipe). + * + * This is probably not what you want, you may want shouldANSI() or something + * more specific. Think about how the output should work with a pager or + * entirely non-interactive scripting use. + * + * The user may be redirecting the Lix output to a pager, but have stderr + * connected to a terminal. Think about where you are outputting the text when + * deciding whether to use STDERR_FILENO or STDOUT_FILENO. + * + * \param fileno file descriptor number to check if it is a tty + */ +bool isOutputARealTerminal(StandardOutputStream fileno); + /** * Determine whether ANSI escape sequences are appropriate for the * present output. + * + * This follows the rules described on https://bixense.com/clicolors/ + * with CLICOLOR defaulted to enabled (and thus ignored). + * + * That is to say, the following procedure is followed in order: + * - NO_COLOR or NOCOLOR set -> always disable colour + * - CLICOLOR_FORCE or FORCE_COLOR set -> enable colour + * - The output is a tty; TERM != "dumb" -> enable colour + * - Otherwise -> disable colour + * + * \param fileno which file descriptor number to consider. Use the one you are outputting to */ -bool isTTY(); +bool shouldANSI(StandardOutputStream fileno = StandardOutputStream::Stderr); /** * Truncate a string to 'width' printable characters. If 'filterAll' diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 997110617b3..01e9d41b12e 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -70,7 +70,7 @@ class SimpleLogger : public Logger : printBuildLogs(printBuildLogs) { systemd = getEnv("IN_SYSTEMD") == "1"; - tty = isTTY(); + tty = shouldANSI(); } bool isVerbose() override diff --git a/src/libutil/terminal.cc b/src/libutil/terminal.cc index b5765487c25..3508f83f45b 100644 --- a/src/libutil/terminal.cc +++ b/src/libutil/terminal.cc @@ -61,12 +61,28 @@ inline std::pair charWidthUTF8Helper(std::string_view s) namespace nix { -bool isTTY() +bool isOutputARealTerminal(StandardOutputStream fileno) { - static const bool tty = isatty(STDERR_FILENO) && getEnv("TERM").value_or("dumb") != "dumb" - && !(getEnv("NO_COLOR").has_value() || getEnv("NOCOLOR").has_value()); + return isatty(int(fileno)) && getEnv("TERM").value_or("dumb") != "dumb"; +} - return tty; +bool shouldANSI(StandardOutputStream fileno) +{ + // Implements the behaviour described by https://bixense.com/clicolors/ + // As well as https://force-color.org/ for compatibility, since it fits in the same shape. + // NO_COLOR CLICOLOR CLICOLOR_FORCE Colours? + // set x x No + // unset x set Yes + // unset x unset If attached to a terminal + // [we choose the "modern" approach of colour-by-default] + auto compute = [](StandardOutputStream fileno) -> bool { + bool mustNotColour = getEnv("NO_COLOR").has_value() || getEnv("NOCOLOR").has_value(); + bool shouldForce = getEnv("CLICOLOR_FORCE").has_value() || getEnv("FORCE_COLOR").has_value(); + bool isTerminal = isOutputARealTerminal(fileno); + return !mustNotColour && (shouldForce || isTerminal); + }; + static bool cached[2] = {compute(StandardOutputStream::Stdout), compute(StandardOutputStream::Stderr)}; + return cached[int(fileno) - 1]; } std::string filterANSIEscapes(std::string_view s, bool filterAll, unsigned int width) diff --git a/src/nix/main.cc b/src/nix/main.cc index a6077f5e9ad..b0c837f8535 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -408,7 +408,7 @@ void mainWrapped(int argc, char ** argv) settings.verboseBuild = false; // If on a terminal, progress will be displayed via progress bars etc. (thus verbosity=notice) - if (nix::isTTY()) { + if (nix::isOutputARealTerminal(StandardOutputStream::Stderr)) { verbosity = lvlNotice; } else { verbosity = lvlInfo; diff --git a/src/nix/nix-env/nix-env.cc b/src/nix/nix-env/nix-env.cc index f165c069cd8..deada8ae08e 100644 --- a/src/nix/nix-env/nix-env.cc +++ b/src/nix/nix-env/nix-env.cc @@ -1073,7 +1073,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) return; } - bool tty = isTTY(); + bool tty = shouldANSI(); RunPager pager; Table table; From 1073d88f83401e55a12ced890107a2f908b0ec20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Tue, 21 Jan 2025 13:54:34 +0100 Subject: [PATCH 09/14] rebase my version of nix on my nixpkgs --- flake.lock | 20 ++++++++++---------- flake.nix | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/flake.lock b/flake.lock index 3075eabc233..7e2f5a9cdc4 100644 --- a/flake.lock +++ b/flake.lock @@ -63,18 +63,18 @@ }, "nixpkgs": { "locked": { - "lastModified": 1747179050, - "narHash": "sha256-qhFMmDkeJX9KJwr5H32f1r7Prs7XbQWtO0h3V0a0rFY=", - "owner": "NixOS", - "repo": "nixpkgs", - "rev": "adaa24fbf46737f3f1b5497bf64bae750f82942e", - "type": "github" + "lastModified": 1747213200, + "narHash": "sha256-LLzQSQioyp6I11hs/7tb0l7CpyX42TZS2DrCRMjonaw=", + "ref": "refs/heads/main", + "rev": "01a4c54a8a4412f806a55e2f577116ccd8d47b2d", + "shallow": true, + "type": "git", + "url": "https://github.com/Mic92/nixpkgs" }, "original": { - "owner": "NixOS", - "ref": "nixos-unstable", - "repo": "nixpkgs", - "type": "github" + "shallow": true, + "type": "git", + "url": "https://github.com/Mic92/nixpkgs" } }, "nixpkgs-23-11": { diff --git a/flake.nix b/flake.nix index 6a6f2cfd8d1..6e2696f8bc5 100644 --- a/flake.nix +++ b/flake.nix @@ -1,7 +1,7 @@ { description = "The purely functional package manager"; - inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable"; + inputs.nixpkgs.url = "git+https://github.com/Mic92/nixpkgs?shallow=1"; inputs.nixpkgs-regression.url = "github:NixOS/nixpkgs/215d4d0fd80ca5163643b03a33fde804a29cc1e2"; inputs.nixpkgs-23-11.url = "github:NixOS/nixpkgs/a62e6edd6d5e1fa0329b8653c801147986f8d446"; From d9519604ea6496a4d655a4feb5d7e18d7ce700f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Wed, 12 Mar 2025 11:17:04 +0100 Subject: [PATCH 10/14] stop testing on 32-bit in my CI --- flake.nix | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/flake.nix b/flake.nix index 6e2696f8bc5..85b153aa641 100644 --- a/flake.nix +++ b/flake.nix @@ -34,7 +34,9 @@ officialRelease = false; - linux32BitSystems = [ "i686-linux" ]; + linux32BitSystems = [ + #"i686-linux" + ]; linux64BitSystems = [ "x86_64-linux" "aarch64-linux" From d711df67d095fd041ae5fa5b3d72a6e8dfb08a7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Mon, 31 Mar 2025 10:20:14 +0200 Subject: [PATCH 11/14] escape .git for tarball cache Fix for https://github.com/NixOS/nix/issues/10575 --- src/libfetchers/git-utils.cc | 89 ++++++++++++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 8 deletions(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 993d7fb08d7..ebbdb2c07c8 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -60,6 +60,78 @@ bool operator==(const git_oid & oid1, const git_oid & oid2) return git_oid_equal(&oid1, &oid2); } +namespace { + +int matchesDotPlusGit(const std::string& str) { + // String must have at least 4 characters (at least one '.' plus "git") + if (str.size() < 4) { + return 0; + } + + // Count consecutive dots at the beginning + size_t dotCount = 0; + while (dotCount < str.size() && str[dotCount] == '.') { + dotCount++; + } + + // Must have at least one dot + if (dotCount == 0) { + return 0; + } + + // After the dots, check if the remaining string is exactly "git" + if ((str.size() == dotCount + 3) && + (str[dotCount] == 'g') && + (str[dotCount + 1] == 'i') && + (str[dotCount + 2] == 't')) { + return dotCount; + } + return 0; +} + +std::string escapeDotGit(const std::string& filename) { + // Check if this filename matches the pattern of dots followed by "git" + int dotCount = matchesDotPlusGit(filename); + if (dotCount == 0) { + // Not a dot+git pattern, return as is + return filename; + } + + std::string result(dotCount * 2, '.'); // String with 2*dotCount dots + result += "git"; + + return result; +} + +std::string unescapeDotGit(const std::string filename) { + // Check if this filename matches the pattern of dots followed by "git" + int dotCount = matchesDotPlusGit(filename); + // Ensure dots are even for unescaping (must be divisible by 2) + if (dotCount == 0 || dotCount % 2 != 0) { + // Can't unescape an odd number of dots, return as is + return filename; + } + + // Create a new string with half the dots plus "git" + std::string result(dotCount / 2, '.'); // String with dotCount/2 dots + result += "git"; + + return result; +} + +const git_tree_entry* gitTreebuilderGet(git_treebuilder *bld, std::string name) +{ + auto escapedName = escapeDotGit(name); + return git_treebuilder_get(bld, escapedName.c_str()); +} + +const std::string gitTreeEntryName(const git_tree_entry *entry) +{ + auto escapedName = git_tree_entry_name(entry); + return unescapeDotGit(escapedName); +} +} + namespace nix { struct GitSourceAccessor; @@ -677,6 +749,9 @@ struct GitSourceAccessor : SourceAccessor std::optional lfsFetch = std::nullopt; }; + struct Submodule + {}; + Sync state_; GitSourceAccessor(ref repo_, const Hash & rev, bool smudgeLfs) @@ -772,7 +847,7 @@ struct GitSourceAccessor : SourceAccessor for (size_t n = 0; n < count; ++n) { auto entry = git_tree_entry_byindex(tree.get(), n); // FIXME: add to cache - res.emplace(std::string(git_tree_entry_name(entry)), DirEntry{}); + res.emplace(std::string(gitTreeEntryName(entry)), DirEntry{}); } return res; @@ -834,7 +909,7 @@ struct GitSourceAccessor : SourceAccessor if (git_tree_entry_dup(Setter(copy), entry)) throw Error("dupping tree entry: %s", git_error_last()->message); - auto entryName = std::string_view(git_tree_entry_name(entry)); + auto entryName = gitTreeEntryName(entry); if (entryName == name) res = copy.get(); @@ -875,9 +950,6 @@ struct GitSourceAccessor : SourceAccessor return entry; } - struct Submodule - {}; - std::variant getTree(State & state, const CanonPath & path) { if (path.isRoot()) { @@ -1013,7 +1085,7 @@ struct GitFileSystemObjectSinkImpl : GitFileSystemObjectSink const git_tree_entry * entry; Tree prevTree = nullptr; - if (!pendingDirs.empty() && (entry = git_treebuilder_get(pendingDirs.back().builder.get(), name.c_str()))) { + if (!pendingDirs.empty() && (entry = gitTreebuilderGet(pendingDirs.back().builder.get(), name))) { /* Clone a tree that we've already finished. This happens if a tarball has directory entries that are not contiguous. */ @@ -1051,7 +1123,8 @@ struct GitFileSystemObjectSinkImpl : GitFileSystemObjectSink { assert(!pendingDirs.empty()); auto & pending = pendingDirs.back(); - if (git_treebuilder_insert(nullptr, pending.builder.get(), name.c_str(), &oid, mode)) + auto escapedName = escapeDotGit(name); + if (git_treebuilder_insert(nullptr, pending.builder.get(), escapedName.c_str(), &oid, mode)) throw Error("adding a file to a tree builder: %s", git_error_last()->message); }; @@ -1185,7 +1258,7 @@ struct GitFileSystemObjectSinkImpl : GitFileSystemObjectSink for (auto & c : CanonPath(relTargetLeft)) { if (auto builder = std::get_if(&curDir)) { assert(*builder); - if (!(entry = git_treebuilder_get(*builder, std::string(c).c_str()))) + if (!(entry = gitTreebuilderGet(*builder, std::string(c)))) throw Error("cannot find hard link target '%s' for path '%s'", target, path); curDir = *git_tree_entry_id(entry); } else if (auto oid = std::get_if(&curDir)) { From e3566ef4c76e45262de9f5a7ca62cb9b3f64c3b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Fri, 13 Jun 2025 22:01:26 +0200 Subject: [PATCH 12/14] turn assertion if hash mismatches into proper error --- src/libflake/flake.cc | 14 +++++- tests/functional/flakes/meson.build | 1 + tests/functional/flakes/nar-hash-mismatch.sh | 52 ++++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100755 tests/functional/flakes/nar-hash-mismatch.sh diff --git a/src/libflake/flake.cc b/src/libflake/flake.cc index b31bef21103..8dd6c636a2c 100644 --- a/src/libflake/flake.cc +++ b/src/libflake/flake.cc @@ -34,7 +34,19 @@ static StorePath copyInputToStore( auto narHash = state.store->queryPathInfo(storePath)->narHash; input.attrs.insert_or_assign("narHash", narHash.to_string(HashFormat::SRI, true)); - assert(!originalInput.getNarHash() || storePath == originalInput.computeStorePath(*state.store)); + if (originalInput.getNarHash() && storePath != originalInput.computeStorePath(*state.store)) { + throw Error( + "NAR hash mismatch for flake input '%s':\n" + " expected: %s (store path: %s)\n" + " got: %s (store path: %s)\n" + "This typically happens when the content at the specified path has changed since the NAR hash was recorded.", + input.to_string(), + originalInput.getNarHash()->to_string(HashFormat::SRI, true), + originalInput.computeStorePath(*state.store).to_string(), + narHash.to_string(HashFormat::SRI, true), + storePath.to_string() + ); + } return storePath; } diff --git a/tests/functional/flakes/meson.build b/tests/functional/flakes/meson.build index 801fefc6f9a..b087e407a38 100644 --- a/tests/functional/flakes/meson.build +++ b/tests/functional/flakes/meson.build @@ -34,6 +34,7 @@ suites += { 'source-paths.sh', 'old-lockfiles.sh', 'trace-ifd.sh', + 'nar-hash-mismatch.sh', ], 'workdir': meson.current_source_dir(), } diff --git a/tests/functional/flakes/nar-hash-mismatch.sh b/tests/functional/flakes/nar-hash-mismatch.sh new file mode 100755 index 00000000000..e60077f3f06 --- /dev/null +++ b/tests/functional/flakes/nar-hash-mismatch.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash + +source ./common.sh + +requireGit + +clearStore +clearCache + +# Create a test flake with NAR hash mismatch +tmpDir=$TEST_ROOT/nar-hash-test +rm -rf "$tmpDir" +mkdir -p "$tmpDir" +cd "$tmpDir" + +# Setup git repo with a sub-flake +initGitRepo . +mkdir sub +echo '{ outputs = { self }: { test = "hello"; }; }' > sub/flake.nix +git add sub +git commit -m "add sub" + +# Get the original hash and create main flake that references it +hash=$(nix hash path ./sub) +echo "$hash" > sub.narHash + +cat > flake.nix << EOF +{ + outputs = { self }: + let + hash = builtins.readFile ./sub.narHash; + cleanHash = builtins.substring 0 (builtins.stringLength hash - 1) hash; + subFlake = builtins.getFlake "path:\${toString ./sub}?narHash=\${cleanHash}"; + in + { inherit (subFlake) test; }; +} +EOF + +git add flake.nix sub.narHash + +# Modify sub-flake to create hash mismatch +echo '{ outputs = { self }: { test = "modified"; }; }' > sub/flake.nix + +# Test that evaluation fails with proper error message (not assertion failure) +if output=$(nix eval .#test 2>&1); then + fail "Expected evaluation to fail, but it succeeded" +fi + +# Verify error message contains expected content and no crash indicators +grep -q "NAR hash mismatch" <<< "$output" || fail "Expected 'NAR hash mismatch' in error output" +! grep -q "Assertion.*failed" <<< "$output" || fail "Should not contain assertion failure" +! grep -q "Aborted" <<< "$output" || fail "Should not contain 'Aborted'" \ No newline at end of file From eb450f93956de05963b6fb6887fe6dbd149472e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Wed, 6 Aug 2025 16:52:52 +0200 Subject: [PATCH 13/14] parse structured attrs lazy --- src/libexpr/primops.cc | 6 ++-- src/libstore/derivation-options.cc | 30 +++++++++---------- src/libstore/derivations.cc | 9 ++++-- .../include/nix/store/parsed-derivations.hh | 26 ++++++++++++++-- src/libstore/parsed-derivations.cc | 15 ++++------ 5 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 6af179e4e0c..f6fda9c0fd6 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1371,8 +1371,10 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName *attr->value, pos, "while evaluating the `__structuredAttrs` " - "attribute passed to builtins.derivationStrict")) - jsonObject = StructuredAttrs{.structuredAttrs = json::object()}; + "attribute passed to builtins.derivationStrict")) { + jsonObject = StructuredAttrs{}; + jsonObject->structuredAttrs = json::object(); + } /* Check whether null attributes should be ignored. */ bool ignoreNulls = false; diff --git a/src/libstore/derivation-options.cc b/src/libstore/derivation-options.cc index f1515c30864..f0a686c4b3f 100644 --- a/src/libstore/derivation-options.cc +++ b/src/libstore/derivation-options.cc @@ -17,8 +17,8 @@ static std::optional getStringAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name) { if (parsed) { - auto i = parsed->structuredAttrs.find(name); - if (i == parsed->structuredAttrs.end()) + auto i = parsed->getStructuredAttrs().find(name); + if (i == parsed->getStructuredAttrs().end()) return {}; else { if (!i->is_string()) @@ -37,8 +37,8 @@ getStringAttr(const StringMap & env, const StructuredAttrs * parsed, const std:: static bool getBoolAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name, bool def) { if (parsed) { - auto i = parsed->structuredAttrs.find(name); - if (i == parsed->structuredAttrs.end()) + auto i = parsed->getStructuredAttrs().find(name); + if (i == parsed->getStructuredAttrs().end()) return def; else { if (!i->is_boolean()) @@ -58,8 +58,8 @@ static std::optional getStringsAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name) { if (parsed) { - auto i = parsed->structuredAttrs.find(name); - if (i == parsed->structuredAttrs.end()) + auto i = parsed->getStructuredAttrs().find(name); + if (i == parsed->getStructuredAttrs().end()) return {}; else { if (!i->is_array()) @@ -104,27 +104,27 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt DerivationOptions defaults = {}; if (shouldWarn && parsed) { - if (get(parsed->structuredAttrs, "allowedReferences")) { + if (get(parsed->getStructuredAttrs(), "allowedReferences")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'allowedReferences'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "allowedRequisites")) { + if (get(parsed->getStructuredAttrs(), "allowedRequisites")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'allowedRequisites'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "disallowedRequisites")) { + if (get(parsed->getStructuredAttrs(), "disallowedRequisites")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'disallowedRequisites'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "disallowedReferences")) { + if (get(parsed->getStructuredAttrs(), "disallowedReferences")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'disallowedReferences'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "maxSize")) { + if (get(parsed->getStructuredAttrs(), "maxSize")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'maxSize'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "maxClosureSize")) { + if (get(parsed->getStructuredAttrs(), "maxClosureSize")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'maxClosureSize'; use 'outputChecks' instead"); } @@ -134,7 +134,7 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt .outputChecks = [&]() -> OutputChecksVariant { if (parsed) { std::map res; - if (auto outputChecks = get(parsed->structuredAttrs, "outputChecks")) { + if (auto outputChecks = get(parsed->getStructuredAttrs(), "outputChecks")) { for (auto & [outputName, output] : getObject(*outputChecks)) { OutputChecks checks; @@ -183,7 +183,7 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt std::map res; if (parsed) { - if (auto udr = get(parsed->structuredAttrs, "unsafeDiscardReferences")) { + if (auto udr = get(parsed->getStructuredAttrs(), "unsafeDiscardReferences")) { for (auto & [outputName, output] : getObject(*udr)) { if (!output.is_boolean()) throw Error("attribute 'unsafeDiscardReferences.\"%s\"' must be a Boolean", outputName); @@ -214,7 +214,7 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt std::map ret; if (parsed) { - auto e = optionalValueAt(parsed->structuredAttrs, "exportReferencesGraph"); + auto e = optionalValueAt(parsed->getStructuredAttrs(), "exportReferencesGraph"); if (!e || !e->is_object()) return ret; for (auto & [key, value] : getObject(*e)) { diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 1afc343d7b6..26d49ff5d69 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -1401,7 +1401,7 @@ nlohmann::json Derivation::toJSON(const StoreDirConfig & store) const res["env"] = env; if (structuredAttrs) - res["structuredAttrs"] = structuredAttrs->structuredAttrs; + res["structuredAttrs"] = structuredAttrs->getStructuredAttrs(); return res; } @@ -1470,8 +1470,11 @@ Derivation Derivation::fromJSON( throw; } - if (auto structuredAttrs = get(json, "structuredAttrs")) - res.structuredAttrs = StructuredAttrs{*structuredAttrs}; + if (auto structuredAttrs = get(json, "structuredAttrs")) { + StructuredAttrs attrs; + attrs.rawJson = structuredAttrs->dump(); + res.structuredAttrs = attrs; + } return res; } diff --git a/src/libstore/include/nix/store/parsed-derivations.hh b/src/libstore/include/nix/store/parsed-derivations.hh index edef1b2d243..a8baf27fbc5 100644 --- a/src/libstore/include/nix/store/parsed-derivations.hh +++ b/src/libstore/include/nix/store/parsed-derivations.hh @@ -1,10 +1,12 @@ #pragma once ///@file -#include - #include "nix/util/types.hh" +#include "nix/util/error.hh" #include "nix/store/path.hh" +#include "nix/util/json-utils.hh" + +#include namespace nix { @@ -18,10 +20,30 @@ struct StructuredAttrs { static constexpr std::string_view envVarName{"__json"}; +private: + mutable std::optional parsedJson; + +public: + std::string rawJson; + // For building up JSON during derivation construction nlohmann::json structuredAttrs; bool operator==(const StructuredAttrs &) const = default; + const nlohmann::json & getStructuredAttrs() const { + if (!rawJson.empty() && !parsedJson) { + try { + parsedJson = nlohmann::json::parse(rawJson); + } catch (std::exception & e) { + throw Error("cannot process %s attribute: %s", envVarName, e.what()); + } + return *parsedJson; + } else if (parsedJson) { + return *parsedJson; + } + return structuredAttrs; + } + /** * Unconditionally parse from a JSON string. Used by `tryExtract`. */ diff --git a/src/libstore/parsed-derivations.cc b/src/libstore/parsed-derivations.cc index 1006bbc0aed..f488815c53c 100644 --- a/src/libstore/parsed-derivations.cc +++ b/src/libstore/parsed-derivations.cc @@ -10,13 +10,10 @@ namespace nix { StructuredAttrs StructuredAttrs::parse(std::string_view encoded) { - try { - return StructuredAttrs{ - .structuredAttrs = nlohmann::json::parse(encoded), - }; - } catch (std::exception & e) { - throw Error("cannot process %s attribute: %s", envVarName, e.what()); - } + StructuredAttrs result; + result.rawJson = std::string(encoded); + // Don't validate JSON here - delay parsing until first access + return result; } std::optional StructuredAttrs::tryExtract(StringPairs & env) @@ -33,7 +30,7 @@ std::optional StructuredAttrs::tryExtract(StringPairs & env) std::pair StructuredAttrs::unparse() const { - return {envVarName, structuredAttrs.dump()}; + return {envVarName, rawJson.empty() ? getStructuredAttrs().dump() : rawJson}; } void StructuredAttrs::checkKeyNotInUse(const StringPairs & env) @@ -104,7 +101,7 @@ nlohmann::json StructuredAttrs::prepareStructuredAttrs( const DerivationOutputs & outputs) const { /* Copy to then modify */ - auto json = structuredAttrs; + auto json = getStructuredAttrs(); /* Add an "outputs" object containing the output paths. */ nlohmann::json outputsJson; From ac112072e3ff34b2ca277ef11dbb6fd918f8c566 Mon Sep 17 00:00:00 2001 From: Philipp Otterbein Date: Wed, 6 Aug 2025 23:40:56 +0200 Subject: [PATCH 14/14] string copy elision in `StructuredAttrs::parse` revert to simplified `Derivation::parseDerivation` code --- src/libexpr/primops.cc | 2 +- src/libfetchers/git-utils.cc | 21 ++++++++++--------- src/libflake/flake.cc | 3 +-- src/libmain/shared.cc | 1 - src/libstore/derivations.cc | 11 ++++------ .../include/nix/store/parsed-derivations.hh | 5 +++-- src/libstore/parsed-derivations.cc | 6 +++--- 7 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index f6fda9c0fd6..77c9a08ba11 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1538,7 +1538,7 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName warn( "In derivation '%s': setting structured attributes via '__json' is deprecated, and may be disallowed in future versions of Nix. Set '__structuredAttrs = true' instead.", drvName); - drv.structuredAttrs = StructuredAttrs::parse(s); + drv.structuredAttrs = StructuredAttrs::parse(std::move(s)); } else { drv.env.emplace(key, s); if (i->name == state.sBuilder) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index ebbdb2c07c8..1eb73a81911 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -62,7 +62,8 @@ bool operator==(const git_oid & oid1, const git_oid & oid2) namespace { -int matchesDotPlusGit(const std::string& str) { +int matchesDotPlusGit(const std::string & str) +{ // String must have at least 4 characters (at least one '.' plus "git") if (str.size() < 4) { return 0; @@ -80,16 +81,15 @@ int matchesDotPlusGit(const std::string& str) { } // After the dots, check if the remaining string is exactly "git" - if ((str.size() == dotCount + 3) && - (str[dotCount] == 'g') && - (str[dotCount + 1] == 'i') && - (str[dotCount + 2] == 't')) { + if ((str.size() == dotCount + 3) && (str[dotCount] == 'g') && (str[dotCount + 1] == 'i') + && (str[dotCount + 2] == 't')) { return dotCount; } return 0; } -std::string escapeDotGit(const std::string& filename) { +std::string escapeDotGit(const std::string & filename) +{ // Check if this filename matches the pattern of dots followed by "git" int dotCount = matchesDotPlusGit(filename); if (dotCount == 0) { @@ -103,7 +103,8 @@ std::string escapeDotGit(const std::string& filename) { return result; } -std::string unescapeDotGit(const std::string filename) { +std::string unescapeDotGit(const std::string filename) +{ // Check if this filename matches the pattern of dots followed by "git" int dotCount = matchesDotPlusGit(filename); // Ensure dots are even for unescaping (must be divisible by 2) @@ -119,18 +120,18 @@ std::string unescapeDotGit(const std::string filename) { return result; } -const git_tree_entry* gitTreebuilderGet(git_treebuilder *bld, std::string name) +const git_tree_entry * gitTreebuilderGet(git_treebuilder * bld, std::string name) { auto escapedName = escapeDotGit(name); return git_treebuilder_get(bld, escapedName.c_str()); } -const std::string gitTreeEntryName(const git_tree_entry *entry) +const std::string gitTreeEntryName(const git_tree_entry * entry) { auto escapedName = git_tree_entry_name(entry); return unescapeDotGit(escapedName); } -} +} // namespace namespace nix { diff --git a/src/libflake/flake.cc b/src/libflake/flake.cc index 8dd6c636a2c..04c2fdaa097 100644 --- a/src/libflake/flake.cc +++ b/src/libflake/flake.cc @@ -44,8 +44,7 @@ static StorePath copyInputToStore( originalInput.getNarHash()->to_string(HashFormat::SRI, true), originalInput.computeStorePath(*state.store).to_string(), narHash.to_string(HashFormat::SRI, true), - storePath.to_string() - ); + storePath.to_string()); } return storePath; diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 316c6ad7a46..dfa4f6c633c 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -8,7 +8,6 @@ #include "nix/util/signals.hh" #include "nix/util/terminal.hh" - #include #include #include diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 26d49ff5d69..8e49a75a782 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -474,14 +474,11 @@ Derivation parseDerivation( expect(str, '('); auto name = parseString(str).toOwned(); expect(str, ','); - auto value = parseString(str); - if (name == StructuredAttrs::envVarName) { - drv.structuredAttrs = StructuredAttrs::parse(*std::move(value)); - } else { - drv.env.insert_or_assign(std::move(name), std::move(value).toOwned()); - } + auto value = parseString(str).toOwned(); expect(str, ')'); + drv.env.insert_or_assign(std::move(name), std::move(value)); } + drv.structuredAttrs = StructuredAttrs::tryExtract(drv.env); expect(str, ')'); return drv; @@ -1077,7 +1074,7 @@ void BasicDerivation::applyRewrites(const StringMap & rewrites) // TODO rewrite the JSON AST properly, rather than dump parse round trip. auto [_, jsonS] = structuredAttrs->unparse(); jsonS = rewriteStrings(std::move(jsonS), rewrites); - structuredAttrs = StructuredAttrs::parse(jsonS); + structuredAttrs = StructuredAttrs::parse(std::move(jsonS)); } } diff --git a/src/libstore/include/nix/store/parsed-derivations.hh b/src/libstore/include/nix/store/parsed-derivations.hh index a8baf27fbc5..dc66eb8c5a0 100644 --- a/src/libstore/include/nix/store/parsed-derivations.hh +++ b/src/libstore/include/nix/store/parsed-derivations.hh @@ -30,7 +30,8 @@ public: bool operator==(const StructuredAttrs &) const = default; - const nlohmann::json & getStructuredAttrs() const { + const nlohmann::json & getStructuredAttrs() const + { if (!rawJson.empty() && !parsedJson) { try { parsedJson = nlohmann::json::parse(rawJson); @@ -47,7 +48,7 @@ public: /** * Unconditionally parse from a JSON string. Used by `tryExtract`. */ - static StructuredAttrs parse(std::string_view encoded); + static StructuredAttrs parse(std::string && encoded); /** * Like `tryParse`, but removes the env var which encoded the structured diff --git a/src/libstore/parsed-derivations.cc b/src/libstore/parsed-derivations.cc index f488815c53c..113f1fd1f89 100644 --- a/src/libstore/parsed-derivations.cc +++ b/src/libstore/parsed-derivations.cc @@ -8,10 +8,10 @@ namespace nix { -StructuredAttrs StructuredAttrs::parse(std::string_view encoded) +StructuredAttrs StructuredAttrs::parse(std::string && encoded) { StructuredAttrs result; - result.rawJson = std::string(encoded); + result.rawJson = std::move(encoded); // Don't validate JSON here - delay parsing until first access return result; } @@ -23,7 +23,7 @@ std::optional StructuredAttrs::tryExtract(StringPairs & env) if (jsonAttr != env.end()) { auto encoded = std::move(jsonAttr->second); env.erase(jsonAttr); - return parse(encoded); + return parse(std::move(encoded)); } else return {}; }