Merge pull request #172366 from ncfavier/wrappers-cleanup

make{,Binary}Wrapper: some cleanups

doc/stdenv/stdenv.chapter.md

@@ -869,7 +869,7 @@ makeWrapper $out/bin/foo $wrapperfile --set FOOBAR baz
 makeWrapper $out/bin/foo $wrapperfile --prefix PATH : ${lib.makeBinPath [ hello git ]}
 ```
 
-There’s many more kinds of arguments, they are documented in `nixpkgs/pkgs/build-support/setup-hooks/make-wrapper.sh` for the `makeWrapper` implementation and in `nixpkgs/pkgs/build-support/setup-hooks/make-binary-wrapper.sh` for the `makeBinaryWrapper` implementation.
+There’s many more kinds of arguments, they are documented in `nixpkgs/pkgs/build-support/setup-hooks/make-wrapper.sh` for the `makeWrapper` implementation and in `nixpkgs/pkgs/build-support/setup-hooks/make-binary-wrapper/make-binary-wrapper.sh` for the `makeBinaryWrapper` implementation.
 
 `wrapProgram` is a convenience function you probably want to use most of the time, implemented by both `makeWrapper` and `makeBinaryWrapper`.
 

pkgs/applications/editors/neovim/utils.nix

@@ -108,7 +108,7 @@ let
             hostprog_check_table);
         in
         [
-          "--argv0" "$0" "--add-flags" (lib.escapeShellArgs flags)
+          "--inherit-argv0" "--add-flags" (lib.escapeShellArgs flags)
         ] ++ lib.optionals withRuby [
           "--set" "GEM_HOME" "${rubyEnv}/${rubyEnv.ruby.gemPath}"
         ] ++ lib.optionals (binPath != "") [

pkgs/applications/editors/vscode/generic.nix

@@ -72,7 +72,7 @@ let
       ++ lib.optionals stdenv.isLinux [
         autoPatchelfHook
         nodePackages.asar
-        (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; })
+        (wrapGAppsHook.override { inherit makeWrapper; })
       ];
 
     dontBuild = true;

pkgs/applications/networking/browsers/brave/default.nix

@@ -104,7 +104,7 @@ stdenv.mkDerivation rec {
 
   nativeBuildInputs = [
     dpkg
-    (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; })
+    (wrapGAppsHook.override { inherit makeWrapper; })
   ];
 
   buildInputs = [

pkgs/applications/networking/browsers/firefox/wrapper.nix

@@ -1,4 +1,4 @@
-{ stdenv, lib, makeDesktopItem, makeWrapper, lndir, config
+{ stdenv, lib, makeDesktopItem, makeWrapper, makeBinaryWrapper, lndir, config
 , fetchurl, zip, unzip, jq, xdg-utils, writeText
 
 ## various stuff that can be plugged in
@@ -229,7 +229,7 @@ let
           # Symbolic link: wrap the link's target.
           oldExe="$(readlink -v --canonicalize-existing "$executablePath")"
           rm "$executablePath"
-        elif wrapperCmd=$(strings -dw "$executablePath" | sed -n '/^makeCWrapper/,/^$/ p'); [[ $wrapperCmd ]]; then
+        elif wrapperCmd=$(${makeBinaryWrapper.extractCmd} "$executablePath"); [[ $wrapperCmd ]]; then
           # If the executable is a binary wrapper, we need to update its target to
           # point to $out, but we can't just edit the binary in-place because of length
           # issues. So we extract the command used to create the wrapper and add the
@@ -237,10 +237,7 @@ let
           parseMakeCWrapperCall() {
             shift # makeCWrapper
             oldExe=$1; shift
-            for arg do case $arg in
+            oldWrapperArgs=("$@")
-              --inherit-argv0) oldWrapperArgs+=(--argv0 '$0');; # makeWrapper doesn't understand --inherit-argv0
-              *) oldWrapperArgs+=("$arg");;
-            esac done
           }
           eval "parseMakeCWrapperCall ''${wrapperCmd//"${browser}"/"$out"}"
           rm "$executablePath"

pkgs/applications/networking/instant-messengers/discord/linux.nix

@@ -1,5 +1,5 @@
 { pname, version, src, meta, binaryName, desktopName, autoPatchelfHook
-, makeDesktopItem, lib, stdenv, wrapGAppsHook, makeWrapper, alsa-lib, at-spi2-atk
+, makeDesktopItem, lib, stdenv, wrapGAppsHook, makeShellWrapper, alsa-lib, at-spi2-atk
 , at-spi2-core, atk, cairo, cups, dbus, expat, fontconfig, freetype, gdk-pixbuf
 , glib, gtk3, libcxx, libdrm, libnotify, libpulseaudio, libuuid, libX11
 , libXScrnSaver, libXcomposite, libXcursor, libXdamage, libXext, libXfixes
@@ -24,7 +24,8 @@ stdenv.mkDerivation rec {
     libxshmfence
     mesa
     nss
-    (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; })
+    wrapGAppsHook
+    makeShellWrapper
   ];
 
   dontWrapGApps = true;
@@ -78,7 +79,7 @@ stdenv.mkDerivation rec {
     patchelf --set-interpreter ${stdenv.cc.bintools.dynamicLinker} \
         $out/opt/${binaryName}/${binaryName}
 
-    wrapProgram $out/opt/${binaryName}/${binaryName} \
+    wrapProgramShell $out/opt/${binaryName}/${binaryName} \
         "''${gappsWrapperArgs[@]}" \
         --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--enable-features=UseOzonePlatform --ozone-platform=wayland}}" \
         --prefix XDG_DATA_DIRS : "${gtk3}/share/gsettings-schemas/${gtk3.name}/" \

pkgs/applications/networking/instant-messengers/signal-desktop/default.nix

@@ -40,7 +40,7 @@ in stdenv.mkDerivation rec {
   nativeBuildInputs = [
     autoPatchelfHook
     dpkg
-    (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; })
+    (wrapGAppsHook.override { inherit makeWrapper; })
   ];
 
   buildInputs = [

pkgs/applications/science/logic/tlaplus/toolbox.nix

@@ -1,6 +1,6 @@
 { lib
 , fetchzip
-, makeWrapper
+, makeShellWrapper
 , makeDesktopItem
 , stdenv
 , gtk3
@@ -35,8 +35,8 @@ stdenv.mkDerivation rec {
   buildInputs = [ gtk3 ];
 
   nativeBuildInputs = [
-    makeWrapper
+    makeShellWrapper
-    (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; })
+    wrapGAppsHook
   ];
 
   dontWrapGApps = true;
@@ -64,7 +64,7 @@ stdenv.mkDerivation rec {
       --set-interpreter $(cat $NIX_CC/nix-support/dynamic-linker) \
       "$(find "$out/toolbox" -name jspawnhelper)"
 
-    makeWrapper $out/toolbox/toolbox $out/bin/tla-toolbox \
+    makeShellWrapper $out/toolbox/toolbox $out/bin/tla-toolbox \
       --chdir "$out/toolbox" \
       --add-flags "-data ~/.tla-toolbox" \
       --prefix LD_LIBRARY_PATH : "${lib.makeLibraryPath [ gtk3 libXtst glib zlib ]}"  \

pkgs/applications/video/mpv/wrapper.nix

@@ -32,7 +32,7 @@ let
     # All arguments besides the input and output binaries (${mpv}/bin/mpv and
     # $out/bin/mpv). These are used by the darwin specific makeWrapper call
     # used to wrap $out/Applications/mpv.app/Contents/MacOS/mpv as well.
-    mostMakeWrapperArgs = lib.strings.escapeShellArgs ([ "--argv0" "'$0'"
+    mostMakeWrapperArgs = lib.strings.escapeShellArgs ([ "--inherit-argv0"
       # These are always needed (TODO: Explain why)
       "--prefix" "LUA_CPATH" ";" "${mpv.luaEnv}/lib/lua/${mpv.lua.luaversion}/?.so"
       "--prefix" "LUA_PATH" ";" "${mpv.luaEnv}/share/lua/${mpv.lua.luaversion}/?.lua"
@@ -53,7 +53,7 @@ let
     )) ++ extraMakeWrapperArgs)
     ;
     umpvWrapperArgs = lib.strings.escapeShellArgs ([
-      "--argv0" "'$0'"
+      "--inherit-argv0"
       "--set" "MPV" "${placeholder "out"}/bin/mpv"
     ] ++ extraUmpvWrapperArgs)
     ;

pkgs/build-support/setup-hooks/make-binary-wrapper/default.nix

@@ -0,0 +1,27 @@
+{ stdenv
+, lib
+, darwin
+, makeSetupHook
+, dieHook
+, writeShellScript
+, tests
+, cc ? stdenv.cc
+, sanitizers ? []
+}:
+
+makeSetupHook {
+  deps = [ dieHook ]
+    # https://github.com/NixOS/nixpkgs/issues/148189
+    ++ lib.optional (stdenv.isDarwin && stdenv.isAarch64) darwin.cctools;
+
+  substitutions = {
+    cc = "${cc}/bin/cc ${lib.escapeShellArgs (map (s: "-fsanitize=${s}") sanitizers)}";
+
+    # Extract the function call used to create a binary wrapper from its embedded docstring
+    passthru.extractCmd = writeShellScript "extract-binary-wrapper-cmd" ''
+      strings -dw "$1" | sed -n '/^makeCWrapper/,/^$/ p'
+    '';
+
+    passthru.tests = tests.makeBinaryWrapper;
+  };
+} ./make-binary-wrapper.sh

pkgs/build-support/setup-hooks/make-binary-wrapper/make-binary-wrapper.sh

@@ -15,24 +15,25 @@ assertExecutable() {
 # makeWrapper EXECUTABLE OUT_PATH ARGS
 
 # ARGS:
-# --argv0       NAME    : set name of executed process to NAME
+# --argv0       NAME    : set the name of the executed process to NAME
-#                         (otherwise it’s called …-wrapped)
+#                         (if unset or empty, defaults to EXECUTABLE)
 # --inherit-argv0       : the executable inherits argv0 from the wrapper.
 #                         (use instead of --argv0 '$0')
-# --set         VAR VAL : add VAR with value VAL to the executable’s
+# --set         VAR VAL : add VAR with value VAL to the executable's environment
-#                         environment
 # --set-default VAR VAL : like --set, but only adds VAR if not already set in
 #                         the environment
 # --unset       VAR     : remove VAR from the environment
 # --chdir       DIR     : change working directory (use instead of --run "cd DIR")
 # --add-flags   FLAGS   : add FLAGS to invocation of executable
+# TODO(@ncfavier): --append-flags
 
 # --prefix          ENV SEP VAL   : suffix/prefix ENV with VAL, separated by SEP
 # --suffix
 
 # To troubleshoot a binary wrapper after you compiled it,
 # use the `strings` command or open the binary file in a text editor.
-makeWrapper() {
+makeWrapper() { makeBinaryWrapper "$@"; }
+makeBinaryWrapper() {
     local NIX_CFLAGS_COMPILE= NIX_CFLAGS_LINK=
     local original="$1"
     local wrapper="$2"
@@ -43,7 +44,7 @@ makeWrapper() {
     mkdir -p "$(dirname "$wrapper")"
 
     makeDocumentedCWrapper "$original" "$@" | \
-      @CC@ \
+      @cc@ \
         -Wall -Werror -Wpedantic \
         -Wno-overlength-strings \
         -Os \
@@ -52,7 +53,8 @@ makeWrapper() {
 }
 
 # Syntax: wrapProgram <PROGRAM> <MAKE-WRAPPER FLAGS...>
-wrapProgram() {
+wrapProgram() { wrapProgramBinary "$@"; }
+wrapProgramBinary() {
     local prog="$1"
     local hidden
 
@@ -63,8 +65,6 @@ wrapProgram() {
       hidden="${hidden}_"
     done
     mv "$prog" "$hidden"
-    # Silence warning about unexpanded $0:
-    # shellcheck disable=SC2016
     makeWrapper "$hidden" "$prog" --inherit-argv0 "${@:2}"
 }
 
@@ -311,8 +311,9 @@ void set_env_suffix(char *env, char *sep, char *suffix) {
 "
 }
 
-# Embed a C string which shows up as readable text in the compiled binary wrapper
+# Embed a C string which shows up as readable text in the compiled binary wrapper,
-# documentationString ARGS
+# giving instructions for recreating the wrapper.
+# Keep in sync with makeBinaryWrapper.extractCmd
 docstring() {
     printf '%s' "const char * DOCSTRING = \"$(escapeStringLiteral "
 
@@ -333,7 +334,7 @@ makeCWrapper $(formatArgs "$@")
 
 # formatArgs EXECUTABLE ARGS
 formatArgs() {
-    printf '%s' "$1"
+    printf '%s' "${1@Q}"
     shift
     while [ $# -gt 0 ]; do
         case "$1" in

pkgs/build-support/setup-hooks/make-wrapper.sh

@@ -11,16 +11,18 @@ assertExecutable() {
 # makeWrapper EXECUTABLE OUT_PATH ARGS
 
 # ARGS:
-# --argv0       NAME    : set name of executed process to NAME
+# --argv0       NAME    : set the name of the executed process to NAME
-#                         (otherwise it’s called …-wrapped)
+#                         (if unset or empty, defaults to EXECUTABLE)
-# --set         VAR VAL : add VAR with value VAL to the executable’s
+# --inherit-argv0       : the executable inherits argv0 from the wrapper.
-#                         environment
+#                         (use instead of --argv0 '$0')
+# --set         VAR VAL : add VAR with value VAL to the executable's environment
 # --set-default VAR VAL : like --set, but only adds VAR if not already set in
 #                         the environment
 # --unset       VAR     : remove VAR from the environment
 # --chdir       DIR     : change working directory (use instead of --run "cd DIR")
 # --run         COMMAND : run command before the executable
 # --add-flags   FLAGS   : add FLAGS to invocation of executable
+# TODO(@ncfavier): --append-flags
 
 # --prefix          ENV SEP VAL   : suffix/prefix ENV with VAL, separated by SEP
 # --suffix
@@ -29,7 +31,8 @@ assertExecutable() {
 # --prefix-contents ENV SEP FILES : like --suffix-each, but contents of FILES
 #                                   are read first and used as VALS
 # --suffix-contents
-makeWrapper() {
+makeWrapper() { makeShellWrapper "$@"; }
+makeShellWrapper() {
     local original="$1"
     local wrapper="$2"
     local params varName value command separator n fileNames
@@ -165,6 +168,9 @@ makeWrapper() {
         elif [[ "$p" == "--argv0" ]]; then
             argv0="${params[$((n + 1))]}"
             n=$((n + 1))
+        elif [[ "$p" == "--inherit-argv0" ]]; then
+            # Whichever comes last of --argv0 and --inherit-argv0 wins
+            argv0='$0'
         else
             die "makeWrapper doesn't understand the arg $p"
         fi
@@ -193,7 +199,8 @@ filterExisting() {
 }
 
 # Syntax: wrapProgram <PROGRAM> <MAKE-WRAPPER FLAGS...>
-wrapProgram() {
+wrapProgram() { wrapProgramShell "$@"; }
+wrapProgramShell() {
     local prog="$1"
     local hidden
 
@@ -204,7 +211,5 @@ wrapProgram() {
       hidden="${hidden}_"
     done
     mv "$prog" "$hidden"
-    # Silence warning about unexpanded $0:
+    makeWrapper "$hidden" "$prog" --inherit-argv0 "${@:2}"
-    # shellcheck disable=SC2016
-    makeWrapper "$hidden" "$prog" --argv0 '$0' "${@:2}"
 }

pkgs/build-support/setup-hooks/wrap-gapps-hook/default.nix

@@ -1,7 +1,7 @@
 { stdenv
 , lib
 , makeSetupHook
-, makeBinaryWrapper
+, makeWrapper
 , gobject-introspection
 , isGraphical ? true
 , gtk3
@@ -34,7 +34,7 @@ makeSetupHook {
   ] ++ [
 
     # We use the wrapProgram function.
-    makeBinaryWrapper
+    makeWrapper
   ];
   substitutions = {
     passthru.tests = let

pkgs/test/default.nix

@@ -71,7 +71,15 @@ with pkgs;
 
   dhall = callPackage ./dhall { };
 
-  makeWrapper = callPackage ./make-wrapper {};
+  makeWrapper = callPackage ./make-wrapper { };
+  makeBinaryWrapper = callPackage ./make-binary-wrapper {
+    makeBinaryWrapper = pkgs.makeBinaryWrapper.override {
+      # Enable sanitizers in the tests only, to avoid the performance cost in regular usage.
+      # The sanitizers cause errors on aarch64-darwin, see https://github.com/NixOS/nixpkgs/pull/150079#issuecomment-994132734
+      sanitizers = pkgs.lib.optionals (! (pkgs.stdenv.isDarwin && pkgs.stdenv.isAarch64))
+        [ "undefined" "address" ];
+    };
+  };
 
   pkgs-lib = recurseIntoAttrs (import ../pkgs-lib/tests { inherit pkgs; });
 }

pkgs/tools/security/cryptomator/default.nix

@@ -1,7 +1,7 @@
 { lib, stdenv, fetchFromGitHub
 , autoPatchelfHook
 , fuse, jffi
-, maven, jdk, jre, makeWrapper, glib, wrapGAppsHook
+, maven, jdk, jre, makeShellWrapper, glib, wrapGAppsHook
 }:
 
 let
@@ -65,7 +65,7 @@ in stdenv.mkDerivation rec {
     rm $out/share/cryptomator/libs/jff*.jar
     cp -f ${jffi}/share/java/jffi-complete.jar $out/share/cryptomator/libs/
 
-    makeWrapper ${jre}/bin/java $out/bin/cryptomator \
+    makeShellWrapper ${jre}/bin/java $out/bin/cryptomator \
       --add-flags "--class-path '$out/share/cryptomator/libs/*'" \
       --add-flags "--module-path '$out/share/cryptomator/mods'" \
       --add-flags "-Dcryptomator.logDir='~/.local/share/Cryptomator/logs'" \
@@ -101,8 +101,8 @@ in stdenv.mkDerivation rec {
   nativeBuildInputs = [
     autoPatchelfHook
     maven
-    makeWrapper
+    makeShellWrapper
-    (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; })
+    wrapGAppsHook
     jdk
   ];
   buildInputs = [ fuse jre glib jffi ];

pkgs/top-level/all-packages.nix

@@ -774,7 +774,9 @@ with pkgs;
   makeInitrdNG = callPackage ../build-support/kernel/make-initrd-ng.nix;
   makeInitrdNGTool = callPackage ../build-support/kernel/make-initrd-ng-tool.nix {};
 
-  makeWrapper = makeSetupHook
+  makeWrapper = makeShellWrapper;
+
+  makeShellWrapper = makeSetupHook
     { deps = [ dieHook ];
       substitutions = {
         shell = targetPackages.runtimeShell;
@@ -783,30 +785,7 @@ with pkgs;
     }
     ../build-support/setup-hooks/make-wrapper.sh;
 
-  makeBinaryWrapper = let
+  makeBinaryWrapper = callPackage ../build-support/setup-hooks/make-binary-wrapper { };
-    f = { cc, sanitizers }: let
-      san = lib.concatMapStringsSep " " (s: "-fsanitize=${s}") sanitizers;
-      script = runCommand "make-binary-wrapper.sh" {} ''
-        substitute ${../build-support/setup-hooks/make-binary-wrapper.sh} $out \
-          --replace " @CC@ " " ${cc}/bin/cc ${san} "
-      '';
-    in
-      makeSetupHook {
-        deps = [ dieHook cc ];
-        substitutions.passthru.tests = callPackage ../test/make-binary-wrapper {
-          makeBinaryWrapper = makeBinaryWrapper.override {
-            sanitizers = (if stdenv.isDarwin && stdenv.isAarch64
-              then [ ]
-              else [ "undefined" "address" ]
-            );
-          };
-        };
-      } script;
-  in
-    lib.makeOverridable f {
-      cc = stdenv.cc;
-      sanitizers = [ ];
-    };
 
   makeModulesClosure = { kernel, firmware, rootModules, allowMissing ? false }:
     callPackage ../build-support/kernel/modules-closure.nix {
@@ -903,7 +882,9 @@ with pkgs;
 
   findXMLCatalogs = makeSetupHook { } ../build-support/setup-hooks/find-xml-catalogs.sh;
 
-  wrapGAppsHook = callPackage ../build-support/setup-hooks/wrap-gapps-hook { };
+  wrapGAppsHook = callPackage ../build-support/setup-hooks/wrap-gapps-hook {
+    makeWrapper = makeBinaryWrapper;
+  };
 
   wrapGAppsHook4 = wrapGAppsHook.override { gtk3 = gtk4; };