substitute: Deprecate `replacements`, introduce `replacementsList`

Also:
- Add tests
- Treewide update
- Improve docs

8 files changed, 159 insertions, 11 deletions

diff --git a/pkgs/build-support/substitute/substitute.nix b/pkgs/build-support/substitute/substitute.nix
index 7f0332334585b..37233a306840d 100644
--- a/pkgs/build-support/substitute/substitute.nix
+++ b/pkgs/build-support/substitute/substitute.nix
@@ -1,14 +1,58 @@
-{ stdenvNoCC }:
+{ lib, stdenvNoCC }:
+/*
+This is a wrapper around `substitute` in the stdenv.
 
+Attribute arguments:
+- `name` (optional): The name of the resulting derivation
+- `src`: The path to the file to substitute
+- `substitutions`: The list of substitution arguments to pass
+  See https://nixos.org/manual/nixpkgs/stable/#fun-substitute
+- `replacements`: Deprecated version of `substitutions`
+  that doesn't support spaces in arguments.
+
+Example:
+
+```nix
+{ substitute }:
+substitute {
+  src = ./greeting.txt;
+  substitutions = [
+    "--replace"
+    "world"
+    "paul"
+  ];
+}
+```
+
+See ../../test/substitute for tests
+*/
 args:
 
-# This is a wrapper around `substitute` in the stdenv.
-# The `replacements` attribute should be a list of list of arguments
-# to `substitute`, such as `[ "--replace" "sourcetext" "replacementtext" ]`
-stdenvNoCC.mkDerivation ({
+let
   name = if args ? name then args.name else baseNameOf (toString args.src);
+  deprecationReplacement = lib.pipe args.replacements [
+    lib.toList
+    (map (lib.splitString " "))
+    lib.concatLists
+    (lib.concatMapStringsSep " " lib.strings.escapeNixString)
+  ];
+  optionalDeprecationWarning =
+    # substitutions is only available starting 24.05.
+    # TODO: Remove support for replacements sometime after the next release
+    lib.warnIf (args ? replacements && lib.isInOldestRelease 2405) ''
+      pkgs.substitute: For "${name}", `replacements` is used, which is deprecated since it doesn't support arguments with spaces. Use `substitutions` instead:
+        substitutions = [ ${deprecationReplacement} ];'';
+in
+optionalDeprecationWarning
+stdenvNoCC.mkDerivation ({
+  inherit name;
   builder = ./substitute.sh;
   inherit (args) src;
   preferLocalBuild = true;
   allowSubstitutes = false;
-} // args // { replacements = args.replacements; })
+} // args // lib.optionalAttrs (args ? substitutions) {
+  substitutions =
+    assert lib.assertMsg (lib.isList args.substitutions) ''
+      pkgs.substitute: For "${name}", `substitutions` is passed, which is expected to be a list, but it's a ${builtins.typeOf args.substitutions} instead.'';
+    lib.escapeShellArgs args.substitutions;
+})
diff --git a/pkgs/build-support/substitute/substitute.sh b/pkgs/build-support/substitute/substitute.sh
index dbac275a80ede..d50a82446639d 100644
--- a/pkgs/build-support/substitute/substitute.sh
+++ b/pkgs/build-support/substitute/substitute.sh
@@ -8,7 +8,13 @@ if test -n "$dir"; then
     mkdir -p $out/$dir
 fi
 
-substitute $src $target $replacements
+substitutionsList=($replacements)
+
+if [[ -v substitutions ]]; then
+    eval "substitutionsList+=($substitutions)"
+fi
+
+substitute $src $target "${substitutionsList[@]}"
 
 if test -n "$isExecutable"; then
     chmod +x $target
