From 93a42a597145a2ff0214ddc6a9828921056c3657 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 11 Jun 2025 22:08:03 +0000 Subject: [PATCH 01/22] flake: Add meson formatter This adds a meson.format file that mostly mirrors the projects meson style and a pre-commit hook to enforce this style. Some low-diff files are formatted. --- maintainers/flake-module.nix | 112 ++++++++++++++++++ meson.build | 10 +- meson.format | 7 ++ meson.options | 15 ++- .../windows-version/meson.build | 2 +- .../include/nix/expr/tests/meson.build | 8 +- src/libexpr/primops/meson.build | 2 +- .../linux/include/nix/store/meson.build | 4 +- src/libstore/linux/meson.build | 4 +- .../freebsd/include/nix/util/meson.build | 4 +- src/libutil/freebsd/meson.build | 4 +- src/libutil/include/nix/util/meson.build | 4 +- .../linux/include/nix/util/meson.build | 5 +- src/libutil/linux/meson.build | 5 +- .../windows/include/nix/util/meson.build | 6 +- src/perl/t/meson.build | 4 +- tests/functional/plugins/meson.build | 4 +- .../test-libstoreconsumer/meson.build | 4 +- 18 files changed, 152 insertions(+), 52 deletions(-) create mode 100644 meson.format diff --git a/maintainers/flake-module.nix b/maintainers/flake-module.nix index d2bae32b6..d443d1a74 100644 --- a/maintainers/flake-module.nix +++ b/maintainers/flake-module.nix @@ -37,6 +37,118 @@ fi ''}"; }; + meson-format = { + enable = true; + files = "(meson.build|meson.options)$"; + entry = "${pkgs.writeScript "format-meson" '' + #!${pkgs.runtimeShell} + for file in "$@"; do + ${lib.getExe pkgs.meson} format -ic ${../meson.format} "$file" + done + ''}"; + excludes = [ + # We haven't applied formatting to these files yet + ''^doc/manual/meson.build$'' + ''^doc/manual/source/command-ref/meson.build$'' + ''^doc/manual/source/development/meson.build$'' + ''^doc/manual/source/language/meson.build$'' + ''^doc/manual/source/meson.build$'' + ''^doc/manual/source/release-notes/meson.build$'' + ''^doc/manual/source/store/meson.build$'' + ''^misc/bash/meson.build$'' + ''^misc/fish/meson.build$'' + ''^misc/launchd/meson.build$'' + ''^misc/meson.build$'' + ''^misc/systemd/meson.build$'' + ''^misc/zsh/meson.build$'' + ''^nix-meson-build-support/$'' + ''^nix-meson-build-support/big-objs/meson.build$'' + ''^nix-meson-build-support/common/meson.build$'' + ''^nix-meson-build-support/deps-lists/meson.build$'' + ''^nix-meson-build-support/export/meson.build$'' + ''^nix-meson-build-support/export-all-symbols/meson.build$'' + ''^nix-meson-build-support/generate-header/meson.build$'' + ''^nix-meson-build-support/libatomic/meson.build$'' + ''^nix-meson-build-support/subprojects/meson.build$'' + ''^scripts/meson.build$'' + ''^src/external-api-docs/meson.build$'' + ''^src/internal-api-docs/meson.build$'' + ''^src/libcmd/include/nix/cmd/meson.build$'' + ''^src/libcmd/meson.build$'' + ''^src/libcmd/nix-meson-build-support$'' + ''^src/libexpr/include/nix/expr/meson.build$'' + ''^src/libexpr/meson.build$'' + ''^src/libexpr/nix-meson-build-support$'' + ''^src/libexpr-c/meson.build$'' + ''^src/libexpr-c/nix-meson-build-support$'' + ''^src/libexpr-test-support/meson.build$'' + ''^src/libexpr-test-support/nix-meson-build-support$'' + ''^src/libexpr-tests/meson.build$'' + ''^src/libexpr-tests/nix-meson-build-support$'' + ''^src/libfetchers/include/nix/fetchers/meson.build$'' + ''^src/libfetchers/meson.build$'' + ''^src/libfetchers/nix-meson-build-support$'' + ''^src/libfetchers-c/meson.build$'' + ''^src/libfetchers-c/nix-meson-build-support$'' + ''^src/libfetchers-tests/meson.build$'' + ''^src/libfetchers-tests/nix-meson-build-support$'' + ''^src/libflake/include/nix/flake/meson.build$'' + ''^src/libflake/meson.build$'' + ''^src/libflake/nix-meson-build-support$'' + ''^src/libflake-c/meson.build$'' + ''^src/libflake-c/nix-meson-build-support$'' + ''^src/libflake-tests/meson.build$'' + ''^src/libflake-tests/nix-meson-build-support$'' + ''^src/libmain/include/nix/main/meson.build$'' + ''^src/libmain/meson.build$'' + ''^src/libmain/nix-meson-build-support$'' + ''^src/libmain-c/meson.build$'' + ''^src/libmain-c/nix-meson-build-support$'' + ''^src/libstore/include/nix/store/meson.build$'' + ''^src/libstore/meson.build$'' + ''^src/libstore/nix-meson-build-support$'' + ''^src/libstore/unix/include/nix/store/meson.build$'' + ''^src/libstore/unix/meson.build$'' + ''^src/libstore/windows/meson.build$'' + ''^src/libstore-c/meson.build$'' + ''^src/libstore-c/nix-meson-build-support$'' + ''^src/libstore-test-support/include/nix/store/tests/meson.build$'' + ''^src/libstore-test-support/meson.build$'' + ''^src/libstore-test-support/nix-meson-build-support$'' + ''^src/libstore-tests/meson.build$'' + ''^src/libstore-tests/nix-meson-build-support$'' + ''^src/libutil/meson.build$'' + ''^src/libutil/nix-meson-build-support$'' + ''^src/libutil/unix/include/nix/util/meson.build$'' + ''^src/libutil/unix/meson.build$'' + ''^src/libutil/windows/meson.build$'' + ''^src/libutil-c/meson.build$'' + ''^src/libutil-c/nix-meson-build-support$'' + ''^src/libutil-test-support/include/nix/util/tests/meson.build$'' + ''^src/libutil-test-support/meson.build$'' + ''^src/libutil-test-support/nix-meson-build-support$'' + ''^src/libutil-tests/meson.build$'' + ''^src/libutil-tests/nix-meson-build-support$'' + ''^src/nix/meson.build$'' + ''^src/nix/nix-meson-build-support$'' + ''^src/perl/lib/Nix/meson.build$'' + ''^src/perl/meson.build$'' + ''^tests/functional/ca/meson.build$'' + ''^tests/functional/common/meson.build$'' + ''^tests/functional/dyn-drv/meson.build$'' + ''^tests/functional/flakes/meson.build$'' + ''^tests/functional/git-hashing/meson.build$'' + ''^tests/functional/local-overlay-store/meson.build$'' + ''^tests/functional/meson.build$'' + ''^src/libcmd/meson.options$'' + ''^src/libexpr/meson.options$'' + ''^src/libstore/meson.options$'' + ''^src/libutil/meson.options$'' + ''^src/libutil-c/meson.options$'' + ''^src/nix/meson.options$'' + ''^src/perl/meson.options$'' + ]; + }; nixfmt-rfc-style = { enable = true; excludes = [ diff --git a/meson.build b/meson.build index 9f7471143..023791979 100644 --- a/meson.build +++ b/meson.build @@ -1,13 +1,13 @@ # This is just a stub project to include all the others as subprojects # for development shell purposes -project('nix-dev-shell', 'cpp', +project( + 'nix-dev-shell', + 'cpp', version : files('.version'), subproject_dir : 'src', - default_options : [ - 'localstatedir=/nix/var', - ], - meson_version : '>= 1.1' + default_options : [ 'localstatedir=/nix/var' ], + meson_version : '>= 1.1', ) # Internal Libraries diff --git a/meson.format b/meson.format new file mode 100644 index 000000000..4876dd962 --- /dev/null +++ b/meson.format @@ -0,0 +1,7 @@ +indent_by = ' ' +space_array = true +kwargs_force_multiline = false +wide_colon = true +group_arg_value = true +indent_before_comments = ' ' +use_editor_config = true diff --git a/meson.options b/meson.options index 329fe06bf..30670902e 100644 --- a/meson.options +++ b/meson.options @@ -1,13 +1,22 @@ # vim: filetype=meson -option('doc-gen', type : 'boolean', value : false, +option( + 'doc-gen', + type : 'boolean', + value : false, description : 'Generate documentation', ) -option('unit-tests', type : 'boolean', value : true, +option( + 'unit-tests', + type : 'boolean', + value : true, description : 'Build unit tests', ) -option('bindings', type : 'boolean', value : true, +option( + 'bindings', + type : 'boolean', + value : true, description : 'Build language bindings (e.g. Perl)', ) diff --git a/nix-meson-build-support/windows-version/meson.build b/nix-meson-build-support/windows-version/meson.build index 3a008e5df..ed4caaa9a 100644 --- a/nix-meson-build-support/windows-version/meson.build +++ b/nix-meson-build-support/windows-version/meson.build @@ -2,5 +2,5 @@ if host_machine.system() == 'windows' # https://learn.microsoft.com/en-us/cpp/porting/modifying-winver-and-win32-winnt?view=msvc-170 # #define _WIN32_WINNT_WIN8 0x0602 # We currently don't use any API which requires higher than this. - add_project_arguments([ '-D_WIN32_WINNT=0x0602' ], language: 'cpp') + add_project_arguments([ '-D_WIN32_WINNT=0x0602' ], language : 'cpp') endif diff --git a/src/libexpr-test-support/include/nix/expr/tests/meson.build b/src/libexpr-test-support/include/nix/expr/tests/meson.build index 710bd8d4e..e44d25582 100644 --- a/src/libexpr-test-support/include/nix/expr/tests/meson.build +++ b/src/libexpr-test-support/include/nix/expr/tests/meson.build @@ -1,9 +1,5 @@ # Public headers directory -include_dirs = [include_directories('../../..')] +include_dirs = [ include_directories('../../..') ] -headers = files( - 'libexpr.hh', - 'nix_api_expr.hh', - 'value/context.hh', -) +headers = files('libexpr.hh', 'nix_api_expr.hh', 'value/context.hh') diff --git a/src/libexpr/primops/meson.build b/src/libexpr/primops/meson.build index f910fe237..b8abc6409 100644 --- a/src/libexpr/primops/meson.build +++ b/src/libexpr/primops/meson.build @@ -1,6 +1,6 @@ generated_headers += gen_header.process( 'derivation.nix', - preserve_path_from: meson.project_source_root(), + preserve_path_from : meson.project_source_root(), ) sources += files( diff --git a/src/libstore/linux/include/nix/store/meson.build b/src/libstore/linux/include/nix/store/meson.build index a664aefa9..eb2ae2583 100644 --- a/src/libstore/linux/include/nix/store/meson.build +++ b/src/libstore/linux/include/nix/store/meson.build @@ -1,5 +1,3 @@ include_dirs += include_directories('../..') -headers += files( - 'personality.hh', -) +headers += files('personality.hh') diff --git a/src/libstore/linux/meson.build b/src/libstore/linux/meson.build index 6fc193cf8..93084a8ae 100644 --- a/src/libstore/linux/meson.build +++ b/src/libstore/linux/meson.build @@ -1,5 +1,3 @@ -sources += files( - 'personality.cc', -) +sources += files('personality.cc') subdir('include/nix/store') diff --git a/src/libutil/freebsd/include/nix/util/meson.build b/src/libutil/freebsd/include/nix/util/meson.build index 561c8796c..283c49b95 100644 --- a/src/libutil/freebsd/include/nix/util/meson.build +++ b/src/libutil/freebsd/include/nix/util/meson.build @@ -2,6 +2,4 @@ include_dirs += include_directories('../..') -headers += files( - 'freebsd-jail.hh', -) +headers += files('freebsd-jail.hh') diff --git a/src/libutil/freebsd/meson.build b/src/libutil/freebsd/meson.build index 8ffdc2832..34508efd0 100644 --- a/src/libutil/freebsd/meson.build +++ b/src/libutil/freebsd/meson.build @@ -1,5 +1,3 @@ -sources += files( - 'freebsd-jail.cc', -) +sources += files('freebsd-jail.cc') subdir('include/nix/util') diff --git a/src/libutil/include/nix/util/meson.build b/src/libutil/include/nix/util/meson.build index e30b8dacd..6c95a6ced 100644 --- a/src/libutil/include/nix/util/meson.build +++ b/src/libutil/include/nix/util/meson.build @@ -1,6 +1,6 @@ # Public headers directory -include_dirs = [include_directories('../..')] +include_dirs = [ include_directories('../..') ] headers = files( 'abstract-setting-to-json.hh', @@ -63,8 +63,8 @@ headers = files( 'source-path.hh', 'split.hh', 'std-hash.hh', - 'strings.hh', 'strings-inline.hh', + 'strings.hh', 'suggestions.hh', 'sync.hh', 'tarfile.hh', diff --git a/src/libutil/linux/include/nix/util/meson.build b/src/libutil/linux/include/nix/util/meson.build index e28ad8e05..ac5f318f8 100644 --- a/src/libutil/linux/include/nix/util/meson.build +++ b/src/libutil/linux/include/nix/util/meson.build @@ -2,7 +2,4 @@ include_dirs += include_directories('../..') -headers += files( - 'cgroup.hh', - 'linux-namespaces.hh', -) +headers += files('cgroup.hh', 'linux-namespaces.hh') diff --git a/src/libutil/linux/meson.build b/src/libutil/linux/meson.build index b8053a5bb..f3fe07c9c 100644 --- a/src/libutil/linux/meson.build +++ b/src/libutil/linux/meson.build @@ -1,6 +1,3 @@ -sources += files( - 'cgroup.cc', - 'linux-namespaces.cc', -) +sources += files('cgroup.cc', 'linux-namespaces.cc') subdir('include/nix/util') diff --git a/src/libutil/windows/include/nix/util/meson.build b/src/libutil/windows/include/nix/util/meson.build index 1bd56c4bd..f0d4c37e9 100644 --- a/src/libutil/windows/include/nix/util/meson.build +++ b/src/libutil/windows/include/nix/util/meson.build @@ -2,8 +2,4 @@ include_dirs += include_directories('../..') -headers += files( - 'signals-impl.hh', - 'windows-async-pipe.hh', - 'windows-error.hh', -) +headers += files('signals-impl.hh', 'windows-async-pipe.hh', 'windows-error.hh') diff --git a/src/perl/t/meson.build b/src/perl/t/meson.build index dbd1139f3..cd98453c3 100644 --- a/src/perl/t/meson.build +++ b/src/perl/t/meson.build @@ -5,9 +5,7 @@ # src #--------------------------------------------------- -nix_perl_tests = files( - 'init.t', -) +nix_perl_tests = files('init.t') foreach f : nix_perl_tests diff --git a/tests/functional/plugins/meson.build b/tests/functional/plugins/meson.build index ae66e3036..7cbc935c9 100644 --- a/tests/functional/plugins/meson.build +++ b/tests/functional/plugins/meson.build @@ -1,8 +1,6 @@ libplugintest = shared_module( 'plugintest', 'plugintest.cc', - dependencies : [ - dependency('nix-expr'), - ], + dependencies : [ dependency('nix-expr') ], build_by_default : false, ) diff --git a/tests/functional/test-libstoreconsumer/meson.build b/tests/functional/test-libstoreconsumer/meson.build index e5a1cc182..d27647ec4 100644 --- a/tests/functional/test-libstoreconsumer/meson.build +++ b/tests/functional/test-libstoreconsumer/meson.build @@ -1,8 +1,6 @@ libstoreconsumer_tester = executable( 'test-libstoreconsumer', 'main.cc', - dependencies : [ - dependency('nix-store'), - ], + dependencies : [ dependency('nix-store') ], build_by_default : false, ) From 57c72dee9b27b50b2a3b7a738cf33329acfb7236 Mon Sep 17 00:00:00 2001 From: Pol Dellaiera Date: Thu, 12 Jun 2025 09:41:37 +0200 Subject: [PATCH 02/22] docker: make sure `nix config check` works --- docker.nix | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docker.nix b/docker.nix index e3a0635d0..99316fc84 100644 --- a/docker.nix +++ b/docker.nix @@ -280,7 +280,6 @@ let ln -s ${profile} $out/nix/var/nix/profiles/default-1-link ln -s /nix/var/nix/profiles/default-1-link $out/nix/var/nix/profiles/default - ln -s /nix/var/nix/profiles/default $out${userHome}/.nix-profile ln -s ${channel} $out/nix/var/nix/profiles/per-user/${uname}/channels-1-link ln -s /nix/var/nix/profiles/per-user/${uname}/channels-1-link $out/nix/var/nix/profiles/per-user/${uname}/channels @@ -332,7 +331,7 @@ pkgs.dockerTools.buildLayeredImageWithNixDb { ''; config = { - Cmd = [ "${userHome}/.nix-profile/bin/bash" ]; + Cmd = [ (lib.getExe pkgs.bashInteractive) ]; User = "${toString uid}:${toString gid}"; Env = [ "USER=${uname}" From 5abaf361a41facb2baeee05aa58b9b5e3aa08f45 Mon Sep 17 00:00:00 2001 From: Pol Dellaiera Date: Thu, 12 Jun 2025 19:06:13 +0200 Subject: [PATCH 03/22] docker: reduce duplicates, use `coreutils-full` --- docker.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker.nix b/docker.nix index e3a0635d0..8262d1b70 100644 --- a/docker.nix +++ b/docker.nix @@ -290,7 +290,7 @@ let echo "${channelURL} ${channelName}" > $out${userHome}/.nix-channels mkdir -p $out/bin $out/usr/bin - ln -s ${pkgs.coreutils}/bin/env $out/usr/bin/env + ln -s ${pkgs.coreutils-full}/bin/env $out/usr/bin/env ln -s ${pkgs.bashInteractive}/bin/bash $out/bin/sh '' From 5862f38d00b3d84ee9272f90983a52ba1b9c71ee Mon Sep 17 00:00:00 2001 From: Pol Dellaiera Date: Thu, 12 Jun 2025 19:07:20 +0200 Subject: [PATCH 04/22] docker: use `callPackage`, parametrise the image build --- docker.nix | 90 +++++++++++++++++++++++--------------- flake.nix | 3 +- tests/nixos/nix-docker.nix | 10 +---- 3 files changed, 58 insertions(+), 45 deletions(-) diff --git a/docker.nix b/docker.nix index 8262d1b70..060dcd8f0 100644 --- a/docker.nix +++ b/docker.nix @@ -1,6 +1,10 @@ { - pkgs ? import { }, - lib ? pkgs.lib, + # Core dependencies + pkgs, + lib, + runCommand, + buildPackages, + # Image configuration name ? "nix", tag ? "latest", bundleNixpkgs ? true, @@ -14,36 +18,52 @@ gid ? 0, uname ? "root", gname ? "root", + # Default Packages + nix, + bashInteractive, + coreutils-full, + gnutar, + gzip, + gnugrep, + which, + curl, + less, + wget, + man, + cacert, + findutils, + iana-etc, + git, + openssh, + # Other dependencies + shadow, }: let - defaultPkgs = - with pkgs; - [ - nix - bashInteractive - coreutils-full - gnutar - gzip - gnugrep - which - curl - less - wget - man - cacert.out - findutils - iana-etc - git - openssh - ] - ++ extraPkgs; + defaultPkgs = [ + nix + bashInteractive + coreutils-full + gnutar + gzip + gnugrep + which + curl + less + wget + man + cacert.out + findutils + iana-etc + git + openssh + ] ++ extraPkgs; users = { root = { uid = 0; - shell = "${pkgs.bashInteractive}/bin/bash"; + shell = lib.getExe bashInteractive; home = "/root"; gid = 0; groups = [ "root" ]; @@ -52,7 +72,7 @@ let nobody = { uid = 65534; - shell = "${pkgs.shadow}/bin/nologin"; + shell = lib.getExe' shadow "nologin"; home = "/var/empty"; gid = 65534; groups = [ "nobody" ]; @@ -63,7 +83,7 @@ let // lib.optionalAttrs (uid != 0) { "${uname}" = { uid = uid; - shell = "${pkgs.bashInteractive}/bin/bash"; + shell = lib.getExe bashInteractive; home = "/home/${uname}"; gid = gid; groups = [ "${gname}" ]; @@ -170,7 +190,7 @@ let baseSystem = let nixpkgs = pkgs.path; - channel = pkgs.runCommand "channel-nixos" { inherit bundleNixpkgs; } '' + channel = runCommand "channel-nixos" { inherit bundleNixpkgs; } '' mkdir $out if [ "$bundleNixpkgs" ]; then ln -s ${ @@ -182,11 +202,11 @@ let echo "[]" > $out/manifest.nix fi ''; - rootEnv = pkgs.buildPackages.buildEnv { + rootEnv = buildPackages.buildEnv { name = "root-profile-env"; paths = defaultPkgs; }; - manifest = pkgs.buildPackages.runCommand "manifest.nix" { } '' + manifest = buildPackages.runCommand "manifest.nix" { } '' cat > $out < $out${userHome}/.nix-channels mkdir -p $out/bin $out/usr/bin - ln -s ${pkgs.coreutils-full}/bin/env $out/usr/bin/env - ln -s ${pkgs.bashInteractive}/bin/bash $out/bin/sh + ln -s ${lib.getExe' coreutils-full "env"} $out/usr/bin/env + ln -s ${lib.getExe bashInteractive} $out/bin/sh '' + (lib.optionalString (flake-registry-path != null) '' @@ -300,7 +320,7 @@ let globalFlakeRegistryPath="$nixCacheDir/flake-registry.json" ln -s ${flake-registry-path} $out$globalFlakeRegistryPath mkdir -p $out/nix/var/nix/gcroots/auto - rootName=$(${pkgs.nix}/bin/nix --extra-experimental-features nix-command hash file --type sha1 --base32 <(echo -n $globalFlakeRegistryPath)) + rootName=$(${lib.getExe' nix "nix"} --extra-experimental-features nix-command hash file --type sha1 --base32 <(echo -n $globalFlakeRegistryPath)) ln -s $globalFlakeRegistryPath $out/nix/var/nix/gcroots/auto/$rootName '') ); @@ -332,7 +352,7 @@ pkgs.dockerTools.buildLayeredImageWithNixDb { ''; config = { - Cmd = [ "${userHome}/.nix-profile/bin/bash" ]; + Cmd = [ (lib.getExe bashInteractive) ]; User = "${toString uid}:${toString gid}"; Env = [ "USER=${uname}" diff --git a/flake.nix b/flake.nix index 7d7c4d4c2..69bd2a21a 100644 --- a/flake.nix +++ b/flake.nix @@ -404,8 +404,7 @@ dockerImage = let pkgs = nixpkgsFor.${system}.native; - image = import ./docker.nix { - inherit pkgs; + image = pkgs.callPackage ./docker.nix { tag = pkgs.nix.version; }; in diff --git a/tests/nixos/nix-docker.nix b/tests/nixos/nix-docker.nix index c58a00cdd..f1c218585 100644 --- a/tests/nixos/nix-docker.nix +++ b/tests/nixos/nix-docker.nix @@ -1,21 +1,15 @@ # Test the container built by ../../docker.nix. { - lib, config, - nixpkgs, - hostPkgs, ... }: let pkgs = config.nodes.machine.nixpkgs.pkgs; - nixImage = import ../../docker.nix { - inherit (config.nodes.machine.nixpkgs) pkgs; - }; - nixUserImage = import ../../docker.nix { - inherit (config.nodes.machine.nixpkgs) pkgs; + nixImage = pkgs.callPackage ../../docker.nix { }; + nixUserImage = pkgs.callPackage ../../docker.nix { name = "nix-user"; uid = 1000; gid = 1000; From 6eb4ee68556af97ab88f3f909f3158e57429f6af Mon Sep 17 00:00:00 2001 From: Pol Dellaiera Date: Thu, 12 Jun 2025 19:18:07 +0200 Subject: [PATCH 05/22] docker: replace `git` with `gitMinimal` --- docker.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker.nix b/docker.nix index 060dcd8f0..e97ed1752 100644 --- a/docker.nix +++ b/docker.nix @@ -33,7 +33,7 @@ cacert, findutils, iana-etc, - git, + gitMinimal, openssh, # Other dependencies shadow, @@ -54,7 +54,7 @@ let cacert.out findutils iana-etc - git + gitMinimal openssh ] ++ extraPkgs; From 6587e7bcff6bfc7718cea96e64823fc2742d3d6e Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 12 Jun 2025 19:42:50 +0000 Subject: [PATCH 06/22] libexpr: Add and use `lambda` getter --- src/libcmd/repl.cc | 2 +- src/libexpr-tests/primops.cc | 4 ++-- src/libexpr/eval-profiler.cc | 2 +- src/libexpr/eval.cc | 22 +++++++++++----------- src/libexpr/include/nix/expr/value.hh | 3 +++ src/libexpr/primops.cc | 4 ++-- src/libexpr/print.cc | 8 ++++---- src/libexpr/value-to-xml.cc | 12 ++++++------ src/libflake/flake.cc | 4 ++-- src/nix-build/nix-build.cc | 4 ++-- src/nix/flake.cc | 4 ++-- 11 files changed, 36 insertions(+), 33 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index f01f12860..8170bd579 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -484,7 +484,7 @@ ProcessLineResult NixRepl::processLine(std::string line) auto path = state->coerceToPath(noPos, v, context, "while evaluating the filename to edit"); return {path, 0}; } else if (v.isLambda()) { - auto pos = state->positions[v.payload.lambda.fun->pos]; + auto pos = state->positions[v.lambda().fun->pos]; if (auto path = std::get_if(&pos.origin)) return {*path, pos.line}; else diff --git a/src/libexpr-tests/primops.cc b/src/libexpr-tests/primops.cc index 2f864f2c2..6c301f157 100644 --- a/src/libexpr-tests/primops.cc +++ b/src/libexpr-tests/primops.cc @@ -667,8 +667,8 @@ namespace nix { auto v = eval("derivation"); ASSERT_EQ(v.type(), nFunction); ASSERT_TRUE(v.isLambda()); - ASSERT_NE(v.payload.lambda.fun, nullptr); - ASSERT_TRUE(v.payload.lambda.fun->hasFormals()); + ASSERT_NE(v.lambda().fun, nullptr); + ASSERT_TRUE(v.lambda().fun->hasFormals()); } TEST_F(PrimOpTest, currentTime) { diff --git a/src/libexpr/eval-profiler.cc b/src/libexpr/eval-profiler.cc index 7053e4ec7..b65bc3a4d 100644 --- a/src/libexpr/eval-profiler.cc +++ b/src/libexpr/eval-profiler.cc @@ -204,7 +204,7 @@ FrameInfo SampleStack::getFrameInfoFromValueAndPos(const Value & v, std::spanpos; - case tLambda: return payload.lambda.fun->pos; + case tLambda: return lambda().fun->pos; case tApp: return payload.app.left->determinePos(pos); default: return pos; } @@ -610,7 +610,7 @@ std::optional EvalState::getDoc(Value & v) }; } if (v.isLambda()) { - auto exprLambda = v.payload.lambda.fun; + auto exprLambda = v.lambda().fun; std::ostringstream s; std::string name; @@ -1567,13 +1567,13 @@ void EvalState::callFunction(Value & fun, std::span args, Value & vRes, if (vCur.isLambda()) { - ExprLambda & lambda(*vCur.payload.lambda.fun); + ExprLambda & lambda(*vCur.lambda().fun); auto size = (!lambda.arg ? 0 : 1) + (lambda.hasFormals() ? lambda.formals->formals.size() : 0); Env & env2(allocEnv(size)); - env2.up = vCur.payload.lambda.env; + env2.up = vCur.lambda().env; Displacement displ = 0; @@ -1603,7 +1603,7 @@ void EvalState::callFunction(Value & fun, std::span args, Value & vRes, symbols[i.name]) .atPos(lambda.pos) .withTrace(pos, "from call site") - .withFrame(*fun.payload.lambda.env, lambda) + .withFrame(*fun.lambda().env, lambda) .debugThrow(); } env2.values[displ++] = i.def->maybeThunk(*this, env2); @@ -1630,7 +1630,7 @@ void EvalState::callFunction(Value & fun, std::span args, Value & vRes, .atPos(lambda.pos) .withTrace(pos, "from call site") .withSuggestions(suggestions) - .withFrame(*fun.payload.lambda.env, lambda) + .withFrame(*fun.lambda().env, lambda) .debugThrow(); } unreachable(); @@ -1825,14 +1825,14 @@ void EvalState::autoCallFunction(const Bindings & args, Value & fun, Value & res } } - if (!fun.isLambda() || !fun.payload.lambda.fun->hasFormals()) { + if (!fun.isLambda() || !fun.lambda().fun->hasFormals()) { res = fun; return; } - auto attrs = buildBindings(std::max(static_cast(fun.payload.lambda.fun->formals->formals.size()), args.size())); + auto attrs = buildBindings(std::max(static_cast(fun.lambda().fun->formals->formals.size()), args.size())); - if (fun.payload.lambda.fun->formals->ellipsis) { + if (fun.lambda().fun->formals->ellipsis) { // If the formals have an ellipsis (eg the function accepts extra args) pass // all available automatic arguments (which includes arguments specified on // the command line via --arg/--argstr) @@ -1840,7 +1840,7 @@ void EvalState::autoCallFunction(const Bindings & args, Value & fun, Value & res attrs.insert(v); } else { // Otherwise, only pass the arguments that the function accepts - for (auto & i : fun.payload.lambda.fun->formals->formals) { + for (auto & i : fun.lambda().fun->formals->formals) { auto j = args.get(i.name); if (j) { attrs.insert(*j); @@ -1850,7 +1850,7 @@ Nix attempted to evaluate a function as a top level expression; in this case it must have its arguments supplied either by default values, or passed explicitly with '--arg' or '--argstr'. See https://nixos.org/manual/nix/stable/language/constructs.html#functions.)", symbols[i.name]) - .atPos(i.pos).withFrame(*fun.payload.lambda.env, *fun.payload.lambda.fun).debugThrow(); + .atPos(i.pos).withFrame(*fun.lambda().env, *fun.lambda().fun).debugThrow(); } } } diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index febe36f80..26ec5ff38 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -482,6 +482,9 @@ public: NixFloat fpoint() const { return payload.fpoint; } + + Lambda lambda() const + { return payload.lambda; } }; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 48bc272f9..a24695bcd 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3138,12 +3138,12 @@ static void prim_functionArgs(EvalState & state, const PosIdx pos, Value * * arg if (!args[0]->isLambda()) state.error("'functionArgs' requires a function").atPos(pos).debugThrow(); - if (!args[0]->payload.lambda.fun->hasFormals()) { + if (!args[0]->lambda().fun->hasFormals()) { v.mkAttrs(&state.emptyBindings); return; } - const auto &formals = args[0]->payload.lambda.fun->formals->formals; + const auto &formals = args[0]->lambda().fun->formals->formals; auto attrs = state.buildBindings(formals.size()); for (auto & i : formals) attrs.insert(i.name, state.getBool(i.def), i.pos); diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 06bae9c5c..f0e0eed2d 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -453,13 +453,13 @@ private: if (v.isLambda()) { output << "lambda"; - if (v.payload.lambda.fun) { - if (v.payload.lambda.fun->name) { - output << " " << state.symbols[v.payload.lambda.fun->name]; + if (v.lambda().fun) { + if (v.lambda().fun->name) { + output << " " << state.symbols[v.lambda().fun->name]; } std::ostringstream s; - s << state.positions[v.payload.lambda.fun->pos]; + s << state.positions[v.lambda().fun->pos]; output << " @ " << filterANSIEscapes(toView(s)); } } else if (v.isPrimOp()) { diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc index e26fff71b..54ff06f9e 100644 --- a/src/libexpr/value-to-xml.cc +++ b/src/libexpr/value-to-xml.cc @@ -126,18 +126,18 @@ static void printValueAsXML(EvalState & state, bool strict, bool location, break; } XMLAttrs xmlAttrs; - if (location) posToXML(state, xmlAttrs, state.positions[v.payload.lambda.fun->pos]); + if (location) posToXML(state, xmlAttrs, state.positions[v.lambda().fun->pos]); XMLOpenElement _(doc, "function", xmlAttrs); - if (v.payload.lambda.fun->hasFormals()) { + if (v.lambda().fun->hasFormals()) { XMLAttrs attrs; - if (v.payload.lambda.fun->arg) attrs["name"] = state.symbols[v.payload.lambda.fun->arg]; - if (v.payload.lambda.fun->formals->ellipsis) attrs["ellipsis"] = "1"; + if (v.lambda().fun->arg) attrs["name"] = state.symbols[v.lambda().fun->arg]; + if (v.lambda().fun->formals->ellipsis) attrs["ellipsis"] = "1"; XMLOpenElement _(doc, "attrspat", attrs); - for (auto & i : v.payload.lambda.fun->formals->lexicographicOrder(state.symbols)) + for (auto & i : v.lambda().fun->formals->lexicographicOrder(state.symbols)) doc.writeEmptyElement("attr", singletonAttrs("name", state.symbols[i.name])); } else - doc.writeEmptyElement("varpat", singletonAttrs("name", state.symbols[v.payload.lambda.fun->arg])); + doc.writeEmptyElement("varpat", singletonAttrs("name", state.symbols[v.lambda().fun->arg])); break; } diff --git a/src/libflake/flake.cc b/src/libflake/flake.cc index 24252d710..41141d41d 100644 --- a/src/libflake/flake.cc +++ b/src/libflake/flake.cc @@ -252,8 +252,8 @@ static Flake readFlake( if (auto outputs = vInfo.attrs()->get(sOutputs)) { expectType(state, nFunction, *outputs->value, outputs->pos); - if (outputs->value->isLambda() && outputs->value->payload.lambda.fun->hasFormals()) { - for (auto & formal : outputs->value->payload.lambda.fun->formals->formals) { + if (outputs->value->isLambda() && outputs->value->lambda().fun->hasFormals()) { + for (auto & formal : outputs->value->lambda().fun->formals->formals) { if (formal.name != state.sSelf) flake.inputs.emplace(state.symbols[formal.name], FlakeInput { .ref = parseFlakeRef(state.fetchSettings, std::string(state.symbols[formal.name])) diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 80ebf6bfa..e302aca67 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -387,8 +387,8 @@ static void main_nix_build(int argc, char * * argv) return false; } bool add = false; - if (v.type() == nFunction && v.payload.lambda.fun->hasFormals()) { - for (auto & i : v.payload.lambda.fun->formals->formals) { + if (v.type() == nFunction && v.lambda().fun->hasFormals()) { + for (auto & i : v.lambda().fun->formals->formals) { if (state->symbols[i.name] == "inNixShell") { add = true; break; diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 95cf85663..13f7363fc 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -492,8 +492,8 @@ struct CmdFlakeCheck : FlakeCommand if (!v.isLambda()) { throw Error("overlay is not a function, but %s instead", showType(v)); } - if (v.payload.lambda.fun->hasFormals() - || !argHasName(v.payload.lambda.fun->arg, "final")) + if (v.lambda().fun->hasFormals() + || !argHasName(v.lambda().fun->arg, "final")) throw Error("overlay does not take an argument named 'final'"); // FIXME: if we have a 'nixpkgs' input, use it to // evaluate the overlay. From 441fa86e82da65a71e01a5c45e3459287ab4d01e Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 12 Jun 2025 19:48:42 +0000 Subject: [PATCH 07/22] libexpr: Add and use `thunk` getter --- src/libexpr/eval.cc | 10 +++++----- src/libexpr/include/nix/expr/eval-inline.hh | 4 ++-- src/libexpr/include/nix/expr/value.hh | 5 ++++- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 51036a223..badb271f2 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -160,10 +160,10 @@ bool Value::isTrivial() const internalType != tApp && internalType != tPrimOpApp && (internalType != tThunk - || (dynamic_cast(payload.thunk.expr) - && ((ExprAttrs *) payload.thunk.expr)->dynamicAttrs.empty()) - || dynamic_cast(payload.thunk.expr) - || dynamic_cast(payload.thunk.expr)); + || (dynamic_cast(thunk().expr) + && ((ExprAttrs *) thunk().expr)->dynamicAttrs.empty()) + || dynamic_cast(thunk().expr) + || dynamic_cast(thunk().expr)); } @@ -2163,7 +2163,7 @@ void EvalState::forceValueDeep(Value & v) try { // If the value is a thunk, we're evaling. Otherwise no trace necessary. auto dts = debugRepl && i.value->isThunk() - ? makeDebugTraceStacker(*this, *i.value->payload.thunk.expr, *i.value->payload.thunk.env, i.pos, + ? makeDebugTraceStacker(*this, *i.value->thunk().expr, *i.value->thunk().env, i.pos, "while evaluating the attribute '%1%'", symbols[i.name]) : nullptr; diff --git a/src/libexpr/include/nix/expr/eval-inline.hh b/src/libexpr/include/nix/expr/eval-inline.hh index 6e5759c0b..97bf71a6b 100644 --- a/src/libexpr/include/nix/expr/eval-inline.hh +++ b/src/libexpr/include/nix/expr/eval-inline.hh @@ -89,9 +89,9 @@ Env & EvalState::allocEnv(size_t size) void EvalState::forceValue(Value & v, const PosIdx pos) { if (v.isThunk()) { - Env * env = v.payload.thunk.env; + Env * env = v.thunk().env; assert(env || v.isBlackhole()); - Expr * expr = v.payload.thunk.expr; + Expr * expr = v.thunk().expr; try { v.mkBlackhole(); //checkInterrupt(); diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 26ec5ff38..29f8ac379 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -485,6 +485,9 @@ public: Lambda lambda() const { return payload.lambda; } + + ClosureThunk thunk() const + { return payload.thunk; } }; @@ -492,7 +495,7 @@ extern ExprBlackHole eBlackHole; bool Value::isBlackhole() const { - return internalType == tThunk && payload.thunk.expr == (Expr*) &eBlackHole; + return internalType == tThunk && thunk().expr == (Expr*) &eBlackHole; } void Value::mkBlackhole() From f07a9f863e16bb71ec330abf6c2ec5d5bff68788 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 12 Jun 2025 19:51:44 +0000 Subject: [PATCH 08/22] libexpr: Add and use `primOpApp` getter --- src/libexpr/eval.cc | 10 +++++----- src/libexpr/include/nix/expr/value.hh | 3 +++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index badb271f2..f9bff7b98 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -525,9 +525,9 @@ std::ostream & operator<<(std::ostream & output, const PrimOp & primOp) const PrimOp * Value::primOpAppPrimOp() const { - Value * left = payload.primOpApp.left; + Value * left = primOpApp().left; while (left && !left->isPrimOp()) { - left = left->payload.primOpApp.left; + left = left->primOpApp().left; } if (!left) @@ -1702,7 +1702,7 @@ void EvalState::callFunction(Value & fun, std::span args, Value & vRes, Value * primOp = &vCur; while (primOp->isPrimOpApp()) { argsDone++; - primOp = primOp->payload.primOpApp.left; + primOp = primOp->primOpApp().left; } assert(primOp->isPrimOp()); auto arity = primOp->primOp()->arity; @@ -1718,8 +1718,8 @@ void EvalState::callFunction(Value & fun, std::span args, Value & vRes, Value * vArgs[maxPrimOpArity]; auto n = argsDone; - for (Value * arg = &vCur; arg->isPrimOpApp(); arg = arg->payload.primOpApp.left) - vArgs[--n] = arg->payload.primOpApp.right; + for (Value * arg = &vCur; arg->isPrimOpApp(); arg = arg->primOpApp().left) + vArgs[--n] = arg->primOpApp().right; for (size_t i = 0; i < argsLeft; ++i) vArgs[argsDone + i] = args[i]; diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 29f8ac379..797e31191 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -488,6 +488,9 @@ public: ClosureThunk thunk() const { return payload.thunk; } + + FunctionApplicationThunk primOpApp() const + { return payload.primOpApp; } }; From c041d71406f706d971ee0ad53d555450de7ad03d Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 12 Jun 2025 19:53:44 +0000 Subject: [PATCH 09/22] libexpr: Add and use `app` getter --- src/libexpr/eval.cc | 2 +- src/libexpr/include/nix/expr/eval-inline.hh | 2 +- src/libexpr/include/nix/expr/value.hh | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index f9bff7b98..37018007f 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -148,7 +148,7 @@ PosIdx Value::determinePos(const PosIdx pos) const switch (internalType) { case tAttrs: return attrs()->pos; case tLambda: return lambda().fun->pos; - case tApp: return payload.app.left->determinePos(pos); + case tApp: return app().left->determinePos(pos); default: return pos; } #pragma GCC diagnostic pop diff --git a/src/libexpr/include/nix/expr/eval-inline.hh b/src/libexpr/include/nix/expr/eval-inline.hh index 97bf71a6b..7d13d7cc7 100644 --- a/src/libexpr/include/nix/expr/eval-inline.hh +++ b/src/libexpr/include/nix/expr/eval-inline.hh @@ -106,7 +106,7 @@ void EvalState::forceValue(Value & v, const PosIdx pos) } } else if (v.isApp()) - callFunction(*v.payload.app.left, *v.payload.app.right, v, pos); + callFunction(*v.app().left, *v.app().right, v, pos); } diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 797e31191..d25511c45 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -491,6 +491,9 @@ public: FunctionApplicationThunk primOpApp() const { return payload.primOpApp; } + + FunctionApplicationThunk app() const + { return payload.app; } }; From e4df1891237b58050e7b10380f7f62551ade2783 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 12 Jun 2025 19:57:46 +0000 Subject: [PATCH 10/22] libexpr: Add and use `pathStr` getter --- src/libexpr-c/nix_api_value.cc | 2 +- src/libexpr/eval.cc | 6 +++--- src/libexpr/include/nix/expr/value.hh | 5 ++++- src/libexpr/primops.cc | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 298d94845..8afe35a4b 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -252,7 +252,7 @@ const char * nix_get_path_string(nix_c_context * context, const nix_value * valu // We could use v.path().to_string().c_str(), but I'm concerned this // crashes. Looks like .path() allocates a CanonPath with a copy of the // string, then it gets the underlying data from that. - return v.payload.path.path; + return v.pathStr(); } NIXC_CATCH_ERRS_NULL } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 37018007f..66ac2e9fd 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2368,7 +2368,7 @@ BackedStringView EvalState::coerceToString( !canonicalizePath && !copyToStore ? // FIXME: hack to preserve path literals that end in a // slash, as in /foo/${x}. - v.payload.path.path + v.pathStr() : copyToStore ? store->printStorePath(copyPathToStore(context, v.path())) : std::string(v.path().path.abs()); @@ -2643,7 +2643,7 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st ValuePrinter(*this, v2, errorPrintOptions)) .debugThrow(); } - if (strcmp(v1.payload.path.path, v2.payload.path.path) != 0) { + if (strcmp(v1.pathStr(), v2.pathStr()) != 0) { error( "path '%s' is not equal to path '%s'", ValuePrinter(*this, v1, errorPrintOptions), @@ -2811,7 +2811,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v return // FIXME: compare accessors by their fingerprint. v1.payload.path.accessor == v2.payload.path.accessor - && strcmp(v1.payload.path.path, v2.payload.path.path) == 0; + && strcmp(v1.pathStr(), v2.pathStr()) == 0; case nNull: return true; diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index d25511c45..fc9ce1b39 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -445,7 +445,7 @@ public: assert(internalType == tPath); return SourcePath( ref(payload.path.accessor->shared_from_this()), - CanonPath(CanonPath::unchecked_t(), payload.path.path)); + CanonPath(CanonPath::unchecked_t(), pathStr())); } std::string_view string_view() const @@ -494,6 +494,9 @@ public: FunctionApplicationThunk app() const { return payload.app; } + + const char * pathStr() const + { return payload.path.path; } }; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index a24695bcd..60f44ca62 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -651,7 +651,7 @@ struct CompareValues // Note: we don't take the accessor into account // since it's not obvious how to compare them in a // reproducible way. - return strcmp(v1->payload.path.path, v2->payload.path.path) < 0; + return strcmp(v1->pathStr(), v2->pathStr()) < 0; case nList: // Lexicographic comparison for (size_t i = 0;; i++) { From bc6b52aff012592a9794df979c0617c85f46c2c3 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 12 Jun 2025 20:01:38 +0000 Subject: [PATCH 11/22] libexpr: Add and use `pathAccessor` getter --- src/libexpr/eval.cc | 4 ++-- src/libexpr/include/nix/expr/value.hh | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 66ac2e9fd..ae422a3d4 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2636,7 +2636,7 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st return; case nPath: - if (v1.payload.path.accessor != v2.payload.path.accessor) { + if (v1.pathAccessor() != v2.pathAccessor()) { error( "path '%s' is not equal to path '%s' because their accessors are different", ValuePrinter(*this, v1, errorPrintOptions), @@ -2810,7 +2810,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v case nPath: return // FIXME: compare accessors by their fingerprint. - v1.payload.path.accessor == v2.payload.path.accessor + v1.pathAccessor() == v2.pathAccessor() && strcmp(v1.pathStr(), v2.pathStr()) == 0; case nNull: diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index fc9ce1b39..80bee59e9 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -444,7 +444,7 @@ public: { assert(internalType == tPath); return SourcePath( - ref(payload.path.accessor->shared_from_this()), + ref(pathAccessor()->shared_from_this()), CanonPath(CanonPath::unchecked_t(), pathStr())); } @@ -497,6 +497,9 @@ public: const char * pathStr() const { return payload.path.path; } + + SourceAccessor * pathAccessor() const + { return payload.path.accessor; } }; From 7b46eb9958e7681958020b68d9ea30a52be7cf09 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 12 Jun 2025 22:29:05 +0000 Subject: [PATCH 12/22] libexpr: Remove non-const overload of `listElems` This overload isn't actually necessary anywhere and doesn't make much sense. The pointers to `Value`s are themselves const, but the `Value`s are mutable. A non-const member function implies that the object itself can be modified but this doesn't make much sense considering the return type: `Value * const * `, which is a pointer to a constant array of pointers to mutable values. --- src/libexpr/include/nix/expr/value.hh | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 80bee59e9..fcc118c7e 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -410,11 +410,6 @@ public: return internalType == tList1 || internalType == tList2 || internalType == tListN; } - Value * const * listElems() - { - return internalType == tList1 || internalType == tList2 ? payload.smallList : payload.bigList.elems; - } - std::span listItems() const { assert(isList()); From 699db04df3e755b9c3411567ca332709c39ba07d Mon Sep 17 00:00:00 2001 From: jayeshv Date: Fri, 13 Jun 2025 12:28:27 +0200 Subject: [PATCH 13/22] Fix a minor typo --- doc/manual/source/installation/installing-binary.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual/source/installation/installing-binary.md b/doc/manual/source/installation/installing-binary.md index 6a1a5ddca..21c156374 100644 --- a/doc/manual/source/installation/installing-binary.md +++ b/doc/manual/source/installation/installing-binary.md @@ -25,7 +25,7 @@ This performs the default type of installation for your platform: We recommend the multi-user installation if it supports your platform and you can authenticate with `sudo`. -The installer can configured with various command line arguments and environment variables. +The installer can be configured with various command line arguments and environment variables. To show available command line flags: ```console From e27a06278376a3721d06a6cc88ae711bd0937406 Mon Sep 17 00:00:00 2001 From: Pol Dellaiera Date: Sat, 14 Jun 2025 10:37:36 +0200 Subject: [PATCH 14/22] docker: remove last use of `pkgs.` Follow-up of https://github.com/NixOS/nix/pull/13354 --- docker.nix | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docker.nix b/docker.nix index 598d1a125..14648f0d1 100644 --- a/docker.nix +++ b/docker.nix @@ -2,6 +2,7 @@ # Core dependencies pkgs, lib, + dockerTools, runCommand, buildPackages, # Image configuration @@ -325,7 +326,7 @@ let ); in -pkgs.dockerTools.buildLayeredImageWithNixDb { +dockerTools.buildLayeredImageWithNixDb { inherit name From b2596a76158e20b07d9edac48fcb56e7cc8ec0b7 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 15 Jun 2025 16:51:38 +0000 Subject: [PATCH 15/22] libutil: Add custom PeekSort implementation Unlike std::sort and std::stable_sort, this implementation does not lead to out-of-bounds memory reads or other undefined behavior when the predicate is not strict weak ordering. This makes it possible to use this function in libexpr for builtins.sort, where an incorrectly implemented comparator in the user nix code currently can crash and burn the evaluator by invoking C++ UB. --- src/libutil-tests/meson.build | 1 + src/libutil-tests/sort.cc | 274 +++++++++++++++++++++ src/libutil/include/nix/util/meson.build | 1 + src/libutil/include/nix/util/sort.hh | 299 +++++++++++++++++++++++ 4 files changed, 575 insertions(+) create mode 100644 src/libutil-tests/sort.cc create mode 100644 src/libutil/include/nix/util/sort.hh diff --git a/src/libutil-tests/meson.build b/src/libutil-tests/meson.build index f2552550d..b3776e094 100644 --- a/src/libutil-tests/meson.build +++ b/src/libutil-tests/meson.build @@ -65,6 +65,7 @@ sources = files( 'position.cc', 'processes.cc', 'references.cc', + 'sort.cc', 'spawn.cc', 'strings.cc', 'suggestions.cc', diff --git a/src/libutil-tests/sort.cc b/src/libutil-tests/sort.cc new file mode 100644 index 000000000..8eee961c8 --- /dev/null +++ b/src/libutil-tests/sort.cc @@ -0,0 +1,274 @@ +#include +#include +#include "nix/util/sort.hh" + +#include +#include +#include +#include + +namespace nix { + +struct MonotonicSubranges : public ::testing::Test +{ + std::vector empty_; + std::vector basic_ = {1, 0, -1, -100, 10, 10, 20, 40, 5, 5, 20, 10, 10, 1, -5}; +}; + +TEST_F(MonotonicSubranges, empty) +{ + ASSERT_EQ(weaklyIncreasingPrefix(empty_.begin(), empty_.end()), empty_.begin()); + ASSERT_EQ(weaklyIncreasingSuffix(empty_.begin(), empty_.end()), empty_.begin()); + ASSERT_EQ(strictlyDecreasingPrefix(empty_.begin(), empty_.end()), empty_.begin()); + ASSERT_EQ(strictlyDecreasingSuffix(empty_.begin(), empty_.end()), empty_.begin()); +} + +TEST_F(MonotonicSubranges, basic) +{ + ASSERT_EQ(strictlyDecreasingPrefix(basic_.begin(), basic_.end()), basic_.begin() + 4); + ASSERT_EQ(strictlyDecreasingSuffix(basic_.begin(), basic_.end()), basic_.begin() + 12); + std::reverse(basic_.begin(), basic_.end()); + ASSERT_EQ(weaklyIncreasingPrefix(basic_.begin(), basic_.end()), basic_.begin() + 5); + ASSERT_EQ(weaklyIncreasingSuffix(basic_.begin(), basic_.end()), basic_.begin() + 11); +} + +template +class SortTestPermutations : public ::testing::Test +{ + std::vector initialData = {std::numeric_limits::max(), std::numeric_limits::min(), 0, 0, 42, 126, 36}; + std::vector vectorData; + std::list listData; + +public: + std::vector scratchVector; + std::list scratchList; + std::vector empty; + + void SetUp() override + { + vectorData = initialData; + std::sort(vectorData.begin(), vectorData.end()); + listData = std::list(vectorData.begin(), vectorData.end()); + } + + bool nextPermutation() + { + std::next_permutation(vectorData.begin(), vectorData.end()); + std::next_permutation(listData.begin(), listData.end()); + scratchList = listData; + scratchVector = vectorData; + return vectorData == initialData; + } +}; + +using SortPermutationsTypes = ::testing::Types; + +TYPED_TEST_SUITE(SortTestPermutations, SortPermutationsTypes); + +TYPED_TEST(SortTestPermutations, insertionsort) +{ + while (!this->nextPermutation()) { + auto & list = this->scratchList; + insertionsort(list.begin(), list.end()); + ASSERT_TRUE(std::is_sorted(list.begin(), list.end())); + auto & vector = this->scratchVector; + insertionsort(vector.begin(), vector.end()); + ASSERT_TRUE(std::is_sorted(vector.begin(), vector.end())); + } +} + +TYPED_TEST(SortTestPermutations, peeksort) +{ + while (!this->nextPermutation()) { + auto & vector = this->scratchVector; + peeksort(vector.begin(), vector.end()); + ASSERT_TRUE(std::is_sorted(vector.begin(), vector.end())); + } +} + +TEST(InsertionSort, empty) +{ + std::vector empty; + insertionsort(empty.begin(), empty.end()); +} + +struct RandomPeekSort : public ::testing::TestWithParam< + std::tuple> +{ + using ValueType = int; + std::vector data_; + std::mt19937 urng_; + std::uniform_int_distribution distribution_; + + void SetUp() override + { + auto [maxSize, min, max, iterations] = GetParam(); + urng_ = std::mt19937(GTEST_FLAG_GET(random_seed)); + distribution_ = std::uniform_int_distribution(min, max); + } + + auto regenerate() + { + auto [maxSize, min, max, iterations] = GetParam(); + std::size_t dataSize = std::uniform_int_distribution(0, maxSize)(urng_); + data_.resize(dataSize); + std::generate(data_.begin(), data_.end(), [&]() { return distribution_(urng_); }); + } +}; + +TEST_P(RandomPeekSort, defaultComparator) +{ + auto [maxSize, min, max, iterations] = GetParam(); + + for (std::size_t i = 0; i < iterations; ++i) { + regenerate(); + peeksort(data_.begin(), data_.end()); + ASSERT_TRUE(std::is_sorted(data_.begin(), data_.end())); + /* Sorting is idempotent */ + peeksort(data_.begin(), data_.end()); + ASSERT_TRUE(std::is_sorted(data_.begin(), data_.end())); + } +} + +TEST_P(RandomPeekSort, greater) +{ + auto [maxSize, min, max, iterations] = GetParam(); + + for (std::size_t i = 0; i < iterations; ++i) { + regenerate(); + peeksort(data_.begin(), data_.end(), std::greater{}); + ASSERT_TRUE(std::is_sorted(data_.begin(), data_.end(), std::greater{})); + /* Sorting is idempotent */ + peeksort(data_.begin(), data_.end(), std::greater{}); + ASSERT_TRUE(std::is_sorted(data_.begin(), data_.end(), std::greater{})); + } +} + +TEST_P(RandomPeekSort, brokenComparator) +{ + auto [maxSize, min, max, iterations] = GetParam(); + + /* This is a pretty nice way of modeling a worst-case scenario for a broken comparator. + If the sorting algorithm doesn't break in such case, then surely all deterministic + predicates won't break it. */ + auto comp = [&]([[maybe_unused]] const auto & lhs, [[maybe_unused]] const auto & rhs) -> bool { + return std::uniform_int_distribution(0, 1)(urng_); + }; + + for (std::size_t i = 0; i < iterations; ++i) { + regenerate(); + auto originalData = data_; + peeksort(data_.begin(), data_.end(), comp); + /* Check that the output is just a reordering of the input. This is the + contract of the implementation in regard to comparators that don't + define a strict weak order. */ + std::sort(data_.begin(), data_.end()); + std::sort(originalData.begin(), originalData.end()); + ASSERT_EQ(originalData, data_); + } +} + +TEST_P(RandomPeekSort, stability) +{ + auto [maxSize, min, max, iterations] = GetParam(); + + for (std::size_t i = 0; i < iterations; ++i) { + regenerate(); + std::vector> pairs; + + /* Assign sequential ids to objects. After the sort ids for equivalent + elements should be in ascending order. */ + std::transform( + data_.begin(), data_.end(), std::back_inserter(pairs), [id = std::size_t{0}](auto && val) mutable { + return std::pair{val, ++id}; + }); + + auto comp = [&]([[maybe_unused]] const auto & lhs, [[maybe_unused]] const auto & rhs) -> bool { + return lhs.first > rhs.first; + }; + + peeksort(pairs.begin(), pairs.end(), comp); + ASSERT_TRUE(std::is_sorted(pairs.begin(), pairs.end(), comp)); + + for (auto begin = pairs.begin(), end = pairs.end(); begin < end; ++begin) { + auto key = begin->first; + auto innerEnd = std::find_if_not(begin, end, [key](const auto & lhs) { return lhs.first == key; }); + ASSERT_TRUE(std::is_sorted(begin, innerEnd, [](const auto & lhs, const auto & rhs) { + return lhs.second < rhs.second; + })); + begin = innerEnd; + } + } +} + +using RandomPeekSortParamType = RandomPeekSort::ParamType; + +INSTANTIATE_TEST_SUITE_P( + PeekSort, + RandomPeekSort, + ::testing::Values( + RandomPeekSortParamType{128, std::numeric_limits::min(), std::numeric_limits::max(), 1024}, + RandomPeekSortParamType{7753, -32, 32, 128}, + RandomPeekSortParamType{11719, std::numeric_limits::min(), std::numeric_limits::max(), 64}, + RandomPeekSortParamType{4063, 0, 32, 256}, + RandomPeekSortParamType{771, -8, 8, 2048}, + RandomPeekSortParamType{433, 0, 1, 2048}, + RandomPeekSortParamType{0, 0, 0, 1}, /* empty case */ + RandomPeekSortParamType{ + 1, std::numeric_limits::min(), std::numeric_limits::max(), 1}, /* single element */ + RandomPeekSortParamType{ + 2, std::numeric_limits::min(), std::numeric_limits::max(), 2}, /* two elements */ + RandomPeekSortParamType{55425, std::numeric_limits::min(), std::numeric_limits::max(), 128})); + +template +struct SortProperty : public ::testing::Test +{}; + +using SortPropertyTypes = ::testing::Types; +TYPED_TEST_SUITE(SortProperty, SortPropertyTypes); + +RC_GTEST_TYPED_FIXTURE_PROP(SortProperty, peeksortSorted, (std::vector vec)) +{ + peeksort(vec.begin(), vec.end()); + RC_ASSERT(std::is_sorted(vec.begin(), vec.end())); +} + +RC_GTEST_TYPED_FIXTURE_PROP(SortProperty, peeksortSortedGreater, (std::vector vec)) +{ + auto comp = std::greater(); + peeksort(vec.begin(), vec.end(), comp); + RC_ASSERT(std::is_sorted(vec.begin(), vec.end(), comp)); +} + +RC_GTEST_TYPED_FIXTURE_PROP(SortProperty, insertionsortSorted, (std::vector vec)) +{ + insertionsort(vec.begin(), vec.end()); + RC_ASSERT(std::is_sorted(vec.begin(), vec.end())); +} + +RC_GTEST_PROP(SortProperty, peeksortStability, (std::vector> vec)) +{ + auto comp = [](auto lhs, auto rhs) { return lhs.first < rhs.first; }; + auto copy = vec; + std::stable_sort(copy.begin(), copy.end(), comp); + peeksort(vec.begin(), vec.end(), comp); + RC_ASSERT(copy == vec); +} + +RC_GTEST_TYPED_FIXTURE_PROP(SortProperty, peeksortSortedLinearComparisonComplexity, (std::vector vec)) +{ + peeksort(vec.begin(), vec.end()); + RC_ASSERT(std::is_sorted(vec.begin(), vec.end())); + std::size_t comparisonCount = 0; + auto countingComp = [&](auto lhs, auto rhs) { + ++comparisonCount; + return lhs < rhs; + }; + + peeksort(vec.begin(), vec.end(), countingComp); + + /* In the sorted case comparison complexify should be linear. */ + RC_ASSERT(comparisonCount <= vec.size()); +} + +} // namespace nix diff --git a/src/libutil/include/nix/util/meson.build b/src/libutil/include/nix/util/meson.build index 6c95a6ced..22438c1d0 100644 --- a/src/libutil/include/nix/util/meson.build +++ b/src/libutil/include/nix/util/meson.build @@ -59,6 +59,7 @@ headers = files( 'signals.hh', 'signature/local-keys.hh', 'signature/signer.hh', + 'sort.hh', 'source-accessor.hh', 'source-path.hh', 'split.hh', diff --git a/src/libutil/include/nix/util/sort.hh b/src/libutil/include/nix/util/sort.hh new file mode 100644 index 000000000..0affdf3ce --- /dev/null +++ b/src/libutil/include/nix/util/sort.hh @@ -0,0 +1,299 @@ +#pragma once + +#include +#include +#include +#include +#include +#include + +/** + * @file + * + * In-house implementation of sorting algorithms. Used for cases when several properties + * need to be upheld regardless of the stdlib implementation of std::sort or + * std::stable_sort. + * + * PeekSort implementation is adapted from reference implementation + * https://github.com/sebawild/powersort licensed under the MIT License. + * + */ + +/* PeekSort attribution: + * + * MIT License + * + * Copyright (c) 2022 Sebastian Wild + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +namespace nix { + +/** + * Merge sorted runs [begin, middle) with [middle, end) in-place [begin, end). + * Uses a temporary working buffer by first copying [begin, end) to it. + * + * @param begin Start of the first subrange to be sorted. + * @param middle End of the first sorted subrange and the start of the second. + * @param end End of the second sorted subrange. + * @param workingBegin Start of the working buffer. + * @param comp Comparator implementing an operator()(const ValueType& lhs, const ValueType& rhs). + * + * @pre workingBegin buffer must have at least std::distance(begin, end) elements. + * + * @note We can't use std::inplace_merge or std::merge, because their behavior + * is undefined if the comparator is not strict weak ordering. + */ +template< + std::forward_iterator Iter, + std::random_access_iterator BufIter, + typename Comparator = std::less>> +void mergeSortedRunsInPlace(Iter begin, Iter middle, Iter end, BufIter workingBegin, Comparator comp = {}) +{ + const BufIter workingMiddle = std::move(begin, middle, workingBegin); + const BufIter workingEnd = std::move(middle, end, workingMiddle); + + Iter output = begin; + BufIter workingLeft = workingBegin; + BufIter workingRight = workingMiddle; + + while (workingLeft != workingMiddle && workingRight != workingEnd) { + /* Note the inversion here !comp(...., ....). This is required for the merge to be stable. + If a == b where a if from the left part and b is the the right, then we have to pick + a. */ + *output++ = !comp(*workingRight, *workingLeft) ? std::move(*workingLeft++) : std::move(*workingRight++); + } + + std::move(workingLeft, workingMiddle, output); + std::move(workingRight, workingEnd, output); +} + +/** + * Simple insertion sort. + * + * Does not require that the std::iter_value_t is copyable. + * + * @param begin Start of the range to sort. + * @param end End of the range to sort. + * @comp Comparator the defines the ordering. Order of elements if the comp is not strict weak ordering + * is not specified. + * @throws Nothing. + * + * Note on exception safety: this function provides weak exception safety + * guarantees. To elaborate: if the comparator throws or move assignment + * throws (value type is not nothrow_move_assignable) then the range is left in + * a consistent, but unspecified state. + * + * @note This can't be implemented in terms of binary search if the strict weak ordering + * needs to be handled in a well-defined but unspecified manner. + */ +template>> +void insertionsort(Iter begin, Iter end, Comparator comp = {}) +{ + if (begin == end) + return; + for (Iter current = std::next(begin); current != end; ++current) { + for (Iter insertionPoint = current; + insertionPoint != begin && comp(*insertionPoint, *std::prev(insertionPoint)); + --insertionPoint) { + std::swap(*insertionPoint, *std::prev(insertionPoint)); + } + } +} + +/** + * Find maximal i <= end such that [begin, i) is strictly decreasing according + * to the specified comparator. + */ +template>> +Iter strictlyDecreasingPrefix(Iter begin, Iter end, Comparator && comp = {}) +{ + if (begin == end) + return begin; + while (std::next(begin) != end && /* *std::next(begin) < begin */ + comp(*std::next(begin), *begin)) + ++begin; + return std::next(begin); +} + +/** + * Find minimal i >= start such that [i, end) is strictly decreasing according + * to the specified comparator. + */ +template>> +Iter strictlyDecreasingSuffix(Iter begin, Iter end, Comparator && comp = {}) +{ + if (begin == end) + return end; + while (std::prev(end) > begin && /* *std::prev(end) < *std::prev(end, 2) */ + comp(*std::prev(end), *std::prev(end, 2))) + --end; + return std::prev(end); +} + +/** + * Find maximal i <= end such that [begin, i) is weakly increasing according + * to the specified comparator. + */ +template>> +Iter weaklyIncreasingPrefix(Iter begin, Iter end, Comparator && comp = {}) +{ + return strictlyDecreasingPrefix(begin, end, std::not_fn(std::forward(comp))); +} + +/** + * Find minimal i >= start such that [i, end) is weakly increasing according + * to the specified comparator. + */ +template>> +Iter weaklyIncreasingSuffix(Iter begin, Iter end, Comparator && comp = {}) +{ + return strictlyDecreasingSuffix(begin, end, std::not_fn(std::forward(comp))); +} + +/** + * Peeksort stable sorting algorithm. Sorts elements in-place. + * Allocates additional memory as needed. + * + * @details + * PeekSort is a stable, near-optimal natural mergesort. Most importantly, like any + * other mergesort it upholds the "Ord safety" property. Meaning that even for + * comparator predicates that don't satisfy strict weak ordering it can't result + * in infinite loops/out of bounds memory accesses or other undefined behavior. + * + * As a quick reminder, strict weak ordering relation operator< must satisfy + * the following properties. Keep in mind that in C++ an equvalence relation + * is specified in terms of operator< like so: a ~ b iff !(a < b) && !(b < a). + * + * 1. a < a === false - relation is irreflexive + * 2. a < b, b < c => a < c - transitivity + * 3. a ~ b, a ~ b, b ~ c => a ~ c, transitivity of equivalence + * + * @see https://www.wild-inter.net/publications/munro-wild-2018 + * @see https://github.com/Voultapher/sort-research-rs/blob/main/writeup/sort_safety/text.md#property-analysis + * + * The order of elements when comp is not strict weak ordering is not specified, but + * is not undefined. The output is always some permutation of the input, regardless + * of the comparator provided. + * Relying on ordering in such cases is erroneous, but this implementation + * will happily accept broken comparators and will not crash. + * + * @param begin Start of the range to be sorted. + * @param end End of the range to be sorted. + * @comp comp Comparator implementing an operator()(const ValueType& lhs, const ValueType& rhs). + * + * @throws std::bad_alloc if the temporary buffer can't be allocated. + * + * @return Nothing. + * + * Note on exception safety: this function provides weak exception safety + * guarantees. To elaborate: if the comparator throws or move assignment + * throws (value type is not nothrow_move_assignable) then the range is left in + * a consistent, but unspecified state. + * + */ +template>> +/* ValueType must be default constructible to create the temporary buffer */ + requires std::is_default_constructible_v> +void peeksort(Iter begin, Iter end, Comparator comp = {}) +{ + auto length = std::distance(begin, end); + + /* Special-case very simple inputs. This is identical to how libc++ does it. */ + switch (length) { + case 0: + [[fallthrough]]; + case 1: + return; + case 2: + if (comp(*--end, *begin)) /* [a, b], b < a */ + std::swap(*begin, *end); + return; + } + + using ValueType = std::iter_value_t; + auto workingBuffer = std::vector(length); + + /* + * sorts [begin, end), assuming that [begin, leftRunEnd) and + * [rightRunBegin, end) are sorted. + * Modified implementation from: + * https://github.com/sebawild/powersort/blob/1d078b6be9023e134c4f8f6de88e2406dc681e89/src/sorts/peeksort.h + */ + auto peeksortImpl = [&workingBuffer, + &comp](auto & peeksortImpl, Iter begin, Iter end, Iter leftRunEnd, Iter rightRunBegin) { + if (leftRunEnd == end || rightRunBegin == begin) + return; + + /* Dispatch to simpler insertion sort implementation for smaller cases + Cut-off limit is the same as in libstdc++ + https://github.com/gcc-mirror/gcc/blob/d9375e490072d1aae73a93949aa158fcd2a27018/libstdc%2B%2B-v3/include/bits/stl_algo.h#L4977 + */ + static constexpr std::size_t insertionsortThreshold = 16; + size_t length = std::distance(begin, end); + if (length <= insertionsortThreshold) + return insertionsort(begin, end, comp); + + Iter middle = std::next(begin, (length / 2)); /* Middle split between m and m - 1 */ + + if (middle <= leftRunEnd) { + /* |XXXXXXXX|XX X| */ + peeksortImpl(peeksortImpl, leftRunEnd, end, std::next(leftRunEnd), rightRunBegin); + mergeSortedRunsInPlace(begin, leftRunEnd, end, workingBuffer.begin(), comp); + return; + } else if (middle >= rightRunBegin) { + /* |XX X|XXXXXXXX| */ + peeksortImpl(peeksortImpl, begin, rightRunBegin, leftRunEnd, std::prev(rightRunBegin)); + mergeSortedRunsInPlace(begin, rightRunBegin, end, workingBuffer.begin(), comp); + return; + } + + /* Find middle run, i.e., run containing m - 1 */ + Iter i, j; + + if (!comp(*middle, *std::prev(middle)) /* *std::prev(middle) <= *middle */) { + i = weaklyIncreasingSuffix(leftRunEnd, middle, comp); + j = weaklyIncreasingPrefix(std::prev(middle), rightRunBegin, comp); + } else { + i = strictlyDecreasingSuffix(leftRunEnd, middle, comp); + j = strictlyDecreasingPrefix(std::prev(middle), rightRunBegin, comp); + std::reverse(i, j); + } + + if (i == begin && j == end) + return; /* single run */ + + if (middle - i < j - middle) { + /* |XX x|xxxx X| */ + peeksortImpl(peeksortImpl, begin, i, leftRunEnd, std::prev(i)); + peeksortImpl(peeksortImpl, i, end, j, rightRunBegin); + mergeSortedRunsInPlace(begin, i, end, workingBuffer.begin(), comp); + } else { + /* |XX xxx|x X| */ + peeksortImpl(peeksortImpl, begin, j, leftRunEnd, i); + peeksortImpl(peeksortImpl, j, end, std::next(j), rightRunBegin); + mergeSortedRunsInPlace(begin, j, end, workingBuffer.begin(), comp); + } + }; + + peeksortImpl(peeksortImpl, begin, end, /*leftRunEnd=*/begin, /*rightRunBegin=*/end); +} + +} From 351d898c43202ca9c3f94daf7d727b3b7bbd3daa Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 15 Jun 2025 16:51:40 +0000 Subject: [PATCH 16/22] libexpr: Switch builtins.sort primop to use peeksort This prevents C++ level undefined behavior from affecting the evaluator. Stdlib implementation details should not affect eval, regardless of the build platform. Even erroneous usage of `builtins.sort` should not make it possible to crash the evaluator or produce results that depend on the host platform. --- src/libexpr/primops.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 60f44ca62..75d7465dd 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -14,6 +14,7 @@ #include "nix/expr/value-to-xml.hh" #include "nix/expr/primops.hh" #include "nix/fetchers/fetch-to-store.hh" +#include "nix/util/sort.hh" #include #include @@ -3695,10 +3696,14 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value return state.forceBool(vBool, pos, "while evaluating the return value of the sorting function passed to builtins.sort"); }; - /* FIXME: std::sort can segfault if the comparator is not a strict - weak ordering. What to do? std::stable_sort() seems more - resilient, but no guarantees... */ - std::stable_sort(list.begin(), list.end(), comparator); + /* NOTE: Using custom implementation because std::sort and std::stable_sort + are not resilient to comparators that violate strict weak ordering. Diagnosing + incorrect implementations is a O(n^3) problem, so doing the checks is much more + expensive that doing the sorting. For this reason we choose to use sorting algorithms + that are can't be broken by invalid comprators. peeksort (mergesort) + doesn't misbehave when any of the strict weak order properties is + violated - output is always a reordering of the input. */ + peeksort(list.begin(), list.end(), comparator); v.mkList(list); } From ddcfc81ff1fe01e4ab74cec80709cac8f77361d5 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 15 Jun 2025 16:51:42 +0000 Subject: [PATCH 17/22] libexpr: Document requirements for comparator passed to builtins.sort --- src/libexpr/primops.cc | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 75d7465dd..ba568e38d 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3725,6 +3725,32 @@ static RegisterPrimOp primop_sort({ This is a stable sort: it preserves the relative order of elements deemed equal by the comparator. + + *comparator* must impose a strict weak ordering on the set of values + in the *list*. This means that for any elements *a*, *b* and *c* from the + *list*, *comparator* must satisfy the following relations: + + 1. Transitivity + + ```nix + comparator a b && comparator b c -> comparator a c + ``` + + 1. Irreflexivity + + ```nix + comparator a a == false + ``` + + 1. Transitivity of equivalence + + ```nix + let equiv = a: b: (!comparator a b && !comparator b a); in + equiv a b && equiv b c -> equiv a c + ``` + + If the *comparator* violates any of these properties, then `builtins.sort` + reorders elements in an unspecified manner. )", .fun = prim_sort, }); From f9170a84f6eb6a162c800c39c17d3b34eff87dda Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 15 Jun 2025 16:51:44 +0000 Subject: [PATCH 18/22] tests/functional/lang: Add sort stability test for lists langer than 16 elements libstdc++'s std::stable_sort and new builtins.sort implementation special-case ranges with length less than or equal to 16 and delegate to insertionsort. Having a larger e2e test would allow catching sort stability issues at functional level as well. --- tests/functional/lang/eval-okay-sort.exp | 2 +- tests/functional/lang/eval-okay-sort.nix | 74 ++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/tests/functional/lang/eval-okay-sort.exp b/tests/functional/lang/eval-okay-sort.exp index 899119e20..fcb3b2224 100644 --- a/tests/functional/lang/eval-okay-sort.exp +++ b/tests/functional/lang/eval-okay-sort.exp @@ -1 +1 @@ -[ [ 42 77 147 249 483 526 ] [ 526 483 249 147 77 42 ] [ "bar" "fnord" "foo" "xyzzy" ] [ { key = 1; value = "foo"; } { key = 1; value = "fnord"; } { key = 2; value = "bar"; } ] [ [ ] [ ] [ 1 ] [ 1 4 ] [ 1 5 ] [ 1 6 ] [ 2 ] [ 2 3 ] [ 3 ] [ 3 ] ] ] +[ [ 42 77 147 249 483 526 ] [ 526 483 249 147 77 42 ] [ "bar" "fnord" "foo" "xyzzy" ] [ { key = 1; value = "foo"; } { key = 1; value = "fnord"; } { key = 2; value = "bar"; } ] [ { key = 1; value = "foo"; } { key = 1; value = "foo2"; } { key = 1; value = "foo3"; } { key = 1; value = "foo4"; } { key = 1; value = "foo5"; } { key = 1; value = "foo6"; } { key = 1; value = "foo7"; } { key = 1; value = "foo8"; } { key = 2; value = "bar"; } { key = 2; value = "bar2"; } { key = 2; value = "bar3"; } { key = 2; value = "bar4"; } { key = 2; value = "bar5"; } { key = 3; value = "baz"; } { key = 3; value = "baz2"; } { key = 3; value = "baz3"; } { key = 3; value = "baz4"; } { key = 4; value = "biz1"; } ] [ [ ] [ ] [ 1 ] [ 1 4 ] [ 1 5 ] [ 1 6 ] [ 2 ] [ 2 3 ] [ 3 ] [ 3 ] ] ] diff --git a/tests/functional/lang/eval-okay-sort.nix b/tests/functional/lang/eval-okay-sort.nix index 412bda4a0..7a3b7f71b 100644 --- a/tests/functional/lang/eval-okay-sort.nix +++ b/tests/functional/lang/eval-okay-sort.nix @@ -37,6 +37,80 @@ with builtins; value = "fnord"; } ]) + (sort (x: y: x.key < y.key) [ + { + key = 1; + value = "foo"; + } + { + key = 2; + value = "bar"; + } + { + key = 1; + value = "foo2"; + } + { + key = 2; + value = "bar2"; + } + { + key = 2; + value = "bar3"; + } + { + key = 2; + value = "bar4"; + } + { + key = 1; + value = "foo3"; + } + { + key = 3; + value = "baz"; + } + { + key = 3; + value = "baz2"; + } + { + key = 1; + value = "foo4"; + } + { + key = 3; + value = "baz3"; + } + { + key = 1; + value = "foo5"; + } + { + key = 1; + value = "foo6"; + } + { + key = 2; + value = "bar5"; + } + { + key = 3; + value = "baz4"; + } + { + key = 1; + value = "foo7"; + } + { + key = 4; + value = "biz1"; + } + { + key = 1; + value = "foo8"; + } + ]) (sort lessThan [ [ 1 From 18dc96269d01f7fa1e2e61086444a89f5ce18890 Mon Sep 17 00:00:00 2001 From: Pol Dellaiera Date: Sun, 15 Jun 2025 22:58:26 +0200 Subject: [PATCH 19/22] docker: add basics OpenContainers labels --- docker.nix | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docker.nix b/docker.nix index 14648f0d1..74e970e4b 100644 --- a/docker.nix +++ b/docker.nix @@ -19,6 +19,13 @@ gid ? 0, uname ? "root", gname ? "root", + Labels ? { + "org.opencontainers.image.title" = "Nix"; + "org.opencontainers.image.source" = "https://github.com/NixOS/nix"; + "org.opencontainers.image.vendor" = "Nix project"; + "org.opencontainers.image.version" = nix.version; + "org.opencontainers.image.description" = "Nix container image"; + }, # Default Packages nix, bashInteractive, @@ -352,6 +359,7 @@ dockerTools.buildLayeredImageWithNixDb { ''; config = { + inherit Labels; Cmd = [ (lib.getExe bashInteractive) ]; User = "${toString uid}:${toString gid}"; Env = [ From bb44347fac3b97cc19f1d0ab0594478cc928cbd8 Mon Sep 17 00:00:00 2001 From: Pol Dellaiera Date: Sun, 15 Jun 2025 23:04:50 +0200 Subject: [PATCH 20/22] docker: expose `config.Cmd` as parameter --- docker.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker.nix b/docker.nix index 74e970e4b..6cfa7d551 100644 --- a/docker.nix +++ b/docker.nix @@ -26,6 +26,7 @@ "org.opencontainers.image.version" = nix.version; "org.opencontainers.image.description" = "Nix container image"; }, + Cmd ? [ (lib.getExe bashInteractive) ], # Default Packages nix, bashInteractive, @@ -359,8 +360,7 @@ dockerTools.buildLayeredImageWithNixDb { ''; config = { - inherit Labels; - Cmd = [ (lib.getExe bashInteractive) ]; + inherit Cmd Labels; User = "${toString uid}:${toString gid}"; Env = [ "USER=${uname}" From d64c92216409b08ca40f43e601fbdae59114e812 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Tue, 17 Jun 2025 08:45:29 +0200 Subject: [PATCH 21/22] libstore: fix race condition when creating state directories Running parallel nix in nix can lead to multiple instances trying to create the state directories and failing on the `createSymlink` step, because the link already exists. `replaceSymlink` is already idempotent, so let's use that. Resolves #2706 --- src/libstore/local-store.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 76fadba86..e53cab2dc 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -133,7 +133,7 @@ LocalStore::LocalStore(ref config) Path gcRootsDir = config->stateDir + "/gcroots"; if (!pathExists(gcRootsDir)) { createDirs(gcRootsDir); - createSymlink(profilesDir, gcRootsDir + "/profiles"); + replaceSymlink(profilesDir, gcRootsDir + "/profiles"); } for (auto & perUserDir : {profilesDir + "/per-user", gcRootsDir + "/per-user"}) { From 77f6b6532f582a9db2bd6317f4fd272c32a05c7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20=C4=8Cun=C3=A1t?= Date: Wed, 18 Jun 2025 10:05:02 +0200 Subject: [PATCH 22/22] tests: fixup with jq-1.8.0 --- tests/functional/flakes/flakes.sh | 2 +- tests/functional/flakes/relative-paths.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/functional/flakes/flakes.sh b/tests/functional/flakes/flakes.sh index e8b051198..ce695a6cb 100755 --- a/tests/functional/flakes/flakes.sh +++ b/tests/functional/flakes/flakes.sh @@ -160,7 +160,7 @@ expect 1 nix build -o "$TEST_ROOT/result" "$flake2Dir#bar" --no-update-lock-file nix build -o "$TEST_ROOT/result" "$flake2Dir#bar" --commit-lock-file [[ -e "$flake2Dir/flake.lock" ]] [[ -z $(git -C "$flake2Dir" diff main || echo failed) ]] -[[ $(jq --indent 0 . < "$flake2Dir/flake.lock") =~ ^'{"nodes":{"flake1":{"locked":{"lastModified":'.*',"narHash":"sha256-'.*'","ref":"refs/heads/master","rev":"'.*'","revCount":2,"type":"git","url":"file:///'.*'"},"original":{"id":"flake1","type":"indirect"}},"root":{"inputs":{"flake1":"flake1"}}},"root":"root","version":7}'$ ]] +[[ $(jq --indent 0 --compact-output . < "$flake2Dir/flake.lock") =~ ^'{"nodes":{"flake1":{"locked":{"lastModified":'.*',"narHash":"sha256-'.*'","ref":"refs/heads/master","rev":"'.*'","revCount":2,"type":"git","url":"file:///'.*'"},"original":{"id":"flake1","type":"indirect"}},"root":{"inputs":{"flake1":"flake1"}}},"root":"root","version":7}'$ ]] # Rerunning the build should not change the lockfile. nix build -o "$TEST_ROOT/result" "$flake2Dir#bar" diff --git a/tests/functional/flakes/relative-paths.sh b/tests/functional/flakes/relative-paths.sh index 5810fdebe..7480cd504 100644 --- a/tests/functional/flakes/relative-paths.sh +++ b/tests/functional/flakes/relative-paths.sh @@ -69,7 +69,7 @@ git -C "$rootFlake" add flake.nix sub2/flake.nix git -C "$rootFlake" add sub2/flake.lock [[ $(nix eval "$subflake2#y") = 15 ]] -[[ $(jq --indent 0 . < "$subflake2/flake.lock") =~ ^'{"nodes":{"root":{"inputs":{"root":"root_2","sub1":"sub1"}},"root_2":{"inputs":{"sub0":"sub0"},"locked":{"path":"..","type":"path"},"original":{"path":"..","type":"path"},"parent":[]},"root_3":{"inputs":{"sub0":"sub0_2"},"locked":{"path":"../","type":"path"},"original":{"path":"../","type":"path"},"parent":["sub1"]},"sub0":{"locked":{"path":"sub0","type":"path"},"original":{"path":"sub0","type":"path"},"parent":["root"]},"sub0_2":{"locked":{"path":"sub0","type":"path"},"original":{"path":"sub0","type":"path"},"parent":["sub1","root"]},"sub1":{"inputs":{"root":"root_3"},"locked":{"path":"../sub1","type":"path"},"original":{"path":"../sub1","type":"path"},"parent":[]}},"root":"root","version":7}'$ ]] +[[ $(jq --indent 0 --compact-output . < "$subflake2/flake.lock") =~ ^'{"nodes":{"root":{"inputs":{"root":"root_2","sub1":"sub1"}},"root_2":{"inputs":{"sub0":"sub0"},"locked":{"path":"..","type":"path"},"original":{"path":"..","type":"path"},"parent":[]},"root_3":{"inputs":{"sub0":"sub0_2"},"locked":{"path":"../","type":"path"},"original":{"path":"../","type":"path"},"parent":["sub1"]},"sub0":{"locked":{"path":"sub0","type":"path"},"original":{"path":"sub0","type":"path"},"parent":["root"]},"sub0_2":{"locked":{"path":"sub0","type":"path"},"original":{"path":"sub0","type":"path"},"parent":["sub1","root"]},"sub1":{"inputs":{"root":"root_3"},"locked":{"path":"../sub1","type":"path"},"original":{"path":"../sub1","type":"path"},"parent":[]}},"root":"root","version":7}'$ ]] # Make sure there are no content locks for relative path flakes. (! grep "$TEST_ROOT" "$subflake2/flake.lock")