diff --git a/pkgs/development/libraries/science/math/libamplsolver/default.nix b/pkgs/development/libraries/science/math/libamplsolver/default.nix
index c0bc89b492ea2..a40091bac8b50 100644
--- a/pkgs/development/libraries/science/math/libamplsolver/default.nix
+++ b/pkgs/development/libraries/science/math/libamplsolver/default.nix
@@ -12,7 +12,7 @@ stdenv.mkDerivation rec {
   patches = [
     (substitute {
       src = ./libamplsolver-sharedlib.patch;
-      replacements = [ "--replace" "@sharedlibext@" "${stdenv.hostPlatform.extensions.sharedLibrary}" ];
+      substitutions = [ "--replace" "@sharedlibext@" "${stdenv.hostPlatform.extensions.sharedLibrary}" ];
     })
   ];
 
diff --git a/pkgs/development/tools/misc/binutils/2.38/default.nix b/pkgs/development/tools/misc/binutils/2.38/default.nix
index fd9a3277532e9..474a565f351f9 100644
--- a/pkgs/development/tools/misc/binutils/2.38/default.nix
+++ b/pkgs/development/tools/misc/binutils/2.38/default.nix
@@ -91,7 +91,7 @@ stdenv.mkDerivation {
      # this patch is from debian:
      # https://sources.debian.org/data/main/b/binutils/2.38-3/debian/patches/mips64-default-n64.diff
      (if stdenv.targetPlatform.isMusl
-      then substitute { src = ./mips64-default-n64.patch; replacements = [ "--replace" "gnuabi64" "muslabi64" ]; }
+      then substitute { src = ./mips64-default-n64.patch; substitutions = [ "--replace" "gnuabi64" "muslabi64" ]; }
       else ./mips64-default-n64.patch)
   # On PowerPC, when generating assembly code, GCC generates a `.machine`
   # custom instruction which instructs the assembler to generate code for this
diff --git a/pkgs/development/tools/misc/binutils/default.nix b/pkgs/development/tools/misc/binutils/default.nix
index 2183d45b6f0d2..3a5a07dadabbf 100644
--- a/pkgs/development/tools/misc/binutils/default.nix
+++ b/pkgs/development/tools/misc/binutils/default.nix
@@ -105,7 +105,7 @@ stdenv.mkDerivation (finalAttrs: {
      # this patch is from debian:
      # https://sources.debian.org/data/main/b/binutils/2.38-3/debian/patches/mips64-default-n64.diff
      (if stdenv.targetPlatform.isMusl
-      then substitute { src = ./mips64-default-n64.patch; replacements = [ "--replace" "gnuabi64" "muslabi64" ]; }
+      then substitute { src = ./mips64-default-n64.patch; substitutions = [ "--replace" "gnuabi64" "muslabi64" ]; }
       else ./mips64-default-n64.patch)
   # This patch fixes a bug in 2.40 on MinGW, which breaks DXVK when cross-building from Darwin.
   # See https://sourceware.org/bugzilla/show_bug.cgi?id=30079
diff --git a/pkgs/games/shattered-pixel-dungeon/summoning-pixel-dungeon.nix b/pkgs/games/shattered-pixel-dungeon/summoning-pixel-dungeon.nix
index 488110684c096..5e7ed1b817e0c 100644
--- a/pkgs/games/shattered-pixel-dungeon/summoning-pixel-dungeon.nix
+++ b/pkgs/games/shattered-pixel-dungeon/summoning-pixel-dungeon.nix
@@ -18,7 +18,7 @@ callPackage ./generic.nix rec {
 
   patches = [(substitute {
     src = ./disable-git-version.patch;
-    replacements = [ "--subst-var-by" "version" version ];
+    substitutions = [ "--subst-var-by" "version" version ];
   })];
 
   depsHash = "sha256-0P/BcjNnbDN25DguRcCyzPuUG7bouxEx1ySodIbSwvg=";
diff --git a/pkgs/test/default.nix b/pkgs/test/default.nix
index f66b05b9f6f1b..9868bbc6033b6 100644
--- a/pkgs/test/default.nix
+++ b/pkgs/test/default.nix
@@ -177,4 +177,6 @@ with pkgs;
   auto-patchelf-hook = callPackage ./auto-patchelf-hook { };
 
   systemd = callPackage ./systemd { };
+
+  substitute = recurseIntoAttrs (callPackage ./substitute { });
 }
diff --git a/pkgs/test/substitute/default.nix b/pkgs/test/substitute/default.nix
new file mode 100644
index 0000000000000..3ff346b146584
--- /dev/null
+++ b/pkgs/test/substitute/default.nix
@@ -0,0 +1,96 @@
+{ substitute, testers, runCommand }:
+let
+  # Ofborg doesn't allow any traces on stderr,
+  # so mock `lib` to not trace warnings,
+  # because substitute gives a deprecation warning
+  substituteSilent = substitute.override (prevArgs: {
+    lib = prevArgs.lib.extend (finalLib: prevLib: {
+      trivial = prevLib.trivial // {
+        warn = msg: value: value;
+      };
+    });
+  });
+in {
+
+  substitutions = testers.testEqualContents {
+    assertion = "substitutions-spaces";
+    actual = substitute {
+      src = builtins.toFile "source" ''
+        Hello world!
+      '';
+      substitutions = [
+        "--replace-fail"
+        "Hello world!"
+        "Yo peter!"
+      ];
+    };
+    expected = builtins.toFile "expected" ''
+      Yo peter!
+    '';
+  };
+
+  legacySingleReplace = testers.testEqualContents {
+    assertion = "substitute-single-replace";
+    actual = substituteSilent {
+      src = builtins.toFile "source" ''
+        Hello world!
+      '';
+      replacements = [
+        "--replace-fail" "world" "paul"
+      ];
+    };
+    expected = builtins.toFile "expected" ''
+      Hello paul!
+    '';
+  };
+
+  legacyString = testers.testEqualContents {
+    assertion = "substitute-string";
+    actual = substituteSilent {
+      src = builtins.toFile "source" ''
+        Hello world!
+      '';
+      # Not great that this works at all, but is supported
+      replacements = "--replace-fail world string";
+    };
+    expected = builtins.toFile "expected" ''
+      Hello string!
+    '';
+  };
+
+  legacySingleArg = testers.testEqualContents {
+    assertion = "substitute-single-arg";
+    actual = substituteSilent {
+      src = builtins.toFile "source" ''
+        Hello world!
+      '';
+      # Not great that this works at all, but is supported
+      replacements = [
+        "--replace-fail world list"
+      ];
+    };
+    expected = builtins.toFile "expected" ''
+      Hello list!
+    '';
+  };
+
+  legacyVar = testers.testEqualContents {
+    assertion = "substitute-var";
+    actual = substituteSilent {
+      src = builtins.toFile "source" ''
+        @greeting@ @name@!
+      '';
+      # Not great that this works at all, but is supported
+      replacements = [
+        "--subst-var name"
+        "--subst-var-by greeting Yo"
+      ];
+      name = "peter";
+    };
+    expected = builtins.toFile "expected" ''
+      Yo peter!
+    '';
+  };
+
+
+}